eth-infinitism / account-abstraction

GNU General Public License v3.0
1.51k stars 624 forks source link

Counterfactual ERC-1271 signature validation #188

Closed derekchiang closed 1 year ago

derekchiang commented 1 year ago

I'm running into an issue that I realized may be a fundamental limitation of ERC-4337, so I want to start a discussion here to see if there's a potential solution.

As we know, in ERC-4337, the account is not deployed until the first UserOp is mined. Normally this is great, since we can use the counterfactual address to receive assets without having to deploy the account first.

However, if I want to sign a message for a DApp (e.g. OpenSea), this becomes an issue since the DApp is going to look at my address, see that it has no code, and assume that my account is EOA, and reject my signature. It wouldn't know to validate my signature through ERC-1271 since, well, the contract isn't there yet.

TLDR: a new (undeployed) ERC-4337 account can't have its signed messages validated through ERC-1271, which can cause it to be rejected by DApps. How can we solve this problem?

hazim-j commented 1 year ago

I think this is a limitation of ERC-1271 in general. I'd also be interested to see if there are any viable solutions for this.

A possible solution might be to extend 1271 so that it can interact with a factory contract. In this case the client could make a static call to the factory that deploys and calls isValidSignature within the same function. I wonder if this would be viable?

Alternatively, the current app layer solutions I've seen are:

  1. App takes on the cost of deploying the account for users.
  2. Assign statuses to accounts on the UI that are simple enough for users to figure out why their sigs are failing (e.g. "Active" and "Inactive").
yoavw commented 1 year ago

This question has come up a couple of times before. There are a few different ways to address it. Extending 1271 to interact with a factory contract though a new ERC is one of the options, but will take some time to get adoption.

A couple of other ways:

1. Verifier side solution

Pros:

Cons:

2. Bundler RPC solution

Pros:

Cons:

3. Account solution

Pros:

Cons:

4. EntryPoint solution - solving it in ERC-4337 itself

Pros:

Cons:

Personally I lean toward 3 or 4, but please provide feedback. @derekchiang @hazim-j @drortirosh @shahafn

derekchiang commented 1 year ago

@hazim-j @yoavw great ideas. Hazim's idea is a variant of Yoav's "verifier side solution" so I won't address it separately. Here are my thoughts on the four ideas Yoav listed:

  1. Verifier side solution: signing twice is terrible UX so it's an easy no for me.

  2. Bundler RPC solution: this imposes too much work on the verifier since they have to either trust a bundler or run a bundler themselves. Easy no for me.

  3. Account solution: this complicates the work of account implementors by a significant amount. Implementing the logic in a BaseAccount helps, but generally speaking I don't think we should make design decisions assuming that most accounts will be inheriting from our BaseAccount. This would create a slippery slope for us to hide more and more complexity inside BaseAccount, eventually to a point where most account implementors have no choice but to inherit from our BaseAccount, thus hurting diversity in account implementations.

  4. EntryPoint solution: this is IMO the best solution since it fits ERC-4337's overall mantra that most of the complexity should be tackled by a global singleton contract, thus keeping account implementations simple. However, this is also a slippery slope since it creates the expectation that the EntryPoint will be continuously upgraded as new standards emerge, which is bad since upgrading the EntryPoint is wildly disruptive to the ecosystem.

So the most practical path forward, IMO, is to go with 4, since ERC-4337 has not been officially deployed yet, so upgrading the EntryPoint at this point won't disrupt the ecosystem. If and when a new signing standard emerges, we can have another discussion about it.

In any case, these solutions don't in themselves solve the problem I brought up, since the verifier still needs to be aware of ERC-4337 and update their verification flow accordingly. So we still need to propose some sort of standard where the signer can somehow signal to the verifier that the signature is to be validated through ERC-4337. I assume the most natural way is to append some magic bytes in front of the signature (only if the account hasn't been deployed)?

drortirosh commented 1 year ago

In any case, these solutions don't in themselves solve the problem I brought up, since the verifier still needs to be aware of ERC-4337 and update their verification flow accordingly

Right. AA account deployment can be deferred, but if someone want to do some validation prior actual creation it has to know its AA parameters, and simulate it (along with the needed operation, e.g. isValidateSignature)

We think about adding "eth_call" functionality into the simulateHandleOp, so that you can make call to (and get return value of) a function no the not-yet-created account. You do need the UserOp that creates this account, though.

That is, we add to simulateHandleOp another parameter (calldata) which is called on the created account, and we return its return value (and success/revert status) in the ExecutionResult error

Bottom line: With this addition, it is possible to make a view call (isValidateSignature) on undeployed account, but it requires the Dapp to know the actual initCode of that account.

yoavw commented 1 year ago

What @drortirosh described is a generalization of (4) and seems like the cleanest approach. Makes it possible to simulate account creation combined with an arbitrary call (such as isValidSignature). It solves counterfactual EIP-1271 validation and any other future need for counterfactual calls.

@derekchiang it'll be great to have a standard to convey the initCode (and the fact that it's an ERC-4337 account) as part of the signature. But how is it currently handled by dapps that support EIP-1271? A dapp that only supports EOAs doesn't need to ask the user for the account address. It just ecrecovers the signature. But a dapp that supports EIP-1271 needs to get an additional detail - the account address against which it'll run isValidSignature. Is there a standard for conveying this detail? The information needed for counterfactual validation (initCode) could be conveyed the same way as the address.

derekchiang commented 1 year ago

@yoavw ERC-1271 itself doesn't define how the account address is supposed to be communicated. Currently the typical DApp flow is just:

As another example, in WalletConnect, the account address is communicated as part of the protocol.

As for communicating the initCode, perhaps we propose a standard as an extension to ERC-1271, wherein a signature looks like <erc4337_magic_bytes><init_code_length><init_code><signature>? Encoding the initCode inside the signature itself makes it maximally compatible with existing protocols like WalletConnect.

yoavw commented 1 year ago

How will the magic byte be distinguished from a signature? isValidSignature doesn't require a specific type of signature, so any byte we choose could also be a valid first byte for some signature scheme.

On Mon, 30 Jan 2023, 21:16 Derek Chiang, @.***> wrote:

@yoavw https://github.com/yoavw ERC-1271 itself doesn't define how the account address is supposed to be communicated. Currently the typical DApp flow is just:

  • Ask the user what their address is
  • Then ask the user to sign a message to prove that they own that address.

In WalletConnect for example, the account address is communicated as part of the protocol.

As for communicating the initCode, perhaps we propose a standard as an extension to ERC-1271, wherein a signature looks like `"?

— Reply to this email directly, view it on GitHub https://github.com/eth-infinitism/account-abstraction/issues/188#issuecomment-1409198722, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFNCULR3AMW7URRYQMZH3LWVAHRVANCNFSM6AAAAAAUJOI34I . You are receiving this because you were mentioned.Message ID: @.***>

derekchiang commented 1 year ago

I meant magic bytes -- so it'd be a long enough sequence that collisions are unlikely. Not sure how long it needs to be though?

yoavw commented 1 year ago

If it's a new standard anyway, wouldn't it be safer to make it explicit and not embed it in a single signature field?

derekchiang commented 1 year ago

There are three parties to consider here:

Transporter would be like WalletConnect or Web3Provider, the protocol through which verifier and signer talk to each other. Embedding the initCode into the signature means that the transporter doesn't have to be updated.

yoavw commented 1 year ago

We could do that with a prefix but it would have to be quite long to make collisions unlikely. How many transporters would have to be updated to support the new standard? The signer and the verifier need to know about it in any case, and the transporter is WalletConnect or one of the Web3Providers - how many of them are actively used?

derekchiang commented 1 year ago

Actually, it occurred to me that we don’t need magic bytes. We could simply stipulate that the verifier goes through the 4337 validation flow if and only if the EOA validation flow fails.

So the verifier would first assume that the account is EOA (if there’s no code at the address). If the signature fails to validate, then the verifier tries to interpret the signature as 4337, e.g. in the form of “”.

This places more burden on the verifier, but in practice if the signature is not 4337, the validation should fail very quickly, since the “init_code_len” will likely be very big, or the factory address in “init_code” will point to an empty address.

Security-wise, the chance that an invalid EOA signature accidentally passes as a valid 4337 signature should be nil, and it should be impossible for an attacker to construct a 4337 signature that passes validation for a specific EOA address, since it’s generally impossible to construct an “init_code” that deploys to a specific EOA address.

On Mon, Jan 30, 2023 at 5:56 PM Yoav Weiss @.***> wrote:

We could do that with a prefix but it would have to be quite long to make collisions unlikely. How many transporters would have to be updated to support the new standard? The signer and the verifier need to know about it in any case, and the transporter is WalletConnect or one of the Web3Providers - how many of them are actively used?

On Tue, Jan 31, 2023 at 12:52 AM Derek Chiang @.***> wrote:

There are three parties to consider here:

  • Verifier
  • Transporter
  • Signer

Transporter would be like WalletConnect or Web3Provider, the protocol through which verifier and signer talk to each other. Embedding the initCode into the signature means that the transporter doesn't have to be updated.

— Reply to this email directly, view it on GitHub < https://github.com/eth-infinitism/account-abstraction/issues/188#issuecomment-1409481547 , or unsubscribe < https://github.com/notifications/unsubscribe-auth/AAFNCUITB4CQMOMYMQ3KAATWVBA4NANCNFSM6AAAAAAUJOI34I

. You are receiving this because you were mentioned.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/eth-infinitism/account-abstraction/issues/188#issuecomment-1409484469, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALW5RXZLPOEQ5ISPT62TR3WVBBKNANCNFSM6AAAAAAUJOI34I . You are receiving this because you were mentioned.Message ID: @.***>

ch4r10t33r commented 1 year ago

4. EntryPoint solution - solving it in ERC-4337 itself

  • Add another simulation function EntryPoint.validateERC1271Signature(account,initCode,sig,hash) , almost identical to simulateHandleOp but generates the UserOp itself, so no tracing is needed.
  • The function generates a UserOp with {sender=account, initCode=initCode, callData=0x}.
  • Run the same flow as simulateHandleOp which disregards SIG_FAILED. At the end, require(<account>.isValidSignature(hash,sig) == 0x1626ba7e, "Bad signature")
  • If successful, revert with ExecutionResult similarly to simulateHandleOp.
  • Verifier just eth_call's this function. No tracing needed. The account is generated and the signature is verified within the same call.

IMHO, option 4 is the best and will be easier for everyone to adopt, given that we are still in the early stages of AA using ERC4337.

Ivshti commented 1 year ago

I'm very opinionated here in that I think that a solution should extend 1271 rather than have anything to do with 4337. Signatures are a completely different problem than the scope of 4337.

Plus, solving this separately would allow legacy SCW to solve this problem as well.

Here's what I propose as a solution to off-chain signature validation (which is 99% of the cases): If the signature is produced when the account is not deployed, formulate it in this format: 0x{magic_bytes}{create2 factory addr}{salt}{bytecode}{actual sig}. Then, off-chain signature verifiers could perform the following steps:

  1. has magic bytes? simulate deployment and call isValidSignature
  2. the address has contract code? call isValidSignature
  3. call ecrecover

This does not require any changes to 4337, does not add extra complexity, and is super simple to implement. Some of you already proposed something along those lines.

@yoavw As for onchain signature validation, the same signature format could still be used, but it requires extra standartization to make it work, which could be part of 4337 (entrypoint) in order to simplify the work for contract developers.

Ivshti commented 1 year ago

@derekchiang

Actually, it occurred to me that we don’t need magic bytes. We could simply stipulate that the verifier goes through the 4337 validation flow if and only if the EOA validation flow fails. So the verifier would first assume that the account is EOA (if there’s no code at the address). If the signature fails to validate, then the verifier tries to interpret the signature as 4337, e.g. in the form of “”. This places more burden on the verifier, but in practice if the signature is not 4337, the validation should fail very quickly, since the “init_code_len” will likely be very big, or the factory address in “init_code” will point to an empty address. Security-wise, the chance that an invalid EOA signature accidentally passes as a valid 4337 signature should be nil, and it should be impossible for an attacker to construct a 4337 signature that passes validation for a specific EOA address, since it’s generally impossible to construct an “init_code” that deploys to a specific EOA address.

This is dangerous, as a valid ecrecover signature may also be a valid 1271 signature for a different account. Consider the following: an AA account uses an EOA account as a signer, and uses the same signature format (because it doesn't have a reason not to). In such cases, if a verifier tries ecrecover first, it will end up concluding that the EOA that signed is the actual account, when in fact there's another layer above it.

The problem is well defined here: https://github.com/AmbireTech/signature-validator/blob/main/index.js#L37

drortirosh commented 1 year ago

Doing ecrecover() first is dangerous: the guideline is to do contract access first, and only if it fails, to fallback to ecrecover. The reason: in the future, it would be possible to upgrade an EOA into a contract (actual method is TBD, but that's the goal) If I upgrade my EOA into a contract, I want all previous manual signatures to be revoked, as there is now a new signer (and also modifying this signer would revoke any previous signatures it signed)

Another reason to check the code first is that it is slightly cheaper than ecrecover(), but the above security concern outweighs it anyway.

Ivshti commented 1 year ago

@drortirosh agreed. But even if this never happens (which is possible through adoption of 4337), the straight-forward implementation of an AA account may very likely use signatures that are also valid EOA signatures for the underlying signer.

In summary, do not do ecrecover() first, magic bytes are definitely needed

Full proposal for counterfactual EIP 1271 signatures

Wallet side

Note that we're passing factoryCalldata instead of salt and bytecode. We do this in order to make verification compliant with any factory interface. We do not need to calculate the address based on create2Factory/salt/bytecode, because EIP1271 verification presumes we already know the account address we're verifying the signature for.

As @yoavw said, {magicBytes} may need to be long to avoid the possibility of a collision with an actual ECDSA signature, but an additional length check may help with this - just the create2 address and salt already occupy 52 bytes, so a 16 byte magic prefix already pushes the sig length above the standard packing for r,s,v sigs.

Alternatively, magicBytes can be added as a suffix. Then, thanks to v having a limited set of values, the magicBytes can be crafted in such a way that an ecrecover signature will never end in those magicBytes

Verifier side

Off-chain

Full signature verification will be performed in the following order:

An example of this verification flow (except the first, counterfactual step that I'm proposing here) can be seen here: https://github.com/ambireTech/signature-validator/

On-chain

This is implementation specific, but I'd argue that in 99% of the cases needing to verify a sig onchain will happen after the wallet has been deployed. The opposite isn't true though, because many dapps now require signatures to accept terms of use, etc.

That said, the principle is the same as the off-chain verification.

In any case, I think this is definitely out of scope of EIP 4337, as signatures can get extremely complicated quite quickly, when you consider the whole matrix of possibilities (prefixed vs unprefixed, EIP 712 for signing typed data, EIP 1271 vs EOA signatures).

Security risks

Calling an arbitrary address create2Factory with arbitrary calldata is normally not dangerous off-chain, but it could be on-chain. But the security risks there are equal to the ones of EIP1271, as EIP1271 also involves an arbitrary call.

Agusx1211 commented 1 year ago

I think this is the best approach, the only thing left to define is how a smart contract wallet can differentiate between a dapp that supports this "advanced verification" and one that doesn't, because the wallet can still do the deployment if that's the only option.

Maybe a new rpc_sign* method?

Ivshti commented 1 year ago

@Agusx1211 so far, from pretty much every mainstream dapp tested, there's only a handful that support EIP1271 off-chain verification at all. In the majority of cases, the problem is "how to find out whether the dapp supports eip1271", and in the minority (afaik only snapshot and guild.xyz) adoption will probably be easy considering that they are generally aware and supportive of eip1271

That said, an RPC method seems to be the most reasonable solution, as there's no universal way to transmit dapp/wallet metadata

Off topic, part of the problem has been that implementing eip1271 is more difficult than it seems - in theory you only need to do a check and a call, in practice you need to compute hash, etc. We've started working on a non-disruptive (as much as possible) PR to ethers v6 to address that, by adding a universal sig verifier method based on signature-validator

Agusx1211 commented 1 year ago

adoption will probably be easy considering that they are generally aware and supportive of eip1271

Makes sense, but wallets could be hesitant to implement it first (because in the meantime, legacy 1271 is broken), and dapps may not integrate it because "no wallet uses it".

Ivshti commented 1 year ago

In all fairness, there's only a handful of SC wallets live on mainnet with adoption so this could be the easier path. As for dapps, this may be speculation but I think implementing a library seems the more likely way to implement 1271 than coding it from scratch, so then it would be up to the library.

drortirosh commented 1 year ago

AFAIK, this is the logic for checking signature (assuming that off-chain you check the prefix and if it has one, decode it into signature/factory/callData

    function checkSignature(address signer, byte32 hash, bytes calldata signature, address factory, bytes calldata callData) external returns (bool) {
        if (signer.code.length==0 && factory != address(0)) {
            (bool success, bytes memory ret) = factory.call(callData);
            require(success && ret.length>=32, "factory create failed");
            (address createdAddress) = abi.decode(ret, (address));
            require(createdAddress == signer, "signer doesn't match created account");
        }

        return SignatureChecker.isValidSignatureNow(signer, hash, signature);
    }

This logic works for both EOA and contracts

(note: OZ's SignatureChecker tries to ecrecover first. as I said above, this is potentially dangerous (not today, someday in the future), and slightly less efficient gas-wise.)

yoavw commented 1 year ago

@Ivshti The proposal looks good, and I also think the solution should be an extension of 1271 rather than 4337. It was easier to solve it in EntryPoint as part of 4337, but IMHO it's worth the effort to solve it for all contracts.

Magic-bytes - you're right, either length or a special v value should be enough.

Regarding the signature check order, why check for the counterfactual case before the on-chain contract? If there's already a contract, it should take precedence over both counterfactual check and ecrecover. Maybe the user previously deployed it, then rotated the keys on-chain. The current configuration prevails in this case.

Would be nice to also cover the case where the factory itself doesn't exist yet. E.g. factory address is 0, and factoryData is actually a multicall that deploys the factory and calls it to deploy the SCW. Not a must, but nice to have.

Verifiers will most likely want to eth_call a contract that just gets signature and performs all these checks (including the multicall that creates the SCW if needed). Maybe the proposal should contain the code for such a singleton contract, similarly to the code in EIP-2470. It'll make the life of library maintainers much easier. Whether it's an EIP-1271 signature, a counterfactual EIP-1271, or an ECDSA, their flow is the same - eth_call SignatureVerifier.verify(hash,sig).

Ivshti commented 1 year ago

@yoavw

My initial thought was that checking for magic bytes is really cheap & fast, so it makes sense to start with this, but when I think twice about it, it's a micro-optimization and the more correct flow is to start with the on-chain contract, therefore allowing full freedom for EIP1271 signatures (I mean, including a sig that ends with the magic value). You're also right about the rotation.

The factory case sounds like an unecessary edge case to handle, not to mention that we need to know the factoryDeployer to know the factoryAddress, we need to know whether it's create2 or create, and we need the salt. Unless you mean that factoryData is the calldata passed to the maker multicall contract, but then what happens if multiple contracts get deployed? And implementers also need to go through logs to find deployed contracts.

The singleton proposal is great - it really sounds like the right way to go and I can't think of any immediate drawback. One could argue that the work that SignatureVerifier does can easily be done off-chain, and library implementers would still have a lot of offchain work to do anyway, namely to get to hash, either by doing a prefixed/unprefixed keccak of the input, or an EIP712 digest. With that said, I do like leaving less room for error.

Ivshti commented 1 year ago

A strong argument in favor of the singleton that makes it a no-brainer is that it can also be used to make on-chain verification easier. If the cost of a CALL is too much for a particular use case, they can just use the singleton source code as a library.

yoavw commented 1 year ago

The singleton approach keeps the implementation simple for library maintainers, and doesn't add any cost (in terms of eth_calls). They already have to perform at least one call to support EIP-1271 (isValidSignature), and if they implement support for counterfactual then it becomes two calls (check for codehash, build a complex multicall and eth_call it). With the singleton it's just one eth_call.

Also leaves no room for errors. All libraries implement a canonical flow with strict ordering (onchain > counterfactual > ecrecover).

And then there's the benefit of having the same logic available on-chain as you mentioned.

If a library maintainer wants to use it on a chain where the singleton doesn't exist (or a local hardhat during testing), just use a multicall that deploys the singleton and then follows the same flow.

Ivshti commented 1 year ago

Seems that we've got most things figured out, I am going to start drafting an EIP

Ivshti commented 1 year ago

I wrote this up as an EIP: https://github.com/AmbireTech/EIPs/blob/master/EIPS/eip-6492.md @yoavw @Agusx1211 @derekchiang feedback wil be appreciated :pray:

Ivshti commented 1 year ago

The EIP has been merged https://eips.ethereum.org/EIPS/eip-6492

Next step would be the singleton which is currently in development here https://github.com/AmbireTech/eip-6492/blob/main/contracts/UniversalSigValidator.sol

@yoavw regarding the order, it turns out we have to start with the magicBytes check so as to not invalidate counterfactual sigs after the contractis deployed. Then, we check for contract code and traditional EIP1271, and finally, we do the ecrecover.

Ivshti commented 1 year ago

hey everyone, here's ERC-6492 implemented in an universal library which can verify 6492, 712, 1271 sigs: https://github.com/AmbireTech/signature-validator/pull/3

@yoavw I managed to do it without a singleton, without relying on any predeployed contracts and without special RPC calls. This is thanks to an assembly trick to return value from the constructor of a contract, combined with doing an eth_call to deploy the contract and do the actual verification. Here's how it works: https://github.com/AmbireTech/signature-validator/blob/6492-verification/contracts/DeploylessUniversalSigValidator.sol

drortirosh commented 1 year ago

Cool. We use similar method using revert with a custom error at the end of the constructor. This makes the call usable only in view-mode, and also give some type information to the return value(s) In your case, I just wonder if it worth reporting back the mode of signature you actually return - was it an eoa, deployed contract or undeployed one (you could check it separately, by checking the signature and/or getCode().length, but why repeat the test if we already do it in evm code ?

Ivshti commented 1 year ago

@drortirosh damn, I didn't think of revert, this would negate the need for assembly. I think it's slightly counterintuitive for the caller and may be confusing for whoever wants to understand the system (seeing it returns an error). So I'll keep it the way it is.

Reporting back the mode is not necessary for any bit of code in the verifier, but if the user of the verifier needs it, it may make sense to return it as well and parse the output with abi.decode rather than just do 0x01 ===

Ivshti commented 1 year ago

With the existance of ERC-6492, and a reference implementation, plus a library to verify it should we close this issue? @derekchiang @Agusx1211 @yoavw

yoavw commented 1 year ago

@Ivshti I replied in the ethereum-magicians thread. Sorry again for taking so long to reply.

yoavw commented 1 year ago

@Ivshti I replied in the ethereum-magicians thread. Sorry again for taking so long to reply.

yoavw commented 1 year ago

@Ivshti I replied in the ethereum-magicians thread. Sorry again for taking so long to reply.

Ivshti commented 1 year ago

@yoavw no worries, you misunderstood the EIP though. The issue of validating against the original auth rules of the wallet is not even possible, because in case it's already deployed, we cannot "roll it back" to it's original version. We will always call the live, deployed version, if it exists, therefore taking key rotation into account.

GavinXu520 commented 1 year ago

Great!

z0r0z commented 11 months ago

late to the game, but nice work @Ivshti 👌