From 8f667262a31d327a545ab879eb14bcf0b1ceea02 Mon Sep 17 00:00:00 2001 From: ReposCollector Date: Fri, 29 May 2026 09:09:09 -0400 Subject: [PATCH 1/6] P0: bound solvency-critical & authority surfaces - EtherFiAdmin: absolute per-report rebase delta cap (SINGLE_REPORT_REBASE_DELTA_CAP_BPS), independent of elapsedTime, so a long-spanning report cannot move the exchange rate by an outsized absolute amount while passing the annualized APR check. - EtherFiRestaker: add whenNotPaused to all fund-movement functions (previously the pause gated nothing) and add a Guardian fast-halt path (guardianPause). - Blacklister: bound the instant multisig setBlacklistUntil path to MAX_MULTISIG_BLACKLIST_DURATION; document moving permanent blacklist behind the timelock. Tests: blacklist duration bound + restaker pause-gating (passing on fork). Co-Authored-By: Claude Opus 4.8 --- src/governance/Blacklister.sol | 19 ++++++++++++++++ src/oracle/EtherFiAdmin.sol | 22 ++++++++++++++++++ src/restaking/EtherFiRestaker.sol | 27 ++++++++++++++-------- test/Blacklist.t.sol | 19 ++++++++++++++++ test/EtherFiRestaker.t.sol | 38 +++++++++++++++++++++++++++++++ 5 files changed, 116 insertions(+), 9 deletions(-) diff --git a/src/governance/Blacklister.sol b/src/governance/Blacklister.sol index bcefd584..0a4cd3f2 100644 --- a/src/governance/Blacklister.sol +++ b/src/governance/Blacklister.sol @@ -10,10 +10,19 @@ contract Blacklister is Initializable, UUPSUpgradeable, RolesLibrary { uint256 public constant BLACKLIST_DURATION = 3 days; + /// @dev Upper bound on a blacklist the Operating Multisig can set instantly. + /// Anything longer (including a permanent blacklist) must go through the + /// 2-day Operation Timelock (`blacklistUserPermanent`), so a single compromised + /// multisig cannot permanently and irreversibly freeze a targeted user with no + /// auto-recovery. Legitimate permanent blacklists (e.g. sanctions) are still + /// possible, just behind the timelock. (Pillar 3: bound a compromised credential.) + uint256 public constant MAX_MULTISIG_BLACKLIST_DURATION = 90 days; + mapping(address => uint256) public blacklistedUntil; error BlacklistedUser(address user); error UserAlreadyBlacklisted(address user); + error BlacklistDurationTooLong(); event UserBlacklisted(address user); event UserUnblacklisted(address user); @@ -35,11 +44,21 @@ contract Blacklister is Initializable, UUPSUpgradeable, RolesLibrary { emit UserBlacklistedUntil(user, block.timestamp + BLACKLIST_DURATION); } + /// @notice Operating Multisig blacklists `user` for `until` seconds from now. + /// @dev Bounded by MAX_MULTISIG_BLACKLIST_DURATION; use `blacklistUserPermanent` + /// (timelock) for anything longer or permanent. function setBlacklistUntil(address user, uint256 until) external onlyOperatingMultisig { + if (until > MAX_MULTISIG_BLACKLIST_DURATION) revert BlacklistDurationTooLong(); blacklistedUntil[user] = block.timestamp + until; emit UserBlacklistedUntil(user, block.timestamp + until); } + /// @notice Explicit permanent blacklist (Operating Multisig). + /// @dev Kept as the deliberate "permanent" verb. NOTE (security review, PR #385): + /// consider moving this behind the 2-day Operation Timelock (`onlyAdmin`) so a + /// compromised multisig cannot instantly and irreversibly freeze a targeted user + /// with no auto-recovery. Left as multisig here pending a product/compliance call, + /// since legitimate permanent blacklists (e.g. sanctions) may need to be instant. function blacklistUser(address user) external onlyOperatingMultisig { blacklistedUntil[user] = type(uint256).max; emit UserBlacklisted(user); diff --git a/src/oracle/EtherFiAdmin.sol b/src/oracle/EtherFiAdmin.sol index ea2e8e2a..81a72a49 100644 --- a/src/oracle/EtherFiAdmin.sol +++ b/src/oracle/EtherFiAdmin.sol @@ -89,6 +89,18 @@ contract EtherFiAdmin is Initializable, OwnableUpgradeable, UUPSUpgradeable, Rol uint256 public constant STALE_REPORT_FINALIZATION_COOLDOWN = 7200; // 1 day uint256 public constant BASIS_POINTS_DENOMINATOR = 10_000; + // Absolute single-report rebase cap, in bps of current TVL, independent of how + // long the report spans. `acceptableRebaseAprInBps` is annualized over elapsedTime, + // so a report covering a long window (e.g. a stale-report catch-up, or a maliciously + // back-dated `refSlotFrom`) permits a proportionally larger ABSOLUTE move while still + // passing the APR check. This hard ceiling bounds the per-report share-rate delta + // regardless of elapsedTime: a single report can never move TVL by more than this, + // so a compromised oracle quorum cannot inflate/deflate the exchange rate arbitrarily + // in one report. A legitimate move beyond this requires deliberate governance action. + // 200 bps = 2% of TVL; normal daily reports move ~1-3 bps, so this only trips on + // anomalous reports while still leaving generous headroom for multi-week catch-ups. + int256 public constant SINGLE_REPORT_REBASE_DELTA_CAP_BPS = 200; + struct ConstructorAddresses { address etherFiOracle; address stakingManager; @@ -410,6 +422,16 @@ contract EtherFiAdmin is Initializable, OwnableUpgradeable, UUPSUpgradeable, Rol } int256 absApr = (apr > 0) ? apr : - apr; if (absApr > acceptableRebaseAprInBps) return (false, "EtherFiAdmin: TVL changed too much"); + + // Absolute per-report cap, independent of elapsedTime. The APR check above is + // annualized, so a long-spanning report can pass it while still moving an + // outsized absolute amount of TVL in a single rebase. Bound the absolute move + // to SINGLE_REPORT_REBASE_DELTA_CAP_BPS of current TVL so no single report can + // shift the exchange rate beyond this, regardless of the window it covers. + int256 absRewards = (_report.accruedRewards > 0) ? int256(_report.accruedRewards) : -int256(_report.accruedRewards); + if (currentTVL > 0 && absRewards * int256(BASIS_POINTS_DENOMINATOR) > currentTVL * SINGLE_REPORT_REBASE_DELTA_CAP_BPS) { + return (false, "EtherFiAdmin: rebase delta exceeds absolute cap"); + } return (true, ""); } diff --git a/src/restaking/EtherFiRestaker.sol b/src/restaking/EtherFiRestaker.sol index 5311afac..a17a12fc 100644 --- a/src/restaking/EtherFiRestaker.sol +++ b/src/restaking/EtherFiRestaker.sol @@ -143,7 +143,7 @@ contract EtherFiRestaker is Initializable, UUPSUpgradeable, OwnableUpgradeable, /// @notice Transfer stETH to a recipient for instant withdrawal /// @param recipient The address to receive stETH /// @param amount The amount of stETH to transfer - function transferStETH(address recipient, uint256 amount) external { + function transferStETH(address recipient, uint256 amount) external whenNotPaused { if(msg.sender != etherFiRedemptionManager) revert IncorrectCaller(); if (amount > lido.balanceOf(address(this))) revert InsufficientBalance(); IERC20(address(lido)).safeTransfer(recipient, amount); @@ -158,7 +158,7 @@ contract EtherFiRestaker is Initializable, UUPSUpgradeable, OwnableUpgradeable, /// @notice Request for a specific amount of stETH holdings /// @param _amount the amount of stETH to request - function stEthRequestWithdrawal(uint256 _amount) public onlyExecutorOperations returns (uint256[] memory) { + function stEthRequestWithdrawal(uint256 _amount) public onlyExecutorOperations whenNotPaused returns (uint256[] memory) { rateLimiter.consume(STETH_REQUEST_WITHDRAWAL_LIMIT_ID, _amountToGwei(_amount)); uint256 minAmount = lidoWithdrawalQueue.MIN_STETH_WITHDRAWAL_AMOUNT(); @@ -192,7 +192,7 @@ contract EtherFiRestaker is Initializable, UUPSUpgradeable, OwnableUpgradeable, /// @notice Claim a batch of withdrawal requests if they are finalized sending the ETH to the this contract back /// @param _requestIds array of request ids to claim /// @param _hints checkpoint hint for each id. Can be obtained with `findCheckpointHints()` - function stEthClaimWithdrawals(uint256[] calldata _requestIds, uint256[] calldata _hints) external onlyHousekeepingOperations { + function stEthClaimWithdrawals(uint256[] calldata _requestIds, uint256[] calldata _hints) external onlyHousekeepingOperations whenNotPaused { lidoWithdrawalQueue.claimWithdrawals(_requestIds, _hints); _withdrawEther(); @@ -201,7 +201,7 @@ contract EtherFiRestaker is Initializable, UUPSUpgradeable, OwnableUpgradeable, } // Send the ETH back to the liquidity pool - function withdrawEther() public onlyHousekeepingOperations { + function withdrawEther() public onlyHousekeepingOperations whenNotPaused { _withdrawEther(); } @@ -245,7 +245,7 @@ contract EtherFiRestaker is Initializable, UUPSUpgradeable, OwnableUpgradeable, } // deposit the token in holding into the restaking strategy - function depositIntoStrategy(address token, uint256 amount) external onlyExecutorOperations returns (uint256) { + function depositIntoStrategy(address token, uint256 amount) external onlyExecutorOperations whenNotPaused returns (uint256) { rateLimiter.consume(DEPOSIT_INTO_STRATEGY_LIMIT_ID, _amountToGwei(amount)); IERC20(token).safeIncreaseAllowance(address(eigenLayerStrategyManager), amount); @@ -260,7 +260,7 @@ contract EtherFiRestaker is Initializable, UUPSUpgradeable, OwnableUpgradeable, /// Made easy for operators /// @param token the token to withdraw /// @param amount the amount of token to withdraw - function queueWithdrawals(address token, uint256 amount) public onlyExecutorOperations returns (bytes32[] memory) { + function queueWithdrawals(address token, uint256 amount) public onlyExecutorOperations whenNotPaused returns (bytes32[] memory) { rateLimiter.consume(QUEUE_WITHDRAWALS_LIMIT_ID, _amountToGwei(amount)); uint256 shares = getEigenLayerRestakingStrategy(token).underlyingToSharesView(amount); @@ -280,7 +280,7 @@ contract EtherFiRestaker is Initializable, UUPSUpgradeable, OwnableUpgradeable, function completeQueuedWithdrawals( IDelegationManager.Withdrawal[] memory _queuedWithdrawals, IERC20[][] memory _tokens - ) external onlyHousekeepingOperations { + ) external onlyHousekeepingOperations whenNotPaused { uint256 num = _queuedWithdrawals.length; bool[] memory receiveAsTokens = new bool[](num); for (uint256 i = 0; i < num; i++) { @@ -397,12 +397,21 @@ contract EtherFiRestaker is Initializable, UUPSUpgradeable, OwnableUpgradeable, return total; } - // Pauses the contract + // Pauses the contract (multisig path) function pauseContract() external onlyOperatingMultisig { _pause(); } - // Unpauses the contract + /// @notice Fast emergency halt callable by the Guardian (Hypernative / EOA keys). + /// @dev Pillar 2 (halt is cheap, redundant, parallel): the Guardian can stop fund + /// movement immediately without assembling the multisig. Resume stays deliberate + /// (multisig-only `unPauseContract`). This mirrors the token/LP halt model where + /// the Guardian can pause but only the Operating Multisig can unpause. + function guardianPause() external onlyGuardian { + _pause(); + } + + // Unpauses the contract (deliberate, multisig-only) function unPauseContract() external onlyOperatingMultisig { _unpause(); } diff --git a/test/Blacklist.t.sol b/test/Blacklist.t.sol index 7f7e7779..184afd39 100644 --- a/test/Blacklist.t.sol +++ b/test/Blacklist.t.sol @@ -72,6 +72,25 @@ contract BlacklistTest is TestSetup { blacklisterInstance.blacklistUser(rando); } + // PR #385 security review: the instant multisig `setBlacklistUntil` path is bounded + // to MAX_MULTISIG_BLACKLIST_DURATION so a compromised multisig cannot set an + // arbitrarily long (effectively permanent) blacklist via the custom-duration path. + function test_setBlacklistUntil_rejects_over_max() public { + address u = vm.addr(0xD00D); + uint256 maxDur = blacklisterInstance.MAX_MULTISIG_BLACKLIST_DURATION(); + vm.prank(owner); + vm.expectRevert(Blacklister.BlacklistDurationTooLong.selector); + blacklisterInstance.setBlacklistUntil(u, maxDur + 1); + } + + function test_setBlacklistUntil_allows_up_to_max() public { + address u = vm.addr(0xD00D); + uint256 maxDur = blacklisterInstance.MAX_MULTISIG_BLACKLIST_DURATION(); + vm.prank(owner); + blacklisterInstance.setBlacklistUntil(u, maxDur); + assertEq(blacklisterInstance.blacklistedUntil(u), block.timestamp + maxDur); + } + // ------------------------------------------------------------------------- // AuctionManager // ------------------------------------------------------------------------- diff --git a/test/EtherFiRestaker.t.sol b/test/EtherFiRestaker.t.sol index 45a9ae82..ce3b22d9 100644 --- a/test/EtherFiRestaker.t.sol +++ b/test/EtherFiRestaker.t.sol @@ -292,4 +292,42 @@ contract EtherFiRestakerTest is TestSetup { etherFiRestakerInstance.getTotalPooledEther(); } + // PR #385 security review (H1): the restaker's pause previously gated nothing — + // pauseContract() called _pause() but no money-movement function carried + // whenNotPaused. This asserts the pause now actually halts fund movement, and that + // the Guardian (HN/EOA keys) can fire it for a fast, redundant halt (Pillar 2). + function test_guardianPause_halts_fund_movement() public { + vm.startPrank(roleRegistryInstance.owner()); + roleRegistryInstance.grantRole(roleRegistryInstance.GUARDIAN_ROLE(), bob); + vm.stopPrank(); + + assertFalse(etherFiRestakerInstance.paused()); + + // Guardian fast-halt + vm.prank(bob); + etherFiRestakerInstance.guardianPause(); + assertTrue(etherFiRestakerInstance.paused()); + + // whenNotPaused is the first gate on transferStETH, so the pause check fires + // before the caller check — proves fund movement is actually halted. + vm.expectRevert("Pausable: paused"); + etherFiRestakerInstance.transferStETH(bob, 1); + + // withdrawEther (owner holds HOUSEKEEPING_OPERATIONS) passes the role gate then + // reverts on the pause gate. + vm.prank(owner); + vm.expectRevert("Pausable: paused"); + etherFiRestakerInstance.withdrawEther(); + + // a non-guardian cannot fire the guardian halt + vm.prank(alice); + vm.expectRevert(RoleRegistry.OnlyGuardian.selector); + etherFiRestakerInstance.guardianPause(); + + // resume is deliberate / multisig-only (owner holds OPERATION_MULTISIG) + vm.prank(owner); + etherFiRestakerInstance.unPauseContract(); + assertFalse(etherFiRestakerInstance.paused()); + } + } From 4af869dd273617834b664498839bf0e6a2c1e3b1 Mon Sep 17 00:00:00 2001 From: ReposCollector Date: Fri, 29 May 2026 09:46:59 -0400 Subject: [PATCH 2/6] Drop blacklist duration cap from P0 Removing the setBlacklistUntil 90-day cap added earlier. It only guarded against an accidental over-long blacklist, which is operationally recoverable (the multisig can shorten/clear it). It gave no protection against a compromised multisig, which can permanently blacklist via blacklistUser regardless. Net: friction without a real security gain. Reverts Blacklister.sol and Blacklist.t.sol to base. Co-Authored-By: Claude Opus 4.8 --- src/governance/Blacklister.sol | 19 ------------------- test/Blacklist.t.sol | 19 ------------------- 2 files changed, 38 deletions(-) diff --git a/src/governance/Blacklister.sol b/src/governance/Blacklister.sol index 0a4cd3f2..bcefd584 100644 --- a/src/governance/Blacklister.sol +++ b/src/governance/Blacklister.sol @@ -10,19 +10,10 @@ contract Blacklister is Initializable, UUPSUpgradeable, RolesLibrary { uint256 public constant BLACKLIST_DURATION = 3 days; - /// @dev Upper bound on a blacklist the Operating Multisig can set instantly. - /// Anything longer (including a permanent blacklist) must go through the - /// 2-day Operation Timelock (`blacklistUserPermanent`), so a single compromised - /// multisig cannot permanently and irreversibly freeze a targeted user with no - /// auto-recovery. Legitimate permanent blacklists (e.g. sanctions) are still - /// possible, just behind the timelock. (Pillar 3: bound a compromised credential.) - uint256 public constant MAX_MULTISIG_BLACKLIST_DURATION = 90 days; - mapping(address => uint256) public blacklistedUntil; error BlacklistedUser(address user); error UserAlreadyBlacklisted(address user); - error BlacklistDurationTooLong(); event UserBlacklisted(address user); event UserUnblacklisted(address user); @@ -44,21 +35,11 @@ contract Blacklister is Initializable, UUPSUpgradeable, RolesLibrary { emit UserBlacklistedUntil(user, block.timestamp + BLACKLIST_DURATION); } - /// @notice Operating Multisig blacklists `user` for `until` seconds from now. - /// @dev Bounded by MAX_MULTISIG_BLACKLIST_DURATION; use `blacklistUserPermanent` - /// (timelock) for anything longer or permanent. function setBlacklistUntil(address user, uint256 until) external onlyOperatingMultisig { - if (until > MAX_MULTISIG_BLACKLIST_DURATION) revert BlacklistDurationTooLong(); blacklistedUntil[user] = block.timestamp + until; emit UserBlacklistedUntil(user, block.timestamp + until); } - /// @notice Explicit permanent blacklist (Operating Multisig). - /// @dev Kept as the deliberate "permanent" verb. NOTE (security review, PR #385): - /// consider moving this behind the 2-day Operation Timelock (`onlyAdmin`) so a - /// compromised multisig cannot instantly and irreversibly freeze a targeted user - /// with no auto-recovery. Left as multisig here pending a product/compliance call, - /// since legitimate permanent blacklists (e.g. sanctions) may need to be instant. function blacklistUser(address user) external onlyOperatingMultisig { blacklistedUntil[user] = type(uint256).max; emit UserBlacklisted(user); diff --git a/test/Blacklist.t.sol b/test/Blacklist.t.sol index 184afd39..7f7e7779 100644 --- a/test/Blacklist.t.sol +++ b/test/Blacklist.t.sol @@ -72,25 +72,6 @@ contract BlacklistTest is TestSetup { blacklisterInstance.blacklistUser(rando); } - // PR #385 security review: the instant multisig `setBlacklistUntil` path is bounded - // to MAX_MULTISIG_BLACKLIST_DURATION so a compromised multisig cannot set an - // arbitrarily long (effectively permanent) blacklist via the custom-duration path. - function test_setBlacklistUntil_rejects_over_max() public { - address u = vm.addr(0xD00D); - uint256 maxDur = blacklisterInstance.MAX_MULTISIG_BLACKLIST_DURATION(); - vm.prank(owner); - vm.expectRevert(Blacklister.BlacklistDurationTooLong.selector); - blacklisterInstance.setBlacklistUntil(u, maxDur + 1); - } - - function test_setBlacklistUntil_allows_up_to_max() public { - address u = vm.addr(0xD00D); - uint256 maxDur = blacklisterInstance.MAX_MULTISIG_BLACKLIST_DURATION(); - vm.prank(owner); - blacklisterInstance.setBlacklistUntil(u, maxDur); - assertEq(blacklisterInstance.blacklistedUntil(u), block.timestamp + maxDur); - } - // ------------------------------------------------------------------------- // AuctionManager // ------------------------------------------------------------------------- From df243bd7bf3e0297e8dfe3e49ab91967ddd02b2d Mon Sep 17 00:00:00 2001 From: ReposCollector Date: Fri, 29 May 2026 09:57:31 -0400 Subject: [PATCH 3/6] Restaker: also gate undelegate() with whenNotPaused MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Cursor Bugbot review on #445: undelegate() queues withdrawal of all restaked assets (same fund-flow category as queueWithdrawals), so it must also respect the pause for the halt to be a complete/reliable signal. delegateTo is left ungated — it only changes delegation posture and queues/moves no funds. Test extended. Co-Authored-By: Claude Opus 4.8 --- src/restaking/EtherFiRestaker.sol | 2 +- test/EtherFiRestaker.t.sol | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/restaking/EtherFiRestaker.sol b/src/restaking/EtherFiRestaker.sol index a17a12fc..f3e1b61b 100644 --- a/src/restaking/EtherFiRestaker.sol +++ b/src/restaking/EtherFiRestaker.sol @@ -230,7 +230,7 @@ contract EtherFiRestaker is Initializable, UUPSUpgradeable, OwnableUpgradeable, } // undelegate from the current AVS operator & un-restake all - function undelegate() external onlyOperatingMultisig returns (bytes32[] memory) { + function undelegate() external onlyOperatingMultisig whenNotPaused returns (bytes32[] memory) { bytes32[] memory withdrawalRoots = eigenLayerDelegationManager.undelegate(address(this)); for (uint256 i = 0; i < withdrawalRoots.length; i++) { diff --git a/test/EtherFiRestaker.t.sol b/test/EtherFiRestaker.t.sol index ce3b22d9..a1e6b9ae 100644 --- a/test/EtherFiRestaker.t.sol +++ b/test/EtherFiRestaker.t.sol @@ -319,6 +319,12 @@ contract EtherFiRestakerTest is TestSetup { vm.expectRevert("Pausable: paused"); etherFiRestakerInstance.withdrawEther(); + // undelegate (owner holds OPERATION_MULTISIG) queues withdrawal of ALL restaked + // assets — same fund-flow category, so it must also be halted by the pause. + vm.prank(owner); + vm.expectRevert("Pausable: paused"); + etherFiRestakerInstance.undelegate(); + // a non-guardian cannot fire the guardian halt vm.prank(alice); vm.expectRevert(RoleRegistry.OnlyGuardian.selector); From dfac52512a7257e81371185949f0d12fbf54d810 Mon Sep 17 00:00:00 2001 From: ReposCollector Date: Fri, 29 May 2026 12:16:07 -0400 Subject: [PATCH 4/6] Rebase cap redesign (Yash review) + restaker PausableUntil MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per @0xpanicError's review on #445: Rebase caps — replace the single symmetric 200bps constant with asymmetric, configurable bounds (defaults as proposed): - EtherFiAdmin: negative (slashing) cap on accruedRewards, default 3 bps of TVL (max initial slashing penalty ~2.44 bps), settable behind the operating timelock (maxNegativeRebaseBps, 0 = default). Raise-able during a correlated-slashing event. - LiquidityPool.rebase: positive (reward) upper bound enforced at the share-rate chokepoint, default 25 bps (~1 month at 3% APR), settable behind the operating timelock (maxPositiveRebaseBps, 0 = default). Defense-in-depth independent of the oracle path. Restaker — switch from OZ Pausable to the protocol-wide PausableUntil model: Guardian pauseContractUntil() (auto-expiring, cooldown) + multisig boolean pause; whenNotHalted gates all fund-flow fns (incl. undelegate). Replaces guardianPause(). TestSetup disables both caps for generic suites (large artificial rebases on small TVL); dedicated tests exercise enforcement. Co-Authored-By: Claude Opus 4.8 --- src/core/LiquidityPool.sol | 38 +++++++++++++++++++ src/oracle/EtherFiAdmin.sol | 61 +++++++++++++++++++++---------- src/restaking/EtherFiRestaker.sol | 57 +++++++++++++++++++---------- test/EtherFiRestaker.t.sol | 58 +++++++++++++++++------------ test/TestSetup.sol | 13 +++++++ 5 files changed, 165 insertions(+), 62 deletions(-) diff --git a/src/core/LiquidityPool.sol b/src/core/LiquidityPool.sol index 3999d37b..5ee6db1a 100644 --- a/src/core/LiquidityPool.sol +++ b/src/core/LiquidityPool.sol @@ -52,6 +52,10 @@ contract LiquidityPool is Initializable, OwnableUpgradeable, UUPSUpgradeable, Re uint256 public minWithdrawAmount; bool public escrowMigrationCompleted; + // Override for the per-rebase positive (reward) cap, in bps of TVL. + // 0 = use DEFAULT_MAX_POSITIVE_REBASE_BPS. Settable behind the operating timelock. + uint256 public maxPositiveRebaseBps; + //-------------------------------------------------------------------------------------- //------------------------------------- IMMUTABLES ---------------------------------- //-------------------------------------------------------------------------------------- @@ -73,6 +77,13 @@ contract LiquidityPool is Initializable, OwnableUpgradeable, UUPSUpgradeable, Re //-------------------------------------------------------------------------------------- uint256 public constant SHARE_UNIT = 1e18; + // Default cap on how far a single rebase may INCREASE TVL (rewards), in bps of TVL. + // 25 bps ≈ 1 month of reward accrual at 3% APR — generous for a normal report, + // tight enough that a buggy/compromised rebase caller cannot inflate the share rate + // arbitrarily. Overridable via `maxPositiveRebaseBps` behind the operating timelock. + uint256 public constant DEFAULT_MAX_POSITIVE_REBASE_BPS = 25; + uint256 private constant REBASE_BPS_DENOMINATOR = 10_000; + //-------------------------------------------------------------------------------------- //------------------------------------- EVENTS --------------------------------------- //-------------------------------------------------------------------------------------- @@ -94,6 +105,7 @@ contract LiquidityPool is Initializable, OwnableUpgradeable, UUPSUpgradeable, Re event ValidatorApproved(uint256 indexed validatorId); event ValidatorRegistrationCanceled(uint256 indexed validatorId); event Rebase(uint256 totalEthLocked, uint256 totalEEthShares); + event MaxPositiveRebaseBpsUpdated(uint256 bps); event ProtocolFeePaid(uint128 protocolFees); event WhitelistStatusUpdated(bool value); event MinWithdrawAmountSet(uint256 minWithdrawAmount); @@ -113,6 +125,8 @@ contract LiquidityPool is Initializable, OwnableUpgradeable, UUPSUpgradeable, Re error InvalidValidatorSize(); error InvalidAmountForShare(); error InvalidRate(); + error RebaseExceedsPositiveCap(); + error InvalidMaxPositiveRebaseBps(); error AlreadyMigrated(); error MigrationNotComplete(); error AlreadyRegistered(); @@ -438,6 +452,17 @@ contract LiquidityPool is Initializable, OwnableUpgradeable, UUPSUpgradeable, Re /// @notice Rebase by ether.fi function rebase(int128 _accruedRewards) public { if (msg.sender != address(membershipManager)) revert IncorrectCaller(); + + // Positive (reward) upper bound, enforced at the share-rate chokepoint regardless + // of who calls rebase. A single rebase cannot increase TVL by more than + // effectiveMaxPositiveRebaseBps() of pre-rebase TVL. Defense-in-depth alongside the + // oracle-side negative cap in EtherFiAdmin; the negative side is intentionally not + // re-checked here (the oracle path owns it and bounds it tighter). + if (_accruedRewards > 0) { + uint256 maxIncrease = (getTotalPooledEther() * effectiveMaxPositiveRebaseBps()) / REBASE_BPS_DENOMINATOR; + if (uint256(uint128(_accruedRewards)) > maxIncrease) revert RebaseExceedsPositiveCap(); + } + totalValueOutOfLp = uint128(int128(totalValueOutOfLp) + _accruedRewards); _checkMinAmountForShare(); @@ -445,6 +470,19 @@ contract LiquidityPool is Initializable, OwnableUpgradeable, UUPSUpgradeable, Re emit Rebase(getTotalPooledEther(), eETH.totalShares()); } + /// @notice Override the per-rebase positive (reward) cap, in bps of TVL. + /// @dev Operation-Timelock-gated. 0 resets to DEFAULT_MAX_POSITIVE_REBASE_BPS. + function setMaxPositiveRebaseBps(uint256 _bps) external onlyAdmin { + if (_bps > REBASE_BPS_DENOMINATOR) revert InvalidMaxPositiveRebaseBps(); + maxPositiveRebaseBps = _bps; + emit MaxPositiveRebaseBpsUpdated(_bps); + } + + function effectiveMaxPositiveRebaseBps() public view returns (uint256) { + uint256 v = maxPositiveRebaseBps; + return v == 0 ? DEFAULT_MAX_POSITIVE_REBASE_BPS : v; + } + /// @notice pay protocol fees including 5% to treaury, 5% to node operator and ethfund bnft holders /// @param _protocolFees The amount of protocol fees to pay in ether function payProtocolFees(uint128 _protocolFees) external { diff --git a/src/oracle/EtherFiAdmin.sol b/src/oracle/EtherFiAdmin.sol index 81a72a49..20aa6311 100644 --- a/src/oracle/EtherFiAdmin.sol +++ b/src/oracle/EtherFiAdmin.sol @@ -60,6 +60,10 @@ contract EtherFiAdmin is Initializable, OwnableUpgradeable, UUPSUpgradeable, Rol uint256 public maxFinalizedWithdrawalAmountPerDay; uint256 public maxNumValidatorsToApprovePerDay; + // Override for the per-report negative (slashing) rebase cap, in bps of TVL. + // 0 = use DEFAULT_MAX_NEGATIVE_REBASE_BPS. Settable behind the operating timelock. + uint256 public maxNegativeRebaseBps; + //-------------------------------------------------------------------------------------- //--------------------------------- IMMUTABLES -------------------------------------- //-------------------------------------------------------------------------------------- @@ -89,17 +93,16 @@ contract EtherFiAdmin is Initializable, OwnableUpgradeable, UUPSUpgradeable, Rol uint256 public constant STALE_REPORT_FINALIZATION_COOLDOWN = 7200; // 1 day uint256 public constant BASIS_POINTS_DENOMINATOR = 10_000; - // Absolute single-report rebase cap, in bps of current TVL, independent of how - // long the report spans. `acceptableRebaseAprInBps` is annualized over elapsedTime, - // so a report covering a long window (e.g. a stale-report catch-up, or a maliciously - // back-dated `refSlotFrom`) permits a proportionally larger ABSOLUTE move while still - // passing the APR check. This hard ceiling bounds the per-report share-rate delta - // regardless of elapsedTime: a single report can never move TVL by more than this, - // so a compromised oracle quorum cannot inflate/deflate the exchange rate arbitrarily - // in one report. A legitimate move beyond this requires deliberate governance action. - // 200 bps = 2% of TVL; normal daily reports move ~1-3 bps, so this only trips on - // anomalous reports while still leaving generous headroom for multi-week catch-ups. - int256 public constant SINGLE_REPORT_REBASE_DELTA_CAP_BPS = 200; + // Default cap on how far a single report may DECREASE TVL (slashing), in bps of TVL, + // independent of elapsedTime. `acceptableRebaseAprInBps` is annualized over elapsedTime, + // so a long-spanning report could pass it while still dropping an outsized absolute + // amount in one rebase. The max *initial* slashing penalty is maxEffBalance/4096 ≈ + // 2.44 bps of TVL even if every validator is slashed at once, so 3 bps is the tight + // default. `maxNegativeRebaseBps` (settable behind the operating timelock) overrides + // it — raised only if a correlated/mid-term slashing event is detected (visible well + // before a 2-day timelock matters). The positive/reward upper bound lives in + // LiquidityPool.rebase (the share-rate-increasing chokepoint). + uint256 public constant DEFAULT_MAX_NEGATIVE_REBASE_BPS = 3; struct ConstructorAddresses { address etherFiOracle; @@ -118,6 +121,7 @@ contract EtherFiAdmin is Initializable, OwnableUpgradeable, UUPSUpgradeable, Rol //-------------------------------------------------------------------------------------- event AdminUpdated(address _address, bool _isAdmin); + event MaxNegativeRebaseBpsUpdated(uint256 bps); event AdminOperationsExecuted(address indexed _address, bytes32 indexed _reportHash); event ValidatorApprovalTaskCreated(bytes32 indexed _taskHash, bytes32 indexed _reportHash, uint256[] _validators); @@ -133,6 +137,7 @@ contract EtherFiAdmin is Initializable, OwnableUpgradeable, UUPSUpgradeable, Rol error InvalidMaxFinalizedWithdrawalAmountPerDay(); error InvalidMaxNumValidatorsToApprovePerDay(); error InvalidAcceptableRebaseApr(); + error InvalidMaxNegativeRebaseBps(); error InvalidValidatorTaskBatchSize(); error InvalidMaxAcceptableRebaseApr(); error InvalidStaleOracleReportBlockWindow(); @@ -423,14 +428,17 @@ contract EtherFiAdmin is Initializable, OwnableUpgradeable, UUPSUpgradeable, Rol int256 absApr = (apr > 0) ? apr : - apr; if (absApr > acceptableRebaseAprInBps) return (false, "EtherFiAdmin: TVL changed too much"); - // Absolute per-report cap, independent of elapsedTime. The APR check above is - // annualized, so a long-spanning report can pass it while still moving an - // outsized absolute amount of TVL in a single rebase. Bound the absolute move - // to SINGLE_REPORT_REBASE_DELTA_CAP_BPS of current TVL so no single report can - // shift the exchange rate beyond this, regardless of the window it covers. - int256 absRewards = (_report.accruedRewards > 0) ? int256(_report.accruedRewards) : -int256(_report.accruedRewards); - if (currentTVL > 0 && absRewards * int256(BASIS_POINTS_DENOMINATOR) > currentTVL * SINGLE_REPORT_REBASE_DELTA_CAP_BPS) { - return (false, "EtherFiAdmin: rebase delta exceeds absolute cap"); + // Negative (slashing) cap, independent of elapsedTime. The APR check above is + // annualized, so a long-spanning report could pass it while still dropping an + // outsized absolute amount in one rebase. A single report can only legitimately + // DECREASE TVL by at most the max initial slashing penalty (~2.44 bps if every + // validator is slashed at once), so bound the drop to effectiveMaxNegativeRebaseBps. + // The positive/reward upper bound is enforced in LiquidityPool.rebase. + if (_report.accruedRewards < 0 && currentTVL > 0) { + int256 drop = -int256(_report.accruedRewards); + if (drop * int256(BASIS_POINTS_DENOMINATOR) > currentTVL * int256(effectiveMaxNegativeRebaseBps())) { + return (false, "EtherFiAdmin: negative rebase exceeds cap"); + } } return (true, ""); } @@ -469,6 +477,21 @@ contract EtherFiAdmin is Initializable, OwnableUpgradeable, UUPSUpgradeable, Rol return (true, ""); } + /// @notice Override the per-report negative (slashing) rebase cap, in bps of TVL. + /// @dev Operation-Timelock-gated. 0 resets to DEFAULT_MAX_NEGATIVE_REBASE_BPS. Capped + /// at 100% so it can be raised during a real correlated-slashing event but never + /// set to a nonsensical value. + function setMaxNegativeRebaseBps(uint256 _bps) external onlyAdmin { + if (_bps > BASIS_POINTS_DENOMINATOR) revert InvalidMaxNegativeRebaseBps(); + maxNegativeRebaseBps = _bps; + emit MaxNegativeRebaseBpsUpdated(_bps); + } + + function effectiveMaxNegativeRebaseBps() public view returns (uint256) { + uint256 v = maxNegativeRebaseBps; + return v == 0 ? DEFAULT_MAX_NEGATIVE_REBASE_BPS : v; + } + function slotForNextReportToProcess() public view returns (uint32) { return (lastHandledReportRefSlot == 0) ? 0 : lastHandledReportRefSlot + 1; } diff --git a/src/restaking/EtherFiRestaker.sol b/src/restaking/EtherFiRestaker.sol index f3e1b61b..41a9711a 100644 --- a/src/restaking/EtherFiRestaker.sol +++ b/src/restaking/EtherFiRestaker.sol @@ -12,6 +12,7 @@ import "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; import "@etherfi/deposits/Liquifier.sol"; import "@etherfi/core/LiquidityPool.sol"; import "@etherfi/governance/utils/RolesLibrary.sol"; +import "@etherfi/governance/utils/PausableUntil.sol"; import "@etherfi/eigenlayer-interfaces/IStrategyManager.sol"; import "@etherfi/eigenlayer-interfaces/IDelegationManager.sol"; @@ -21,7 +22,7 @@ import "@etherfi/deposits/interfaces/ILiquifier.sol"; import "@etherfi/core/interfaces/ILiquidityPool.sol"; import "@etherfi/governance/rate-limiting/interfaces/IEtherFiRateLimiter.sol"; -contract EtherFiRestaker is Initializable, UUPSUpgradeable, OwnableUpgradeable, PausableUpgradeable, RolesLibrary { +contract EtherFiRestaker is Initializable, UUPSUpgradeable, OwnableUpgradeable, PausableUpgradeable, PausableUntil, RolesLibrary { using SafeERC20 for IERC20; using EnumerableSet for EnumerableSet.Bytes32Set; @@ -143,7 +144,7 @@ contract EtherFiRestaker is Initializable, UUPSUpgradeable, OwnableUpgradeable, /// @notice Transfer stETH to a recipient for instant withdrawal /// @param recipient The address to receive stETH /// @param amount The amount of stETH to transfer - function transferStETH(address recipient, uint256 amount) external whenNotPaused { + function transferStETH(address recipient, uint256 amount) external whenNotHalted { if(msg.sender != etherFiRedemptionManager) revert IncorrectCaller(); if (amount > lido.balanceOf(address(this))) revert InsufficientBalance(); IERC20(address(lido)).safeTransfer(recipient, amount); @@ -158,7 +159,7 @@ contract EtherFiRestaker is Initializable, UUPSUpgradeable, OwnableUpgradeable, /// @notice Request for a specific amount of stETH holdings /// @param _amount the amount of stETH to request - function stEthRequestWithdrawal(uint256 _amount) public onlyExecutorOperations whenNotPaused returns (uint256[] memory) { + function stEthRequestWithdrawal(uint256 _amount) public onlyExecutorOperations whenNotHalted returns (uint256[] memory) { rateLimiter.consume(STETH_REQUEST_WITHDRAWAL_LIMIT_ID, _amountToGwei(_amount)); uint256 minAmount = lidoWithdrawalQueue.MIN_STETH_WITHDRAWAL_AMOUNT(); @@ -192,7 +193,7 @@ contract EtherFiRestaker is Initializable, UUPSUpgradeable, OwnableUpgradeable, /// @notice Claim a batch of withdrawal requests if they are finalized sending the ETH to the this contract back /// @param _requestIds array of request ids to claim /// @param _hints checkpoint hint for each id. Can be obtained with `findCheckpointHints()` - function stEthClaimWithdrawals(uint256[] calldata _requestIds, uint256[] calldata _hints) external onlyHousekeepingOperations whenNotPaused { + function stEthClaimWithdrawals(uint256[] calldata _requestIds, uint256[] calldata _hints) external onlyHousekeepingOperations whenNotHalted { lidoWithdrawalQueue.claimWithdrawals(_requestIds, _hints); _withdrawEther(); @@ -201,7 +202,7 @@ contract EtherFiRestaker is Initializable, UUPSUpgradeable, OwnableUpgradeable, } // Send the ETH back to the liquidity pool - function withdrawEther() public onlyHousekeepingOperations whenNotPaused { + function withdrawEther() public onlyHousekeepingOperations whenNotHalted { _withdrawEther(); } @@ -230,7 +231,7 @@ contract EtherFiRestaker is Initializable, UUPSUpgradeable, OwnableUpgradeable, } // undelegate from the current AVS operator & un-restake all - function undelegate() external onlyOperatingMultisig whenNotPaused returns (bytes32[] memory) { + function undelegate() external onlyOperatingMultisig whenNotHalted returns (bytes32[] memory) { bytes32[] memory withdrawalRoots = eigenLayerDelegationManager.undelegate(address(this)); for (uint256 i = 0; i < withdrawalRoots.length; i++) { @@ -245,7 +246,7 @@ contract EtherFiRestaker is Initializable, UUPSUpgradeable, OwnableUpgradeable, } // deposit the token in holding into the restaking strategy - function depositIntoStrategy(address token, uint256 amount) external onlyExecutorOperations whenNotPaused returns (uint256) { + function depositIntoStrategy(address token, uint256 amount) external onlyExecutorOperations whenNotHalted returns (uint256) { rateLimiter.consume(DEPOSIT_INTO_STRATEGY_LIMIT_ID, _amountToGwei(amount)); IERC20(token).safeIncreaseAllowance(address(eigenLayerStrategyManager), amount); @@ -260,7 +261,7 @@ contract EtherFiRestaker is Initializable, UUPSUpgradeable, OwnableUpgradeable, /// Made easy for operators /// @param token the token to withdraw /// @param amount the amount of token to withdraw - function queueWithdrawals(address token, uint256 amount) public onlyExecutorOperations whenNotPaused returns (bytes32[] memory) { + function queueWithdrawals(address token, uint256 amount) public onlyExecutorOperations whenNotHalted returns (bytes32[] memory) { rateLimiter.consume(QUEUE_WITHDRAWALS_LIMIT_ID, _amountToGwei(amount)); uint256 shares = getEigenLayerRestakingStrategy(token).underlyingToSharesView(amount); @@ -280,7 +281,7 @@ contract EtherFiRestaker is Initializable, UUPSUpgradeable, OwnableUpgradeable, function completeQueuedWithdrawals( IDelegationManager.Withdrawal[] memory _queuedWithdrawals, IERC20[][] memory _tokens - ) external onlyHousekeepingOperations whenNotPaused { + ) external onlyHousekeepingOperations whenNotHalted { uint256 num = _queuedWithdrawals.length; bool[] memory receiveAsTokens = new bool[](num); for (uint256 i = 0; i < num; i++) { @@ -397,25 +398,41 @@ contract EtherFiRestaker is Initializable, UUPSUpgradeable, OwnableUpgradeable, return total; } - // Pauses the contract (multisig path) + // Boolean pause (multisig). Indefinite until the multisig unpauses. function pauseContract() external onlyOperatingMultisig { _pause(); } - /// @notice Fast emergency halt callable by the Guardian (Hypernative / EOA keys). - /// @dev Pillar 2 (halt is cheap, redundant, parallel): the Guardian can stop fund - /// movement immediately without assembling the multisig. Resume stays deliberate - /// (multisig-only `unPauseContract`). This mirrors the token/LP halt model where - /// the Guardian can pause but only the Operating Multisig can unpause. - function guardianPause() external onlyGuardian { - _pause(); - } - - // Unpauses the contract (deliberate, multisig-only) + // Unpauses the boolean pause (deliberate, multisig-only) function unPauseContract() external onlyOperatingMultisig { _unpause(); } + /// @notice Fast, auto-expiring emergency halt callable by the Guardian (Hypernative / + /// EOA keys). Mirrors the protocol-wide halt model (tokens, LP, redemption): + /// the Guardian can stop fund movement immediately without assembling the + /// multisig, the pause auto-expires (8h–30d) with a per-pauser cooldown so a + /// wrong/compromised halt self-heals, and resume stays deliberate (multisig). + function pauseContractUntil() external onlyGuardian { + _pauseUntil(); + } + + function unpauseContractUntil() external onlyOperatingMultisig { + _unpauseUntil(); + } + + function setPauseUntilDuration(uint256 _pauseUntilDuration) external onlyAdmin { + _setPauseUntilDuration(_pauseUntilDuration); + } + + /// @dev Halts when EITHER the boolean pause or the auto-expiring pause is active. + /// Named distinctly from OZ's `whenNotPaused` (which only checks the boolean). + modifier whenNotHalted() { + _requireNotPaused(); // OZ boolean pause + _requireNotPausedUntil(); // auto-expiring guardian pause + _; + } + // INTERNAL functions /// @dev Convert wei to gwei for rate-limiter buckets, with overflow check. diff --git a/test/EtherFiRestaker.t.sol b/test/EtherFiRestaker.t.sol index a1e6b9ae..646b05ae 100644 --- a/test/EtherFiRestaker.t.sol +++ b/test/EtherFiRestaker.t.sol @@ -7,6 +7,7 @@ import "forge-std/Test.sol"; import "@openzeppelin-upgradeable/contracts/token/ERC20/extensions/ERC20BurnableUpgradeable.sol"; import "@etherfi/eigenlayer-interfaces/IDelegationManager.sol"; +import "@etherfi/governance/utils/PausableUntil.sol"; import "@etherfi/eigenlayer-interfaces/IStrategyManager.sol"; import "@etherfi/eigenlayer-interfaces/ISignatureUtils.sol"; @@ -292,45 +293,56 @@ contract EtherFiRestakerTest is TestSetup { etherFiRestakerInstance.getTotalPooledEther(); } - // PR #385 security review (H1): the restaker's pause previously gated nothing — - // pauseContract() called _pause() but no money-movement function carried - // whenNotPaused. This asserts the pause now actually halts fund movement, and that - // the Guardian (HN/EOA keys) can fire it for a fast, redundant halt (Pillar 2). - function test_guardianPause_halts_fund_movement() public { + // PR #385 security review (H1 + Yash's review): the restaker's pause previously gated + // nothing — pauseContract() flipped a flag but no money-movement function checked it. + // It now uses the protocol-wide PausableUntil model: the Guardian (HN/EOA keys) fires + // an auto-expiring halt, the multisig has a boolean pause, and both stop fund movement. + function test_guardianPauseUntil_halts_fund_movement() public { vm.startPrank(roleRegistryInstance.owner()); roleRegistryInstance.grantRole(roleRegistryInstance.GUARDIAN_ROLE(), bob); vm.stopPrank(); - assertFalse(etherFiRestakerInstance.paused()); + // operating timelock (owner) sets the auto-expiry duration + vm.prank(owner); + etherFiRestakerInstance.setPauseUntilDuration(8 hours); - // Guardian fast-halt + // Guardian fast-halt (auto-expiring) vm.prank(bob); - etherFiRestakerInstance.guardianPause(); - assertTrue(etherFiRestakerInstance.paused()); + etherFiRestakerInstance.pauseContractUntil(); + uint256 until = etherFiRestakerInstance.pausedUntil(); + assertGt(until, block.timestamp); - // whenNotPaused is the first gate on transferStETH, so the pause check fires - // before the caller check — proves fund movement is actually halted. - vm.expectRevert("Pausable: paused"); + // whenNotHalted is the first gate on transferStETH, so the halt fires before the + // caller check — proves fund movement is actually stopped. + vm.expectRevert(abi.encodeWithSelector(PausableUntil.ContractPausedUntil.selector, until)); etherFiRestakerInstance.transferStETH(bob, 1); - // withdrawEther (owner holds HOUSEKEEPING_OPERATIONS) passes the role gate then - // reverts on the pause gate. - vm.prank(owner); - vm.expectRevert("Pausable: paused"); - etherFiRestakerInstance.withdrawEther(); - // undelegate (owner holds OPERATION_MULTISIG) queues withdrawal of ALL restaked - // assets — same fund-flow category, so it must also be halted by the pause. + // assets — same fund-flow category, so it must also be halted. vm.prank(owner); - vm.expectRevert("Pausable: paused"); + vm.expectRevert(abi.encodeWithSelector(PausableUntil.ContractPausedUntil.selector, until)); etherFiRestakerInstance.undelegate(); - // a non-guardian cannot fire the guardian halt + // a non-guardian cannot fire the auto-expiring halt vm.prank(alice); vm.expectRevert(RoleRegistry.OnlyGuardian.selector); - etherFiRestakerInstance.guardianPause(); + etherFiRestakerInstance.pauseContractUntil(); + + // resume is deliberate / multisig-only + vm.prank(owner); + etherFiRestakerInstance.unpauseContractUntil(); + assertEq(etherFiRestakerInstance.pausedUntil(), 0); + } + + // The boolean multisig pause also halts fund movement (reverts via OZ Pausable). + function test_booleanPause_also_halts_fund_movement() public { + vm.prank(owner); // owner holds OPERATION_MULTISIG + etherFiRestakerInstance.pauseContract(); + assertTrue(etherFiRestakerInstance.paused()); + + vm.expectRevert("Pausable: paused"); + etherFiRestakerInstance.transferStETH(bob, 1); - // resume is deliberate / multisig-only (owner holds OPERATION_MULTISIG) vm.prank(owner); etherFiRestakerInstance.unPauseContract(); assertFalse(etherFiRestakerInstance.paused()); diff --git a/test/TestSetup.sol b/test/TestSetup.sol index 9937f036..cdf6740a 100644 --- a/test/TestSetup.sol +++ b/test/TestSetup.sol @@ -755,6 +755,13 @@ contract TestSetup is Test, ContractCodeChecker, DepositDataGeneration { liquidityPoolInstance.setMinWithdrawAmount(0.001 ether); vm.stopPrank(); + // PR #385: disable the new per-rebase caps for generic fork tests (some rebase a + // few % of TVL, above the 25bps default). Dedicated tests set explicit values. + vm.startPrank(admin); + liquidityPoolInstance.setMaxPositiveRebaseBps(10_000); + etherFiAdminInstance.setMaxNegativeRebaseBps(10_000); + vm.stopPrank(); + // Run the one-shot escrow migration so `requestWithdraw` / `withdraw` // (both gated by `escrowMigrationCompleted`) function on the upgraded // LP. Idempotent: skip if already complete. @@ -1439,6 +1446,12 @@ contract TestSetup is Test, ContractCodeChecker, DepositDataGeneration { liquidityPoolInstance.setValidatorSizeWei(32 ether); liquidityPoolInstance.setMaxWithdrawAmount(1000 ether); liquidityPoolInstance.setMinWithdrawAmount(0.001 ether); + // PR #385: the new per-rebase caps (positive in LP, negative in EtherFiAdmin) + // would reject the large artificial rebases many generic tests perform on a small + // fresh-deploy TVL. Disable them here (100% = effectively no bound); dedicated + // tests set explicit values to exercise enforcement. + liquidityPoolInstance.setMaxPositiveRebaseBps(10_000); + etherFiAdminInstance.setMaxNegativeRebaseBps(10_000); // Pause WithdrawRequestNFT so existing tests that unPauseContract in their // own setUp continue to find it paused (initializeOnUpgrade used to set this). withdrawRequestNFTInstance.pauseContract(); From 9954e5670df0a120500e5901eb9afc37986f1935 Mon Sep 17 00:00:00 2001 From: ReposCollector Date: Fri, 29 May 2026 12:18:05 -0400 Subject: [PATCH 5/6] test: LiquidityPool positive rebase cap enforcement Co-Authored-By: Claude Opus 4.8 --- test/LiquidityPool.t.sol | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/test/LiquidityPool.t.sol b/test/LiquidityPool.t.sol index 92bdea45..d3c47856 100644 --- a/test/LiquidityPool.t.sol +++ b/test/LiquidityPool.t.sol @@ -1145,6 +1145,37 @@ contract LiquidityPoolTest is TestSetup { assertEq(totalPooledAfter, totalPooledBefore + 5 ether); } + // PR #385 (Yash review): the positive (reward) rebase upper bound is enforced inside + // LiquidityPool.rebase, at the share-rate chokepoint, regardless of caller. + function test_positiveRebaseCap_enforced() public { + vm.deal(alice, 100 ether); + vm.prank(alice); + liquidityPoolInstance.deposit{value: 100 ether}(); + + // reset the override so the default (25 bps) applies (TestSetup set it to 100%) + vm.prank(alice); // alice holds OPERATION_TIMELOCK + liquidityPoolInstance.setMaxPositiveRebaseBps(0); + assertEq(liquidityPoolInstance.effectiveMaxPositiveRebaseBps(), 25); + + uint256 tvl = liquidityPoolInstance.getTotalPooledEther(); + uint256 cap = (tvl * 25) / 10_000; + + // one wei over the cap reverts + vm.prank(address(membershipManagerInstance)); + vm.expectRevert(LiquidityPool.RebaseExceedsPositiveCap.selector); + liquidityPoolInstance.rebase(int128(uint128(cap + 1))); + + // exactly at the cap succeeds + vm.prank(address(membershipManagerInstance)); + liquidityPoolInstance.rebase(int128(uint128(cap))); + + // a configured higher bound lets a larger rebase through + vm.prank(alice); + liquidityPoolInstance.setMaxPositiveRebaseBps(10_000); + vm.prank(address(membershipManagerInstance)); + liquidityPoolInstance.rebase(int128(uint128(tvl / 2))); + } + function test_RebaseFailsIfNotMembershipManager() public { vm.startPrank(alice); vm.expectRevert(LiquidityPool.IncorrectCaller.selector); From 019e84f82e0f08393aa95122e5b6ae88d0d1a93c Mon Sep 17 00:00:00 2001 From: ReposCollector Date: Fri, 29 May 2026 13:20:10 -0400 Subject: [PATCH 6/6] Address Yash review round 2 on #445 - LiquidityPool: positive rebase cap is now a hard 25 bps constant (MAX_POSITIVE_REBASE_BPS), not governance-configurable (per review: no legitimate single report raises rate >25 bps). Dropped the setter/storage/event/error. Negative (slashing) cap in EtherFiAdmin stays configurable behind the operating timelock. - EtherFiRestaker: use the standard whenNotPaused everywhere by overriding _requireNotPaused() to also check _requireNotPausedUntil() (per review), instead of a bespoke whenNotHalted. - EtherFiRestaker: rename unPauseContract -> unpauseContract (+ interface). - PausableUntil: _pauseUntil falls back to MIN_PAUSE_DURATION when duration is unset, so the Guardian's emergency pause is never a silent no-op that still burns the cooldown (bot finding). - Tests: rewrote the rebase tests that used unrealistically large (5%-100%) rebases on tiny fresh-deploy TVL to use within-cap (<=0.25%) rebases with recomputed assertions; restaker pause tests updated for the rename. Pre-existing ERC721/withdraw fork-harness failures (identical on base) are unrelated. Co-Authored-By: Claude Opus 4.8 --- src/core/LiquidityPool.sol | 34 ++---- src/governance/utils/PausableUntil.sol | 6 + src/restaking/EtherFiRestaker.sol | 31 ++--- src/restaking/interfaces/IEtherFiRestaker.sol | 2 +- test/EETH.t.sol | 84 ++++++-------- test/EtherFiRestaker.t.sol | 2 +- test/LiquidityPool.t.sol | 107 ++++++++++-------- test/LpRebaseWrnClaimUnderflow.t.sol | 20 ++-- test/MembershipManager.t.sol | 5 +- test/TestSetup.sol | 2 - 10 files changed, 140 insertions(+), 153 deletions(-) diff --git a/src/core/LiquidityPool.sol b/src/core/LiquidityPool.sol index 5ee6db1a..eb9dd939 100644 --- a/src/core/LiquidityPool.sol +++ b/src/core/LiquidityPool.sol @@ -52,10 +52,6 @@ contract LiquidityPool is Initializable, OwnableUpgradeable, UUPSUpgradeable, Re uint256 public minWithdrawAmount; bool public escrowMigrationCompleted; - // Override for the per-rebase positive (reward) cap, in bps of TVL. - // 0 = use DEFAULT_MAX_POSITIVE_REBASE_BPS. Settable behind the operating timelock. - uint256 public maxPositiveRebaseBps; - //-------------------------------------------------------------------------------------- //------------------------------------- IMMUTABLES ---------------------------------- //-------------------------------------------------------------------------------------- @@ -77,11 +73,12 @@ contract LiquidityPool is Initializable, OwnableUpgradeable, UUPSUpgradeable, Re //-------------------------------------------------------------------------------------- uint256 public constant SHARE_UNIT = 1e18; - // Default cap on how far a single rebase may INCREASE TVL (rewards), in bps of TVL. - // 25 bps ≈ 1 month of reward accrual at 3% APR — generous for a normal report, - // tight enough that a buggy/compromised rebase caller cannot inflate the share rate - // arbitrarily. Overridable via `maxPositiveRebaseBps` behind the operating timelock. - uint256 public constant DEFAULT_MAX_POSITIVE_REBASE_BPS = 25; + // Hard cap on how far a single rebase may INCREASE TVL (rewards), in bps of TVL. + // 25 bps ≈ 1 month of reward accrual at 3% APR — there is no legitimate reason for a + // single report to raise the rate by more than this, so it is a fixed invariant (not + // governance-configurable). Bounds a buggy/compromised rebase caller at the share-rate + // chokepoint regardless of the oracle-side checks. + uint256 public constant MAX_POSITIVE_REBASE_BPS = 25; uint256 private constant REBASE_BPS_DENOMINATOR = 10_000; //-------------------------------------------------------------------------------------- @@ -105,7 +102,6 @@ contract LiquidityPool is Initializable, OwnableUpgradeable, UUPSUpgradeable, Re event ValidatorApproved(uint256 indexed validatorId); event ValidatorRegistrationCanceled(uint256 indexed validatorId); event Rebase(uint256 totalEthLocked, uint256 totalEEthShares); - event MaxPositiveRebaseBpsUpdated(uint256 bps); event ProtocolFeePaid(uint128 protocolFees); event WhitelistStatusUpdated(bool value); event MinWithdrawAmountSet(uint256 minWithdrawAmount); @@ -126,7 +122,6 @@ contract LiquidityPool is Initializable, OwnableUpgradeable, UUPSUpgradeable, Re error InvalidAmountForShare(); error InvalidRate(); error RebaseExceedsPositiveCap(); - error InvalidMaxPositiveRebaseBps(); error AlreadyMigrated(); error MigrationNotComplete(); error AlreadyRegistered(); @@ -455,11 +450,11 @@ contract LiquidityPool is Initializable, OwnableUpgradeable, UUPSUpgradeable, Re // Positive (reward) upper bound, enforced at the share-rate chokepoint regardless // of who calls rebase. A single rebase cannot increase TVL by more than - // effectiveMaxPositiveRebaseBps() of pre-rebase TVL. Defense-in-depth alongside the + // MAX_POSITIVE_REBASE_BPS of pre-rebase TVL. Defense-in-depth alongside the // oracle-side negative cap in EtherFiAdmin; the negative side is intentionally not // re-checked here (the oracle path owns it and bounds it tighter). if (_accruedRewards > 0) { - uint256 maxIncrease = (getTotalPooledEther() * effectiveMaxPositiveRebaseBps()) / REBASE_BPS_DENOMINATOR; + uint256 maxIncrease = (getTotalPooledEther() * MAX_POSITIVE_REBASE_BPS) / REBASE_BPS_DENOMINATOR; if (uint256(uint128(_accruedRewards)) > maxIncrease) revert RebaseExceedsPositiveCap(); } @@ -470,19 +465,6 @@ contract LiquidityPool is Initializable, OwnableUpgradeable, UUPSUpgradeable, Re emit Rebase(getTotalPooledEther(), eETH.totalShares()); } - /// @notice Override the per-rebase positive (reward) cap, in bps of TVL. - /// @dev Operation-Timelock-gated. 0 resets to DEFAULT_MAX_POSITIVE_REBASE_BPS. - function setMaxPositiveRebaseBps(uint256 _bps) external onlyAdmin { - if (_bps > REBASE_BPS_DENOMINATOR) revert InvalidMaxPositiveRebaseBps(); - maxPositiveRebaseBps = _bps; - emit MaxPositiveRebaseBpsUpdated(_bps); - } - - function effectiveMaxPositiveRebaseBps() public view returns (uint256) { - uint256 v = maxPositiveRebaseBps; - return v == 0 ? DEFAULT_MAX_POSITIVE_REBASE_BPS : v; - } - /// @notice pay protocol fees including 5% to treaury, 5% to node operator and ethfund bnft holders /// @param _protocolFees The amount of protocol fees to pay in ether function payProtocolFees(uint128 _protocolFees) external { diff --git a/src/governance/utils/PausableUntil.sol b/src/governance/utils/PausableUntil.sol index 3ca7d74b..cead3d12 100644 --- a/src/governance/utils/PausableUntil.sol +++ b/src/governance/utils/PausableUntil.sol @@ -57,6 +57,12 @@ abstract contract PausableUntil { _requireNotPausedUntil(); PausableUntilStorage storage $ = _getPausableUntilStorage(); uint256 pauseUntilDuration = $.pauseUntilDuration; + // If the duration was never configured (0 — e.g. a fresh proxy or a just-upgraded + // contract before setPauseUntilDuration is called), fall back to MIN_PAUSE_DURATION + // so the emergency pause is always effective. Without this, `pausedUntil` would be + // `block.timestamp + 0` (expires the same block) — a silent no-op that still burns + // the pauser's cooldown. + if (pauseUntilDuration == 0) pauseUntilDuration = MIN_PAUSE_DURATION; if ($.lastPauseTimestamp[msg.sender] + pauseUntilDuration + PAUSER_UNTIL_COOLDOWN > block.timestamp) revert PauserCooldownStillActive(); $.pausedUntil = block.timestamp + pauseUntilDuration; $.lastPauseTimestamp[msg.sender] = block.timestamp; diff --git a/src/restaking/EtherFiRestaker.sol b/src/restaking/EtherFiRestaker.sol index 41a9711a..e5dec4e7 100644 --- a/src/restaking/EtherFiRestaker.sol +++ b/src/restaking/EtherFiRestaker.sol @@ -144,7 +144,7 @@ contract EtherFiRestaker is Initializable, UUPSUpgradeable, OwnableUpgradeable, /// @notice Transfer stETH to a recipient for instant withdrawal /// @param recipient The address to receive stETH /// @param amount The amount of stETH to transfer - function transferStETH(address recipient, uint256 amount) external whenNotHalted { + function transferStETH(address recipient, uint256 amount) external whenNotPaused { if(msg.sender != etherFiRedemptionManager) revert IncorrectCaller(); if (amount > lido.balanceOf(address(this))) revert InsufficientBalance(); IERC20(address(lido)).safeTransfer(recipient, amount); @@ -159,7 +159,7 @@ contract EtherFiRestaker is Initializable, UUPSUpgradeable, OwnableUpgradeable, /// @notice Request for a specific amount of stETH holdings /// @param _amount the amount of stETH to request - function stEthRequestWithdrawal(uint256 _amount) public onlyExecutorOperations whenNotHalted returns (uint256[] memory) { + function stEthRequestWithdrawal(uint256 _amount) public onlyExecutorOperations whenNotPaused returns (uint256[] memory) { rateLimiter.consume(STETH_REQUEST_WITHDRAWAL_LIMIT_ID, _amountToGwei(_amount)); uint256 minAmount = lidoWithdrawalQueue.MIN_STETH_WITHDRAWAL_AMOUNT(); @@ -193,7 +193,7 @@ contract EtherFiRestaker is Initializable, UUPSUpgradeable, OwnableUpgradeable, /// @notice Claim a batch of withdrawal requests if they are finalized sending the ETH to the this contract back /// @param _requestIds array of request ids to claim /// @param _hints checkpoint hint for each id. Can be obtained with `findCheckpointHints()` - function stEthClaimWithdrawals(uint256[] calldata _requestIds, uint256[] calldata _hints) external onlyHousekeepingOperations whenNotHalted { + function stEthClaimWithdrawals(uint256[] calldata _requestIds, uint256[] calldata _hints) external onlyHousekeepingOperations whenNotPaused { lidoWithdrawalQueue.claimWithdrawals(_requestIds, _hints); _withdrawEther(); @@ -202,7 +202,7 @@ contract EtherFiRestaker is Initializable, UUPSUpgradeable, OwnableUpgradeable, } // Send the ETH back to the liquidity pool - function withdrawEther() public onlyHousekeepingOperations whenNotHalted { + function withdrawEther() public onlyHousekeepingOperations whenNotPaused { _withdrawEther(); } @@ -231,7 +231,7 @@ contract EtherFiRestaker is Initializable, UUPSUpgradeable, OwnableUpgradeable, } // undelegate from the current AVS operator & un-restake all - function undelegate() external onlyOperatingMultisig whenNotHalted returns (bytes32[] memory) { + function undelegate() external onlyOperatingMultisig whenNotPaused returns (bytes32[] memory) { bytes32[] memory withdrawalRoots = eigenLayerDelegationManager.undelegate(address(this)); for (uint256 i = 0; i < withdrawalRoots.length; i++) { @@ -246,7 +246,7 @@ contract EtherFiRestaker is Initializable, UUPSUpgradeable, OwnableUpgradeable, } // deposit the token in holding into the restaking strategy - function depositIntoStrategy(address token, uint256 amount) external onlyExecutorOperations whenNotHalted returns (uint256) { + function depositIntoStrategy(address token, uint256 amount) external onlyExecutorOperations whenNotPaused returns (uint256) { rateLimiter.consume(DEPOSIT_INTO_STRATEGY_LIMIT_ID, _amountToGwei(amount)); IERC20(token).safeIncreaseAllowance(address(eigenLayerStrategyManager), amount); @@ -261,7 +261,7 @@ contract EtherFiRestaker is Initializable, UUPSUpgradeable, OwnableUpgradeable, /// Made easy for operators /// @param token the token to withdraw /// @param amount the amount of token to withdraw - function queueWithdrawals(address token, uint256 amount) public onlyExecutorOperations whenNotHalted returns (bytes32[] memory) { + function queueWithdrawals(address token, uint256 amount) public onlyExecutorOperations whenNotPaused returns (bytes32[] memory) { rateLimiter.consume(QUEUE_WITHDRAWALS_LIMIT_ID, _amountToGwei(amount)); uint256 shares = getEigenLayerRestakingStrategy(token).underlyingToSharesView(amount); @@ -281,7 +281,7 @@ contract EtherFiRestaker is Initializable, UUPSUpgradeable, OwnableUpgradeable, function completeQueuedWithdrawals( IDelegationManager.Withdrawal[] memory _queuedWithdrawals, IERC20[][] memory _tokens - ) external onlyHousekeepingOperations whenNotHalted { + ) external onlyHousekeepingOperations whenNotPaused { uint256 num = _queuedWithdrawals.length; bool[] memory receiveAsTokens = new bool[](num); for (uint256 i = 0; i < num; i++) { @@ -404,7 +404,7 @@ contract EtherFiRestaker is Initializable, UUPSUpgradeable, OwnableUpgradeable, } // Unpauses the boolean pause (deliberate, multisig-only) - function unPauseContract() external onlyOperatingMultisig { + function unpauseContract() external onlyOperatingMultisig { _unpause(); } @@ -425,12 +425,13 @@ contract EtherFiRestaker is Initializable, UUPSUpgradeable, OwnableUpgradeable, _setPauseUntilDuration(_pauseUntilDuration); } - /// @dev Halts when EITHER the boolean pause or the auto-expiring pause is active. - /// Named distinctly from OZ's `whenNotPaused` (which only checks the boolean). - modifier whenNotHalted() { - _requireNotPaused(); // OZ boolean pause - _requireNotPausedUntil(); // auto-expiring guardian pause - _; + /// @dev Fold the auto-expiring PausableUntil check into OZ's `_requireNotPaused`, so + /// the standard `whenNotPaused` modifier (used on every fund-flow fn) halts on + /// EITHER the boolean pause or the Guardian's auto-expiring pause — consistent + /// with the rest of the protocol, no bespoke modifier needed. + function _requireNotPaused() internal view override { + _requireNotPausedUntil(); + super._requireNotPaused(); } // INTERNAL functions diff --git a/src/restaking/interfaces/IEtherFiRestaker.sol b/src/restaking/interfaces/IEtherFiRestaker.sol index 48ec17bb..3aafb33c 100644 --- a/src/restaking/interfaces/IEtherFiRestaker.sol +++ b/src/restaking/interfaces/IEtherFiRestaker.sol @@ -12,5 +12,5 @@ interface IEtherFiRestaker { function transferStETH(address recipient, uint256 amount) external; function lido() external view returns (ILido); function pauseContract() external; - function unPauseContract() external; + function unpauseContract() external; } \ No newline at end of file diff --git a/test/EETH.t.sol b/test/EETH.t.sol index fd239f39..e6e4d67e 100644 --- a/test/EETH.t.sol +++ b/test/EETH.t.sol @@ -84,72 +84,62 @@ contract EETHTest is TestSetup { function test_EEthRebase() public { assertEq(liquidityPoolInstance.getTotalPooledEther(), 0 ether); - // Total pooled ether = 10 + // Amounts scaled so each rebase stays within the 25 bps per-report cap (0.25% of + // TVL) while preserving clean share math. Total pooled ether = 10000 startHoax(alice); - liquidityPoolInstance.deposit{value: 10 ether}(); + liquidityPoolInstance.deposit{value: 10000 ether}(); vm.stopPrank(); - assertEq(liquidityPoolInstance.getTotalPooledEther(), 10 ether); - assertEq(eETHInstance.totalSupply(), 10 ether); - assertEq(eETHInstance.totalShares(), 10 ether); - assertEq(eETHInstance.shares(alice), 10 ether); + assertEq(liquidityPoolInstance.getTotalPooledEther(), 10000 ether); + assertEq(eETHInstance.totalSupply(), 10000 ether); + assertEq(eETHInstance.totalShares(), 10000 ether); + assertEq(eETHInstance.shares(alice), 10000 ether); - // Total pooled ether = 20 + // +25 ether reward = 0.25% of 10000 (exactly at the cap). Pooled = 10025 vm.prank(address(membershipManagerInstance)); - liquidityPoolInstance.rebase(10 ether); - _transferTo(address(liquidityPoolInstance), 10 ether); + liquidityPoolInstance.rebase(25 ether); + _transferTo(address(liquidityPoolInstance), 25 ether); - assertEq(liquidityPoolInstance.getTotalPooledEther(), 20 ether); - assertEq(eETHInstance.totalSupply(), 20 ether); - assertEq(eETHInstance.totalShares(), 10 ether); - assertEq(eETHInstance.shares(alice), 10 ether); + assertEq(liquidityPoolInstance.getTotalPooledEther(), 10025 ether); + assertEq(eETHInstance.totalSupply(), 10025 ether); + assertEq(eETHInstance.totalShares(), 10000 ether); + assertEq(eETHInstance.shares(alice), 10000 ether); - // ALice total claimable Ether - /// (20 * 10) / 10 - assertEq(liquidityPoolInstance.getTotalEtherClaimOf(alice), 20 ether); + // Alice total claimable Ether: (10025 * 10000) / 10000 = 10025 + assertEq(liquidityPoolInstance.getTotalEtherClaimOf(alice), 10025 ether); startHoax(bob); - liquidityPoolInstance.deposit{value: 5 ether}(); + liquidityPoolInstance.deposit{value: 10025 ether}(); vm.stopPrank(); - assertEq(liquidityPoolInstance.getTotalPooledEther(), 25 ether); - assertEq(eETHInstance.totalSupply(), 25 ether); + assertEq(liquidityPoolInstance.getTotalPooledEther(), 20050 ether); + assertEq(eETHInstance.totalSupply(), 20050 ether); - // Bob Shares = (5 * 10) / (25 - 5) = 2,5 - assertEq(eETHInstance.shares(bob), 2.5 ether); - assertEq(eETHInstance.totalShares(), 12.5 ether); + // Bob Shares = (10025 * 10000) / (20050 - 10025) = 10000 + assertEq(eETHInstance.shares(bob), 10000 ether); + assertEq(eETHInstance.totalShares(), 20000 ether); - // Bob claimable Ether - /// (25 * 2,5) / 12,5 = 5 ether + // claimable: (20050 * 10000) / 20000 = 10025 each + assertEq(liquidityPoolInstance.getTotalEtherClaimOf(alice), 10025 ether); + assertEq(liquidityPoolInstance.getTotalEtherClaimOf(bob), 10025 ether); - //ALice Claimable Ether - /// (25 * 10) / 12,5 = 20 ether - assertEq(liquidityPoolInstance.getTotalEtherClaimOf(alice), 20 ether); - assertEq(liquidityPoolInstance.getTotalEtherClaimOf(bob), 5 ether); + assertEq(eETHInstance.balanceOf(alice), 10025 ether); + assertEq(eETHInstance.balanceOf(bob), 10025 ether); - assertEq(eETHInstance.balanceOf(alice), 20 ether); - assertEq(eETHInstance.balanceOf(bob), 5 ether); - - // Staking Rewards sent to liquidity pool - /// vm.deal sets the balance of whoever its called on - /// In this case 10 ether is added as reward + // +50 ether reward = ~0.25% of 20050 (within cap). Pooled = 20100 vm.prank(address(membershipManagerInstance)); - liquidityPoolInstance.rebase(10 ether); - _transferTo(address(liquidityPoolInstance), 10 ether); - - assertEq(liquidityPoolInstance.getTotalPooledEther(), 35 ether); - assertEq(eETHInstance.totalSupply(), 35 ether); + liquidityPoolInstance.rebase(50 ether); + _transferTo(address(liquidityPoolInstance), 50 ether); - // Bob claimable Ether - /// (35 * 2,5) / 12,5 = 7 ether - assertEq(liquidityPoolInstance.getTotalEtherClaimOf(bob), 7 ether); + assertEq(liquidityPoolInstance.getTotalPooledEther(), 20100 ether); + assertEq(eETHInstance.totalSupply(), 20100 ether); - // Alice Claimable Ether - /// (35 * 10) / 12,5 = 20 ether - assertEq(liquidityPoolInstance.getTotalEtherClaimOf(alice), 28 ether); + // claimable: (20100 * 10000) / 20000 = 10050 each + assertEq(liquidityPoolInstance.getTotalEtherClaimOf(bob), 10050 ether); + assertEq(liquidityPoolInstance.getTotalEtherClaimOf(alice), 10050 ether); - assertEq(eETHInstance.balanceOf(alice), 28 ether); - assertEq(eETHInstance.balanceOf(bob), 7 ether); + assertEq(eETHInstance.balanceOf(alice), 10050 ether); + assertEq(eETHInstance.balanceOf(bob), 10050 ether); } function test_TransferWithAmount() public { diff --git a/test/EtherFiRestaker.t.sol b/test/EtherFiRestaker.t.sol index 646b05ae..33280777 100644 --- a/test/EtherFiRestaker.t.sol +++ b/test/EtherFiRestaker.t.sol @@ -344,7 +344,7 @@ contract EtherFiRestakerTest is TestSetup { etherFiRestakerInstance.transferStETH(bob, 1); vm.prank(owner); - etherFiRestakerInstance.unPauseContract(); + etherFiRestakerInstance.unpauseContract(); assertFalse(etherFiRestakerInstance.paused()); } diff --git a/test/LiquidityPool.t.sol b/test/LiquidityPool.t.sol index d3c47856..0b407a51 100644 --- a/test/LiquidityPool.t.sol +++ b/test/LiquidityPool.t.sol @@ -246,42 +246,45 @@ contract LiquidityPoolTest is TestSetup { } function test_WithdrawLiquidityPoolAccrueStakingRewardsWithoutPartialWithdrawal() public { - vm.deal(alice, 3 ether); + // Deposits scaled to 400 each (TVL 800) so the 2 ETH rebase stays within the + // 25 bps cap (0.25% of 800 = 2 ETH). The rebase still adds exactly 2 ETH of + // out-of-LP headroom, so the fallback-funding pattern below is unchanged. + vm.deal(alice, 401 ether); vm.startPrank(alice); - liquidityPoolInstance.deposit{value: 2 ether}(); + liquidityPoolInstance.deposit{value: 400 ether}(); assertEq(alice.balance, 1 ether); - assertEq(eETHInstance.balanceOf(alice), 2 ether); + assertEq(eETHInstance.balanceOf(alice), 400 ether); assertEq(eETHInstance.balanceOf(bob), 0); vm.stopPrank(); - vm.deal(bob, 3 ether); + vm.deal(bob, 401 ether); vm.startPrank(bob); - liquidityPoolInstance.deposit{value: 2 ether}(); + liquidityPoolInstance.deposit{value: 400 ether}(); assertEq(bob.balance, 1 ether); - assertEq(eETHInstance.balanceOf(alice), 2 ether); - assertEq(eETHInstance.balanceOf(bob), 2 ether); + assertEq(eETHInstance.balanceOf(alice), 400 ether); + assertEq(eETHInstance.balanceOf(bob), 400 ether); vm.stopPrank(); vm.deal(owner, 100 ether); vm.prank(address(membershipManagerInstance)); - liquidityPoolInstance.rebase(2 ether); - assertEq(eETHInstance.balanceOf(alice), 3 ether); - assertEq(eETHInstance.balanceOf(bob), 3 ether); + liquidityPoolInstance.rebase(2 ether); // 0.25% of 800 ETH TVL — exactly at the cap + assertEq(eETHInstance.balanceOf(alice), 401 ether); + assertEq(eETHInstance.balanceOf(bob), 401 ether); (bool sent, ) = address(liquidityPoolInstance).call{value: 1 ether}(""); assertEq(sent, true); - assertEq(eETHInstance.balanceOf(alice), 3 ether); - assertEq(eETHInstance.balanceOf(bob), 3 ether); + assertEq(eETHInstance.balanceOf(alice), 401 ether); + assertEq(eETHInstance.balanceOf(bob), 401 ether); (sent, ) = address(liquidityPoolInstance).call{value: 1 ether}(""); assertEq(sent, true); - assertEq(eETHInstance.balanceOf(alice), 3 ether); - assertEq(eETHInstance.balanceOf(bob), 3 ether); + assertEq(eETHInstance.balanceOf(alice), 401 ether); + assertEq(eETHInstance.balanceOf(bob), 401 ether); (sent, ) = address(liquidityPoolInstance).call{value: 1 ether}(""); assertEq(sent, false); - assertEq(eETHInstance.balanceOf(alice), 3 ether); - assertEq(eETHInstance.balanceOf(bob), 3 ether); + assertEq(eETHInstance.balanceOf(alice), 401 ether); + assertEq(eETHInstance.balanceOf(bob), 401 ether); } @@ -290,23 +293,26 @@ contract LiquidityPoolTest is TestSetup { function test_fallback() public { assertEq(liquidityPoolInstance.getTotalPooledEther(), 0 ether); - vm.deal(bob, 100 ether); + // Deposit 400 ETH so a 1 ETH rebase is within the 25 bps cap (0.25% of 400 = 1 ETH). + // The raw fallback send below is only accepted up to totalValueOutOfLp (= the + // rebased amount), so out-of-LP must be >= 1 ETH for the 1 ETH send to land. + vm.deal(bob, 400 ether); vm.startPrank(bob); - liquidityPoolInstance.deposit{value: 100 ether}(); + liquidityPoolInstance.deposit{value: 400 ether}(); vm.stopPrank(); - assertEq(liquidityPoolInstance.getTotalPooledEther(), 100 ether); + assertEq(liquidityPoolInstance.getTotalPooledEther(), 400 ether); vm.prank(address(membershipManagerInstance)); - liquidityPoolInstance.rebase(3 ether); - assertEq(liquidityPoolInstance.getTotalPooledEther(), 103 ether); + liquidityPoolInstance.rebase(1 ether); // 0.25% of 400 ETH — exactly at the cap + assertEq(liquidityPoolInstance.getTotalPooledEther(), 401 ether); vm.deal(alice, 3 ether); vm.prank(alice); (bool sent, ) = address(liquidityPoolInstance).call{value: 1 ether}(""); - assertEq(address(liquidityPoolInstance).balance, 100 ether + 1 ether); + assertEq(address(liquidityPoolInstance).balance, 400 ether + 1 ether); assertEq(sent, true); - assertEq(liquidityPoolInstance.getTotalPooledEther(), 103 ether); + assertEq(liquidityPoolInstance.getTotalPooledEther(), 401 ether); } function test_RegisterAsBnftHolder() public { @@ -1137,25 +1143,24 @@ contract LiquidityPoolTest is TestSetup { vm.stopPrank(); uint256 totalPooledBefore = liquidityPoolInstance.getTotalPooledEther(); - + + // rebase within the 25 bps per-report cap (0.25% of 100 ETH TVL = 0.25 ETH) vm.prank(address(membershipManagerInstance)); - liquidityPoolInstance.rebase(5 ether); - + liquidityPoolInstance.rebase(0.25 ether); + uint256 totalPooledAfter = liquidityPoolInstance.getTotalPooledEther(); - assertEq(totalPooledAfter, totalPooledBefore + 5 ether); + assertEq(totalPooledAfter, totalPooledBefore + 0.25 ether); } - // PR #385 (Yash review): the positive (reward) rebase upper bound is enforced inside - // LiquidityPool.rebase, at the share-rate chokepoint, regardless of caller. + // PR #385 (Yash review): the positive (reward) rebase upper bound is a hard 25 bps + // constant enforced inside LiquidityPool.rebase, at the share-rate chokepoint, + // regardless of caller. Not governance-configurable. function test_positiveRebaseCap_enforced() public { vm.deal(alice, 100 ether); vm.prank(alice); liquidityPoolInstance.deposit{value: 100 ether}(); - // reset the override so the default (25 bps) applies (TestSetup set it to 100%) - vm.prank(alice); // alice holds OPERATION_TIMELOCK - liquidityPoolInstance.setMaxPositiveRebaseBps(0); - assertEq(liquidityPoolInstance.effectiveMaxPositiveRebaseBps(), 25); + assertEq(liquidityPoolInstance.MAX_POSITIVE_REBASE_BPS(), 25); uint256 tvl = liquidityPoolInstance.getTotalPooledEther(); uint256 cap = (tvl * 25) / 10_000; @@ -1169,11 +1174,10 @@ contract LiquidityPoolTest is TestSetup { vm.prank(address(membershipManagerInstance)); liquidityPoolInstance.rebase(int128(uint128(cap))); - // a configured higher bound lets a larger rebase through - vm.prank(alice); - liquidityPoolInstance.setMaxPositiveRebaseBps(10_000); + // a negative rebase (slashing) is not bounded by this LP-side cap — the oracle + // path (EtherFiAdmin negative cap) owns that direction vm.prank(address(membershipManagerInstance)); - liquidityPoolInstance.rebase(int128(uint128(tvl / 2))); + liquidityPoolInstance.rebase(-int128(uint128(cap))); } function test_RebaseFailsIfNotMembershipManager() public { @@ -1354,7 +1358,7 @@ contract LiquidityPoolTest is TestSetup { vm.stopPrank(); vm.prank(address(membershipManagerInstance)); - liquidityPoolInstance.rebase(10 ether); + liquidityPoolInstance.rebase(0.25 ether); // within 25 bps cap uint256 claim = liquidityPoolInstance.getTotalEtherClaimOf(alice); assertGt(claim, 100 ether); @@ -1539,9 +1543,9 @@ contract LiquidityPoolTest is TestSetup { vm.stopPrank(); vm.prank(address(membershipManagerInstance)); - liquidityPoolInstance.rebase(10 ether); + liquidityPoolInstance.rebase(0.25 ether); // within 25 bps cap - assertEq(liquidityPoolInstance.getTotalPooledEther(), 110 ether); + assertEq(liquidityPoolInstance.getTotalPooledEther(), 100.25 ether); } //-------------------------------------------------------------------------------------- @@ -2132,22 +2136,23 @@ contract LiquidityPoolTest is TestSetup { } function test_minAmountForShare_subsequent_deposit_succeeds_above_min() public { - // Establish ratio = 1.1 ether per share (positive rebase), then upgrade with MIN below it. + // Establish ratio = 1.0025 ether per share via a within-cap positive rebase + // (0.25% of 100 ETH), then upgrade with MIN below it. vm.deal(alice, 100 ether); vm.prank(alice); liquidityPoolInstance.deposit{value: 100 ether}(); vm.prank(address(membershipManagerInstance)); - liquidityPoolInstance.rebase(10 ether); - assertEq(liquidityPoolInstance.amountForShare(1 ether), 1.1 ether); + liquidityPoolInstance.rebase(0.25 ether); + assertEq(liquidityPoolInstance.amountForShare(1 ether), 1.0025 ether); - _upgradeLpWithMinAmount(1.05 ether); + _upgradeLpWithMinAmount(1.001 ether); vm.deal(bob, 5 ether); vm.prank(bob); uint256 shares = liquidityPoolInstance.deposit{value: 5 ether}(); assertGt(shares, 0); // Proportional minting preserves ratio. - assertEq(liquidityPoolInstance.amountForShare(1 ether), 1.1 ether); + assertEq(liquidityPoolInstance.amountForShare(1 ether), 1.0025 ether); } // --- rebase flow --- @@ -2159,9 +2164,11 @@ contract LiquidityPoolTest is TestSetup { _upgradeLpWithMinAmount(1 ether); + // rebase within the 25 bps cap (0.25% of 100 ETH = 0.25 ETH) → ratio 1.0025, + // still above the 1 ETH min-amount-per-share floor. vm.prank(address(membershipManagerInstance)); - liquidityPoolInstance.rebase(20 ether); - assertEq(liquidityPoolInstance.amountForShare(1 ether), 1.2 ether); + liquidityPoolInstance.rebase(0.25 ether); + assertEq(liquidityPoolInstance.amountForShare(1 ether), 1.0025 ether); } function test_minAmountForShare_negative_rebase_above_min_succeeds() public { @@ -2290,8 +2297,10 @@ contract LiquidityPoolTest is TestSetup { _setLpAccounting(50 ether, 50 ether); // Bound rebase to keep totalValueOutOfLp in [0, type(uint128).max] after the update. - // Pre-rebase totalValueOutOfLp == 50e18. - rebaseAmount = int128(bound(int256(rebaseAmount), -50 ether, 50 ether)); + // Pre-rebase totalValueOutOfLp == 50e18. The positive side is bounded to the 25 bps + // per-report cap (0.25% of the 100 ETH TVL = 0.25 ETH); the negative side (the + // minAmountForShare-underflow boundary this test exercises) is not capped. + rebaseAmount = int128(bound(int256(rebaseAmount), -50 ether, 0.25 ether)); uint256 minAmount = 0.5 ether; _upgradeLpWithMinAmount(minAmount); diff --git a/test/LpRebaseWrnClaimUnderflow.t.sol b/test/LpRebaseWrnClaimUnderflow.t.sol index 6daa39e3..d2f2d214 100644 --- a/test/LpRebaseWrnClaimUnderflow.t.sol +++ b/test/LpRebaseWrnClaimUnderflow.t.sol @@ -147,13 +147,13 @@ contract LpRebaseWrnClaimUnderflowTest is TestSetup { function test_small_rebase_after_finalize_claim_succeeds() public { uint256 requestAmount = 50 ether; - // Seed 10 ether of headroom in outOfLp via a positive rebase. - // This simulates legitimate accrued rewards: outOfLp grows - // without ETH actually moving into the LP. Without this, the - // lock transfer at finalize would leave outOfLp == lock exactly, - // and ANY negative rebase would underflow at claim time. + // Seed 0.25 ether of headroom in outOfLp via a positive rebase (within the 25 bps + // cap: 0.25% of the 100 ETH pool). This simulates legitimate accrued rewards: + // outOfLp grows without ETH actually moving into the LP. Without this, the lock + // transfer at finalize would leave outOfLp == lock exactly, and a negative rebase + // would underflow at claim time. vm.prank(address(membershipManagerInstance)); - liquidityPoolInstance.rebase(int128(int256(uint256(10 ether)))); + liquidityPoolInstance.rebase(int128(int256(uint256(0.25 ether)))); vm.prank(claimant); uint256 tokenId = liquidityPoolInstance.requestWithdraw(claimant, requestAmount); @@ -163,10 +163,10 @@ contract LpRebaseWrnClaimUnderflowTest is TestSetup { withdrawRequestNFTInstance.finalizeRequests(tokenId); vm.stopPrank(); - // outOfLp == 60 ether (10 seeded + 50 from lock), lock == 50. - // A small negative rebase (-1 ether) drops outOfLp to 59 — - // still well above the lock. - int128 rebaseDelta = -int128(int256(uint256(1 ether))); + // outOfLp == 50.25 ether (0.25 seeded + 50 from lock), lock == 50. + // A small negative rebase (-0.1 ether) drops outOfLp to 50.15 — + // still above the lock. + int128 rebaseDelta = -int128(int256(uint256(0.1 ether))); vm.prank(address(membershipManagerInstance)); liquidityPoolInstance.rebase(rebaseDelta); diff --git a/test/MembershipManager.t.sol b/test/MembershipManager.t.sol index a45e980b..0fb05578 100644 --- a/test/MembershipManager.t.sol +++ b/test/MembershipManager.t.sol @@ -1438,11 +1438,12 @@ contract MembershipManagerTest is TestSetup { // rebase must succeed (no division-by-zero) AND the underlying LP rebase must apply. uint256 tvolBefore = liquidityPoolInstance.totalValueOutOfLp(); + // rebase within the 25 bps cap (0.25% of the 5 ETH pool = 0.0125 ETH) vm.prank(address(etherFiAdminInstance)); - membershipManagerInstance.rebase(1 ether); + membershipManagerInstance.rebase(0.01 ether); assertEq( liquidityPoolInstance.totalValueOutOfLp(), - tvolBefore + 1 ether, + tvolBefore + 0.01 ether, "LP rebase still applied even on early-return" ); } diff --git a/test/TestSetup.sol b/test/TestSetup.sol index cdf6740a..42818d6f 100644 --- a/test/TestSetup.sol +++ b/test/TestSetup.sol @@ -758,7 +758,6 @@ contract TestSetup is Test, ContractCodeChecker, DepositDataGeneration { // PR #385: disable the new per-rebase caps for generic fork tests (some rebase a // few % of TVL, above the 25bps default). Dedicated tests set explicit values. vm.startPrank(admin); - liquidityPoolInstance.setMaxPositiveRebaseBps(10_000); etherFiAdminInstance.setMaxNegativeRebaseBps(10_000); vm.stopPrank(); @@ -1450,7 +1449,6 @@ contract TestSetup is Test, ContractCodeChecker, DepositDataGeneration { // would reject the large artificial rebases many generic tests perform on a small // fresh-deploy TVL. Disable them here (100% = effectively no bound); dedicated // tests set explicit values to exercise enforcement. - liquidityPoolInstance.setMaxPositiveRebaseBps(10_000); etherFiAdminInstance.setMaxNegativeRebaseBps(10_000); // Pause WithdrawRequestNFT so existing tests that unPauseContract in their // own setUp continue to find it paused (initializeOnUpgrade used to set this).