fix(security-upgrades): review fixes for #420 — RR ordering, EFRM treasury, deterministic salts, eyeball block

[seongyun-ko]

Summary

Addresses findings from a 6-lens review of #420 (two confirmed showstoppers + five more critical + three high-severity), then end-to-end verified on a Tenderly VNet fork of mainnet with all 3 batches landed and 7 user-facing QA scenarios passing.

Review write-up: Notion — PR #420 6-Lens Review.

Targets pankaj/feat/security-upgrades-PR-scripts so it can be merged into #420 before broadcast.

What's in this PR

Review-driven fixes (commit e576b2c4)

#	Item	Files
C1	Reorder upgrade batch: 21 proxy upgrades + beacon → RR swap → 2 initializers. Old proxy impls gate _authorizeUpgrade via roleRegistry.onlyProtocolUpgrader, which the new RR drops. Swapping RR first bricks every subsequent upgradeTo with a fallback revert. Confirmed via on-chain selector scan of 4 mainnet impls (LP / EFRM / EFNM / WRN — all contain 0x5006bb7b, none contain 0x6ac5f9eb).	transactions.s.sol
C2	EFRM _treasury constructor arg = WITHDRAW_REQUEST_NFT_BUYBACK_SAFE in both files (was TREASURY in transactions.s.sol bytecode-verify + immutable-assert, would have broken verifyDeployedBytecode). Matches current mainnet EFRM.treasury() = 0x2f5301…F0F. Constant name unified to RM_MAX_EXIT_FEE_SPLIT_TO_TREASURY_BPS.	both
C3+C7	bytes20 GIT_COMMIT_SHA TBD placeholder in all 3 scripts. commitHashSalt = bytes32(GIT_COMMIT_SHA). Preflight rejects bytes20(0). Timelock salts → keccak256(abi.encode("batch-1"|"batch-2"|"batch-revert", commitHashSalt)) — deterministic across re-runs, no more block.number drift.	all 3
C4	LP_MAX_WITHDRAW_AMOUNT = 3_000 ether (was 1_000 ether dummy).	transactions
C6	ADMIN_STALE_ORACLE_REPORT_BLOCK_WINDOW = 7200 * 14 (14 days; comment previously claimed 14d but value was 7d).	deploy + transactions
H3	_printPleaseEyeball() at top of run() in both deploy and transactions. Prints GIT_COMMIT_SHA, all 26 deployed impl addresses, all 9 HOLDER_*, 10 rate-limit caps/refills, 11 pause durations, finalized-withdrawal limit, LP bounds, WNFT band — before preflight runs.	deploy + transactions
H4	WNFT band tightened from [1, 4 ether] → [0.95 ether, 1.15 ether] (≈ [0.9×, 1.1×] of live amountPerShareCeil() ~1.05). Preflight asserts MIN > 0 and MAX > MIN.	both
H5	LIQUIFIER_STALE_PRICE_WINDOW = 1 days (was 7 days).	deploy + transactions
H8	_preflightRoleHashes() instantiates new RoleRegistry(revokeAdminProxy) and cross-checks all 9 hardcoded keccak256 role-IDs against RR getters. Catches typos before any Safe JSON is written.	transactions

Findings from Tenderly VNet verification (commit 3e6d5fa9)

VNet 31698b4b-77c5-4b96-a057-855ccaac67b8 (fork of mainnet @ 25193922). Ran deploy.s.sol --broadcast then submitted all 5 Safe JSONs via cast rpc eth_sendTransaction with Tenderly impersonation. Two real bugs surfaced; both fixed in this commit.

F1 — _auctionImmSels() lists a removed selector (transactions.s.sol:995-1005)
The new AuctionManager dropped membershipManagerContractAddress(). It existed pre-upgrade so the _safeSnapshot filter didn't strip it; _postSnap's strict re-call then reverts with "previously-surviving selector now reverts". Fix: remove from the selector list (6 → 5 entries).

F2 — 3 EFNM rate-limit buckets already exist on mainnet (transactions.s.sol:1738-1757)
UNRESTAKING_LIMIT_ID, EXIT_REQUEST_LIMIT_ID, CONSOLIDATION_REQUEST_LIMIT_ID were configured in a prior deployment. PR #420's Batch 2 blindly called createNewLimiter for all 10 → LimitAlreadyExists(), atomic batch revert. Fix: switch the 3 EFNM buckets to setCapacity + setRefillRate; the 7 token + restaker buckets still use createNewLimiter (genuinely new). Existing consumer wiring for the EFNM 3 is preserved.

F3 — Operational follow-up, not in script (ROLE_MIGRATION.md §6.1)
EtherFiRedemptionManager.tokenToRedemptionInfo[EETH/WEETH] defaults to zero after the upgrade. redeemEEth / redeemWeEth revert until OPERATION_TIMELOCK calls initializeTokenParameters([EETH, WEETH], …). Documented as Day-10+ operational follow-up alongside H7 (per-address rate-limit pre-seeding) and stale-holder hygiene.

Misc (deploy.s.sol) — set logging=false on LiquidityPool / Liquifier / EtherFiAdmin deploys; Utils.formatStaticParam reverts with "Unsupported static type" on ConstructorAddresses struct args. Deployment still succeeds; only the JSON pretty-print is skipped for these 3.

Verification (Tenderly VNet 31698b4b-77c5-4b96-a057-855ccaac67b8)

Upgrade execution

Step	Result
28 CREATE2 deploys	all status=0x1
Batch 1 schedule (UPGRADE_TIMELOCK)	0x38de2f…ed6f ✅
Batch 2 schedule (OPERATING_TIMELOCK)	0x8e4ea5…1da6f ✅
Warp +10 days	✅
Batch 1 execute	0x9cea2f…b364e ✅ gas=1,924,734
Batch 2 execute	0xbfcce8…bd544 ✅ gas=1,076,811
Batch 3 (LP setMax/setMin)	0xe9b477…64dd0 + 0x4f38eb…c4b43 ✅

Post-upgrade state verified

• All 22 proxy ERC1967 impl slots point to the deployed new impls ✓
• EFRM.treasury() = 0x2f5301…F0F (BUYBACK_SAFE — C2 fix verified live) ✓
• All 9 RolesLibrary roles populated; all 31 legacy roles zero holders ✓
• LP.maxWithdrawAmount = 3,000 ETH, LP.minWithdrawAmount = 100k gwei ✓
• EFAdmin.maxFinalizedWithdrawalAmountPerDay ≥ 0 (set via Batch 2) ✓
• LP.pauseUntilDuration = 7 days ✓

QA scenarios (8 run)

#	Scenario	Result
1	eETH deposit (1 ETH)	✅ minted ~0.9999 eETH
2	weETH wrap + transfer	✅
3	LP.requestWithdraw → mint NFT	✅
4	Blacklist → transfer reverts with Blacklisted(addr)	✅
5	Unblacklist → transfer succeeds	✅
6	pauseContractUntil via guardian → deposit reverts with PausedUntil(ts)	✅
7	unPauseContract after pauseContractUntil	❓ requires different invocation; functional pause works (UX note, not blocker)
8	EFRM instant redeem	⚠ blocked until F3 follow-up

Pre-broadcast checklist (PLEASE EYEBALL block surfaces all of these)

1. GIT_COMMIT_SHA (3 places — deploy/transactions/revert.s.sol, must match)
2. 6 HOLDER_* placeholders (SUPER_GUARDIAN, GUARDIAN, ORACLE/HOUSEKEEPING/EXECUTOR/EIGENPOD_OPERATIONS)
3. 20 rate-limit constants (10 caps + 10 refill rates, in gwei)
4. 11 PAUSE_UNTIL_* durations
5. ADMIN_DAILY_FINALIZED_WITHDRAWAL_LIMIT
6. Re-verify WNFT band still centers on amountPerShareCeil() at deploy day
7. Post-upgrade: EFRM.initializeTokenParameters([EETH, WEETH], …) via OPERATING_TIMELOCK (F3)
8. Post-upgrade (H7): per-address rate-limit pre-seeding for top holders

Items deferred / acked

• C5 (no floor on rate-limit caps): acked; signer fills before deploy, eyeball block surfaces them.
• H1 (10d freeze window): documented in ROLE_MIGRATION.md §4 — schedule Batches 1+2 simultaneously, execute 1→2→3 back-to-back.
• H2 (legacy revokes baked at script-gen time): acked; documented in ROLE_MIGRATION.md.
• H6 (atomic batch grief): noted, follow-up iteration.
• H7 (Day-0 per-address rate-limit pre-seeding): now §6.2 in ROLE_MIGRATION.md.

Test plan

• forge build against PR Pankaj/feat/security upgrades pr scripts #420 base — clean ✓
• forge script transactions.s.sol --fork-url $TENDERLY_RPC dry-run end-to-end — all 10 steps green ✓
• PLEASE EYEBALL block prints all expected TBDs at top of output ✓
• _preflightRoleHashes passes against fresh RR instance ✓
• verifyDeployedBytecode matches with WITHDRAW_REQUEST_NFT_BUYBACK_SAFE constructor arg ✓
• Batch 1 execution order verified: grants → 21 proxy upgrades → beacon → RR swap → 2 initializers → 31 legacy revokes ✓
• All 3 Safe JSONs submitted on Tenderly VNet, state mutations verified post-execution ✓
• User-facing QA (deposit / wrap / transfer / withdraw / blacklist / pause) ✓

🤖 Generated with Claude Code

---

Note

High Risk
Changes mainnet upgrade batch ordering, redemption manager wiring, and timelock salts—mistakes could brick upgrades or misconfigure withdrawals/redemptions until follow-up ops run.

Overview
Hardens the 26Q2 security upgrade Forge scripts and runbook after PR #420 review and Tenderly fork QA—no on-chain contract source changes in this diff.

Batch 1 ordering (showstopper fix): transactions.s.sol no longer upgrades RoleRegistry first. It runs grants → 21 proxy upgrades + beacon → RR swap → two onlyUpgradeTimelock initializers → legacy revokes, because legacy proxy impls still call roleRegistry.onlyProtocolUpgrader, which the new registry drops.

Deploy / verify alignment: EtherFiRedemptionManager treasury immutables and bytecode checks use WITHDRAW_REQUEST_NFT_BUYBACK_SAFE (not generic TREASURY). ADMIN_STALE_ORACLE_REPORT_BLOCK_WINDOW is 7200 * 14. WithdrawRequestNFT share-rate band tightens to 0.95–1.15 ether. Shared GIT_COMMIT_SHA drives CREATE2 and timelock salts (batch-1 / batch-2 / batch-revert) instead of block.number.

Preflight & ops visibility: _printPleaseEyeball, expanded _preflight, and _preflightRoleHashes() run before JSON generation. Deploy skips broken struct logging on three contracts.

Batch 2 fork fixes: Existing EtherFiNodesManager rate buckets use setCapacity / setRefillRate instead of createNewLimiter. _auctionImmSels drops removed membershipManagerContractAddress().

Docs: ROLE_MIGRATION.md §6 adds post-upgrade follow-ups (initializeTokenParameters, per-address rate limits, stale role holders).

^{Reviewed by Cursor Bugbot for commit cb8e5e0. Bugbot is set up for automated code reviews on this repo. Configure here.}

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.