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?