chore(ens): draft - Empowering the ENS Foundation - #101
Conversation
Tally draft 2913928210729141431 by nick.eth. Two calls: 1. transfer 1,000,000 ENS to the Foundation Safe (3-of-5) 2. swapOwner on the Endowment Safe: DAO timelock -> EndowmentTimelock (OZ TimelockController v4.3.2, 9-day delay, Foundation Safe as proposer, Security Council veto wrapper with cancel rights) Manually derived calldata matches the draft byte-for-byte.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d95846c34
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| (endowmentModulesBefore,) = endowmentSafe.getModulesPaginated(SENTINEL_OWNERS, 10); | ||
| assertEq(endowmentModulesBefore.length, 2, "endowment: expected 2 enabled modules"); |
There was a problem hiding this comment.
Pin the enabled Endowment modules
The assertion accepts any two enabled modules, even though these modules bypass the new timelock and can execute Endowment transactions directly. If an unintended module was enabled at the reviewed fork, the test would snapshot and preserve it while still reporting that the expected Karpatkey modules were untouched; assert the two expected module addresses rather than only their count.
Useful? React with 👍 / 👎.
| assertEq(foundationSafe.getOwners().length, 5); | ||
| assertEq(foundationSafe.getThreshold(), 3); | ||
| assertTrue(foundationSafe.isOwner(_proposer()), "nick.eth must be a Foundation Safe owner"); |
There was a problem hiding this comment.
Verify every Foundation Safe owner
This only proves that the Safe has five owners and that one is the proposer; the other four can be arbitrary addresses while the review still passes. Because this Safe receives the ENS grant and is the sole operational proposer for the Endowment timelock, assert that its complete owner set matches the five approved board signers.
Useful? React with 👍 / 👎.
| assertTrue(endowmentTimelock.hasRole(PROPOSER_ROLE, ENS_FOUNDATION_SAFE)); | ||
| assertTrue(endowmentTimelock.hasRole(PROPOSER_ROLE, SC_VETO_CONTRACT)); | ||
| assertTrue(endowmentTimelock.hasRole(EXECUTOR_ROLE, address(0)), "executor role must be open"); | ||
| assertTrue(endowmentTimelock.hasRole(TIMELOCK_ADMIN_ROLE, ENDOWMENT_TIMELOCK), "self-administered"); |
There was a problem hiding this comment.
Prove the Timelock has no additional privileged accounts
These positive role checks do not establish the claimed role configuration: an unexpected proposer or TIMELOCK_ADMIN_ROLE holder can coexist with the expected accounts while every assertion passes. Such an account could schedule Endowment transactions or grant and revoke privileged roles, so the review needs to validate the complete grant/revoke history or otherwise prove the exact privileged membership rather than checking only selected addresses.
Useful? React with 👍 / 👎.
| ISecurityCouncil scVeto = ISecurityCouncil(SC_VETO_CONTRACT); | ||
| assertEq(scVeto.owner(), SECURITY_COUNCIL_SAFE); | ||
| assertEq(scVeto.timelock(), ENDOWMENT_TIMELOCK); | ||
| assertGt(scVeto.expiration(), block.timestamp, "veto power must not be expired"); |
There was a problem hiding this comment.
Pin the Security Council veto expiration
Checking only that the wrapper has not expired at the fork allows an expiration shortly after the simulated proposal lifecycle to pass, even though the review claims the veto remains active until August 2028. In that scenario queued Endowment operations would lose Security Council oversight much earlier than reviewed, so assert the expected expiration timestamp exactly.
Useful? React with 👍 / 👎.
| ISecurityCouncil scVeto = ISecurityCouncil(SC_VETO_CONTRACT); | ||
| assertEq(scVeto.owner(), SECURITY_COUNCIL_SAFE); | ||
| assertEq(scVeto.timelock(), ENDOWMENT_TIMELOCK); | ||
| assertGt(scVeto.expiration(), block.timestamp, "veto power must not be expired"); |
There was a problem hiding this comment.
Pin the veto wrapper implementation
The getter assertions and successful veto call do not prove that this privileged proposer is the claimed cancel-only wrapper. A different implementation could return the expected owner, timelock, and expiration while exposing an additional path that schedules arbitrary Endowment operations, and the review would still pass; assert the reviewed runtime bytecode or code hash so the veto-only property is reproducible.
Useful? React with 👍 / 👎.
- Foundation cannot revoke the SC wrapper's PROPOSER_ROLE directly (role admin = the timelock itself, so changes are delayed + vetoable) - updateDelay reverts unless called by the timelock itself - the SC wrapper does not forward schedule calls (cancel-only) - role-admin invariants asserted in _beforeProposal
- pin both Endowment module addresses (Zodiac Roles + Allowance Module) and require the pagination next-pointer to prove list completeness - pin all five Foundation Safe owners exactly - negative role checks for every plausible privileged candidate + document the complete role event history (10 events, deploy..renounce) - pin SC veto expiration to 1849227635 (2028-08-07) - pin runtime codehashes of the EndowmentTimelock and the veto wrapper
|
Addressed all five Codex P1 comments in d5171bc:
|
Prove on-fork that neither Endowment module can alter the Safe's owner set: karpatkey's MANAGER role rejects swapOwner via execTransactionWithRole, and non-modules are stopped at execTransactionFromModule (GS104). Closes the last probe-only leg of the 'no on-chain eviction path' claim.
Draft calldata review — Empowering the ENS Foundation
Review of the Tally draft by nick.eth. Calldata was derived manually from the proposal spec and matches the draft byte-for-byte. Test simulates the full lifecycle and passes.
What it does
0x9C7d…A19E, 3-of-5: nick, alexurbelis, kartik + 2 unlabeled EOAs).EndowmentTimelock(0x0bcC…406C).Impact: the DAO gives up direct control of the Endowment. After this executes, only the Foundation Safe can initiate Endowment transactions, everything waits 9 days, and the Security Council can cancel during that window (until Aug 2028). The DAO keeps no role on the new timelock — reverting this later requires the Foundation's cooperation.
What checks out
Findings (none block the calldata)
Reproduce