Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OpenZeppelin Contracts's SignatureChecker may revert on invalid EIP-1271 signers #76

Closed
wants to merge 1 commit into from

Conversation

imhunterand
Copy link

SignatureChecker.isValidSignatureNow is not expected to revert. However, an incorrect assumption about Solidity 0.8's abi.decode allows some cases to revert, given a target contract that doesn't implement EIP-1271 as expected.

The contracts that may be affected are those that use SignatureChecker to check the validity of a signature and handle invalid signatures in a way other than reverting. We believe this to be unlikely.

Patches

The issue was patched in 4.7.1.

References

OpenZeppelin/openzeppelin-contracts#3552

Additional Information

Please invited my github to contributed/collaborate at your project community.
Regards,

Signed-off-by: imhunterand <hostbugbounty1337@gmail.com>
Copy link
Contributor

@nguyer nguyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this. Instead of editing package-lock.json directly, the updated package should be added to both package.json and package-lock.json by running npm install @openzeppelin/contracts@v4.7.1

samples/solidity/package-lock.json Show resolved Hide resolved
@nguyer
Copy link
Contributor

nguyer commented Sep 26, 2022

Closing for now, due to lack of response. Please feel free to re-open if updated properly. Thanks!

@nguyer nguyer closed this Sep 26, 2022
@imhunterand
Copy link
Author

Why this has been closed without merged ?

@nguyer
Copy link
Contributor

nguyer commented Sep 26, 2022

Hi @imhunterand. I'm happy to reopen it. I requested changes a while ago and the PR feedback was not addressed though. If you'd also like to update package.json I'm happy to merge this.

@imhunterand
Copy link
Author

Hi thanks for your respond, could you merged this pls ?

Regards

@nguyer
Copy link
Contributor

nguyer commented Sep 26, 2022

Yes, I'm happy to merge it if you address the changes that I requested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants