[p-diogo]

Summary

Implements work-leaves P3-1, P3-2, P3-4 of Option E Phase 3 (revocable session-key signing), per docs/specs/cred/phase3-implementation-spec.md §3.1–3.4 in the private repo. Do not merge — this is unaudited code touching the one function that can permanently brick a user's vault (audit-risk memo §1); it needs a real review pass and the external audit (D2) before it goes anywhere near a real account.

• P3-1 — TotalReclawAccount: a SimpleAccount (eth-infinitism v0.7.0) subclass overriding _validateSignature to accept an owner-EOA signature (tried first, byte-identical to stock) or a scoped, revocable session-key signature.
• P3-2 — validator logic ported from SessionKeyModule.sol (never deployed — replaced by this design per the account-model memo §5, which found the standalone-module design cannot pass ERC-7562 bundler validation).
• P3-4 — 35 tests (29 fast unit + 6 read-only fork tests against live Gnosis), plus a storage-layout-diff CI gate.

D1 (account model: UUPS over kernel v3) is recorded as approved per session context; PF-1/PF-2 (Pimlico sponsorship of self-targeted UserOps, relay eth_call constraints) are noted as still open in the source docs but don't block this contract-and-tests leaf.

What was ported verbatim vs. rewritten

Verbatim from SessionKeyModule.sol (spec: "re-deriving them is pure risk"):

• _grantDigest + the four EIP-712 typehash/domain constants
• _isCallDataInScope / _isBatchInScope / _inSelectors (the cred-6 per-inner-call executeBatch invariant)
• _recoverEcdsa with the EIP-2 malleability guard
• Monotonic _minNonces replay protection
• _tryDecodeInstallSig / _decodeInstallSig self-external try/catch decode pattern

Mechanically changed (per the spec's table): msg.sender→address(this); _grants[account][signer]/_minNonces[account][signer] (module storage, the shape ERC-7562 rejects) → signer-keyed maps in the account's own ERC-7201 namespaced storage; g.verifyingContract != address(this) now binds to the account (P3-3, folds out of the module→account move for free); revokeSessionKey(account, signer) gated msg.sender == account → revokeSessionKey(signer) gated by the inherited onlyOwner (accepts the owner EOA directly or a self-call — the SPA's exact revoke pattern, tested); _smartAccountOwner()'s external call → a direct owner storage read (strictly better: no external call during validation, and owner is slot 0, always ERC-7562 [STO-010]-permitted).

Not carried forward (interface cleanup, spec §3.2): ISessionKeyModule.sol's three dead errors, plus two more in SessionKeyModule.sol itself I found by the same principle (UnknownGrantVersion, InvalidSignatureLength — declared, never reverted). TotalReclawAccount declares zero custom errors: validation fails closed via SIG_VALIDATION_FAILED, and revokeSessionKey reuses the inherited onlyOwner's existing require. The old files are left untouched on disk (still compile, still tested by their own suite) — deleting them is a separate call, not asked for in this work-leaf.

One genuinely rewritten piece, flagged for scrutiny — _validateSignature's owner path uses ECDSA.tryRecover instead of stock's ECDSA.recover. See "Uncertainty" below; this is not a stylistic choice, it's required for the feature to function at all.

Small additive fix in SessionKeyModule.sol (in scope per spec §3.4's explicit line-numbered callout): a grantDigest external pure wrapper, so SessionKeyModule.t.sol's _grantDigestExt calls the production digest path instead of hand-copying the EIP-712 formula — the exact duplication pattern that let the §2 bytes4[] selector-hashing bug ship in grant.py unnoticed (that Python fix is P3-0, out of scope here).

Structural note: contracts/account/ is a separate Foundry project

Not a stylistic choice — adding account-abstraction/openzeppelin-contracts to the outer contracts/lib/ + a contracts/remappings.txt was tried first and reverted after it broke an unrelated, currently-passing, real-deployed-contract test: solc embeds the full project-wide settings.remappings list into every compiled contract's metadata, regardless of whether that contract imports anything through them. Verified empirically — adding just the two new remapping lines (nothing else) shifted EventfulDataEdge's bytecode enough to break test/DataEdgeStagingCreate2.t.sol's pinned CREATE2 address for the real, live staging DataEdge (0xE7a4D2677B686e13775Ba9092631089e35F0BB91). A fully separate Foundry project root (contracts/account/foundry.toml, own lib/, own remappings.txt) is the only fix that leaves the outer project's compiled output byte-for-byte unaffected. Re-verified after every change in this PR: 26/26 tests still pass, unchanged, in the outer contracts/ project.

CI: added a second job (forge-test-account) to .github/workflows/forge-test.yml, isolated the same way — its own working directory, own lib install, own forge build/forge test invocation.

Fork-test results (real output)

Ran 6 tests for test/TotalReclawAccountFork.t.sol:TotalReclawAccountForkTest
[PASS] test_fork_out_of_scope_userop_rejected_on_factory_account() (gas: 3022680)
[PASS] test_fork_owner_signed_userop_validates_on_factory_account() (gas: 2856206)
[PASS] test_fork_revocation_blocks_reuse_on_factory_account() (gas: 2946701)
[PASS] test_fork_session_key_validates_on_factory_account() (gas: 3024192)
[PASS] test_fork_upgrade_is_idempotent_on_real_account() (gas: 2699499)
[PASS] test_fork_upgrade_preserves_real_account_state() (gas: 2706752)
Suite result: ok. 6 passed; 0 failed; 0 skipped

test_fork_upgrade_preserves_real_account_state forks live Gnosis mainnet, takes the real deployed account 0x2c0CF74B2b76110708CA431796367779e3738250 (66 UserOps of history as of the block last checked — re-verified live via cast immediately before writing the test: owner() = 0x9858EfFD232B4033E47d90003D41EC34EcaEda94, ERC-1967 impl slot = 0x68641DE71cfEa5a5d0D29712449Ee254bb1400C2, factory accountImplementation() matches), upgrades it via the owner EOA's direct upgradeToAndCall (the audit-risk memo §5 escape hatch — no bundler, no signature needed, vm.prank spoofs msg.sender), and asserts owner(), raw slot 0, and the EntryPoint nonce are all byte-unchanged, plus that the owner can still drive execute() directly post-upgrade. A second test proves upgrade idempotence on the same real account.

Since I don't (and shouldn't) hold the real owner's private key, the three signature-shaped assertions (owner-signed validates, session-key validates, out-of-scope doesn't) run against a fresh account deployed through the same real, live factory + real baseline implementation, owned by a Foundry-controlled keypair — same factory, same deployment pattern, controllable signing.

Full suite: 35/35 passing (29 unit + 6 fork). Unit suite is network-free (fresh ERC1967Proxy + controlled keys) and covers the six executeBatch scope-invariant tests ported from SessionKeyModule.t.sol:424-591, plus grant-binding, replay, malleability, and revocation edge cases.

Storage-layout gate

contracts/account/script/storage-layout-diff.sh wires forge inspect ... storageLayout (already emitted per foundry.toml, never consumed before this) as a real CI gate — B2 mitigation (a colliding state variable at slot 0 is catastrophic and silent). Current output:

OK — 1 plain state variable(s), byte-identical between TotalReclawAccount and SimpleAccount:
  slot   0 offset 0 owner: t_address

TotalReclawAccount declares zero plain state variables; all session-key state lives in ERC-7201 namespaced storage (fixed slot, inline assembly), which correctly doesn't show up here at all.

The two traps the spec called out — confirmed avoided

1. Did not port SessionKeyModule.t.sol:86's test_create2_address_unchanged_after_module_install (tautological — block.chainid isn't an input to the expression it re-derives twice). Wrote a real pinned-address test instead (test_pinned_implementation_create2_address), mirroring DataEdgeStagingCreate2.t.sol:69's independently-hardcoded-literal pattern. Note: this pin is a placeholder salt, not a commitment to a real deployment address — that's decided by the (out-of-scope) deployment plan.
2. Did not carry forward ISessionKeyModule.sol's dead errors or its incorrect "MUST be the master wallet" docstring — see "not carried forward" above.

Uncertainty — flagged, not smoothed over

The one thing I want a second pair of eyes on hardest: _validateSignature's owner path calls ECDSA.tryRecover instead of stock SimpleAccount's ECDSA.recover. I did not do this casually — I traced it to a real correctness requirement: stock's ECDSA.recover reverts on any signature that isn't a well-formed 65-byte blob, and a session-key lazy-install signature is a much longer abi.encode(PermissionGrant, ecdsaSig) blob. A literal "try owner's exact code path first" port would revert the entire validateUserOp on every lazy-install UserOp before ever reaching the session-key branch — not merely insecure, structurally inert (the feature would never work). I reasoned through why this doesn't create a NEW acceptance the way that matters (traced ecrecover's "returns zero" failure mode as a pure function of (v,r,s), independent of which hash is checked — documented at length in the function's NatSpec) but this is exactly the kind of reasoning I'd want an auditor with real EIP-4337 depth to either confirm or refute, not take on my say-so. It's the same territory the audit-risk memo names as B1 (owner path broken → account bricked forever) and lists as reason #1 for external review.

Secondary, lower-stakes items I'm reasonably but not 100% confident on:

• License: TotalReclawAccount.sol is GPL-3.0 (not this repo's usual MIT) because it directly inherits from eth-infinitism's GPL-3.0 SimpleAccount. Flagged in the file's own NatSpec as a licensing question for a sanity check, not resolved unilaterally.
• The pinned CREATE2 salt (totalreclaw.TotalReclawAccount.v1) is a test-only placeholder, not a deployment commitment — the actual deployment plan (out of scope here) may choose differently, in which case the pin needs updating to match, which is cheap since nothing is deployed at that address yet.
• I did not attempt to also exercise the upgrade path through a full EntryPoint.handleOps simulation (deposit funding, gas estimation) — fork tests prank as the EntryPoint directly for validateUserOp, which is a standard, narrower, and I believe sufficient test technique for what's being asserted, but it's a real scope choice worth naming.

Constraints honored

• Never deployed anything, never sent a transaction — fork tests are vm.createSelectFork + local pranks/calls, discarded at test end.
• Worked only in this worktree; did not touch the shared checkout.
• Out of scope, untouched: core Rust (P3-5), Python client (P3-6+), SPA (P3-13), subgraph (P3-14), P3-15 docs (CLAUDE.md/SECURITY.md — separate work-leaf).
• No CHANGELOG entry: CHANGELOG-public.md tracks packages promoted to stable registries; nothing here is released or deployed yet.

Test plan

• cd contracts/account && forge build --sizes — compiler run successful
• cd contracts/account && forge test -vv — 35/35 passing (29 unit + 6 live-Gnosis fork)
• cd contracts/account && ./script/storage-layout-diff.sh — layouts byte-identical
• cd contracts && forge test — 26/26 passing, unchanged (outer project untouched)
• Real code review (Pedro / Fable per the crypto-review convention)
• External audit (D2, blocked-on-Pedro per the audit-risk memo)
• E2E staging (PF-1 sponsorship check, bundler acceptance — separate work-leaves)

🤖 Generated with Claude Code

[p-diogo]

Orchestrator review — verified independently, one real defect found and fixed

I re-ran everything rather than accepting the implementation report, because this is the one contract in the phase where a bug is unrecoverable.

Defect found: the build was not reproducible

test_pinned_implementation_create2_address passed locally and failed in CI, predicting a different address (0x7D12D9f0… vs the then-pinned 0x25D4E718…). The implementation report said "35/35 passing" — true locally, while the CI job it added was red.

Root cause was not a flaky test. Foundry's default bytecode_hash = "ipfs" appends a hash of the solc metadata JSON to the bytecode, and that JSON varies with toolchain details beyond the pinned solc_version — so identical source compiled in two environments produces different creationCode and therefore different CREATE2 addresses. Pinning solc_version + optimizer settings is necessary but not sufficient.

This mattered well beyond the test. The deployment plan pins a CREATE2 address for the implementation. A pin that depends on who compiled it is not a pin — no auditor and no user could independently reproduce or verify the deployed bytecode, which is exactly the property being paid for.

Fixed in 6585cad (bytecode_hash = "none" + cbor_metadata = false), pin updated to the deterministic 0x6d17938F…. Confirmed by CI now agreeing with my local build — two independent environments, same predicted address.

I first hypothesised divergent source paths; a two-path local compile disproved it (byte-identical). Recording that so the next person doesn't re-run the same dead end.

Security-critical paths I verified by reading, not by report

• Owner-first ordering — present and correct. The ECDSA.tryRecover-instead-of-recover deviation is justified and necessary: stock recover reverts on any non-65-byte signature, so a literal port would revert on every lazy-install UserOp before the session-key branch ran — making the feature inert, not merely insecure. Verified the substitution cannot widen acceptance: for a well-formed 65-byte signature tryRecover returns the identical address and error, and every non-NoError outcome falls through to a path that can only return SIG_VALIDATION_FAILED. No input stock would accept is rejected; none it would reject is accepted.
• Scope enforcement on the lazy-install path — present. _isCallDataInScope runs after install and before returning success, so a session key cannot execute out-of-scope work on its first op. This was my main concern and it is handled.
• Revocation actually revokes — minNonces[signer] = g.nonce + 1 is set before delete, so the original signed grant cannot be replayed to reinstall itself. Without this, revocation would be defeatable by replaying the old blob.
• ERC-7201 storage constant — recomputed independently from keccak256(abi.encode(uint256(keccak256("totalreclaw.account.sessionkeys")) - 1)) & ~0xff: exact match. No slot-0 collision.
• _disableInitializers reasoning — correct; an upgraded proxy cannot be re-initialized to hijack ownership.
• Master-key check is a direct storage read, not an external CALL — which is the ERC-7562 [STO-010] fix that motivated abandoning the standalone-module design.

Independently re-run

account project : 29 unit + 6 fork (live Gnosis)  = 35/35
outer contracts : 26/26 (unaffected — the separate-project split holds)

The fork tests do upgrade the real deployed account 0x2c0CF74B… and assert owner(), slot 0 and EntryPoint nonce are byte-unchanged. That is the highest-value test in the phase and it passes.

Not merging

Green and reviewed, but leaving the merge to Pedro: D2 (external audit) is being commissioned precisely because a bug here freezes vaults permanently. Merging isn't deploying, so this is a judgement call rather than a gate — say the word and I'll land it.

Worth handing the auditor the reproducibility fix above as context; it is the kind of thing that is invisible in a source-only review.

🤖 Generated with Claude Code