Use Safe as eth_call sender for EIP-1271 contract signature validation - #2577
Open
johanneskares wants to merge 2 commits into
Open
Use Safe as eth_call sender for EIP-1271 contract signature validation#2577johanneskares wants to merge 2 commits into
johanneskares wants to merge 2 commits into
Conversation
On-chain, checkSignatures has the Safe itself call isValidSignature on its owner contracts, so msg.sender is always the Safe. The off-chain check performed a from-less eth_call (msg.sender = address(0)), which diverges from on-chain semantics and makes signatures from msg.sender-dependent owners like the SafeWebAuthnSharedSigner always fail validation. SafeSignatureContract.is_valid already receives safe_address (other signature types use it); thread it into the EIP-1271 eth_call as the sender when provided. Signers that ignore msg.sender are unaffected.
Contributor
|
All contributors have signed the CLA ✍️ ✅ |
This comment has been minimized.
This comment has been minimized.
Author
|
I have read the CLA Document and I hereby sign the CLA |
1 similar comment
Author
|
I have read the CLA Document and I hereby sign the CLA |
Author
|
recheck |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Fixes the root cause behind safe-global/safe-transaction-service#2937.
On-chain,
checkSignatureshas the Safe itself callisValidSignatureon its owner contracts, somsg.senderis always the Safe. The off-chain check inSafeSignatureContract._check_eip1271performs a from-lesseth_call(msg.sender = address(0)), which diverges from the on-chain semantics it is meant to predict.For most owner contracts this makes no difference, but signatures from
msg.sender-dependent owners always fail validation. The canonical example is theSafeWebAuthnSharedSignerfrom safe-modules (the standard gas-optimized ERC-4337 passkey setup): it stores each Safe's P-256 public key in the Safe's storage and reads it back viaISafe(msg.sender).getStorageAt(...)— in a from-less call there is no configuration to find, so pure-passkey Safes cannot propose transactions or register delegates with the Transaction Service at all.Change
SafeSignatureContract.is_valid()(sync and async) already receivessafe_address— other signature types likeSafeSignatureApprovedHashuse it — but the contract-signature path ignored it. This PR threads it into the EIP-1271eth_callas the sender when provided:safe_addressgiven →isValidSignatureis called withfrom = safe_address, mirroring on-chaincheckSignatures.safe_addressnot given → unchanged from-less call.Owner contracts that ignore
msg.sender(e.g. a Safe validating through its fallback handler) are unaffected.Tests
Added sync + async tests deploying a minimal EIP-1271 signer that returns the magic value only when
msg.sendermatches a configured address (emulating the shared signer's behavior), asserting that validation fails withoutsafe_addressand succeeds with it. All existing tests intest_safe_signature.pypass (38 passed locally against a local node).Notes for safe-transaction-service
The service's proposal endpoints already pass
safe_addresstois_valid, so they gain shared-signer support with a dependency bump and no code change. The delegates v2 endpoint currently passes the delegator as second argument (is_valid(ethereum_client, owner)); supporting shared-signer delegators there needs a small follow-up passing the request'ssafe— happy to submit that PR as well.