Validate contract signatures against the verifying Safe's EIP-1271 convention - #2726
Validate contract signatures against the verifying Safe's EIP-1271 convention#2726Uxío (Uxio0) wants to merge 2 commits into
Conversation
…nvention `SafeSignatureContract.is_valid` tried both EIP-1271 entrypoints and returned True if either answered. On-chain only one is ever called, and the version of the Safe doing the verifying decides which: below 1.5.0 `checkNSignatures` calls the legacy `isValidSignature(bytes,bytes)` with the preimage, from 1.5.0 `checkContractSignature` calls `isValidSignature(bytes32,bytes)` with the hash, and the 1.5.0 CompatibilityFallbackHandler no longer declares the legacy overload at all. So a signature could be valid here and still revert GS024 on the Safe that has to verify it. Add `safe_version` to `is_valid` so only the entrypoint that version calls is checked. It goes on the abstract `SafeSignature` / `SafeSignatureAsync` and on every implementation, the same way `safe_address` already works: validation context that only one signature type reads and the rest ignore. `safe_version=None` keeps the old both-entrypoints behaviour, so existing callers are unaffected. Add `uses_bytes32_eip1271()` as the single home for the version rule and for its failure mode. `VERSION()` is an arbitrary on-chain string, so it raises `CannotCheckEIP1271ContractSignature` instead of leaking packaging's `InvalidVersion` and making every consumer reimplement the translation.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4a7e5212f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
`_check_eip1271` accepted either magic value whichever entrypoint was called. The two interfaces declare different ones and the Safe compares against its own, exactly: `ISignatureValidator.EIP1271_MAGIC_VALUE` is 0x20c13b0b up to 1.4.1 and 0x1626ba7e from 1.5.0. Every Safe fallback handler returns the value matching the entrypoint, so no Safe signer hits this. An owner can be any EIP-1271 contract though, and one that answers the other interface's value was reported valid here and then reverted GS024 on the verifying Safe. Pass the expected value per entrypoint and compare against it alone, sync and async.
|
Good catch, and it holds. Checked the constants in
Both are named Worth noting no Safe signer can hit this: the v1.4.1 handler returns Fixed in f85e4eb: On testing it: the existing suite already covers the pairing being right, since a crossed pairing would fail every contract-signature test at 1.4.1 or 1.5.0. Reaching the rejection needs a signer no Safe can be, so there is a stubbed fallback handler returning the legacy value from the |
1 participant
Validate contract signatures against the verifying Safe's EIP-1271 convention
Problem
SafeSignatureContract.is_validtries both EIP-1271 entrypoints and returnsTrueif either answers:isValidSignature(bytes32,bytes)withsafe_hashisValidSignature(bytes,bytes)withsafe_hash_preimageOn-chain only one is ever called, and the version of the Safe doing the verifying decides which:
checkNSignaturescalls the legacyisValidSignature(data, contractSignature)with the whole preimage (Safe.sol:315in v1.4.1,GnosisSafe.sol:285in v1.3.0 — identical branches, the only v1.4.1 addition is theGS027preimage assertion).checkContractSignaturecallsisValidSignature(bytes32 dataHash, ...)only (Safe.sol:270), and itsCompatibilityFallbackHandlerno longer declares the legacy overload at all.So a signature can be "valid" here and still revert
GS024on the Safe that has to verify it. Consumers that store it end up below threshold with nothing reported at write time.What this changes
safe_versiononis_valid. Only the entrypoint that version calls on-chain is checked. It goes on the abstractSafeSignature/SafeSignatureAsyncand on all ten implementations, the same waysafe_addressalready works: validation-time context that only one signature type reads and the rest ignore.safe_version=Nonekeeps the current behaviour. Existing callers are unaffected — the library cannot always know the verifying Safe's version, so the permissive default stays. Callers that do know should pass it.uses_bytes32_eip1271()is the single home for the version rule and for its failure mode.VERSION()is an arbitrary on-chain string, so it raisesCannotCheckEIP1271ContractSignature(an existing, until now unused,SafeSignatureExceptionsubclass) instead of leakingpackaging'sInvalidVersionand making every consumer reimplement the same translation.The cross-version case comes out for free
A verifying Safe below 1.5.0 with an owner Safe on 1.5.0 can never work: the verifying Safe calls the legacy overload, which a 1.5.0 signer does not declare. Today
is_validaccepts it through attempt 1, which is a permanent false accept. Keying only off the verifying Safe's version rejects it with no extra logic, because we call the entrypoint that will actually be used and it reverts.(bytes preimage, sig)(bytes preimage, sig), overload missing(bytes32 hash, sig)(bytes32 hash, sig)Tests
TestSafeContractSignatureandTestSafeContractSignatureAsynccover v1.3.0, v1.4.1 and v1.5.0 against the deployed contracts:safe_version=Nonestill accepts both on a pre-1.5.0 signer (back-compat guard)277 passed, 12 skippedinsafe_eth/safe/tests/.Notes for the reviewer
is_valid, so the change is additive to the public API.mypypre-commit hook islanguage: systemand fails locally withExecutable mypy not found. Ran separately with the hook's flags: both changed files clean. The one error reported,safe_eth/eth/tests/utils.py:56, is pre-existing in a file this PR does not touch.Consumed by safe-global/safe-queue-service PLA-1907, which needs a release of this.