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

Counterfactual ERC-1271 signature validation #188

Closed
derekchiang opened this issue Jan 28, 2023 · 39 comments
Closed

Counterfactual ERC-1271 signature validation #188

derekchiang opened this issue Jan 28, 2023 · 39 comments

Comments

@derekchiang
Copy link
Contributor

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
Copy link

hazim-j commented Jan 28, 2023

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
Copy link
Contributor

yoavw commented Jan 28, 2023

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

  • For accounts that don't exist, ask the user to provide a call that deploys the account. E.g. a call to EntryPoint.handleOps with a UserOp containing the account's initCode. That is, in addition to the (account,hash,sig) tuple needed for EIP-1271.
  • Verifier creates a multicall, the first call being the user-provided call, and the 2nd one deploys a contract whose constructor calls require(<account>.isValidSignature(hash,sig) == 0x1626ba7e)
  • Verifier uses eth_call to run the multicall locally and checks that it doesn't revert.

Pros:

  • Can be used with any deployment method. Not specific to ERC-4337.
  • Can be abstracted as a singleton contract that everyone eth_call's. Implement an isValidSignatureCounterfactual(account,deploymentCall,sig.hash) which acts as a normal isValidSignature if the account has code, and implements the above multicall if it doesn't.

Cons:

  • Bad UX for 4337 wallets: The user needs to sign twice. Once for the deployment call to work (when the account is deployed and attempts to validate its own deployment), and once for the message itself.

2. Bundler RPC solution

  • Bundlers implements a new eth_validateERC1271Signature(account,initCode,sig,hash) RPC.
  • If the account has code, just call <account>.isValidSignature(hash,sig).
  • If it doesn't, create a UserOp with {sender=account, initCode=initCode, callData=account.isValidSignature(hash,sig)}.
  • Trace simulateHandleOp(UserOp) similarly to how eth_estimateUserOperationGas is implemented. The traced simulation disregards the SIG_VALIDATION_FAILED so the UserOp doesn't need to be signed.
  • If the trace reaches a call to <account>.isValidSignature(hash,sig) and it returns 0x1626ba7e, return true.
  • If the trace terminates without reaching that condition, return false.

Pros:

  • No need to sign twice. The user only signs the message itself, and provides the initCode.
  • No need to construct any fancy transactions at the verifier. Just call a bundler.

Cons:

  • Verification requires a trusted bundler. May or may not be an issue, depending on the trust model. If the verifier was already ok with trusting a 3rd party node to eth_call <account>.isValidSignature(hash,sig), then it may be comfortable with trusting a bundler. Or it could run a local bundler just for performing verifications without trusting a 3rd party RPC.

3. Account solution

  • Same as the first solution (Verifier side solution) but no double-signing. An ERC-4337 account could bypass it through its validateUserOp.
  • In validateUserOp, check if calldata is <account>.isValidSignature(hash,sig).
  • If it is, skip the normal validation flow, and instead require(<account>.isValidSignature(hash,sig) == 0x1626ba7e). If it passes, return ok. Don't pay any eth to EntryPoint (since the account doesn't have any), assume gas cost 0. It means that validation will succeed for the deployment, but only if the call itself is just a self-call to isValidateSignature.
  • Verifier calls a singleton contract, similar to the one above in "Verifier side solution", but deploymentCall is replaced with a call to EntryPoint.handleOps with a UserOp with {sender=account, initCode=initCode, callData=account.isValidSignature(hash,sig), maxFeePerGas=0, maxPriorityFeePerGas=0}.
  • Unlike the "Bundler RPC solution" case above, validation succeeds so no tracing is needed. The eth_call to multicall succeeds. During this multicall, isValidateSignature is actually called twice. Once during deployment (to approve the deployment), and once for the real verification. Doesn't matter because it's off-chain.

Pros:

  • Works without requiring changes in the bundler (or trusting it), and without having the user sign twice.

Cons:

  • Requires the account itself to support this flow by accepting validation if callData is an isValidateSignature with a valid signature. Maybe we could make this transparent by creating a subclass of BaseAccount that implements EIP-1271 and a validateUserOp that skips _validateSignature if callData is isValidateSignature and uses it instead.

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.

Pros:

  • The simplest flow for everyone. No bundler modifications, no double-signing, no account changes. The verifier just calls EntryPoint.validateERC1271Signature with the user-provided (account,initCode,sig,hash) and gets the result. As easy as verifying isValidSignature for a deployed contract.

Cons:

  • Requires a change to EntryPoint. But a safe one because it's a function that always reverts.

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

@derekchiang
Copy link
Contributor Author

@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
Copy link
Contributor

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
Copy link
Contributor

yoavw commented Jan 30, 2023

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
Copy link
Contributor Author

derekchiang commented Jan 30, 2023

@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 (typically fetched through the injected Web3 provider)
  • Then ask the user to sign a message to prove that they own that address.

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
Copy link
Contributor

yoavw commented Jan 30, 2023 via email

@derekchiang
Copy link
Contributor Author

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
Copy link
Contributor

yoavw commented Jan 30, 2023 via email

@derekchiang
Copy link
Contributor Author

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.

@yoavw
Copy link
Contributor

yoavw commented Jan 30, 2023 via email

@derekchiang
Copy link
Contributor Author

derekchiang commented Jan 31, 2023 via email

@ch4r10t33r
Copy link

ch4r10t33r commented Feb 1, 2023

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
Copy link

Ivshti commented Feb 1, 2023

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
Copy link

Ivshti commented Feb 1, 2023

@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 “<init_code_len><init_code>”. 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
Copy link
Contributor

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
Copy link

Ivshti commented Feb 1, 2023

@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

  • If the wallet is deployed, produce a normal EIP1271 signature as defined by the particular wallet
  • If the wallet is not deployed yet, wrap the signature as follows: concat(magicBytes, abi.encodePacked((create2Factory, factoryCalldata, originalEIP1271Signature), (address, bytes32, bytes, bytes, bytes)))

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:

  • check if the signature starts with magic bytes, in which case do an eth_call to a multicall contract that will call the factory first, with the factoryCalldata; Call <address>.isValidSignature as usual
  • check if there's contract code at the address. If so perform EIP1271 verification as usual
  • If all this fails, try ecrecover verification

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
Copy link

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
Copy link

Ivshti commented Feb 1, 2023

@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
Copy link

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
Copy link

Ivshti commented Feb 1, 2023

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
Copy link
Contributor

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
Copy link
Contributor

yoavw commented Feb 1, 2023

@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
Copy link

Ivshti commented Feb 1, 2023

@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
Copy link

Ivshti commented Feb 1, 2023

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
Copy link
Contributor

yoavw commented Feb 1, 2023

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
Copy link

Ivshti commented Feb 7, 2023

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

@Ivshti
Copy link

Ivshti commented Feb 10, 2023

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 🙏

@Ivshti
Copy link

Ivshti commented Mar 3, 2023

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
Copy link

Ivshti commented Mar 8, 2023

hey everyone, here's ERC-6492 implemented in an universal library which can verify 6492, 712, 1271 sigs: AmbireTech/signature-validator#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
Copy link
Contributor

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
Copy link

Ivshti commented Mar 8, 2023

@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
Copy link

Ivshti commented Mar 8, 2023

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
Copy link
Contributor

yoavw commented Mar 11, 2023

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

2 similar comments
@yoavw
Copy link
Contributor

yoavw commented Mar 11, 2023

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

@yoavw
Copy link
Contributor

yoavw commented Mar 11, 2023

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

@Ivshti
Copy link

Ivshti commented Mar 11, 2023

@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
Copy link

Great!

@z0r0z
Copy link

z0r0z commented Oct 16, 2023

late to the game, but nice work @Ivshti 👌

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

No branches or pull requests

9 participants