chore: deploy + upgrade scripts for non-EigenPod withdrawal credentials - #500
Conversation
Deploys the four implementations changed by PR #485 (EtherFiNodesManager, EtherFiNode, StakingManager, EtherFiAdmin) via CREATE2, and builds the UPGRADE_TIMELOCK batch that repoints them and drains the retired Treasury into the LiquidityPool. Constructor args are sourced from the live deployments' immutables. The timelock salt is pinned rather than block-derived so the Safe tx hashes signers reproduce are stable.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Moves TREASURY_LEGACY into the script instead of Deployed.s.sol, trims the comments, and adds: - immutable snapshots for ENM/StakingManager/EtherFiAdmin taken before the batch and re-verified after, plus verifyNotReinitializable on each - fork tests exercising the upgraded contracts: a pod-less validator spin-up, an EL-triggered exit on that pod-less node through the EIP-7002 predeploy, and the classic EigenLayer queue/complete withdrawal path Roles for the fork tests are granted by pranking the UPGRADE_TIMELOCK (RoleRegistry owner) outside the batch, so the proposal calldata and the Safe tx hashes in 3CP-secure#662 are unchanged.
|
|
||
| require(LIQUIDITY_POOL.balance > lpBefore, "liquidity pool received no withdrawal proceeds"); | ||
| console2.log("[OK] EigenLayer withdrawal completed. LP delta:", LIQUIDITY_POOL.balance - lpBefore); | ||
| } |
There was a problem hiding this comment.
Legacy node fails EigenLayer fork test
Medium Severity
forkTestEigenLayerWithdrawal resolves legacy validator 10270 via linkLegacyValidatorIds, then calls getEigenPod, queueETHWithdrawal, and completeQueuedETHWithdrawals. Those manager methods all run _validateNode, which reverts UnknownNode unless the node is in deployedEtherFiNodes. Early IDs are the ones the upgraded contracts themselves treat as not backfilled, so run() can revert after the upgrade batch and skip the rest of verification.
Reviewed by Cursor Bugbot for commit f2ebe53. Configure here.
There was a problem hiding this comment.
Correction after re-checking live head f2ebe53: I do not think this is valid as an UnknownNode issue.
Evidence:
- validator
10270maps to node0xb94ad...6f73. StakingManager.deployedEtherFiNodes(node)is true, andnode.etherFiNodesManager()points back to ENM.getEigenPod(node)returns nonzero pod0xf547...d980, so this fixture should pass_validateNodefor the cited calls.
Smallest next step: no script change for this fixture unless a fork run shows a different revert.
Test suite baseline
Comparing the failing test names as sets: 121 unique failures on each, 0 introduced, 0 fixed. All 128 failures are pre-existing on The pre-existing failures are the fork-fragility this repo's CLAUDE.md already documents — 6 from a missing Worth fixing separately: a 128-failure baseline means the suite can't gate anything, and it masks real regressions. |
Verifying the impl pointer and immutables does not prove the oracle still works, and EtherFiAdmin is the oracle entry point. This applies the upgrade and runs a real cycle: the three live committee members submit at the real quorum of 3, consensus is reached, then executeTasks runs after the postReportWaitTimeInSlots delay. Asserts the rebase applies exactly (TVL +50 ETH), the eETH exchange rate rises, and both report cursors advance. Kept separate from transactions.s.sol to avoid colliding with concurrent work on that file. The oracle config is left untouched: quorum cannot be lowered to 1 because _checkQuorum requires numActiveCommitteeMembers < 2 * quorumSize, so the real members are pranked instead.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1726f3b. Configure here.
| } | ||
| require( | ||
| oracle.isConsensusReached(oracle.generateReportHash(report)), "consensus not reached" | ||
| ); |
There was a problem hiding this comment.
Pending report blocks cycle script
Medium Severity
The readiness loop only calls verifyReport, which does not require the last published report to have been handled. submitReport then reverts LastReportNotHandled whenever a report is sitting between consensus and executeTasks, a routine mainnet window. The upgrade can be fine and the script still never exercises the upgraded EtherFiAdmin.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 1726f3b. Configure here.
There was a problem hiding this comment.
I agree this is valid on live head 1726f3b.
Evidence: OracleReportCycle only gates readiness with verifyReport, but submitReport first runs shouldSubmitReport, which reverts LastReportNotHandled when lastPublishedReportRefSlot != EtherFiAdmin.lastHandledReportRefSlot().
Smallest next step: preflight/sync the pending report state before submitting the new fork report, then rerun the oracle cycle script.
| vm.roll(block.number + 32); | ||
| } | ||
| } | ||
| require(ready, "could not reach a finalized report window"); |
There was a problem hiding this comment.
Report window loop misses last check
Low Severity
Each failed verifyReport warps 32 slots, but the loop can exit after that warp without retrying. With a 40-iteration cap this advances at most 1248 slots before a successful check, short of one 1280-slot period plus the two-epoch finality buffer. A fork taken early in the period can revert even when one more check would pass.
Reviewed by Cursor Bugbot for commit 1726f3b. Configure here.
There was a problem hiding this comment.
Valid on live head 1726f3b. The loop verifies before each 32-slot warp, so the last verification happens after 39 warps (=1248 slots); a 40th failure warps to 1280 and exits without rechecking. Since verifyReport still requires the report epoch finality buffer, forks early in the period can fail here. Smallest next step: retry after the final warp or raise the bound to cover one full report period plus finality. Triage-only, no changes pushed.
Three genuine failures on master, plus three that are local config. - Withdraw.t.sol, Validator-Flows.t.sol: _syncOracleReportState removed and re-added AVS_OPERATOR_1/2 as committee members, but both were rotated off mainnet, so removeCommitteeMember reverts NotRegistered. Write the committee state and quorum directly instead, so setup no longer depends on who is registered today. setQuorumSize cannot be used because _checkQuorum enforces strict majority against the live member count. - EtherFiRedemptionManager.t.sol: test_end_to_end_redeem_stETH assumed a 20% watermark exceeds the restaker's stETH. It no longer does (456,305 stETH against 2,141,454 TVL = 21.3%), so the redeem succeeded where the test expected a revert. Derive the watermark from live state and assert it stays under maxLowWatermarkInBpsOfTvl so future drift fails loudly. - .example.env: document OP_RPC_URL and SCROLL_RPC_URL, which several suites need and CI supplies as secrets. forge test --no-match-contract 'Invariant|Handler': master 1305 passed / 6 failed -> branch 1594 passed / 3 failed. The 3 remaining are OP_RPC_URL not being set locally; they pass in CI.
Correction to the baseline above, and the actual fixMy earlier comment was wrong. The "128 failures on both branch and master" was an artifact of the command I ran, not the state of the suite. I passed With the correct command,
The 6 on master were 3 genuine test bugs and 3 local config. Fixed
Remaining3 × Fork block pinning turned out not to be needed — no test failed on block drift once the command was right. |
0548f92
into
pankaj/feat/non-eigenpod-withdrawal-credentials
2 participants
Deploy and upgrade scripts for the non-EigenPod withdrawal credentials release. Branches off #485.
Governance proposal: 3CP-secure#662.
What's here
script/upgrades/non-eigenpod-creds/deploy.s.sol— CREATE2-deploys the four implementations PR #485 changes:upgradeToupgradeToupgradeToStakingManager.upgradeEtherFiNodeConstructor args are read off the live deployments' immutables rather than copied from an older script — the existing
reaudit-fixesdeploy passes five args toEtherFiNode, which now takes four.script/upgrades/non-eigenpod-creds/transactions.s.sol— builds the UPGRADE_TIMELOCK batch (5 calls: the four upgrades plusTreasury.withdraw), prints the schedule/execute calldata, then fork-simulates schedule → +10 days → execute and asserts the end state.script/deploys/Deployed.s.sol— addsTREASURY_LEGACYfor the retired Treasury at0x6329004E.Notable findings while building this
0x6329004Eis not a proxy; it's plain OZOwnable(src/archive/Treasury.sol) withowner() == UPGRADE_TIMELOCKand an existingwithdraw(uint256,address). 9.712223546514004611 ETH.keccak256(..., block.number)as inCrossPodApproval. A block-derived salt changes the calldata every run, which makes the Safe tx hashes signers reproduce unstable.UPGRADE_TIMELOCK.getMinDelay()is 864000 (10 days).CrossPodApproval/transactions.s.solstill hardcodes 259200, which is stale.depositDataRootGenerator(already on mainnet at0xa98F0A109dDC86c4A1dB974256d106C4B237171A). Its address is baked into the creation code, so reproducing the CREATE2 address requires linking against it. Verified the impl address is sender-independent.forge build --sizesexits non-zero on master, from two test-only invariant handlers (FrozenRateWithdrawalHandler28,107 andProtocolInvariantsHandler29,310). Neither is deployed, but it means the EIP-170 gate can't be used in CI as-is — worth a follow-up given EtherFiNodesManager has 98 bytes of headroom.AvsOperatorManager.adminForwardCallis gated byOPERATION_MULTISIG_ROLE, held by the Operating Safe0x2aCA71and not by UPGRADE_TIMELOCK, so it cannot ride in the upgrade batch. It also isn't 14 AVSs — it's 14 registered operator proxies across 18 AVSs on the legacy AVSDirectory. AderegisterAvsOperators()function is being added here in a follow-up commit.Verification
Local fork (
--fork-url $MAINNET_RPC_URL): CREATE2 addresses match the constants in the calldata, deployed bytecode matches a fresh compile (ContractCodeChecker), all four implementation slots repoint, ENM gainswithdrawalCredentialTarget/disablePodand dropssweepFunds(uint256), Treasury drains to 0 and the LiquidityPool is credited exactly. All assertions pass.Tenderly VNET
5de0fdbf-8001-437d-9a88-4cf2b26d2d23: impls deployed to their predicted addresses at the expected runtime sizes, then the two real Safe txs — schedule 84,006 gas, execute 175,193 gas, both far under the EIP-7825 cap.Note
Low Risk
No production contract changes in the diff—only deployment scripts and fork tests; operational risk is mis-encoded governance calldata if constants drift from CREATE2 deploys.
Overview
Adds Forge upgrade tooling for the non-EigenPod withdrawal-credentials release (PR #485): CREATE2 deployment of four implementations (
EtherFiNodesManager,StakingManager,EtherFiAdmin,EtherFiNode), a UPGRADE_TIMELOCK batch builder that upgrades those proxies/beacon and sweeps the legacy Treasury into the LiquidityPool, plus fork simulation (schedule → 10-day delay → execute) with bytecode, immutables, and post-upgrade smoke tests (pod-less validator, EL exit, EigenLayer withdrawal).OracleReportCycle.s.solapplies the same upgrades directly from the timelock on a fork and runs a full oracle path (three live committee members → consensus →executeTasks), asserting TVL/rebase and cursor advancement on upgradedEtherFiAdmin.Documents
OP_RPC_URLandSCROLL_RPC_URLin.example.envfor fork-dependent tests.Test hardening for drifting mainnet fork state: integration tests force oracle committee/quorum via storage instead of add/remove members; redemption low-watermark test derives blocking bps from live restaker stETH vs TVL.
Reviewed by Cursor Bugbot for commit 93ae801. Bugbot is set up for automated code reviews on this repo. Configure here.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.