refactor(contracts): remove completed #575 PKP-owner backfill machinery

[clawdbot-glitch003]

The one-time #575 PKP-owner backfill is complete on Base mainnet — all pre-existing PKPs are bound (verified 0 unbound, 0 multi-account conflicts, 0 bound-to-non-owner across all accounts). This removes the spent migration machinery in the next facet upgrade. Contract-only; none of the operational backfill tooling lands in the repo.

Changes

• WritesFacet: removed backfillPkpOwners() and the PkpOwnerBackfilled event.
• ViewsFacet.getWalletDerivation: removed the owner == 0 compatibility fallback — ownership enforcement is now unconditional (fail-closed). registerWalletDerivation always sets the owner when it writes pkpData, and the backfill bound every legacy PKP, so a non-zero derivation always has a non-zero owner; an owner == 0 now reverts instead of serving the path.
• Deleted the tasks/backfill-pkp-owners.ts hardhat task and its registration (its on-chain target no longer exists).
• Kept getPkpOwnerMaster (ongoing audits) and the core first-owner binding in registerWalletDerivation.
• Updated Foundry tests (dropped the backfill test; the owner == 0 test now asserts fail-closed), DiamondDeploy selectors, CHANGELOG, and regenerated the alloy bindings.

Supersedes #579 (closed — it also carried the operational Safe/EOA tooling, which we don't want in the repo now that the migration is done).

Verification

forge test 50/50 · cargo check clean · tsc clean (only the pre-existing unrelated verify-diamond-facets.ts warning) · no stray references to the removed function.

Deploy note

This is a diamondCut that replaces WritesFacet and ViewsFacet (getWalletDerivation behavior change — a normal Replace, same selector).

The backfillPkpOwners removal needs an explicit Remove cut — the Rust deployer's auto-diff (get_facet_cuts) only emits Replace/Add, never Remove, so propose-update/update alone leaves 0x41275609 routed to the old facet and still callable. Add to the proposal:

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

Do it after the diamond's configOperator is restored from the temporary migration EOA back to the original, and after retiring that temp key.

🤖 Generated with Claude Code

[claude]

🔒 Automated Security Review (differential)

Base: 062c2c75 → Head: d34edaab

Severity	Count
🔴 CRITICAL	0
🟠 HIGH	0
🟡 MEDIUM	0
🟢 LOW	0
⚪ INFO	1

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

[clawdbot-glitch003]

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.