Skip to content

Add support for EIP-1271 signature verification in the utils package #3904

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

Open
wants to merge 1 commit into
base: v6
Choose a base branch
from

Conversation

Ivshti
Copy link

@Ivshti Ivshti commented Mar 15, 2023

This PR adds three new functions to the utils package, all of which verify messages in a way that's compatible with EIP-1271 but also compatible with the regular ecrecover signature.

  • isMessageSignatureCorrect: verifies a regular, prefixed, string message
  • isTypedDataSignatureCorrect: verifies an EIP-712 typed data signature
  • isFinalDigestSignatureCorrect: verifies a signature for a raw hash (final digest)

Rationale

Contract wallets like Safe, Ambire and Argent are gaining adoption, with Safe in particular holding billions of user funds in ETH only. However, the overall awareness of contract wallet signature verification among dApp developers is really low, as they usually (and reasonably) assume that default functions in ethers (and competing libraries) are sufficient for signature verification.

We believe the scalable way to solve this problem is to implement a universal signature verification function in Ethers that will be used by developers.

If ethers choses to implement a separate function for EIP1271, this function would be nearly worthless, because it's really easy to do new Contract(...).isValidSignature anyway. The real value-add would only come from a universal method.

Furthermore, we are modelling those functions on ethers.utils.verifyMessage, so that migration is easy, but with the following changes:

  • takes the address you're expecting to be signing and returns a bool, which is necessary for ERC-1271 verification (address can't be recovered from signature) but also more intuitive and more ergonomical in 90% of the cases when devs usually do ethers.utils.verifyMessage(...) == targetAddress

It must be noted that the call MUST go first, before the ecrecover, because there may be a valid contract signature that also happens to be a valid ecrecover signature for another address (for example, a signature produced by one of the signers of a Safe multisig).

@Ivshti Ivshti changed the base branch from main to v6 March 15, 2023 15:09
@Ivshti Ivshti force-pushed the universal-sig-verification branch from eb09d6b to 1f44b24 Compare March 15, 2023 16:01
@gergold
Copy link

gergold commented Mar 15, 2023

This is the javascript equivalent of Solidity methods exposed in the SignatureChecker library from Open Zeppelin. We definitely need it here to ensure dApps compatibility with smart contract based wallets.

@odysseus0
Copy link

We as smart contract wallet providers have faced various compatibility issues from projects defaulting to only EOA compatible method when verifying signatures. It would be huge for contract wallet compatibility.

@Oxbobby
Copy link

Oxbobby commented Mar 16, 2023

If we hope to have an Account Abstraction future, a universal method is the way to go. Otherwise, it would be too cumbersome to require each dapp to check whether it's connected to an Account Abstraction or an EOA and afterwards decide on the set of methods to use.

@superKalo
Copy link

In my opinion, this is highly crucial to achieving account abstraction on Ethereum 👏

The absence of this feature will result in many dApp developers resorting to the standard verifyMessage, leading to dApps that lack support for 1271.

@cmihaylov
Copy link

Way to go! We need this in order to pave the way for account abstraction / smart contract wallets and compatibility with dApps. 🚀

@ricmoo
Copy link
Member

ricmoo commented Mar 16, 2023

I am in regular contact with the Account Abstraction team, and I agree we need something, but please be patient. :)

Once we add something, it is present until the next major release and requires support and maintenance. So, I’d rather aim to get things as right as possible the first time.

@Ivshti
Copy link
Author

Ivshti commented Mar 16, 2023

@ricmoo can we at least get your thoughts on the PR? It's remarkably simple and I would say it's also future proof.

@drortirosh
Copy link

supporting https://eips.ethereum.org/EIPS/eip-6492 would give both "normal" erc1271 AND also support counterfactual contracts, so it seems a better deal...

@Ivshti
Copy link
Author

Ivshti commented Mar 20, 2023

hey @drortirosh I completely agree and I can PR 6492 as well. 6492 has other advantages as a "wrapper" on top of 1271, such as 1) immediately discovering that the signature is a SC signature, so validation is faster 2) you can recover the address from the signature itself without any additional info

however, we at Ambire decided to start with 1271 in order to give us a higher chance of convincing @ricmoo, since it has been around for way longer. However, given that 6492 is a wrapper on top of 1271, I could see the rationale for this.

@Sednaoui
Copy link

Sednaoui commented Jun 1, 2023

We were looking into making a similar PR, glad we searched before implementing the same work. A universal method for both EOAs and smart contract wallets is the way to go, since developers always assume and test their applications only with metamask. Without thinking of supporting contract accounts.

We got some bug reports by a few end-users of Candide Wallet, who sometimes find incompatible dapps and aren't able to login in (sign) into many dapps on optimism.

@Ivshti
Copy link
Author

Ivshti commented Jun 5, 2023

We were looking into making a similar PR, glad we searched before implementing the same work. A universal method for both EOAs and smart contract wallets is the way to go, since developers always assume and test their applications only with metamask. Without thinking of supporting contract accounts.

We got some bug reports by a few end-users of Candide Wallet, who sometimes find incompatible dapps and aren't able to login in (sign) into many dapps on optimism.

you should seriously consider also implementing ERC-6492 so that non-deployed contracts can sign messages as well. We can help a lot with this. signature-validator uses a pre-built contract that's really simple to support all types of verifications with a single eth_call, which is very performant when verifying SC sigs.

@drortirosh
Copy link

The reason to use ERC-6492 is twofold:

  • It adds support for undeployed contracts too
    but even more importantly:
  • It is a single eth_call() to do all checks: either ecrecover, isValidSignature on existing contract or isValidSignature on pre-deployed contract (and you can simply ignore the latter if you don't have the contract's construction code)

@Ivshti
Copy link
Author

Ivshti commented Jul 2, 2023

@ricmoo would you be open to merging a PR adding universal verification (EIP-1271, EIP-6492 and ecrecover via one call) as an extra function in utils?

@Ivshti
Copy link
Author

Ivshti commented Oct 30, 2023

@ricmoo as AA adoption is advancing, you should heavily consider adding a utility for 6492 and 1271 and discouraging use of the method that only supports EOAs (via a name change).

Just let us know you're interested and we'd happily update this PR

This is definitely not a niche thing anymore, especially considering that Safe is using 1271 as well, and it holds billions of dollars

@ricmoo
Copy link
Member

ricmoo commented Oct 30, 2023

A lot of thought and design is definitely being put into AA and making things more abstract for Signers, including a focus on contract wallets.

I have been experimenting with AA, and various ways to shoehorn it into v6, but some API changes are needed to make things “graceful”, so it might be more of a v7 thing, which I’m hoping to have an initial beta concept out for by end of November. :)

The migration from v6 to v7 will be much smoother than the last migration, with the goal of no changes being required by the vast majority of existing projects other than changing the major version in the package.json. :)

@Ivshti
Copy link
Author

Ivshti commented Oct 30, 2023

@ricmoo that makes a lot of sense, the PR effort was started before the release of v6 and this thing definitely requires a new major version - as you said, API changes are needed, and for this, a major version is needed

let me know how we can help please - is there a branch for v7 where this PR can be reworked on?

Do you have any concept for the API? What do you think about the API implemented here?

@asadahmedtech
Copy link

asadahmedtech commented Dec 19, 2023

@ricmoo @Ivshti, we at okto.tech have also been working on providing AA wallets at a mass scale. The problem we have seen is at mass adoption users will not deploy contracts on ethereum chain but would interact with dApps on other EVM chains. We wanted to understand if there is any thread we can follow or contribute to, for making 6492 a reality.

@Ivshti
Copy link
Author

Ivshti commented Dec 20, 2023

hey @asadahmedtech, 6492 is backwards compatible with 1271 and at dapp level nothing needs to be changed. Wallets need to start adding the 6492 wrapper format and verifiers need to either implement 6492 verification as per the Solidity in the EIP, or more easily just use the signature-verifier library

@asadahmedtech
Copy link

@Ivshti is the wrapper going to be part of the ethers library, where existing verification functions starts working.
from wallets side is there any additional implementation required to generate signature in a specific format?

@Ivshti
Copy link
Author

Ivshti commented Dec 22, 2023

@asadahmedtech ethers has no plans to implement this as of today, to the best of my knowledge

the wrapper format is described here https://eips.ethereum.org/EIPS/eip-6492 and it's really easy to implement from the wallet side

@asadahmedtech
Copy link

@Ivshti will it make easier for dapps to validate signatures if this functionality is part of ethers library? Currently dApps like opensea would not work with lazy deployment unless the dApps specifically implement this custom verification logic?

@Ivshti
Copy link
Author

Ivshti commented Sep 21, 2024

@asadahmedtech

yes, that's the whole point of adding it here

It needs to be default too, not a new additional method that you still to branch into. So unless ethers implements a universal signature validation function like signature-validator does, it's not gonna be a big improvement, but if it does, it will contrbute significantly.

For example, viem supports both EIP-1271 and EIP-6492 by default.

It's been over a year and AA adoption is much stronger, and it's now much more obvious to everyone that standards like EIP-1271 and EIP-6492 are valuable, necessary and not hard to imlement.

@ricmoo if you're willing to reconsider we'll update this PR and rebase it against the latest version, so that ethers has a universal signature validation method that will make life much easier.

@jxom
Copy link

jxom commented Feb 11, 2025

I would more than highly recommend Ethers supports ERC-1271. Dapps that use Ethers are not compatible with Wallets that use Smart Contract Accounts. It’s a critical feature for SCAs to work properly, and even more when 7702 is launched.

Viem has had support for it for nearly 2 years now.

it would be great if 100% of Dapps supported SCAs, not the 50% that use Viem. ;)

@ricmoo
Copy link
Member

ricmoo commented Feb 12, 2025

Yes, the original plan was to include this in a ContractSigner, but I think it makes sense to include in a utility (that any ContractSigner can use).

I'll add it to the upcoming minor bump. :)

@ricmoo ricmoo added the enhancement New feature or improvement. label Feb 12, 2025
@ricmoo ricmoo added on-deck This Enhancement or Bug is currently being worked on. minor-bump Planned for the next minor version bump. v6 Issues regarding v6 next-patch Issues scheduled for the next arch release. labels Feb 12, 2025
@ricmoo
Copy link
Member

ricmoo commented Feb 12, 2025

@Ivshti I am absolutely willing to include support. What I am most interested in, is what a simple (but flexible) API would look like, in the Ethers API side of things. Any suggestions, PRs, etc. are welcome. I generally don't "just merge" a PR, but it is a great way to demonstrate a working version. I'd also appreciate feedback from @drortirosh regarding any proposed API.

@jxom
Copy link

jxom commented Feb 12, 2025

What if the BaseWallet class (which has knowledge of a Provider) included a verifyHash (and consequently verifyMessage, etc), that routed via a deployless ERC-6492 verification contract? We use @Ivshti's audited one and this has worked very well for us. We didn't create a "contract signer" abstraction, as consumers shouldn't have to think if the account they are working with is a contract or not – consumers shouldn't have to do a eth_getCode call. Here is the discussion that convinced me.

@Ivshti
Copy link
Author

Ivshti commented Mar 21, 2025

@ricmoo @jxom I personally see this as a stand-alone util function that takes in provider, exactly like this PR.

However, unlike this PR, I'd make it use the 6492 verification contract as @jxom mentioned, rather than just 1271 support. 6492 is a backward compatible extension to 1271, and it also includes ecrecover verification in it.

If this sounds good (standalone util function for message verification), we'll proceed to making a PR

@Ivshti
Copy link
Author

Ivshti commented Mar 27, 2025

@jxom @ricmoo thoughts on the above?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement. minor-bump Planned for the next minor version bump. next-patch Issues scheduled for the next arch release. on-deck This Enhancement or Bug is currently being worked on. v6 Issues regarding v6
Projects
None yet
Development

Successfully merging this pull request may close these issues.