fix(EIP-1271) - change function parameter type and selector#29
Conversation
…y (bytes,bytes) The ABI defined isValidSignature(bytes,bytes) with selector 0x20c13b0b instead of the standard isValidSignature(bytes32,bytes) with selector 0x1626ba7e. This meant smart contract wallets implementing standard EIP-1271 could not authorize reshare or resign operations.
iurii-ssv
left a comment
iurii-ssv
left a comment
There was a problem hiding this comment.
Looks reasonable, but we need a review from repo maintainer(s) to merge
|
is it related to #26 @oleg-ssvlabs? |
seems to be related. I did not see that PR. |
momosh-ssv
left a comment
momosh-ssv
left a comment
There was a problem hiding this comment.
Pre-existing: the Alchemy API key in crypto/signature_test.go:18 is hardcoded. The multisig tests make live Sepolia calls and will fail if the key is rotated or rate-limited. Might be worth moving to an env var.
nkryuchkov
left a comment
nkryuchkov
left a comment
There was a problem hiding this comment.
The code LGTM, but I don't have domain knowledge on removing MAGIC_VALUE_ETH_SIGN. Can we add to the PR description why we are removing it?
I can relate to this, and totally agree here... |
This reverts commit 2a177ae.
oleg-ssvlabs
dismissed stale reviews from ljuba-ssv, iurii-ssv, and nkryuchkov
via
a741120
oleg-ssvlabs
requested review from
iurii-ssv,
ljuba-ssv,
momosh-ssv and
nkryuchkov
nkryuchkov
left a comment
nkryuchkov
left a comment
There was a problem hiding this comment.
Can you please write a few words in the PR description about why we used to check for MAGIC_VALUE_PERSONAL_SIGN or MAGIC_VALUE_ETH_SIGN, but now it's only MAGIC_VALUE?
good point. Updated PR description |
nkryuchkov
left a comment
nkryuchkov
left a comment
There was a problem hiding this comment.
Great explanation, thanks
8 participants
Summary
isValidSignature(bytes32,bytes)interface instead of legacyisValidSignature(bytes,bytes)0x20c13b0bto0x1626ba7e)[32]bytedirectly and check only the standard magic valueContext
Smart contract wallets (Gnosis Safe, etc.) implementing standard EIP-1271 did not match the legacy selector, causing reshare and resign operations with multisig owners to fail.
Why there were two magic values
In EIP-1271, when a contract's
isValidSignatureconfirms a valid signature, it returns its own 4-byte function selector as the success indicator. Since a Solidity function selector iskeccak256("functionName(argTypes)")[:4], two different interface versions produce two different magic values:keccak256("isValidSignature(bytes32,bytes)")[:4]=0x1626ba7e(finalized EIP-1271)keccak256("isValidSignature(bytes,bytes)")[:4]=0x20c13b0b(legacy draft)The old code stored both selectors as magic values (mislabeled — the finalized
0x1626ba7ewas calledMAGIC_VALUE_ETH_SIGNand the legacy draft0x20c13b0bwas calledMAGIC_VALUE_PERSONAL_SIGN) to handle contracts implementing either version. But since the ABI was actually using the legacy(bytes,bytes)interface, only0x20c13b0bcould ever be returned — making the other magic value dead code. Now that the ABI is fixed to the standard(bytes32,bytes), only0x1626ba7ecan be returned, so we only need one.Closes #25
Closes #26