fix: Safe WebAuthn owner matching, init-signature sorting, and estimation-path input handling - #216
Conversation
…gets encoded During account init the signature encoder substitutes the WebAuthn shared-signer address for a WebauthnPublicKey owner, but sortSignatures keyed on the per-owner verifier-proxy address. Whenever the two sort differently relative to the other owners (~50% of key pairs) the encoded signatures were not in ascending owner order, so the Safe rejected the init UserOperation with GS026 — after gas estimation had succeeded, since the dummy-signature path resolves the shared signer before sorting. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…sitively createWebAuthnSignerVerifierAddress returned a lowercase address while getOwners() returns checksummed ones, so the indexOf lookups in createSwapOwnerMetaTransactions/createRemoveOwnerMetaTransaction never matched a WebAuthn owner and always threw 'not a current owner'. Checksum the derived address (matching createProxyAddress) and make the owner lookups case-insensitive so lowercase EOA inputs work too. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…agic casing - user-supplied dummySignerSignaturePairs containing raw WebauthnPublicKey signers threw 'Must define isInit parameter' with no way to supply the flag through the overrides API; the init flag is now derived from the factory presence and passed to the signature formatter, matching the expectedSigners path. - the parallelPaymasterInitValues PAYMASTER_SIG_MAGIC check and its entrypoint-v0.9 gate compared case-sensitively, spuriously rejecting uppercase hex paymasterData (which the EIP-712 trimmer and the MultiChain subclass accept) and lowercase entrypoint addresses. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…just casing Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughSafeAccount now propagates initialization state into WebAuthn dummy-signature encoding, normalizes verifier and address comparisons, and adds regression coverage for initialization, deployed, and overridden WebAuthn signer flows. ChangesSafeAccount behavior updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SafeAccount
participant SignatureFormatter
participant WebAuthnSigner
participant Bundler
SafeAccount->>SafeAccount: derive isInit from factory fields
SafeAccount->>SignatureFormatter: format dummy signature with WebAuthn overrides
SignatureFormatter->>WebAuthnSigner: resolve shared or verifier-proxy owner
WebAuthnSigner-->>SignatureFormatter: return owner address
SignatureFormatter-->>SafeAccount: encode userOperation.signature
SafeAccount->>Bundler: submit gas-estimation request
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
| webAuthnSharedSigner, | ||
| // needed when user-supplied dummySignerSignaturePairs contain raw | ||
| // WebauthnPublicKey signers — the encoder requires the init flag | ||
| isInit, |
There was a problem hiding this comment.
For isInit: false, it also needs the verifier, factory, singleton, and proxy creation code values. Without them, it derives the owner using defaults rather than the caller’s custom configuration
The previous fix covered createUserOperation's formatter call only, and only passed the init flag and shared signer: - baseEstimateUserOperationGas still formatted user-supplied dummySignerSignaturePairs without isInit, so raw WebauthnPublicKey signers threw 'Must define isInit parameter' for direct callers. The init flag is now derived from the operation's own initCode/factory field (as the expectedSigners branch already did) and passed along with the WebAuthn verifier config to both formatter calls. - createUserOperation's formatter call omitted the five verifier-config overrides, so deployed accounts (isInit=false) with a custom WebAuthn verifier setup derived the owner address from the DEFAULT_* constants — wrong owner, wrong sort order. The in-scope config values are now forwarded. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes for WebAuthn owner handling in
SafeAccount.WebauthnPublicKeyowner, butsortSignatureskeyed on the per-owner verifier-proxy address. Whenever the two sorted differently relative to the other owners, the encoded signatures were out of ascending owner order and the Safe rejected the op — after gas estimation had already succeeded. Sorting now keys on the address that actually gets encoded.createWebAuthnSignerVerifierAddressreturned a lowercase address whilegetOwners()returns checksummed ones, so the lookups increateSwapOwnerMetaTransactions/createRemoveOwnerMetaTransactionalways threw "not a current owner". The derived address is now checksummed and owner lookups are case-insensitive (lowercase EOA inputs work too).dummySignerSignaturePairscontaining rawWebauthnPublicKeysigners threw "Must define isInit parameter" with no way to supply the flag through the overrides API — in bothcreateUserOperationand the publicbaseEstimateUserOperationGas. The init flag is now derived from the operation’s own init fields at both call sites, and the WebAuthn verifier-config overrides are forwarded to the signature encoder so deployed accounts with custom verifier setups encode the correct owner address instead of one derived from the defaults. ThePAYMASTER_SIG_MAGICcheck and its entrypoint-v0.9 gate also compared case-sensitively, spuriously rejecting uppercase hexpaymasterDataand lowercase entrypoint addresses.getSignerLowerCaseAddressderives owner addresses (WebAuthn → verifier address), not just lowercases input.Tests added in
test/safe/webauthnInitSignatureSort.test.jsandtest/safe/webauthnDummyPairsEstimation.test.js; full offline suite passes.Summary by CodeRabbit