feat(lib): LibTimelockInvariants + authoriser grant-map admin-holder parameterisation - #284
Conversation
|
Warning Review limit reached
Next review available in: 50 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR parameterizes authoriser grant invariants with a distinct admin holder. It also adds deterministic governance timelock derivation, state validation, and tests for valid and invalid configurations. ChangesAuthoriser grant invariants
Governance timelock invariants
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Safe
participant LibTimelockInvariants
participant ZoltuFactory
participant TimelockController
participant AccessControl
Safe->>LibTimelockInvariants: provide proposer/executor Safe
LibTimelockInvariants->>ZoltuFactory: derive deterministic timelock address
ZoltuFactory-->>TimelockController: deploy pinned initialization code
LibTimelockInvariants->>TimelockController: read codehash and minimum delay
LibTimelockInvariants->>AccessControl: validate required and forbidden roles
AccessControl-->>LibTimelockInvariants: return role membership
Possibly related issues
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/LibAuthoriserInvariants.sol (1)
205-224: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winRequire the Safe to relinquish
_ADMINroles after migration.The assertion only proves the timelock gained admin roles. It still passes if the Safe retains all seven roles, leaving a path to bypass timelock governance.
src/lib/LibAuthoriserInvariants.sol#L205-L224: whenadminHolder != tokenOwnerSafe, revert iftokenOwnerSaferetains any of the seven_ADMINroles.test/src/lib/LibAuthoriserInvariants.t.sol#L132-L142: mock those Safe admin-role checks as false before asserting the migrated state passes, and add a failing case where one remains true.🤖 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/LibAuthoriserInvariants.sol` around lines 205 - 224, Require assertExpectedGrants to verify that, when adminHolder differs from tokenOwnerSafe, tokenOwnerSafe no longer holds any of the seven _ADMIN roles and revert if any remain; preserve the existing behavior when both addresses are identical. In test/src/lib/LibAuthoriserInvariants.t.sol:132-142, mock all Safe admin-role checks as false for the passing migration case and add a failing case where one admin role remains true.
🤖 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 `@src/lib/LibTimelockInvariants.sol`:
- Around line 282-294: Rename the internal library helpers `_assertHasRole` and
`_assertNotOpenRole` to unprefixed names, and update every call site in
LibTimelockInvariants to use the new names. Preserve their visibility, behavior,
and parameters.
- Line 3: Update the Solidity pragma in LibTimelockInvariants to use an exact
0.8.25 compiler version instead of allowing later 0.8.x releases, preserving the
requested deterministic compiler behavior.
---
Outside diff comments:
In `@src/lib/LibAuthoriserInvariants.sol`:
- Around line 205-224: Require assertExpectedGrants to verify that, when
adminHolder differs from tokenOwnerSafe, tokenOwnerSafe no longer holds any of
the seven _ADMIN roles and revert if any remain; preserve the existing behavior
when both addresses are identical. In
test/src/lib/LibAuthoriserInvariants.t.sol:132-142, mock all Safe admin-role
checks as false for the passing migration case and add a failing case where one
admin role remains true.
🪄 Autofix (Beta)
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: 64a334d8-31a4-47d6-9ecb-53eb73990ead
📒 Files selected for processing (6)
src/lib/LibAuthoriserInvariants.solsrc/lib/LibTimelockInvariants.soltest/src/lib/LibAuthoriserInvariants.t.soltest/src/lib/LibAuthoriserInvariantsHarness.soltest/src/lib/LibTimelockInvariants.t.soltest/src/lib/LibTimelockInvariantsHarness.sol
ec9653a to
72742aa
Compare
72742aa to
bdfaa30
Compare
8b024bb to
6ce221a
Compare
bdfaa30 to
f2df295
Compare
…parameterisation New src/lib/LibTimelockInvariants.sol: constants and invariant assertions for the ST0x governance timelock (unmodified pre-audited OZ TimelockController from the version-locked 5.6.1 soldeer dep): - TIMELOCK_MIN_DELAY = 48 hours, role-hash mirrors pinned against the live OZ getters. - Per-chain address pins (Base + Ethereum), placeholders until the deploy broadcast executes and a pin PR hydrates them; timelockForChainId with a typed revert on unsupported chains (no cross-chain fallback). - timelockInitCode/expectedTimelockAddress: the exact creation code the deploy broadcasts (Safe as sole proposer+canceller+executor, admin = 0 so the timelock self-administers from birth and the deploy key is never granted anything) and its Zoltu CREATE2 address, derived in-source and validated against the real factory bytecode in tests. - assertTimelockState: codehash + minDelay pins, Safe role set, timelock self-administration, and negative checks (Safe must not hold root admin, no OZ open-role zero-address grants). - TIMELOCK_CANCELLER placeholder for a future dedicated canceller principal; asserted once hydrated. LibAuthoriserInvariants: expectedGrants/assertExpectedGrants gain an adminHolder parameterisation so the post-timelock-migration grant map (the seven _ADMIN roles on the timelock, operational roles unchanged) is expressible through the same single master map; existing overloads are exact collapses with adminHolder = Safe. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RquYKmoEVSfuVHy8kwn1yT
HyperEVM carries 29 live production tokens, so it is governed on the same terms as Base and Ethereum rather than as a follow-up. Adds the per-chain timelock pin placeholder and the timelockForChainId arm, plus the derived-address assertion for it and an explicit test that all three pinned chains resolve. Note the HyperEVM Safe shares Ethereum's address, and the timelock constructor embeds only that Safe, so the Zoltu-derived timelock address is identical on those two chains. That is correct — same policy, same init code, same CREATE2 address — and the pins stay separate constants because the deploy is per-chain.
The admin-holder parameterisation was written against a 13-entry map; the multichain stack underneath now carries the three additional service-signer rows from #280, so the merged map is 16. The structure test still asserted 13 and failed the restacked branch. Widens it to 16 and asserts the new rows explicitly: the additional signer's three action roles are OPERATIONAL, so they must track the signer regardless of who holds the _ADMIN slice — the property that keeps the timelock migration from ever moving them.
Deployed by 20260729-deploy-governance-timelock (Base, run 31378549555) at 0xA34b2968A8B480440B218B6768Ad700FC81379b3. Verified on-chain: 48h min delay, timelock self-administers, Safe holds proposer/canceller/executor, deploy key holds nothing. Equals expectedTimelockAddress(Base Safe), which the structure test asserts.
…ress The 0xA34b... deployment was broadcast when this branch still compiled at optimizer_runs=5000. Commit 0a8c62c lowered it to 2000 (so the H01 fix fits EIP-170), which changes TimelockController's creation bytecode and therefore its CREATE2 address: current source derives 0xdb4b2187... instead. Pinning an address the source can no longer reproduce is worse than not pinning, so the pin goes back to address(0) until a deploy runs against settled compiler settings. The 0xA34b... instance holds no power (nothing was migrated to it) and is simply abandoned. TimelockPinMismatch, added alongside the multi-chain deploy, is what caught this.
f2df295 to
3366565
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/lib/LibAuthoriserInvariants.sol`:
- Around line 214-231: Reject legacy Safe _ADMIN grants in assertExpectedGrants
when adminHolder differs from tokenOwnerSafe by checking each of the seven
expected admin roles on tokenOwnerSafe and reverting if any remain; update
test/src/lib/LibAuthoriserInvariants.t.sol lines 125-153 to assert this
rejection, then mock those Safe role checks as false before the existing passing
post-migration assertion.
🪄 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: 4bc60ec1-d9bd-47c2-bc56-16adcb0e2087
📒 Files selected for processing (4)
src/lib/LibAuthoriserInvariants.solsrc/lib/LibTimelockInvariants.soltest/src/lib/LibAuthoriserInvariants.t.soltest/src/lib/LibTimelockInvariants.t.sol
…older assertExpectedGrants(authoriser, safe, adminHolder) asserted the seven _ADMIN roles onto the admin holder but accepted the Safe RETAINING its own copies alongside — a state where the Safe can grant or revoke action roles with zero delay, which is exactly the bypass the timelock migration exists to close. The gap mattered because the post-execution flip PR repoints the durable cron invariants at this overload while retiring the migration-window suite's exclusive-holder check, leaving the dual-holder state unasserted. Now, whenever the admin holder is distinct from the Safe, every map entry assigned to the admin holder is also asserted NOT held by the Safe, with a typed UnexpectedRetainedAdminGrant revert. The admin slice is identified structurally (grantee == adminHolder) rather than by a hardcoded count. The two-arg collapse (adminHolder == Safe) is unaffected. Resolves the open CodeRabbit thread on this surface; the dual-holder state the test previously blessed is now the rejection case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/lib/LibAuthoriserInvariants.sol`:
- Around line 255-258: Update the retained-grant check in
LibAuthoriserInvariants around the grants loop to inspect only the explicit
seven *_ADMIN roles, rather than matching every grant whose grantee equals
adminHolder. Preserve the existing acl.hasRole validation and
UnexpectedRetainedAdminGrant behavior for those admin roles, while excluding
operational roles such as DEPOSIT, WITHDRAW, and CERTIFY.
🪄 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: d94d3e2e-4b24-431f-ae9c-5ad9aedd8363
📒 Files selected for processing (2)
src/lib/LibAuthoriserInvariants.soltest/src/lib/LibAuthoriserInvariants.t.sol
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ress Matching map entries by grantee == adminHolder mis-slices when the admin holder aliases another grantee (e.g. a service signer as adminHolder would drag its operational DEPOSIT/WITHDRAW/CERTIFY entries into the check and spuriously flag the Safe's legitimate copies). The admin slice is the map's LEADING entries, a position the parameterisation test already pins, so the check now walks grants[0..ADMIN_ROLE_COUNT) with the count as a lib constant. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…parameterisation New src/lib/LibTimelockInvariants.sol: constants and invariant assertions for the ST0x governance timelock (unmodified pre-audited OZ TimelockController from the version-locked 5.6.1 soldeer dep): - TIMELOCK_MIN_DELAY = 48 hours, role-hash mirrors pinned against the live OZ getters. - Per-chain address pins (Base + Ethereum), placeholders until the deploy broadcast executes and a pin PR hydrates them; timelockForChainId with a typed revert on unsupported chains (no cross-chain fallback). - timelockInitCode/expectedTimelockAddress: the exact creation code the deploy broadcasts (Safe as sole proposer+canceller+executor, admin = 0 so the timelock self-administers from birth and the deploy key is never granted anything) and its Zoltu CREATE2 address, derived in-source and validated against the real factory bytecode in tests. - assertTimelockState: codehash + minDelay pins, Safe role set, timelock self-administration, and negative checks (Safe must not hold root admin, no OZ open-role zero-address grants). - TIMELOCK_CANCELLER placeholder for a future dedicated canceller principal; asserted once hydrated. LibAuthoriserInvariants: expectedGrants/assertExpectedGrants gain an adminHolder parameterisation so the post-timelock-migration grant map (the seven _ADMIN roles on the timelock, operational roles unchanged) is expressible through the same single master map; existing overloads are exact collapses with adminHolder = Safe. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RquYKmoEVSfuVHy8kwn1yT
HyperEVM carries 29 live production tokens, so it is governed on the same terms as Base and Ethereum rather than as a follow-up. Adds the per-chain timelock pin placeholder and the timelockForChainId arm, plus the derived-address assertion for it and an explicit test that all three pinned chains resolve. Note the HyperEVM Safe shares Ethereum's address, and the timelock constructor embeds only that Safe, so the Zoltu-derived timelock address is identical on those two chains. That is correct — same policy, same init code, same CREATE2 address — and the pins stay separate constants because the deploy is per-chain.
The admin-holder parameterisation was written against a 13-entry map; the multichain stack underneath now carries the three additional service-signer rows from #280, so the merged map is 16. The structure test still asserted 13 and failed the restacked branch. Widens it to 16 and asserts the new rows explicitly: the additional signer's three action roles are OPERATIONAL, so they must track the signer regardless of who holds the _ADMIN slice — the property that keeps the timelock migration from ever moving them.
Deployed by 20260729-deploy-governance-timelock (Base, run 31378549555) at 0xA34b2968A8B480440B218B6768Ad700FC81379b3. Verified on-chain: 48h min delay, timelock self-administers, Safe holds proposer/canceller/executor, deploy key holds nothing. Equals expectedTimelockAddress(Base Safe), which the structure test asserts.
…ress The 0xA34b... deployment was broadcast when this branch still compiled at optimizer_runs=5000. Commit 0a8c62c lowered it to 2000 (so the H01 fix fits EIP-170), which changes TimelockController's creation bytecode and therefore its CREATE2 address: current source derives 0xdb4b2187... instead. Pinning an address the source can no longer reproduce is worse than not pinning, so the pin goes back to address(0) until a deploy runs against settled compiler settings. The 0xA34b... instance holds no power (nothing was migrated to it) and is simply abandoned. TimelockPinMismatch, added alongside the multi-chain deploy, is what caught this.
… chains Deployed by 20260729-deploy-governance-timelock (run 31385281204), which covered Base, Ethereum and HyperEVM in a single dispatch: Base 0xdb4b2187A685310E6b64170c97B80E90DD4a9B71 Ethereum 0x290961EF70A86aB70B7201D46d29f2f357416b49 HyperEVM 0x290961EF70A86aB70B7201D46d29f2f357416b49 Each was verified on-chain (48h min delay, timelock self-administers, Safe holds proposer/canceller/executor, deploy key holds nothing) and equals expectedTimelockAddress for its chain's Safe, which the structure test asserts. Ethereum and HyperEVM share an address because they share a Safe and the constructor embeds only that Safe.
0f09c0a to
78aeb1d
Compare
The runtime-codehash expectation was derived in-source (keccak256(type(TimelockController).runtimeCode)), which describes whatever the CURRENT compiler settings produce, not what production carries — as soon as the settings change, the source drifts from prod. The optimizer_runs 5000->2000 change demonstrated the failure mode: the deployed Base instance became underivable from source and had to be abandoned. Now the timelock is described by constants, per the repo's pin convention: - TIMELOCK_CREATION_CODE: the audited OZ TimelockController 5.6.1 creation bytecode, compiled once under the settled profile and embedded as a literal. The deploy broadcasts THESE bytes, so the CREATE2 address derivation and the deployed bytecode are immune to future compiler-settings changes. - TIMELOCK_RUNTIME_CODEHASH: the keccak of the runtime those bytes deploy (no immutables, so constructor-independent), asserted by assertTimelockState in place of the removed derivation function. testTimelockPinsMatchCompiledDependency ties both pins to the compiled dependency while the profile remains at the pinned generation; on divergence the pins stand — they describe prod, not source. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s-lib' into fix-284 # Conflicts: # src/lib/LibAuthoriserInvariants.sol # src/lib/LibTimelockInvariants.sol # test/src/lib/LibAuthoriserInvariants.t.sol # test/src/lib/LibTimelockInvariants.t.sol
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 (4)
test/src/lib/LibAuthoriserInvariants.t.sol (3)
91-98: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPin
ADMIN_ROLE_COUNTin the test.The source comment says the retained slice length is pinned, but the test only hardcodes
7. IfADMIN_ROLE_COUNTchanges while the grant map remains unchanged, the test can pass while the production assertion checks the wrong slice. Add an explicit assertion forLibAuthoriserInvariants.ADMIN_ROLE_COUNT.Proposed test fix
RoleGrant[] memory grants = LibAuthoriserInvariants.expectedGrants(safe, timelock); assertEq(grants.length, 16); + assertEq(LibAuthoriserInvariants.ADMIN_ROLE_COUNT, 7); for (uint256 i = 0; i < 7; i++) {This assessment uses the supplied
ADMIN_ROLE_COUNTimplementation and parameterization test.🤖 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 `@test/src/lib/LibAuthoriserInvariants.t.sol` around lines 91 - 98, Add an explicit assertion in testExpectedGrantsAdminHolderParameterisation that LibAuthoriserInvariants.ADMIN_ROLE_COUNT equals 7, while retaining the existing grant loop and length checks.
86-88: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the operational-grant count in the test documentation.
The map contains nine operational entries: three for
GRANTEE_SERVICE_1C66, three forsafe, and three forGRANTEE_SERVICE_3D0C. The comment says six entries and names only one service signer.Proposed documentation fix
- /// track `adminHolder`, the six operational entries stay split between - /// the service signer and the Safe, and the narrower overloads are exact + /// track `adminHolder`; the nine operational entries remain assigned to + /// the two service signers and the Safe. The narrower overloads are exactThis assessment uses the supplied grant map and test comments.
🤖 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 `@test/src/lib/LibAuthoriserInvariants.t.sol` around lines 86 - 88, Update the documentation comment near the admin-holder parameterisation to state that there are nine operational entries, split into three for GRANTEE_SERVICE_1C66, three for safe, and three for GRANTEE_SERVICE_3D0C; leave the seven _ADMIN entries and surrounding test behavior unchanged.
126-143: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCover the
adminHolderroot-admin rejection.The new branch at
src/lib/LibAuthoriserInvariants.sol, Lines [241-243], checksDEFAULT_ADMIN_ROLEonadminHolder. The existing test only coversGRANTEE_TOKEN_OWNER_SAFE. Add a separate case that mockshasRole(bytes32(0), timelock)totrueand expectsUnexpectedDefaultAdmin(clone, timelock).This assessment uses the supplied assertion branch and test coverage.
🤖 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 `@test/src/lib/LibAuthoriserInvariants.t.sol` around lines 126 - 143, Add a separate case in testAssertExpectedGrantsWithDistinctAdminHolder that mocks hasRole(bytes32(0), timelock) as true for the adminHolder and expects the assertion to revert with UnexpectedDefaultAdmin(clone, timelock), covering the adminHolder branch in assertExpectedGrants.src/lib/LibAuthoriserInvariants.sol (1)
216-217: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPass the governance timelock to post-migration callers. If these paths run after migration,
LibInvariants.assertProductionStateandStoxCrossChainParitymust use the three-argument overload. The current two-argument calls setadminHolderto the Safe and then reject the migrated state.🤖 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/LibAuthoriserInvariants.sol` around lines 216 - 217, Update post-migration callers in LibInvariants.assertProductionState and StoxCrossChainParity to use the three-argument assertExpectedGrants overload, passing the governance timelock as the adminHolder instead of the Safe while preserving the existing authoriser and Safe arguments.
🤖 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/LibAuthoriserInvariants.sol`:
- Around line 216-217: Update post-migration callers in
LibInvariants.assertProductionState and StoxCrossChainParity to use the
three-argument assertExpectedGrants overload, passing the governance timelock as
the adminHolder instead of the Safe while preserving the existing authoriser and
Safe arguments.
In `@test/src/lib/LibAuthoriserInvariants.t.sol`:
- Around line 91-98: Add an explicit assertion in
testExpectedGrantsAdminHolderParameterisation that
LibAuthoriserInvariants.ADMIN_ROLE_COUNT equals 7, while retaining the existing
grant loop and length checks.
- Around line 86-88: Update the documentation comment near the admin-holder
parameterisation to state that there are nine operational entries, split into
three for GRANTEE_SERVICE_1C66, three for safe, and three for
GRANTEE_SERVICE_3D0C; leave the seven _ADMIN entries and surrounding test
behavior unchanged.
- Around line 126-143: Add a separate case in
testAssertExpectedGrantsWithDistinctAdminHolder that mocks hasRole(bytes32(0),
timelock) as true for the adminHolder and expects the assertion to revert with
UnexpectedDefaultAdmin(clone, timelock), covering the adminHolder branch in
assertExpectedGrants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a73be80d-5823-4a52-8192-67755524d2d4
📒 Files selected for processing (4)
src/lib/LibAuthoriserInvariants.solsrc/lib/LibTimelockInvariants.soltest/src/lib/LibAuthoriserInvariants.t.soltest/src/lib/LibTimelockInvariants.t.sol
The zero-address (open-role) rejection was tested only for EXECUTOR_ROLE; mutants deleting the proposer, canceller, or DEFAULT_ADMIN open-role checks survived the suite. The strengthened test grants address(0) each of the four roles in turn through the self-administration path, asserts the typed revert per role, revokes, and proves the closed state passes again after each revoke — so each rejection is attributable to the zero-grant alone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Mirror of the open-role walk on the positive checks: the missing-role rejection was tested only for the Safe's proposer role, so mutants deleting the canceller, executor, or self-administration _assertHasRole lines survived. The strengthened test revokes each required grant in turn — proposer, canceller, executor off the Safe, re-granting and re-asserting the restored state after each — and finishes with the timelock's own root admin, which goes last and unrestored: once self-administration is revoked no principal can re-grant it, which is exactly why the invariant pins it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Reviewed 6da08a2: ship — timelock invariants lib with frozen bytecode pins matching the three live deployments, authoriser grant-map admin-holder parameterisation with exclusive-holding enforcement, and per-role walks covering every missing-role and open-role branch; all checks green, all review threads resolved. |
|
Reviewed 6da08a2: ship — timelock invariants lib with frozen bytecode pins matching the three live deployments, authoriser grant-map admin-holder parameterisation with exclusive-holding enforcement, and per-role walks covering every missing-role and open-role branch; all checks green, all review threads resolved. |
2 participants
New src/lib/LibTimelockInvariants.sol: constants and invariant assertions
for the ST0x governance timelock (unmodified pre-audited OZ
TimelockController from the version-locked 5.6.1 soldeer dep):
OZ getters.
broadcast executes and a pin PR hydrates them; timelockForChainId with a
typed revert on unsupported chains (no cross-chain fallback).
deploy broadcasts (Safe as sole proposer+canceller+executor, admin = 0 so
the timelock self-administers from birth and the deploy key is never
granted anything) and its Zoltu CREATE2 address, derived in-source and
validated against the real factory bytecode in tests.
self-administration, and negative checks (Safe must not hold root admin,
no OZ open-role zero-address grants).
principal; asserted once hydrated.
LibAuthoriserInvariants: expectedGrants/assertExpectedGrants gain an
adminHolder parameterisation so the post-timelock-migration grant map (the
seven _ADMIN roles on the timelock, operational roles unchanged) is
expressible through the same single master map; existing overloads are
exact collapses with adminHolder = Safe.
Co-Authored-By: Claude Fable 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01RquYKmoEVSfuVHy8kwn1yT
Summary by CodeRabbit
New Features
Bug Fixes
Tests