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

Add tooling to verify signatures with support for ERC1271 #2532

Merged
merged 9 commits into from
Apr 7, 2021

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Feb 19, 2021

SignatureChecker is a library for seamless verification of signatures with support for both EOA (ECDSA) and smart contract wallets (ERC1271).

While still being a draft, this ERC is supported by smart contract wallets such as Argent.

I believe this feature could greatly improve the compatibility of dapps that relies on message signatures with such wallets. In particular since Argent is now able to sign messages (including ERC721 typedData) through WalletConnect.

Continuation of #2459
FIxes #2535

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@Amxx Amxx added the idea label Feb 19, 2021
@Amxx Amxx force-pushed the feature/SignatureChecker branch 2 times, most recently from 69524f0 to a8011dc Compare February 23, 2021 08:33
@Amxx Amxx force-pushed the feature/SignatureChecker branch from a8011dc to 160fd2e Compare March 10, 2021 14:39
@Amxx Amxx marked this pull request as ready for review March 12, 2021 07:29
@Amxx Amxx changed the title Feature/signature checker Add tooling to verify signatures with support for ERC1271 Mar 12, 2021
@Amxx Amxx force-pushed the feature/SignatureChecker branch from dd12291 to 156ce66 Compare March 23, 2021 15:25
Amxx added 6 commits March 23, 2021 16:29
SignatureChecker is a library for seamless verification of signature with support for both EOA (ECDSA) and smart contract wallets (ERC1271)
@Amxx Amxx force-pushed the feature/SignatureChecker branch from 156ce66 to 9a2b018 Compare March 23, 2021 15:30
return false;
}
} else {
return ECDSA.recover(hash, signature) == signer;
Copy link
Contributor

Choose a reason for hiding this comment

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

From our recommendation in ECDSA:

* IMPORTANT: `hash` _must_ be the result of a hash operation for the
* verification to be secure: it is possible to craft signatures that
* recover to arbitrary addresses for non-hashed data. A safe way to ensure
* this is by receiving a hash of the original message (which may otherwise
* be too long), and then calling {toEthSignedMessageHash} on it.

We should be providing some other link in the docs, but there is more info in this article.

So, is this function really safe? Isn't it vulnerable to crafting signature and hash values that will recover to the signer but which do not correspond to a real ECDSA signature?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So, is this function really safe? Isn't it vulnerable to crafting signature and hash values that will recover to the signer but which do not correspond to a real ECDSA signature?

Our own ECDSA.recover doesn't enforce that. You can technically use it to recover a hash that was not properly formated. The same goes for a smart wallet 1271 implementation (the code in isValidSignature). In both cases, we cannot do anything about it (the hash might be 191, or 712, or anything not yet standardized. The best we can do is adding the warning you mentioned in the interface for 1271.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, for some reason I thought this API was more vulnerable to misuse, but now that I'm looking at it again it I can see that it's the same.

contracts/interfaces/IERC1271.sol Show resolved Hide resolved
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

LGTM!

@frangio frangio enabled auto-merge (squash) April 7, 2021 13:02
@frangio frangio merged commit 0c62124 into OpenZeppelin:master Apr 7, 2021
@Amxx Amxx deleted the feature/SignatureChecker branch April 7, 2021 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tools for ERC1271 signature verification
2 participants