26Q2 - Security Upgrades#385
Conversation
📊 Forge Coverage ReportGenerated by workflow run #752 |
… auto-sweep Addresses review feedback: EtherFiNodesManager.completeQueuedWithdrawals silently dropped the swept amount, so off-chain indexers watching only the manager contract missed those transfers (sweepFunds and completeQueuedETHWithdrawals both already emit it). - IEtherFiNode.completeQueuedWithdrawals now returns the swept balance. - EtherFiNode.completeQueuedWithdrawals forwards the _sweepToLiquidityPool return value. - EtherFiNodesManager.completeQueuedWithdrawals captures that balance and emits FundsTransferred(node, balance) when > 0. - test/EtherFiNodesManager.t.sol setUp upgrades the EtherFiNode beacon impl to the locally compiled one so the new uint256 return is correctly decoded (the deployed mainnet impl still returns void until the beacon-impl upgrade ships). - test_completeQueuedWithdrawals_autoSweeps_to_LP now asserts the manager emits FundsTransferred with the node address as the indexed topic. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
|
||
| import "../interfaces/IRoleRegistry.sol"; | ||
|
|
||
| contract PausableUntil { |
There was a problem hiding this comment.
Does it make sense to make this abstract? All functions internal and never to be deployed ?
There was a problem hiding this comment.
fine to make it abstract
| import "./utils/PausableUntil.sol"; | ||
|
|
||
| contract EtherFiRateLimiter is IEtherFiRateLimiter, Initializable, UUPSUpgradeable, PausableUpgradeable { | ||
| contract EtherFiRateLimiter is IEtherFiRateLimiter, Initializable, UUPSUpgradeable, PausableUpgradeable, PausableUntil { |
There was a problem hiding this comment.
What advantage do we have of adding pausable on Ratelimiter ?
| function _transferShares(address _sender, address _recipient, uint256 _sharesAmount) internal whenNotPaused { | ||
| blacklister.nonBlacklisted(_sender); | ||
| blacklister.nonBlacklisted(_recipient); |
There was a problem hiding this comment.
Need to add this for safe guard from transferFrom similar to MembershipNFT checks:
blacklister.nonBlacklisted(msg.sender);
| } | ||
| } | ||
|
|
||
| uint256 public constant MAX_PAUSE_DURATION = 7 days; |
There was a problem hiding this comment.
We would be reverting back to 1 day?
…hecks yash/feat/more-sanity-checks
…herfi-protocol/smart-contracts into pankaj/feat/security-upgrades
…lize ordering Two follow-up fixes from the multi-lens audit of PR #385. H-5 (AuctionManager): pre-PR, every consumed bid forwarded `bid.amount` to `membershipManagerContractAddress` via `processAuctionFeeTransfer`. That fn was deleted in this branch and `updateSelectedBidInformation` now only flips `bid.isActive=false`, stranding the consumed-bid ETH in the contract. The `treasury` immutable was added but never read. This change rewires `updateSelectedBidInformation` to forward `bid.amount` to `treasury` immediately on consumption, matching the previously-removed revenue path. The `membershipManagerContractAddress` immutable and its constructor param are dropped (unused). Bid-cancellation refund is unaffected (`_cancelBid` still refunds the bidder). The currently-stranded ~0.097 ETH (`accumulatedRevenue` below the 0.2 ETH auto-forward threshold) is left to be handled separately; the per-call forward stops new revenue from getting trapped going forward. H-3 (WithdrawRequestNFT): `finalizeRequests` lacked any guard that the share-rate-freeze sentinel had been pushed. If `initializeShareRateFreezeUpgrade` is skipped between proxy upgrade and the first oracle report, the first `finalizeRequests(N)` pushes `(N, rateNow)` as the trace's first entry. Every legacy tokenId then resolves to `rateNow` via `lowerLookup` instead of the `0` sentinel that signals "fall back to live LP rate", silently mispaying every pre-upgrade holder. This change adds `NotInitialized` + a `length() == 0` precondition to `finalizeRequests`, making the upgrade-ordering trap impossible. Tests: - `TestSetup` pushes the sentinel as part of standard setup (matches post-upgrade reality on mainnet). - `WithdrawRequestNFTIntrusive` gains `clearFinalizationRatesForTest()` so the four tests that explicitly exercise the pre-init transition can reset the trace to length 0. - All AuctionManager constructor call-sites updated for the dropped param. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…cture yash/refactor/repo-structure
| bytes32 ETHERFI_NODES_MANAGER_EIGENLAYER_ADMIN_ROLE = keccak256( | ||
| "ETHERFI_NODES_MANAGER_EIGENLAYER_ADMIN_ROLE" | ||
| ); | ||
| bytes32 ETHERFI_NODES_MANAGER_CALL_FORWARDER_ROLE = keccak256( |
There was a problem hiding this comment.
Missing UPGRADE_TIMELOCK_ROLE check in verification script
Medium Severity
The comment at line 267–269 states that all per-contract roles now map to one of 8 consolidated tier roles and explicitly lists UPGRADE_TIMELOCK_ROLE first. However, the verification code only checks 7 of the 8 roles — UPGRADE_TIMELOCK_ROLE is missing. This means the most privileged role (controlling upgrades) is never verified, defeating the purpose of this safety check.
Reviewed by Cursor Bugbot for commit f756609. Configure here.
…nto seongyun/fix/auction-revenue-wrnft-init-guard
…venue-wrnft-init-guard fix: forward AuctionManager bid revenue + guard WRNFT finalize ordering
…nto seongyun/feat/lp-withdraw-max-burn-guard
…aw-max-burn-guard feat(lp): max-burn segregated withdraw guard
Addresses H-3 from the multi-lens audit of PR #385. PriorityWithdrawalQueue.claimWithdraw was callable on behalf of any `request.user`, including a sanctioned address that became blacklisted between request finalization and claim — proceeds would flow to the blacklisted user via a non-blacklisted accomplice. Add blacklister as an immutable, wire it through the constructor with a zero-check, and call `blacklister.nonBlacklisted(request.user)` at the top of `_claimWithdraw` so both `claimWithdraw` and `batchClaimWithdraw` fail-closed on sanctioned recipients. Cancel path is already covered transitively via eETH's own `nonBlacklisted` on transfer. H-2 — bid-revenue treasury setter was acked rather than implemented. Storage-collision risk with the pre-deprecation DEPRECATED_admin slot makes the safe fix non-trivial; deferred. Test/script call-site updates thread the new PWQ blacklister arg.
* fix(pwq): blacklist gate on PriorityWithdrawalQueue claim Addresses H-3 from the multi-lens audit of PR #385. PriorityWithdrawalQueue.claimWithdraw was callable on behalf of any `request.user`, including a sanctioned address that became blacklisted between request finalization and claim — proceeds would flow to the blacklisted user via a non-blacklisted accomplice. Add blacklister as an immutable, wire it through the constructor with a zero-check, and call `blacklister.nonBlacklisted(request.user)` at the top of `_claimWithdraw` so both `claimWithdraw` and `batchClaimWithdraw` fail-closed on sanctioned recipients. Cancel path is already covered transitively via eETH's own `nonBlacklisted` on transfer. H-2 — bid-revenue treasury setter was acked rather than implemented. Storage-collision risk with the pre-deprecation DEPRECATED_admin slot makes the safe fix non-trivial; deferred. Test/script call-site updates thread the new PWQ blacklister arg. * fix: blacklist msg.sender too --------- Co-authored-by: Yash Saraswat <yash@ether.fi>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes 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.
Reviewed by Cursor Bugbot for commit 87bc3d6. Configure here.
| bytes32 ETHERFI_NODES_MANAGER_ADMIN_ROLE = EtherFiNodesManager(payable(etherFiNodesManagerImpl)).OPERATION_TIMELOCK_ROLE(); | ||
| bytes32 ETHERFI_NODES_MANAGER_CALL_FORWARDER_ROLE = EtherFiNodesManager(payable(etherFiNodesManagerImpl)).EIGENPOD_OPERATIONS_ROLE(); | ||
| bytes32 ETHERFI_NODES_MANAGER_EIGENLAYER_ADMIN_ROLE = EtherFiNodesManager(payable(etherFiNodesManagerImpl)).HOUSEKEEPING_OPERATIONS_ROLE(); | ||
| bytes32 LIQUIDITY_POOL_VALIDATOR_APPROVER_ROLE = LiquidityPool(payable(liquidityPoolImpl)).EXECUTOR_OPERATIONS_ROLE(); |
There was a problem hiding this comment.
Role getters called on contracts that don't define them
Medium Severity
The new code calls role constant getters like EXECUTOR_OPERATIONS_ROLE(), OPERATION_TIMELOCK_ROLE(), EIGENPOD_OPERATIONS_ROLE(), and HOUSEKEEPING_OPERATIONS_ROLE() on StakingManager, EtherFiNodesManager, and LiquidityPool contract instances. These role constants are defined on RoleRegistry, not on those contracts. Grep confirms these function names don't exist in StakingManager.sol, EtherFiNodesManager.sol, or LiquidityPool.sol. Unless RolesLibrary re-exports them (unlikely given the naming pattern), these calls will revert at script initialization. The old code correctly called contract-specific getters that did exist on those contracts. These should reference RoleRegistry directly instead.
Reviewed by Cursor Bugbot for commit 87bc3d6. Configure here.
yash/cleanup-tasks


Note
Medium Risk
Touches many mainnet upgrade, timelock, and migration scripts tied to governance roles and deploy bytecode; mistakes could produce wrong calldata or failed upgrades, though changes are mostly off-chain tooling.
Overview
This PR retools Foundry scripts and ops tooling to match the security upgrade layout:
@etherfi//@scripts//@tests/remappings, domain-based imports, and centralizedRoleRegistrytier roles instead of per-contract role getters.Governance & deployment scripts now reference consolidated roles (
EXECUTOR_OPERATIONS_ROLE,OPERATION_TIMELOCK_ROLE,onlyUpgradeTimelock, etc.), drop checks thatEtherFiNodeholds its ownroleRegistry, and updateEtherFiNode/EtherFiRateLimiterdeploy args (noroleRegistryon node; rate limiter gets EETH/weETH bucket addresses). Timelock helper names shift fromMIN_DELAY_*tominDelay_*. Upgrade verification scripts expectOracleReportwithoutwithdrawalRequestsToInvalidateand use richerLiquidityPool/EtherFiRedemptionManager/Liquifier/WithdrawRequestNFTconstructor shapes in bytecode checks.New operational surface:
script/operations/v0-migration/— batch V0 → V1 membership NFT migration (MembershipV0Migrator,MigrateV0ToV1.s.sol, ID JSON, README).SimulateBatchApprove.s.solis removed.CI / repo hygiene: forge tests get
OP_RPC_URL;foundry.lockis gitignored;Deployed.s.soladds EigenLayer strategy manager constant.Reviewed by Cursor Bugbot for commit 6387b9c. Bugbot is set up for automated code reviews on this repo. Configure here.