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 (0x41275609) is handled by the normal deploy flow: #592 taught the deployer to emit manifest-gated Remove cuts, and diamond-removals.json on main already lists this selector. Once this PR's WritesFacet build (which no longer declares it) is upgraded via propose-update/update, the selector is detected as orphaned and removed in the same cut. No hand-written Remove entry is needed. (If the upgrade were run against a WritesFacet that still declares it, the deployer refuses the removal with a warning — safe either way.)

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

Precondition for every diamond this is applied to (Base, Sepolia, dev): the #575 backfill must already have bound every legacy PKP, because the owner == 0 read path is gone and so is the on-chain repair function. Base mainnet is verified complete. If a non-prod diamond still has an unbound legacy PKP, the owning account can self-heal without operator help: removeWalletDerivation then registerWalletDerivation with the same path re-registers it and sets the owner binding (first-owner rule, existingOwner == 0). No key material is lost since the key is a stateless function of the path.

Node-side cache: get_wallet_derivation results are cached for up to 300s per replica, so a derivation resolved just before the cut stays usable for at most that window. On Base this is moot (no owner-zero PKPs exist), so no cache flush is needed for prod.

Review follow-ups in this PR

Independent Codex (gpt-6-astra) review + adversarial pass surfaced two code gaps, fixed here:

• Deployer plan_removals: if an upgrade was once run without the manifest, the dropped selector ended up alone on the old facet address and every retry refused to remove it (the address was no longer "managed"). Listed selectors on such a stranded address are now removable, but only when every remaining selector on that address is either re-installed or listed, so a core facet can never qualify by accident. Two new unit tests.
• Deployer --removals: an explicitly passed manifest path that does not exist is now a hard error rather than silently meaning "no removals". README documents the flag and its cwd-relative default.
• Foundry DiamondDeploy helper: added the long-missing transferChainSecuredAccountOwnership selector so the test diamond matches WritesFacet's 21-function surface.

🤖 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.

[clawdbot-glitch003]

Codex review (gpt-6-astra), post-merge-with-main

Ran codex review plus an adversarial codex exec pass against the merged branch (HEAD e89e22d's parent). Review pass: no actionable regressions. Adversarial pass: 4 findings, verified against the code.

Fixed in e89e22d

• [P2] Stranded selector cannot be removed on retry (deployer/diamond.rs plan_removals). If an upgrade once ran without the manifest, 0x41275609 ended up alone on the old WritesFacet address; that address was no longer "managed", so every retry refused the removal. Listed selectors on such a stranded address are now removable, gated on every remaining selector there being either re-installed or listed (so a core facet can never qualify). Also: an explicit --removals path that does not exist is now a hard error, and README documents the flag's cwd-relative default.
• [P2] Foundry DiamondDeploy helper omitted transferChainSecuredAccountOwnership (pre-existing). Added; helper now installs all 21 WritesFacet selectors. forge test 50/50.

Operational, not code (documented in the PR body)

• [P1] Unmigrated diamonds fail closed. True by design. Base is verified backfilled. For any other diamond, an owning account self-heals via removeWalletDerivation + registerWalletDerivation (re-register sets the owner when existingOwner == 0); no key material is lost.
• [P1] Node derivation cache (300s) bypasses the new check during rollout. Window is bounded to one TTL and requires an owner-zero PKP to exist. None do on Base, so no prod cache flush is needed.