Pankaj/feat/security upgrades pr scripts#420
Open
pankajjagtapp wants to merge 38 commits into
Open
Conversation
…ion scripts Added a new deployment script for the 26Q2 security upgrades, including the deployment of new implementations and the Blacklister proxy. Introduced a role migration document outlining the necessary role adjustments post-upgrade. The transactions script has been set up for timelocked upgrades and operational parameter configurations, requiring manual input for addresses before execution.
pankajjagtapp
changed the base branch from
master
to
pankaj/feat/security-upgrades
📊 Forge Coverage ReportGenerated by workflow run #732 |
💡 Codex Reviewsmart-contracts/src/EtherFiOracle.sol Lines 257 to 260 in 101125f  Allow committee bootstrap when quorum exceeds one
smart-contracts/src/EtherFiAdmin.sol Lines 360 to 363 in 101125f Initialize report caps before enforcing new validation
The new report guards compare per-day activity against ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
…ation Modified the deployment script to adjust the LIQUIFIER_STALE_PRICE_WINDOW to 7 days and increased ADMIN_MAX_VALIDATOR_TASK_BATCH_SIZE to 100. Updated ADMIN_MAX_FINALIZED_WITHDRAWAL_AMOUNT_PER_DAY to 100,000 ether and reduced ADMIN_MAX_VALIDATORS_TO_APPROVE_PER_DAY to 1,000. Enhanced the role migration documentation to reflect the new role mappings and operational parameters for the security upgrades.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Introduced a new script to revert the security upgrades by re-pointing proxies to their pre-upgrade implementations. The script includes functionality to confirm current implementations, take snapshots before the revert, and execute the revert process while preserving access control. This allows for a controlled rollback of the recent upgrades if necessary.
5 tasks
…nto pankaj/feat/security-upgrades-PR-scripts
…nto pankaj/feat/security-upgrades-PR-scripts
…nto pankaj/feat/security-upgrades-PR-scripts
…nto pankaj/feat/security-upgrades-PR-scripts
…nto pankaj/feat/security-upgrades-PR-scripts
…y-upgrades-PR-scripts
…nto pankaj/feat/security-upgrades-PR-scripts
…stic salts, eyeball block Addresses findings from PR #420 6-lens review: C1: Reorder upgrade batch. RoleRegistry impl swap moved from FIRST to AFTER the 21 proxy upgrades + beacon, BEFORE the 2 onlyUpgradeTimelock initializers. Old proxy impls gate _authorizeUpgrade via roleRegistry.onlyProtocolUpgrader, which the new RR drops. Swapping RR first would brick 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). C2: EFRM _treasury constructor arg now WITHDRAW_REQUEST_NFT_BUYBACK_SAFE in BOTH deploy.s.sol (already correct) and transactions.s.sol bytecode-verify + immutable-assert (was TREASURY, would break verifyDeployedBytecode). Matches current mainnet EFRM.treasury() = 0x2f5301..78f0F. Constant name unified to RM_MAX_EXIT_FEE_SPLIT_TO_TREASURY_BPS across both files (matches contract field maxExitFeeSplitToTreasuryInBps). C3+C7: GIT_COMMIT_SHA TBD placeholder + deterministic timelock salts. Replaces bytes32(0) placeholder with explicit bytes20 GIT_COMMIT_SHA = TBD; must be set to first 20 bytes of release commit before broadcast. commitHashSalt derived from it; preflight rejects bytes20(0) in all 3 scripts. Timelock salts now keccak256(abi.encode("batch-{1,2,revert}", commitHashSalt)) — deterministic across re-runs, no more block.number drift between fork dry-run and broadcast. C4: LP_MAX_WITHDRAW_AMOUNT bumped from 1_000 ether (dummy) to 3_000 ether. C6: ADMIN_STALE_ORACLE_REPORT_BLOCK_WINDOW = 7200 * 14 (14 days; previously value 7200*7 with comment claiming 14 days). H3: New _printPleaseEyeball() block at top of run() in both deploy.s.sol and transactions.s.sol prints every TBD constant — 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, so signers can eyeball values in one place. H4: WNFT share-rate band tightened from [1, 4 ether] to [0.95 ether, 1.15 ether] — ~[0.9, 1.1] x live amountPerShareCeil() snapshot (~1.05). Preflight asserts MIN > 0 and MAX > MIN. H5: LIQUIFIER_STALE_PRICE_WINDOW = 1 days (was 7 days; Chainlink stETH/ETH heartbeat is 24h). H8: New _preflightRoleHashes() instantiates new RoleRegistry(revokeAdminProxy) and cross-checks all 9 hardcoded keccak256(role-string) constants against the RR getters. Catches typos in role strings BEFORE any Safe JSON is written. Operational items still on signer's plate before broadcast: GIT_COMMIT_SHA, 6 HOLDER_* placeholders, 20 rate-limit gwei values, 11 pause durations, finalized-withdrawal setpoint, re-verify WNFT band centers on live rate. The PLEASE EYEBALL block surfaces all of them on every run. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
8 tasks
…ation Verified PR #420 end-to-end on a Tenderly VNet forked from mainnet (31698b4b-77c5-4b96-a057-855ccaac67b8). All 3 batches landed clean: schedule both 10d/2d timelocks together, warp +10d, execute, then the instant LP-bounds batch. 7 of 8 user-facing QA scenarios passed (deposit / wrap / transfer / requestWithdraw / blacklist / unblacklist / pauseContractUntil); two real bugs surfaced and are fixed here, one operational follow-up is documented. F1 (transactions.s.sol _auctionImmSels): new AuctionManager dropped membershipManagerContractAddress(). It existed pre-upgrade so the _safeSnapshot filter let it through; _postSnap then reverted with "previously-surviving selector now reverts". Removed from the selector list (6 -> 5 entries). F2 (transactions.s.sol executeOperatingConfig): UNRESTAKING / EXIT_REQUEST / CONSOLIDATION_REQUEST rate-limit buckets already exist on mainnet from a prior deployment (limitExists() == true). PR #420 unconditionally called createNewLimiter for all 10 buckets -> LimitAlreadyExists() on these 3, atomic batch revert. Switched the 3 EFNM buckets to setCapacity + setRefillRate; the 7 token + restaker buckets still use createNewLimiter (they are genuinely new). The existing consumer wiring for the 3 EFNM buckets is preserved, so no updateConsumer call is needed for them. F3 (ROLE_MIGRATION.md §6.1, operational - no script change): EtherFiRedemptionManager.tokenToRedemptionInfo[EETH/WEETH] defaults to zero after the upgrade. redeemEEth / redeemWeEth revert until OPERATION_TIMELOCK calls initializeTokenParameters([EETH, WEETH], ...). Documented as a Day-10+ operational follow-up alongside H7 (per-address rate-limit pre-seeding) and stale-holder hygiene. Also (deploy.s.sol): set logging=false on LiquidityPool, Liquifier, and EtherFiAdmin deploys because Utils.formatStaticParam reverts with "Unsupported static type" on the ConstructorAddresses struct argument. Deployment still succeeds; only the JSON pretty-print is skipped for these 3. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…://github.com/etherfi-protocol/smart-contracts into seongyun/fix/security-upgrades-scripts-review
…pgrades-scripts-review fix(security-upgrades): review fixes for #420 — RR ordering, EFRM treasury, deterministic salts, eyeball block
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit bc3bee3. Configure here.
| uint256 public constant ADMIN_MAX_REQUESTS_TO_FINALIZE_PER_REPORT = 2_000; | ||
|
|
||
| // oracle — EtherFiOracle immutable params | ||
| uint256 public constant ORACLE_MIN_QUORUM_SIZE = 3; // enforces min 3/5 quorum for consensus |
There was a problem hiding this comment.
Type mismatch for ORACLE_MIN_QUORUM_SIZE across scripts
Low Severity
ORACLE_MIN_QUORUM_SIZE is declared as uint256 in deploy.s.sol but as uint32 in transactions.s.sol. The transactions.s.sol header explicitly states these constructor params "MUST MATCH deploy.s.sol exactly." While abi.encode pads both to identical 32-byte words for the current value of 3, the type divergence undermines the stated invariant and could silently truncate if a future value exceeds uint32.max.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit bc3bee3. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
Medium Risk
Scripts orchestrate wide mainnet proxy/beacon upgrades and timelocked rollback; mistakes in unset
GIT_COMMIT_SHA/PRE_*or batch ordering could brick ops paths, but on-chain logic is unchanged in this diff.Overview
Adds mainnet upgrade runbook tooling under
script/upgrades/security-upgrades/for the 26Q2 security release (PR #385): a CREATE2 deploy script, a timelock revert script, and a role/ops migration guide, plus smallDeployed.s.soladdress constants used by those scripts.deploy.s.soldeploys new implementations (and Blacklister / RevokeAdmin UUPS proxies) via a sharedGIT_COMMIT_SHAsalt, wiresRoleRegistrytoRevokeAdmin, then deploys the rest of the upgraded contracts withblacklisterProxyand mainnet addresses fromDeployed. It bakes in reviewed immutables (admin/oracle caps, redemption ceilings, WRN share-rate band, etc.) and_preflightblocks broadcast untilGIT_COMMIT_SHAand WRN bounds are set.revert.s.solbuilds a 10-dayUPGRADE_TIMELOCKbatch to roll 22 UUPS proxies +RoleRegistryback to snapshotPRE_*impls, reverts the EtherFiNode beacon viaStakingManager.upgradeEtherFiNode, keepsRoleRegistrylast so upgrade auth still works mid-batch, dry-runs on a fork, and emits Saferevert_schedule.json/revert_execute.json. It documents what revert does not undo (one-shot inits, rate buckets, pause durations, role grants).ROLE_MIGRATION.mdis the ops worksheet for the companiontransactions.s.solflow: 9RolesLibraryroles → function map, holder placeholders, rate-limiter buckets, pause durations, and three-batch execution order (upgrade / operating / instant LP bounds), including post-upgrade follow-ups.Deployed.s.solgainsETH2_DEPOSIT_CONTRACTand token constants (WETH,STETH,WSTETH,EIGEN) forDepositAdapter/StakingManagerconstructor args in deploy.Reviewed by Cursor Bugbot for commit bc3bee3. Bugbot is set up for automated code reviews on this repo. Configure here.