Pankaj/feat/security upgrades pr scripts#420
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}
| address constant LIDO = 0xae7ab96520DE3A18E5e111B5EaAb095312D7fE84; | ||
| address constant STETH_ETH_CURVE_POOL = 0xDC24316b9AE028F1497c275EB9192a3Ea0f67022; | ||
| uint256 constant LIQUIFIER_MIN_DISCOUNT_BPS = 100; | ||
| uint256 constant LIQUIFIER_STALE_PRICE_WINDOW = 24 hours; |
There was a problem hiding this comment.
Stale price window mismatch between deploy and verification
High Severity
LIQUIFIER_STALE_PRICE_WINDOW is 24 hours in transactions.s.sol but 7 days in deploy.s.sol. The comment on line 73–74 of transactions.s.sol explicitly states these constructor params "MUST MATCH deploy.s.sol exactly" since verifyDeployedBytecode rebuilds each implementation locally and compares bytecode. This mismatch will cause the Liquifier bytecode verification to fail, or if one value is corrected to match the wrong file, the deployed contract will have an incorrect stale price window.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 7f71453. Configure here.
|
|
||
| // EtherFiAdmin | ||
| int256 constant ADMIN_MAX_REBASE_APR_BPS = 1_000; | ||
| uint256 constant ADMIN_MAX_VALIDATOR_TASK_BATCH_SIZE = 25; |
There was a problem hiding this comment.
Validator batch size mismatch between deploy and verification
High Severity
ADMIN_MAX_VALIDATOR_TASK_BATCH_SIZE is 25 in transactions.s.sol but 100 in deploy.s.sol. These are constructor params that the comment says "MUST MATCH deploy.s.sol exactly." This mismatch will cause verifyDeployedBytecode to fail for the EtherFiAdmin implementation, and the ambiguity means one file encodes a 4x different operational ceiling for validator task batches.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 7f71453. Configure here.
| uint256 constant ADMIN_MAX_VALIDATOR_TASK_BATCH_SIZE = 25; | ||
| uint256 constant ADMIN_STALE_ORACLE_REPORT_BLOCK_WINDOW = 7200 * 7; | ||
| uint256 constant ADMIN_MAX_FINALIZED_WITHDRAWAL_AMOUNT_PER_DAY = 100_000 ether; | ||
| uint256 constant ADMIN_MAX_VALIDATORS_TO_APPROVE_PER_DAY = 2_000; |
There was a problem hiding this comment.
Max validators per day mismatch across scripts
High Severity
ADMIN_MAX_VALIDATORS_TO_APPROVE_PER_DAY is 2_000 in transactions.s.sol but 1_000 in deploy.s.sol. These constructor params "MUST MATCH deploy.s.sol exactly" per the explicit comment. This mismatch causes the EtherFiAdmin bytecode verification to fail and creates ambiguity about the intended daily validator approval ceiling.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 7f71453. Configure here.
| WITHDRAW_REQUEST_NFT_BUYBACK_SAFE, EETH, LIQUIDITY_POOL, MEMBERSHIP_MANAGER, | ||
| ROLE_REGISTRY, blacklisterProxy, ETHERFI_ADMIN, | ||
| WNFT_MIN_ACCEPTABLE_SHARE_RATE, WNFT_MAX_ACCEPTABLE_SHARE_RATE | ||
| ); |
There was a problem hiding this comment.
WithdrawRequestNFT treasury address mismatch between scripts
High Severity
deploy.s.sol constructs WithdrawRequestNFT with TREASURY (0x0c83...) as the treasury argument, while transactions.s.sol uses WITHDRAW_REQUEST_NFT_BUYBACK_SAFE (0x2f53...) — a completely different address. The constructor params must match for bytecode verification to pass. Additionally, _verifyImmutablesNFT checks treasury() against WITHDRAW_REQUEST_NFT_BUYBACK_SAFE, which won't match the deployed value if TREASURY was used.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 7f71453. Configure here.
| bytes memory args = abi.encode(ROLE_REGISTRY); | ||
| bytes memory bc = abi.encodePacked(type(BucketRateLimiter).creationCode, args); | ||
| bucketRateLimiterImpl = deploy(name, args, bc, commitHashSalt, true, factory); | ||
| } |
There was a problem hiding this comment.
Deploy deploys wrong rate limiter implementation contract
High Severity
deploy.s.sol deploys BucketRateLimiter as the new implementation for ETHERFI_RATE_LIMITER, but the operating config in transactions.s.sol calls EtherFiRateLimiter.createNewLimiter.selector and EtherFiRateLimiter.updateConsumers.selector on that proxy after the upgrade. BucketRateLimiter lacks these functions entirely (confirmed by grep), so all Batch B rate-limiter operations would revert. The verification in verifyOperatingConfig using limitExists and isConsumerAllowed would also fail.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 7f71453. Configure here.
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.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 6 total unresolved issues (including 5 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 440734f. Configure here.
| require(WeETHToken(WEETH).pauseUntilDuration() == PAUSE_UNTIL_WEETH, "WeETH pause duration mismatch"); | ||
| require(LiquidityPool(payable(LIQUIDITY_POOL)).pauseUntilDuration() == PAUSE_UNTIL_LIQUIDITY_POOL, "LP pause duration mismatch"); | ||
| require(WithdrawRequestNFT(payable(WITHDRAW_REQUEST_NFT)).pauseUntilDuration()== PAUSE_UNTIL_WITHDRAW_REQUEST_NFT, "NFT pause duration mismatch"); | ||
| require(Liquifier(payable(LIQUIFIER)).pauseUntilDuration() == PAUSE_UNTIL_LIQUIFIER, "Liquifier pause duration mismatch"); |
There was a problem hiding this comment.
Incomplete verification of pause duration configuration
Medium Severity
verifyOperatingConfig only checks pauseUntilDuration for 5 of the 12 contracts that have it set in executeOperatingConfig. The 7 missing verifications are for ETHERFI_NODES_MANAGER, ETHERFI_ADMIN, ETHERFI_REDEMPTION_MANAGER, MEMBERSHIP_MANAGER, MEMBERSHIP_NFT, AUCTION_MANAGER, and NODE_OPERATOR_MANAGER. If any of those setPauseUntilDuration calls silently fail or are removed, the verification step won't catch it.
Reviewed by Cursor Bugbot for commit 440734f. Configure here.
1 participant
Note
Medium Risk
Medium risk because these scripts orchestrate timelocked upgrades and role/parameter configuration across many core proxies; incorrect constants or calldata generation could misconfigure production access control or rate limits.
Overview
Introduces a new
script/upgrades/security-upgradespackage to execute PR #385’s rollout: adeploy.s.solscript to deploy new implementations (plus aBlacklisterproxy), and atransactions.s.solscript that generates and dry-runs two timelocked batches—Batch A upgrades 16 proxies + runs two one-shot migrations + grants tier roles, and Batch B configuresEtherFiRateLimiterbuckets, setspauseUntilDurationacross multiple contracts, and grants guardian/ops roles.Adds
revert.s.solto generate/dry-run a timelocked rollback that repoints the same 16 proxies to pre-upgrade implementations (explicitly not undoing storage migrations, limiter config, pause durations, or role grants), and addsROLE_MIGRATION.mdas an ops worksheet mapping modifiers to roles and listing required role holders/operational parameters (intentionally forcing all non-fixed values to be filled before broadcast via_preflight()).Reviewed by Cursor Bugbot for commit 440734f. Bugbot is set up for automated code reviews on this repo. Configure here.