26Q2 - Security Upgrades#385
Open
pankajjagtapp wants to merge 628 commits into
Open
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
📊 Forge Coverage ReportGenerated by workflow run #770 |
seongyun-ko
changed the title
pankajjagtapp
commented
May 12, 2026
|
|
||
| import "../interfaces/IRoleRegistry.sol"; | ||
|
|
||
| contract PausableUntil { |
Contributor
Author
There was a problem hiding this comment.
Does it make sense to make this abstract? All functions internal and never to be deployed ?
Contributor
There was a problem hiding this comment.
fine to make it abstract
pankajjagtapp
commented
May 12, 2026
| import "./utils/PausableUntil.sol"; | ||
|
|
||
| contract EtherFiRateLimiter is IEtherFiRateLimiter, Initializable, UUPSUpgradeable, PausableUpgradeable { | ||
| contract EtherFiRateLimiter is IEtherFiRateLimiter, Initializable, UUPSUpgradeable, PausableUpgradeable, PausableUntil { |
Contributor
Author
There was a problem hiding this comment.
What advantage do we have of adding pausable on Ratelimiter ?
pankajjagtapp
commented
May 13, 2026
Comment on lines
+203
to
+205
| function _transferShares(address _sender, address _recipient, uint256 _sharesAmount) internal whenNotPaused { | ||
| blacklister.nonBlacklisted(_sender); | ||
| blacklister.nonBlacklisted(_recipient); |
Contributor
Author
There was a problem hiding this comment.
Need to add this for safe guard from transferFrom similar to MembershipNFT checks:
blacklister.nonBlacklisted(msg.sender);
pankajjagtapp
commented
May 13, 2026
| } | ||
| } | ||
|
|
||
| uint256 public constant MAX_PAUSE_DURATION = 7 days; |
Contributor
Author
There was a problem hiding this comment.
We would be reverting back to 1 day?
pankajjagtapp
commented
May 14, 2026
| import "./interfaces/IRoleRegistry.sol"; | ||
| import "./interfaces/IBlacklister.sol"; | ||
|
|
||
| contract WeETH is ERC20Upgradeable, UUPSUpgradeable, OwnableUpgradeable, ERC20PermitUpgradeable, IRateProvider, AssetRecovery { |
Contributor
Author
There was a problem hiding this comment.
Why haven't we added PausableUntil to weETH ? @0xpanicError
There was a problem hiding this comment.
we discussed that pausable until shouldn't be applies on token transfer. so was only added on mint/burn on eeth. not any ops on weETH.
Contributor
There was a problem hiding this comment.
That is not exact description of our discussion
https://discord.com/channels/827197391068856371/1501877625784893490/1504007820696354899
Unless we have high confidence on having only 'blacklistUntil', we should add 'pausableUntill'.
pankajjagtapp
commented
May 14, 2026
Contributor
Author
There was a problem hiding this comment.
Review this Pausing/Unpausing functions. Either make them complete across most contracts, or remove this from here.
Cannot have inconsistency
pankajjagtapp
commented
May 14, 2026
| if (_report.protocolFees < 0) return (false, "EtherFiAdmin: protocol fees can't be negative"); | ||
| int128 totalRewards = _report.protocolFees + _report.accruedRewards; | ||
| // protocol fees are less than 20% of total rewards | ||
| if (_report.protocolFees > 0 && _report.protocolFees * 5 > totalRewards) return (false, "EtherFiAdmin: protocol fees exceed 20% total rewards"); |
Contributor
Author
There was a problem hiding this comment.
Would be best to use a defined constant instead of 5
pankajjagtapp
commented
May 14, 2026
Comment on lines
108
to
114
Contributor
Author
There was a problem hiding this comment.
We need to add per node address sweeping function
pankajjagtapp
commented
May 14, 2026
Comment on lines
301
to
304
Contributor
Author
There was a problem hiding this comment.
Need to get rid of this EtherFIAdmin setter
pankajjagtapp
commented
May 14, 2026
Comment on lines
138
to
140
| function redeemEEth(uint256 eEthAmount, address receiver, address outputToken) public whenNotPaused nonReentrant nonBlacklisted(receiver) { | ||
| _redeemEEth(eEthAmount, receiver, outputToken); | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Do we want to check if the sender is blacklisted or not for all functions here?
Contributor
There was a problem hiding this comment.
shuoldn't it be both caller of the function and receiver of the fund which need to be checked?
seongyun-ko
reviewed
May 15, 2026
| uint128 public accumulatedRevenueThreshold; | ||
|
|
||
| mapping(address => bool) public admins; | ||
| mapping(address => bool) public DEPRECATED_admins; |
Contributor
There was a problem hiding this comment.
let's make the deprecated variable, private
seongyun-ko
reviewed
May 15, 2026
|
|
||
| function initializeOnUpgrade(address _membershipManagerContractAddress, uint128 _accumulatedRevenueThreshold, address _etherFiAdminContractAddress, address _nodeOperatorManagerAddress) external onlyOwner { | ||
| require(_membershipManagerContractAddress != address(0) && _etherFiAdminContractAddress != address(0) && _nodeOperatorManagerAddress != address(0), "No Zero Addresses"); | ||
| function initializeOnUpgrade(address _membershipManagerContractAddress, uint128 _accumulatedRevenueThreshold, address _nodeOperatorManagerAddress) external onlyOwner { |
Contributor
There was a problem hiding this comment.
remove this function, if not needed anymore
Addresses three verification gaps found in the upgrade security review: - Vuln 1: ContractCodeChecker only logged Fail and never reverted, so verifyDeployedBytecode printed [OK] regardless of the real bytecode at the impl addresses. Add assertByteCodeMatch, a hard-reverting gate that masks each contract's own 20-byte self-address (UUPSUpgradeable.__self) and tolerates EETH's EIP-712 domain separator (verified via DOMAIN_SEPARATOR()), rejecting any other byte difference. Switch all 26 verify calls in transactions.s.sol to it. - Vuln 2: extend the storage-layout slot-scan fork coverage to the 8 previously-unverified upgraded proxies (EETH, WeETH, EtherFiAdmin, Liquifier, StakingManager, EtherFiNodesManager, EtherFiRedemptionManager, MembershipManager); assert 0 sequential drift and preserved totalSupply for the value-bearing tokens. - Vuln 4: add _verifyReleaseCommit (--ffi) pinning the checkout to GIT_COMMIT_SHA and rejecting a dirty tree, so verification reflects the audited release commit; loud warning when skipped. Also re-adds the FORK_RPC_URL override so verification can run against the Tenderly testnet where the new impls are deployed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The gate treated a difference as a self-address/word region only when the FIRST unequal byte was the region start. If the on-chain and local addresses (or allowed words) share leading bytes, the difference first appears at an interior offset, so the anchored _eq20/_eq32 window misaligned and the gate reverted on legitimately-matching source (~1/256 per embedded address). Replace the anchored skip with a scan-and-mask pass: find each region at its true boundary (masking only where the on-chain side holds its address/word AND the local side holds its address/word at the same offset, keeping both aligned), then require equality on all non-exempt bytes. First-byte guards keep it ~O(n). Adds regression tests with addresses sharing 19 leading bytes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The 8 extra proxies (EETH, WeETH, EtherFiAdmin, Liquifier, StakingManager, EtherFiNodesManager, EtherFiRedemptionManager, MembershipManager) lived in a separate UpgradeStorageIntegrityExtended.t.sol that gated on block.chainid==1 but never forked in setUp. Under plain 'forge test' (chainid 31337, as CI runs) every test returned immediately and passed vacuously, leaving the coverage gap unfilled. Move the checks into UpgradeStorageIntegrity.t.sol so they inherit its createSelectFork(MAINNET_RPC_URL) setUp and actually run in CI. Switch the base to SecurityUpgradesConstants for the constructor params; delete the separate file. Verified: plain 'forge test --match-test test_extStorage' (no --fork-url) runs all 8 against the mainnet fork, 0 drift. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…erification Step 10 (verifyOperatingConfig) only checked bucket existence + consumer, not the configured capacity/refillRate. Add _assertBucketConfig for all three buckets (EETH mint, EETH burn, STETH_REQUEST_WITHDRAWAL) so they must equal the Constants. Step 11 (_flowEEthRateLimits) only proved decrement + an arbitrary over-limit revert. Add _assertExactBoundary: drain each bucket to PRECISELY zero through its on-chain consumer (a consume(id,0) first flushes time-based refill so lastRefill == now and the drain is exact), then assert one unit over reverts LimitExceeded. This also exercises the STETH_REQUEST_WITHDRAWAL bucket for the first time (no cheap fork flow mints a stETH withdrawal request). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rade-verification-hardening fix: harden security-upgrade bytecode + storage verification
…thub.com/etherfi-protocol/smart-contracts into pankaj/feat/security-upgrades-PR-scripts
The new EtherFiNodesManager re-keys the forwarded-call whitelist from a global (selector-keyed) mapping to a per-caller mapping, which orphans the legacy entries on upgrade. Add the re-grant to the OPERATING_TIMELOCK batch (Batch 2) so the eigenpod-operations role holder keeps forwarding rights post-upgrade. The whitelisted set is pinned as named SEL_* constants (decoded function signatures in comments) and shared by the batch builder and verifyOperatingConfig. withdrawRestakedBeaconChainETH is excluded: it is onlyEigenPodManager, so a forwarded call (msg.sender == EtherFiNode) always reverts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…older A mainnet-fork simulation showed the ENM is already on the per-user whitelist layout: the legacy forwarder 0x7835 currently holds exactly 4 effective entries (startCheckpoint, verifyCheckpointProofs, verifyWithdrawalCredentials, and processClaim on the RewardsCoordinator). The prior set, derived from stale global-era events, was wrong. Batch 2 now grants those 4 entries to HOLDER_EIGENPOD_OPERATIONS_ROLE and revokes them from 0x7835. Add a fork test that upgrades the ENM, applies the migration as the operating timelock, and asserts the legacy caller starts with all 4 and ends with none. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The test previously asserted the legacy caller's per-user entries were live immediately after the upgrade, coupling it to mainnet's incidental state and to a layout assumption (per-user slots could be empty until migration). It now seeds the entries on the legacy caller as the operating timelock, then verifies the migration moves them to the new holder. Also pins the fork block for determinism. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ant-only) 3CP #580 whitelists the call-forwarder for batch queue/complete across backfilled nodes. Grant the same forwardExternalCall selectors on the DelegationManager to HOLDER_EIGENPOD_OPERATIONS_ROLE in Batch 2: queueWithdrawals 0x0dd8dd02, completeQueuedWithdrawals 0x9435bb43, and the singular completeQueuedWithdrawal 0xe4cc3f90 (used by completeQueuedETHWithdrawals). The four names collapse to these three on-chain selectors since queueETHWithdrawal and queueWithdrawals both call DelegationManager.queueWithdrawals. Grant-only: the legacy caller's copies are revoked in a separate later 3CP, so these are not revoked here. verifyOperatingConfig and the fork test assert the holder receives all three. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rded-call-whitelist Re-grant ENM forwarded-call whitelist to eigenpod-ops role in Batch 2
…grades-PR-scripts Pankaj/feat/security upgrades pr scripts
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 2 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.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 84c79e1. Configure here.
| } | ||
| verified++; | ||
| } | ||
| console.log("Post-migration verification passed for ids:", verified); |
There was a problem hiding this comment.
Migration verify runs after broadcast
Medium Severity
The V0 migration script broadcasts all batch transactions inside vm.startBroadcast / vm.stopBroadcast, then runs post-migration checks that can revert with MigrationIncomplete. A failed final verification still leaves earlier batches on-chain while the script run reports failure.
Reviewed by Cursor Bugbot for commit 84c79e1. 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
High Risk
Touches upgrade verification, centralized role semantics, and many mainnet deployment/ops scripts tied to timelocked proxy upgrades and live protocol contracts.
Overview
Prepares the 26Q2 security upgrade batch: records Create2 deployment manifests for the new implementations/proxies, and aligns Forge scripts, verification, and CI with the refactored codebase and consolidated access control.
Source & build layout:
remappings.txtnow routes@etherfi/,@tests/,@scripts/, and@eigenlayer-libraries/; scripts and libs switch from flat../src/imports to those paths. Eigenlayer proof libs use the@eigenlayer-libraries/prefix.foundry.lockis gitignored; forge tests getOP_RPC_URL.Governance / roles (script-facing): Upgrade and ops scripts stop using per-contract role constants (
STAKING_MANAGER_NODE_CREATOR_ROLE,ETHERFI_NODES_MANAGER_*,PROTOCOL_PAUSER, etc.) and instead useRoleRegistrytier roles (OPERATION_TIMELOCK_ROLE,EXECUTOR_OPERATIONS_ROLE,onlyUpgradeTimelock, …).EtherFiNodedeployments drop theroleRegistryconstructor arg;EtherFiRateLimitergains mainnet eETH/weETH immutables in deploy/bytecode scripts.WeETHWithdrawAdapterdeploy/verify paths removewithdrawRequestNFTfrom the constructor and drop Ownable/timelock owner checks.Upgrade safety:
ContractCodeChecker.assertByteCodeMatchadds a reverting bytecode equality gate that masks only address-derived immutables (UUPS__self, optional EIP-712 domain word).VerifyV3Upgradedrops EtherFiNoderoleRegistrychecks and validates consolidated roles.Operations: New V0 → V1 membership NFT migration (
MembershipV0Migrator,MigrateV0ToV1.s.sol, id lists + README). Oracle fork scripts buildOracleReportwithoutwithdrawalRequestsToInvalidate. Hoodi staking part 1 drops NodeOperatorManager admin whitelisting.SimulateBatchApprove.s.solis removed. Timelock delay constants are renamed tominDelay_*across el-exits/operations scripts.Reviewed by Cursor Bugbot for commit 84c79e1. Bugbot is set up for automated code reviews on this repo. Configure here.
Timelock batch simulations (Batch 1 — UPGRADE_TIMELOCK, 10-day delay)
Tenderly public simulations of the two proposed Safe transactions (overriding the existing queued txns, nonces 177 / 178):
UPGRADE_TIMELOCK.scheduleBatch: https://dashboard.tenderly.co/public/simulator/08c1f4d6-da39-478a-8d66-ab8de4323a17UPGRADE_TIMELOCK.executeBatch, now includes the L1SyncPool + AvsOperatorManager upgrades; simulated with the scheduled operation marked ready (i.e. schedule executed + 10-day delay elapsed) so it passes: https://dashboard.tenderly.co/public/simulator/f56b0565-2357-4169-890e-5e496a68617f — ✅ success, gasUsed 2,489,052.