ethers.js icon indicating copy to clipboard operation
ethers.js copied to clipboard

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

Open Ivshti opened this issue 1 year ago • 22 comments

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 avatar Mar 15 '23 15:03 Ivshti

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.

gergold avatar Mar 15 '23 16:03 gergold

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.

odysseus0 avatar Mar 15 '23 19:03 odysseus0

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.

borislav-itskov avatar Mar 16 '23 09:03 borislav-itskov

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.

superKalo avatar Mar 16 '23 10:03 superKalo

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

cmihaylov avatar Mar 16 '23 11:03 cmihaylov

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.

ricmoo avatar Mar 16 '23 11:03 ricmoo

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

Ivshti avatar Mar 16 '23 11:03 Ivshti

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

drortirosh avatar Mar 20 '23 10:03 drortirosh

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.

Ivshti avatar Mar 20 '23 14:03 Ivshti

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.

Sednaoui avatar Jun 01 '23 13:06 Sednaoui

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.

Ivshti avatar Jun 05 '23 10:06 Ivshti

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)

drortirosh avatar Jun 05 '23 11:06 drortirosh

@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 avatar Jul 02 '23 05:07 Ivshti

@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

Ivshti avatar Oct 30 '23 15:10 Ivshti

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. :)

ricmoo avatar Oct 30 '23 16:10 ricmoo

@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?

Ivshti avatar Oct 30 '23 16:10 Ivshti

@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.

asadahmedtech avatar Dec 19 '23 14:12 asadahmedtech

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

Ivshti avatar Dec 20 '23 07:12 Ivshti

@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?

asadahmedtech avatar Dec 20 '23 22:12 asadahmedtech

@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

Ivshti avatar Dec 22 '23 06:12 Ivshti

@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?

asadahmedtech avatar Dec 27 '23 10:12 asadahmedtech