Fix/audit findings - #215
Closed
sherifahmed990 wants to merge 22 commits into
Closed
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>
…isled on-chain createOneTimeAllowanceMetaTransaction passed startAfterInMinutes as the contract's resetBaseMin with resetTimeMin=0, which reverts on-chain for any nonzero value (division by zero in the period alignment) — so a delayed one-time allowance never worked. The parameter is removed. createRecurringAllowanceMetaTransaction documented its last parameter as a relative activation delay, but on-chain resetBaseMin is an absolute epoch-minutes baseline that must be in the past and only aligns period boundaries; a relative value made funds spendable immediately. The parameter is renamed to periodStartBaseInMinutes with truthful docs and a client-side guard against future baselines. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
createAndSignLegacyRawTransaction encoded r and s via getBytes on the 32-byte zero-padded hex from the signer, so any signature whose r or s has a leading zero byte (~0.8% of transactions, deterministic per key/payload under RFC 6979) produced a raw transaction that nodes reject with 'rlp: non-canonical integer (leading zero bytes)'. Encode both through bigintToBytes like the EIP-7702 path already does. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…cision - v0.6 initCode was split at 22 chars instead of 42, truncating the factory address to 10 bytes and leaking its second half into an unprefixed factoryData — every v0.6 deployment simulation was garbage. - the EIP-7702 factory sentinel check only matched the short '0x7702' form; the 20-byte right-padded form accepted by EntryPoint v0.8 fell through, skipping the delegation code override (or triggering a bogus deployment simulation in the callData path). - callTenderlySimulateBundle typed value/gasPrice as number, silently rounding wei amounts above 2^53; bigint is now accepted and serialized as a decimal string. - stateOverrides passed by the caller were rewritten in place (stateDiff renamed to storage); the conversion now works on a copy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…Admin bit packKeySettings shifted expiration into bits 160+ without validating it fits the contract's uint40 field. A millisecond timestamp (>= 2^40) overflowed exactly into bit 200 — the isAdmin flag — registering the key as admin and bypassing the explicit guardrail in createRegisterKeyMetaTransactions. Both expiration and hook are now range-checked with a hint about seconds vs milliseconds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CandidePaymaster compared the entrypoint against the checksummed ENTRYPOINT_V6 constant with strict equality, so a lowercase entrypoint (accepted everywhere else, and lowercased in the sibling Erc7677 comparison) silently skipped the 40k v0.6 paymaster verification buffer, under-provisioning verificationGasLimit once real paymasterAndData replaced the dummy. Both paymasters also added the buffer on top of an explicit overrides.verificationGasLimit, which is documented to be used verbatim — making it impossible to pin the field (e.g. to reproduce a previously signed op). The buffer now only applies to estimated values. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
createPaymasterUserOperation wrote the paymaster fields and a zeroed preVerificationGas directly onto the caller-owned object before calling the bundler; if estimation threw, the caller's op was left with preVerificationGas=0 and stale WorldId paymaster fields, poisoning any retry down another path. Operate on a shallow copy like the other paymasters (which document 'not mutated') and return the copy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Only blockNumber was converted; the returned userOperation kept the bundler's wire-format hex strings in fields the declared type promises as bigint (nonce, gas limits, fees), so comparisons like nonce === 0n were silently always false and bigint arithmetic threw. Convert the numeric fields like getUserOperationReceipt and estimateUserOperationGas already do. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…transient errors The poll loop counted only accumulated sleep, so RPC latency stretched the promised timeout unboundedly (180s became ~455s with a 3s-per-call bundler); the <= comparison ran one extra iteration; the delay preceded the first poll so an already-available receipt still cost a full interval; and a single transient RPC error aborted the poll even though the op could land moments later. Poll immediately, compare elapsed wall-clock time, clamp the final sleep to the remaining budget, and keep polling through errors — surfacing the last one as the TIMEOUT cause. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ling Three related failure-masking defects: - BaseRpcTransport.parseResponse used a key-presence check, so a successful response carrying 'error': null (sent by some non-strict servers) crashed destructuring null instead of returning the result. - HttpTransport.send never checked the HTTP status and unconditionally parsed JSON, so a 401/429/502 with an HTML or plain-text body surfaced as an opaque SyntaxError with the status — the actual diagnostic — lost. Non-JSON and non-envelope error bodies now throw a TransportRpcError carrying the status; proper JSON-RPC error envelopes still report the server's error. - sendJsonRpcRequest's URL path had the same gap plus a TypeError when the body had neither result nor error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
getFeeData's fallback ladder dropped a successfully fetched maxPriorityFeePerGas into the 1-gwei-floor branch whenever eth_gasPrice was unsupported, pinning both fees below the chain's actual required tip (and contradicting the documented fallback order). The priority fee is now used, with the existing clamp raising maxFeePerGas to match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…oveToCallData The token pipeline force-cast the smart account and crashed with a raw 'is not a function' TypeError (wrapped as a generic PAYMASTER_ERROR) when the account doesn't implement the method. Check up front and throw a descriptive error. Also correct the Erc7677Context docs: the context object is forwarded to the RPC verbatim — the 'reserved, not forwarded' claim contradicted the tested behavior — and document the paymasterAddress field that was consumed but undocumented. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
EIP-7702 requires s <= n/2 in authorization tuples; nodes silently skip high-s tuples during set-code processing, so a signer callback returning a standard high-s signature produced an authorization that never applied the delegation with no error surfaced. parseRawSignature now converts to the complementary low-s signature (s' = n - s, flipped parity), matching the private-key path which already signs with lowS. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
createRevokeDelegationTransaction fetched delegation status and the tx nonce for accountAddress but signed with eoaPrivateKey unconditionally. With a mismatched key the broadcast transaction's sender is the other EOA, revoking the signer's delegation instead of the account the method checked and reported on. Add the same up-front key/address check the Calibur revoke path already has. 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>
…int8 page cap - SocialRecoveryModule.getGuardians and .nonce reported empty-result errors under the name 'threshold', misdirecting debugging. - getRecoveryRequestEip712Data only accepted a URL string while every sibling method accepts string | Transport | JsonRpcNode. - AllowanceModule.getDelegates with maxNumberOfResults > 255 threw an opaque ABI out-of-bounds error (the module's page size is a uint8); it now fails with a clear RangeError pointing at pagination. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…just casing Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The BundlerErrorCodeDict translated -32601 to INVALID_USEROPERATION_HASH, but nothing assigns it that meaning: ERC-7769 defines no error code for invalid hashes on the lookup methods, and Voltaire returns -32602 (InvalidFields) with 'Missing/invalid userOpHash' — its only -32601 is the standard JSON-RPC method-not-found, which this mapping was shadowing and misdirecting debugging toward hash issues. -32601 now always translates to METHOD_NOT_FOUND; the INVALID_USEROPERATION_HASH code remains in the BundlerErrorCode union for compatibility but is marked deprecated and never produced. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…RROR BundlerErrorCodeDict covered ERC-7769 codes -32500..-32507 but omitted -32508 (paymaster balance insufficient for all mempool UserOperations), which Voltaire's mempool manager actively raises — it surfaced as inner UNKNOWN_ERROR. Bundler-side -32603 internal errors (raised by Voltaire's execution endpoint) likewise fell to UNKNOWN_ERROR because translateBundlerError only consulted the bundler dict; it now falls through to the standard INTERNAL_ERROR name, matching the -32601 METHOD_NOT_FOUND handling. 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 (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis PR normalizes bundler and transport responses, adds account and allowance validation, corrects Safe WebAuthn handling, updates paymaster estimation, improves receipt polling, canonicalizes EIP-7702 signatures, expands Tenderly handling, and adds regression tests. ChangesSDK reliability and validation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils.ts (1)
541-572: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate parsed JSON before checking envelope keys.
A valid JSON scalar or
nullreaches"result" in responseand throws a nativeTypeError, bypassing the intended transport error normalization.Proposed fix
try { response = JSON.parse(responseText) as JsonRpcResponse; } catch { // ... } + if (response == null || typeof response !== "object") { + throw new TransportRpcError( + -32603, + `HTTP ${fetchResult.status} ${fetchResult.statusText}: malformed JSON-RPC response`.trim(), + response, + ); + } if ("result" in response) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils.ts` around lines 541 - 572, Validate that the parsed value assigned to response is a non-null object before checking envelope keys in the JSON parsing flow. For scalar or null payloads, throw the existing TransportRpcError malformed-response error with the HTTP status context; preserve the result, simulation_results, and error handling for valid object responses.
🧹 Nitpick comments (2)
src/utils7702.ts (1)
449-488: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLGTM! Correctly handles both EIP-2098 compact (64-byte) and standard (65-byte) signatures, and the low-s normalization (
s > n/2→s = n - s, flipyParity) is the standard ECDSA malleability-resistant complement, matching the EIP-7702 low-s requirement and the new regression test.Minor nit:
SECP256K1_N/SECP256K1_HALF_Nare declared afterparseRawSignatureuses them (lines 484-487 vs. 449). This is safe in practice — top-levelconstbindings finish initializing before any exported function can be invoked from outside the module — but placing them above the function would read more naturally.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils7702.ts` around lines 449 - 488, Move the SECP256K1_N and SECP256K1_HALF_N constant declarations above parseRawSignature so the constants are defined before the function references them. Preserve their values and the existing signature parsing and low-s normalization behavior.src/account/Safe/SafeAccount.ts (1)
2187-2238: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMinor inconsistency:
getSignerLowerCaseAddressdoesn't guardnullisInitfor WebAuthn signers like its siblings do.
buildSignaturesFromSingerSignaturePairsandcreateDummySignerSignaturePairForExpectedSignersboth throwRangeError("Must define isInit parameter when using WebAuthn")whenoverrides.isInit == nulland a WebAuthn signer is present.getSignerLowerCaseAddressinstead silently falls through to the non-init verifier-proxy branch (else if (overrides.isInit)treatsundefinedas falsy) — line 2211 checks truthiness, not nullishness.Currently this is safe in practice only because
sortSignatures/buildSignaturesFromSingerSignaturePairswill throw downstream before the mismatched sort key can produce a wrong signature. ButgetSignerLowerCaseAddressispublic staticand could be called directly by consumers computing a sort key for other purposes, where they'd silently get the wrong (non-init) address instead of an actionable error.🔧 Suggested fix for consistency
public static getSignerLowerCaseAddress( signer: Signer, overrides: WebAuthnSignatureOverrides = {}, ): string { if (typeof signer === "string") { return signer.toLowerCase(); - } else if (overrides.isInit) { + } else if (overrides.isInit === true) { // on init the encoded owner is the WebAuthn shared signer, not the // per-owner verifier proxy — sort by the same address that gets encoded const webAuthnSharedSigner = overrides.webAuthnSharedSigner ?? SafeAccount.DEFAULT_WEB_AUTHN_SHARED_SIGNER; return webAuthnSharedSigner.toLowerCase(); + } else if (overrides.isInit == null) { + throw new RangeError("Must define isInit parameter when using WebAuthn"); } else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/account/Safe/SafeAccount.ts` around lines 2187 - 2238, Update getSignerLowerCaseAddress to validate overrides.isInit for WebAuthn signers before selecting the init or non-init address path. When isInit is null or undefined, throw the same RangeError("Must define isInit parameter when using WebAuthn") used by buildSignaturesFromSingerSignaturePairs and createDummySignerSignaturePairForExpectedSigners; preserve existing behavior for explicit true and false values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/account/SendUseroperationResponse.ts`:
- Around line 68-72: Update the polling catch block in SendUseroperationResponse
to retry only explicitly transient transport or server failures; immediately
rethrow semantic JSON-RPC errors such as codes -32601 and -32602 instead of
converting them to a timeout. Preserve lastError handling for retryable failures
and add regression coverage for both deterministic error codes.
- Around line 57-77: Validate timeoutInSeconds and requestIntervalInSeconds
before entering the polling loop, rejecting any non-finite values (including NaN
and infinities) with an appropriate error. Keep the existing wall-clock timeout
and delay behavior unchanged for valid finite inputs.
---
Outside diff comments:
In `@src/utils.ts`:
- Around line 541-572: Validate that the parsed value assigned to response is a
non-null object before checking envelope keys in the JSON parsing flow. For
scalar or null payloads, throw the existing TransportRpcError malformed-response
error with the HTTP status context; preserve the result, simulation_results, and
error handling for valid object responses.
---
Nitpick comments:
In `@src/account/Safe/SafeAccount.ts`:
- Around line 2187-2238: Update getSignerLowerCaseAddress to validate
overrides.isInit for WebAuthn signers before selecting the init or non-init
address path. When isInit is null or undefined, throw the same RangeError("Must
define isInit parameter when using WebAuthn") used by
buildSignaturesFromSingerSignaturePairs and
createDummySignerSignaturePairForExpectedSigners; preserve existing behavior for
explicit true and false values.
In `@src/utils7702.ts`:
- Around line 449-488: Move the SECP256K1_N and SECP256K1_HALF_N constant
declarations above parseRawSignature so the constants are defined before the
function references them. Preserve their values and the existing signature
parsing and low-s normalization behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71767606-ac78-4d7e-990b-7f624322ba85
📒 Files selected for processing (27)
src/Bundler.tssrc/account/Calibur/Calibur7702Account.tssrc/account/Safe/SafeAccount.tssrc/account/Safe/modules/AllowanceModule.tssrc/account/Safe/modules/SocialRecoveryModule.tssrc/account/SendUseroperationResponse.tssrc/account/simple/Simple7702Account.tssrc/errors.tssrc/paymaster/CandidePaymaster.tssrc/paymaster/Erc7677Paymaster.tssrc/paymaster/WorldIdPermissionlessPaymaster.tssrc/transport/BaseRpcTransport.tssrc/transport/HttpTransport.tssrc/transport/JsonRpcNode.tssrc/utils.tssrc/utils7702.tssrc/utilsTenderly.tstest/calibur/packKeySettings.test.jstest/safe/allowanceModule.test.jstest/safe/isDeployed.test.jstest/safe/webauthnInitSignatureSort.test.jstest/transport/Bundler.error-mapping.test.jstest/transport/Bundler.getUserOperationByHash.test.jstest/transport/HttpTransport.test.jstest/transport/SendUseroperationResponse.included.test.jstest/transport/httpErrorHandling.test.jstest/utils7702.test.js
- included() rethrows deterministic protocol errors (-32601 method not
found, -32602 invalid params/hash) immediately instead of retrying
them into a TIMEOUT; transient failures still poll with lastError
preserved.
- included() rejects non-finite timeout/interval values: NaN slipped
past the <= 0 check and made the elapsed-time comparison never true —
an infinite loop.
- sendJsonRpcRequest guards against scalar/null JSON payloads before
the envelope key checks, which threw a raw TypeError ('in' operator
on a primitive) instead of a malformed-response TransportRpcError.
- moved the secp256k1 constants above parseRawSignature for
read-before-use ordering (no behavior change).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/transport/SendUseroperationResponse.included.test.js`:
- Around line 43-46: Update the test around response.included to remove the
Date.now-based 500ms wall-clock assertion, retaining the deterministic errno
rejection and calls === 1 assertions that verify the error is not retried.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 39424de9-7e91-4b19-803a-a87fe38e1dbb
📒 Files selected for processing (5)
src/account/SendUseroperationResponse.tssrc/utils.tssrc/utils7702.tstest/transport/SendUseroperationResponse.included.test.jstest/transport/httpErrorHandling.test.js
🚧 Files skipped from review as they are similar to previous changes (4)
- src/account/SendUseroperationResponse.ts
- test/transport/httpErrorHandling.test.js
- src/utils.ts
- src/utils7702.ts
… test The calls === 1 assertion already proves the deterministic error is not retried; the <500ms Date.now check added only flake risk on loaded CI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
I will distribute the commits on smaller prs |
This was referenced Jul 22, 2026
This was referenced Jul 22, 2026
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
1 participant
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.
Summary by CodeRabbit
included()with wall-clock timeouts, better argument validation, and improved error categorization.