Right, they seem to match. the only problem was the comment that yoav described.
About the code in the ERC: it isn’t executable as it is, at least for small issue: it truncate to length-4 instead be length-32.
Maybe should use the actual tested code from the supporting library - or change the code to a “pseudo-python code”, which is usually shorter, and never expected to be executable…
@yoavw @dror did you guys check my explanation? The issue described by Yoav cannot possibly happen, as evident by the implementation. The text wasn’t clear enough in that regard, but I updated it now: Update EIP-6492: Signature Validation for Predeploy Contracts · AmbireTech/EIPs@6e5b593 · GitHub
@dror can you point to the specific place in the code?I did compile it and test the bytecode with the signature-validation library, so it should be executable
This is a very neat approach! Especially happy to see that off-chain validation was considered and included in the proposal.
It seems like there may be a small bug with signature encoding:
If the contract is not deployed yet, wrap the signature as follows:
concat(abi.encode((create2Factory, factoryCalldata, originalERC1271Signature), (address, bytes32, bytes, bytes, bytes)), magicBytes)
The arguments to abi.encode look like they should be encoded using (address, bytes, bytes32, bytes) instead of (address, bytes32, bytes, bytes, bytes).
Thanks!
It seems like there may be a small bug with signature encoding:
fixed
. The arguments to abi.encode look like they should be encoded using (address, bytes, bytes32, bytes) instead of (address, bytes32, bytes, bytes, bytes).
also fixed
Thanks for the effort! I wonder if this could be combined with EIP-2126. Feels a bit messy to just have magic bytes for every special case (guess more will come up in the future) - I think a “proxy EIP” that contains all the cases that could appear when signing could make it cleaner and easier for implementers.
Yes, the singleton code handles it correctly. The text and the comment in the version I reviewed made it seem otherwise:
// - ERC-6492 suffix check and verification first, while being permissive in case the contract is already deployed so as to not invalidate old sigs
but I see it was clarified now. Looks good now.
There’s still an additional risk due to the multi-chain use case. The verifier doesn’t know which chain the contract may have been deployed on. It’s possible that the user deployed the contract on Polygon, rotated the keys there, but the verifier is running the EIP-6492 signature check against an Ethereum node where the contract hasn’t been deployed. The counterfactual check takes precedence since there’s no deployed contract, so the old key is accepted.
Would it make sense to extend ERC-1271 with another interface, homeChainId(), which is always implemented as uint256 public immutable homeChainId set by the constructor? The EIP-6492 singleton could then require(IERC6492Wallet(_signer).homeChainId()==chainId(), "verifying on the wrong chain").
It doesn’t affect normal ERC-1271 checks for deployed contracts, but ensures that counterfactual checks are performed against the chain where the user intends to deploy the account first. And in the counterfactual signature case, it’ll ensure that if the user rotated keys on the intended chain, the verifier will not mistakenly accept the old keys.
In fact, the same risk exists in EIP-1271 regardless of the counterfactual extension. An attacker who gained access to the old keys that have been revoked, could always deploy the same initcode on a chain where it hasn’t been deployed, and trick verifiers that use that chain. It makes EIP-1271 more vulnerable in a multi-chain world. By using the EIP-6492 singleton to verify signatures (including the homeChainId() check), the verifier can mitigate this risk as well. It would make EIP-6492 the safe way to check for signatures since we already live in this multi-chain world.
@Ivshti do you think it makes sense to add this check to the EIP?
Can’t we just have a new function for pure off-chain validation (e.g., isOffchainValidSignature(), isValidSignatureOffchain()) instead of extending ERC-1271 isValidSignature?
Because extending isValidSignature would complicate relayer’s work since the relayer is the one carrying the transaction onchain and paying for the gas. If the relayer now also possibly has to pay for user’s contract deployment cost, it would become tricky for the relayer to assess this extra cost and even reason about this extra cost to user. Furthermore, malicious user can fail the deployment when the transaction is executed onchain and the relayer has to bear the cost. This basically enforce the same risk of 4337 bundler on all the other dapp relayers.
IMO, user should pay for his own deployment cost.
isOffchainValidSignature(), isValidSignatureOffchain()) offchainI think you’ve misunderstood, even though the call is done on the chain’s state, it’s not paid for - it’s free, as it’s called off-chain through eth_call.
hey @Ivshti I really like this spec, except it doesn’t work for our use case, which doesn’t allow the deployment of the wallet without a ‘secret’.
Any thoughts on adding support for deferring the signature check to the createFactory instead?
can you elaborate on your use case? In general every smart contract wallet should be supported as you can encode whatever you need in the deploy calldata or the signature
Except in our case we don’t want to expose the information necessary to deploy the smart contract as it can be sniffed/stolen prior to the person actually wanting to deploy the smart contract. It’s a signature gated deployment mechanism. Hope that makes sense.
If the deployment information is secret and can be abused, then you’re in a serious problem, as someone could front-run your real deployment transaction…
The idea with counterfactual deployment is that even if someone does front-run you and deploy your contract, he gains nothing, except saving you the gas cost of this deployment, since the deployed contact should be “bound” to your credentials. To my mind, any other scheme is a vulnerability in your model.
It can’t be front run because the msg sender information is included in the signed message for validation.
Or, more accurately, front running the transaction isn’t problematic in our use case because the ownership information is encoded into the signature validation.
And the signature validation to create the account is not known beforehand.
Submitting an eip today for this use case so hopefully it’s more clear.
That’s not how Ethereum works - once you put the transaction into the mempool to have it mined, anyone else can use that “secret” data (read: it’s not secret) to do the same. So you cannot have “secrets” really on-chain.
The correct way to implement smart contract wallets (AA accounts) is in such a way that the deploy data can be used by anyone to deploy the contract, but the contract can only be controlled by a it’s original designated owner. There’s very few ways to do it differently anyway (presuming you’re using CREATE2), unless you’re actually relying on msg.sender, which is the wrong approach - rather, you should be encoding the owner of the account in the deploy code itself - you can do this very easily via constructor arguments rather than relying on msg.sender
Protected against front-running means you also don’t care if someone else creates your account for you, but running this initialization code on-chain.
So there is no problem if the creation code is passed as part of the signature validation - the “secret” part of the creation code is not really a secret but “owner-specific information”, which is good.
So I don’t understand why you say:
I don’t think we’re on the same page.
I agree and am aware of all your points.
Once a signature is exposed, anyone can see it. That’s fine.
Here’s a more complete description of how our factory account creation works:
It accepts the salt for the wallet being created (which is used to compute the counterfactual addresss), the initial controlling owner of this wallet, and a signature/message that is used to validate (via ecdsa or 1271) that the owner can actually be assigned to this salt/counterfactual address. Note that the message is a keccak256 of the salt/owner/optional msg.sender.
The signature/message is validated against a signing key set within the factory.
There is no harm for someone to front run this transaction once this information is exposed. Furthermore, there is the option to enforce that the msg.sender is a specific address as well by adding it to the signature/message.
In our case, the owner of the counterfactual wallet is not known ahead of time, and signatures are not generated until the owner address is assigned to a salt.
Please explain to me how the counterfactual address can be secured against front-running, if the owner is not part of it. The idea of counterfactual address is that I (as an owner) knows that I control the contract address and can move assets to it, knowing that I’m the only one who will be able to own that contract. If the address of the owner is not known in advance, then there is some entity that can arbitrarily create this account and assign it to a different owner, and the owner trusts this entity not to do so.
I see where you’re coming from, so maybe it’s out of scope for EIP-6492.
Here is our EIP, that hopefully makes our use case more clear:
It permits the assignment of counterfactual addresses for a given salt, however, the ‘ownership’ of that salt/account is not set initially. The future ownership assignment is governed by the factory.
So, maybe out of scope here, but we were hoping that EIP-6492 could cover the case where signature validity was delegated to the contract creation factory, which would have it’s own signature validation logic for non-web3 owner assigned and non-deployed accounts
are you sure this is the right way to do things? Anything that relies on “context” for authentication with create2 may be fundamentally insecure, because the deploy data will be leaked in the mempool.
You can get around this, and I think you did, by having the factory govern this, but it seems like a convoluted approach that will break many other things in the future.
The goal of wrapping AA wallets as NFTs can be achieved via an additional manager+721 contract, as seen here: https://twitter.com/Ivshti/status/1653982135085068290