🔒 Automated Security Review (differential)

Base: 062c2c75 → Head: d34edaab

Overall risk: LOW — no exploitable regressions found.

This PR removes the completed #575 PKP-owner backfill migration (backfillPkpOwners) and its owner==0 compatibility fallback in ViewsFacet.getWalletDerivation. That view function is on the node's signing/decryption authorization path, so it was flagged HIGH RISK and run through adversarial modeling. Findings:

• The removed fallback only ever weakened enforcement (silently permitted a read when owner was unset). Removing it strictly narrows accepted requests — it cannot introduce a bypass.
• Verified account.pkpData[pkpId].id has exactly one writer (registerWalletDerivation), which always sets pkpIdToOwnerMaster in the same transaction — so a non-zero derivation with a zero owner is unreachable via the public API.
• The theoretical owner == resolvedMaster == 0 bypass is structurally impossible: getReadOnlyAccount already reverts before resolvedMaster could be read as 0.
• Existing hijack-defense tests are untouched; a new test (test_getWalletDerivation_ownerZeroFailsClosed) explicitly asserts the tightened fail-closed behavior.
• An equivalent removal already shipped on a backport branch days earlier, corroborating this is a reviewed, intentional cleanup.

F1 (INFO): the removed admin migration tool can no longer recover a PKP if the #575 backfill missed it — that would only cause a fail-closed revert (availability impact for the legitimate owner), not a security bypass. Recommend confirming operationally that the backfill ran against every pre-existing pkpId before deploying.

Full report: .security-review/report.md

Codex review

Ran an independent codex review (high reasoning). Two findings; verified both against the code.

[P1] The diamondCut must include an explicit Remove — the deployer won't generate it

Confirmed. The Rust deployer's get_facet_cuts (rust_generator_and_deployer/src/deployer/diamond.rs:35-71) only computes Replace (selectors in the new ABI that already exist on-chain) and Add (new selectors). It never computes Remove for selectors that disappear from the ABI. So make propose_update_base / update alone will leave backfillPkpOwners (0x41275609) routed to the old WritesFacet address — it stays callable, and the source-level removal has no on-chain effect.

Impact is low security-wise (onlyConfigOperatorOrOwner, skip-if-set so it can't overwrite bindings, and all PKPs are already bound), but it defeats the purpose of the PR. The upgrade proposal must add a targeted cut:

{ facetAddress: 0x0000000000000000000000000000000000000000, action: 2 /* Remove */, functionSelectors: [0x41275609] }

The getWalletDerivation (ViewsFacet) change needs no special handling — same selector, so it's a normal Replace.

Follow-up worth filing separately: the deployer can't remove any selector today. A general fix (compute Removes for vanished managed-facet selectors) is valuable but delicate — it must not touch the core DiamondCut/Loupe selectors — so it deserves its own tested PR, not this one.

[P2] Foundry DiamondDeploy helper omits transferChainSecuredAccountOwnership — pre-existing, not from this PR

WritesFacet has 21 public functions; the helper lists 20. But transferChainSecuredAccountOwnership was never in the helper (0 occurrences on main) — this PR correctly went 21→20 by dropping only backfillPkpOwners. No Foundry test exercises transferChainSecuredAccountOwnership (its test uses the Hardhat deployDiamond helper), so there's no current impact. Flagging as a pre-existing gap to fix separately.

Also spot-checked the fail-closed getWalletDerivation for usage-key callers: pkpIdToOwnerMaster stores the master and allApiKeyHashesToMaster[apiKeyHash] resolves any usage key to that same master, so the comparison is correct.