feat(timelock): migrate the upgrade beacons under the timelock too - #290
Conversation
📝 WalkthroughWalkthroughThe migration now transfers ownership of three production upgrade beacons with vault ownership and authorizer roles. The script validates beacon codehashes, preserves implementations, verifies artifacts, and checks post-migration state across three supported chains. ChangesGovernance migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Safe
participant MigrationScript
participant Timelock
participant VaultsAndBeacons
Safe->>MigrationScript: provide live migration state
MigrationScript->>VaultsAndBeacons: validate owners and implementations
MigrationScript->>Timelock: build and verify migration bundle
Safe->>Timelock: submit governance migration
Timelock->>VaultsAndBeacons: transfer vault and beacon ownership
Timelock->>Safe: complete role renouncements
MigrationScript->>VaultsAndBeacons: verify post-migration state
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
15e66b4 to
f15612d
Compare
2f457cd to
8ee276d
Compare
8ee276d to
ff072b0
Compare
53ae210 to
ef6704b
Compare
ff072b0 to
a1250e5
Compare
ef6704b to
3a583cd
Compare
a1250e5 to
c416c40
Compare
3a583cd to
686a1df
Compare
c416c40 to
6e69e39
Compare
686a1df to
bfe333b
Compare
ee70d26 to
af13b89
Compare
bfe333b to
5f51624
Compare
af13b89 to
2c016ca
Compare
18fd996 to
570799b
Compare
2c016ca to
025f7e6
Compare
Vault ownership and the authoriser _ADMIN roles are not the whole admin
surface. Every production token proxies through the chain's three in-use
upgrade beacons, and a beacon owner can upgradeTo a new implementation for
all of them in one transaction — a hostile implementation can re-take vault
ownership and rewrite the authoriser wiring outright. Timelocking
setAuthorizer and vault ownership while leaving the beacons Safe-owned
leaves the delay bypassable by design, so the beacons move in the same
atomic bundle.
- The migration selects still-Safe-owned beacons from live state, pinning
each beacon's codehash to the OZ UpgradeableBeacon bytecode BEFORE
trusting its owner() read, and aborts on any owner outside
{Safe, timelock}.
- Bundle order becomes grants -> vault transfers -> beacon transfers ->
renounces, so the Safe gives nothing up until everything has landed.
- The post-state proves each beacon still serves the implementation it
served before the bundle: this moves upgrade AUTHORITY, never performs an
upgrade.
- LibBeaconInvariants gains an owner-parametric assertion and a
migration-window variant, so the 2026-10-01 forcing function red-lines on
un-migrated beacons, not just un-migrated vaults.
run() and the happy-path test both crossed the legacy-codegen stack limit
once the beacon locals were added. via_ir stays off per CLAUDE.md, so the
selection collapses into a MigrationTargets struct and the post-state /
artifact blocks move into helpers. No behavioural change.
The chain-generic script was only ever exercised against Base, so HyperEVM coverage rested on the chain map resolving rather than on the bundle being correct. Parameterises the happy-path assertions by active chain (authoriser clone, token table, artifact chain id) and adds the HyperEVM leg through the same shared assertions, so neither chain is proven by a copy that can drift. Verified locally against live HyperEVM: 29 vault transfers, 3 beacon transfers, 7 grants, 7 renounces. Soft-skips in CI until HYPEREVM_RPC_URL is provisioned (RAI-1511).
UPGRADEABLE_BEACON_CODEHASH is documented as equal to LibProdDeployV1.PROD_BEACON_BASE_RUNTIME_CODEHASH_V1 but the two copies were tied only transitively — separate fork suites asserting each against the same live beacons. The structural equality is now pinned directly, so a drifted copy surfaces as the source defect it is rather than as an on-chain codehash 'mismatch'. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two coverage gaps on the governance loop: - The full authoring was proven on Base and HyperEVM but never Ethereum, which shares HyperEVM's Safe and timelock yet has its own token table and authoriser clone. testRunAuthorsFullMigrationOnEthereum drives the shared assertion set on a live Ethereum fork, so all three governed chains carry the same proof. - Nothing proved the timelock is not a one-way door. testGovernanceCanMigrateAwayToSuccessor executes the full migration on a Base fork, then hands every surface class onward to a successor principal through the real schedule -> 48h -> execute loop, each leg driven through the Safe's threshold-gated execTransaction (n+1 walk): a vault ownership transfer, a beacon ownership transfer, an authoriser _ADMIN handover with the timelock renouncing its own copy, and a role change on the timelock itself via self-administration. Each leg is proven pending-not-ready inside the window and Done after it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rough it The upgrade path itself now has a literal proof: post-migration a direct Safe upgradeTo reverts OwnableUnauthorizedAccount (the instant path is closed), and the sanctioned path — schedule upgradeTo through the Safe's threshold-gated exec, 48h, execute — completes with the beacon still serving the same implementation (idempotent upgradeTo(currentImpl)). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…gh it The surface that rewires which contract gates every deposit, withdrawal and transfer was asserted as STATE everywhere (authorizer() uniformity) but the PATH had no proof. Post-migration a direct Safe setAuthorizer reverts OwnableUnauthorizedAccount, and the sanctioned schedule -> 48h -> execute path — each leg through the Safe's threshold-gated exec (n+1 walk) — completes, proven with an idempotent re-set of the current authoriser and the wiring asserted unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ing window The last unguarded step in the migration was between CI authoring the bundle and signers executing it: the SafeTxHash cross-check caught a different bundle, but nothing re-checked the artifact against LIVE state at signing time. verify(string) does, locally on the signer's machine: re-runs run()'s full pre-flight, re-derives the expected bundle from current chain state, asserts the downloaded artifact matches byte-exactly (typed MigrationVerifyMismatch pinpoints the first divergent field), and recomputes the canonical MultiSend SafeTxHash at the live nonce for the Safe-UI cross-check. A nonce bump, a moved vault/beacon/role, or a tampered or stale artifact all surface before anyone signs. Deliberately not in the run-script dispatcher: it takes a local path and runs on the signer's machine, per the established verify(string) convention. The runbook's sign-and-execute step now instructs each signer to run it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
10b932c to
e79c442
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@script/20260729-migrate-governance-to-timelock.s.sol`:
- Around line 428-430: Update the BUNDLE_NAME constant used by _emitArtifact and
LibSafeOps.emitTxBuilderJson to include the upgrade-beacon ownership transfer
alongside vault ownership and authoriser admin roles, so the Safe UI title
accurately describes all bundle transactions.
In `@src/lib/LibBeaconInvariants.sol`:
- Around line 266-277: Update assertProdBeaconsOwnershipMigration to validate
each beacon’s codehash against the existing UPGRADEABLE_BEACON_CODEHASH before
calling IOwnable(beacons[i]).owner(). Retain the deployment check and ensure
mismatched codehashes fail before any owner-based migration invariant is
evaluated.
In `@test/script/20260729-migrate-governance-to-timelock.t.sol`:
- Around line 276-291: Update the transfer assertions to iterate the full vault
and beacon tables while maintaining separate selected-transfer indices,
filtering entries by their post-run timelock ownership before comparing against
txs. Apply the same selection rule used by the migration script, incrementing
the expected transfer index only for selected entries, and remove the assumption
that the first expectedTransfers or expectedBeaconTransfers entries are all
migrated.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: 881b944c-1167-485e-ac6d-c2a452743539
📒 Files selected for processing (7)
docs/TIMELOCK.mdscript/20260729-migrate-governance-to-timelock.s.solsrc/lib/LibBeaconInvariants.soltest/script/20260729-migrate-governance-to-timelock.t.soltest/src/concrete/deploy/GovernanceTimelockMigration.t.soltest/src/lib/LibBeaconInvariants.t.soltest/src/lib/LibBeaconInvariantsHarness.sol
|
@coderabbitai review |
|
The Tx Builder meta.name signers read in the Safe UI still described the bundle as vault ownership + authoriser admin roles; the bundle also transfers the three in-use upgrade beacons. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The migration script's selection pins the OZ UpgradeableBeacon codehash before trusting owner(); the ownership sweep and the migration-window drift detector read owner() after only a code-presence check, so a look-alike beacon shadowing the selector could pass either. Both loops now share one deployment+codehash gate, and inverted fork tests prove a look-alike reporting an accepted owner trips BeaconCodehashMismatch before the owner read is consulted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The artifact-order assertions compared transfers against a prefix of the vault/beacon tables, which only matches the script's skip-already- timelocked selection rule when every table entry is Safe-owned. Capture the Safe-owned addresses (not counts) before the run and assert the emitted transfers against that selection, so the proof holds on any fork state including a partial prior run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The post-execution flip list predates the beacon leg; the StoxProdV4 and HyperEvmBeaconOwnership fork tests and the live deploy-script pre-flights assert Safe-owned beacons and red-line after execution until repointed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/LibBeaconInvariants.sol (1)
228-245: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd REUSE metadata to
docs/TIMELOCK.mdand runnix develop -c rainix-sol-legalbefore merge. The four Solidity files already contain the required SPDX identifiers.🤖 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/lib/LibBeaconInvariants.sol` around lines 228 - 245, Update docs/TIMELOCK.md (lines 12-28) with the required REUSE metadata, leaving the Solidity sites in src/lib/LibBeaconInvariants.sol (lines 228-245), test/src/lib/LibBeaconInvariants.t.sol (lines 200-259), script/20260729-migrate-governance-to-timelock.s.sol (lines 299-346), and test/script/20260729-migrate-governance-to-timelock.t.sol (lines 164-174) unchanged because their SPDX identifiers are already correct; before merging, run nix develop -c rainix-sol-legal.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/lib/LibBeaconInvariants.sol`:
- Around line 228-245: Update docs/TIMELOCK.md (lines 12-28) with the required
REUSE metadata, leaving the Solidity sites in src/lib/LibBeaconInvariants.sol
(lines 228-245), test/src/lib/LibBeaconInvariants.t.sol (lines 200-259),
script/20260729-migrate-governance-to-timelock.s.sol (lines 299-346), and
test/script/20260729-migrate-governance-to-timelock.t.sol (lines 164-174)
unchanged because their SPDX identifiers are already correct; before merging,
run nix develop -c rainix-sol-legal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: eaf1f610-97b7-4cfc-8f9e-8da07ccf21ff
📒 Files selected for processing (5)
docs/TIMELOCK.mdscript/20260729-migrate-governance-to-timelock.s.solsrc/lib/LibBeaconInvariants.soltest/script/20260729-migrate-governance-to-timelock.t.soltest/src/lib/LibBeaconInvariants.t.sol
|
Reviewed c4692dd: KEEP — beacon leg codehash-pins before trusting owner() in both the script selection and the lib sweeps, bundle stays atomic with renounces last, implementation immutability proven across the bundle, verify(string) closes authoring-to-signing drift; all three chains' fork suites pass (HyperEVM verified locally against live RPC, RAI-1511 still gates it in CI); all CodeRabbit threads fixed and resolved. |
|
Reviewed c4692dd: KEEP — beacon leg codehash-pins before trusting owner() in both the script selection and the lib sweeps, bundle stays atomic with renounces last, implementation immutability proven across the bundle, verify(string) closes authoring-to-signing drift; all three chains' fork suites pass (HyperEVM verified locally against live RPC, RAI-1511 still gates it in CI); all CodeRabbit threads fixed and resolved. Rulings-conformance: checked against the repo CLAUDE.md rulings and the rulings stated for this work —
|

Vault ownership and the authoriser _ADMIN roles are not the whole admin
surface. Every production token proxies through the chain's three in-use
upgrade beacons, and a beacon owner can upgradeTo a new implementation for
all of them in one transaction — a hostile implementation can re-take vault
ownership and rewrite the authoriser wiring outright. Timelocking
setAuthorizer and vault ownership while leaving the beacons Safe-owned
leaves the delay bypassable by design, so the beacons move in the same
atomic bundle.
each beacon's codehash to the OZ UpgradeableBeacon bytecode BEFORE
trusting its owner() read, and aborts on any owner outside
{Safe, timelock}.
renounces, so the Safe gives nothing up until everything has landed.
served before the bundle: this moves upgrade AUTHORITY, never performs an
upgrade.
migration-window variant, so the 2026-10-01 forcing function red-lines on
un-migrated beacons, not just un-migrated vaults.
run() and the happy-path test both crossed the legacy-codegen stack limit
once the beacon locals were added. via_ir stays off per CLAUDE.md, so the
selection collapses into a MigrationTargets struct and the post-state /
artifact blocks move into helpers. No behavioural change.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation