From 1e4ba9eb39e477570708d846eb57e84a05deb5a6 Mon Sep 17 00:00:00 2001 From: seongyun-ko Date: Sat, 20 Jun 2026 08:53:05 -0400 Subject: [PATCH] test: address Bugbot review on #470 (rebase caller, PWQ ctor args, dead vars) Fixes the open Cursor Bugbot findings against the I3/I10 fork invariant suites (all confirmed against source, not the bot summary): - WithdrawalSolvency: rebasePositive/rebaseNegative pranked membershipManager, but LiquidityPool.rebase requires msg.sender == etherFiAdminContract (LiquidityPool.sol:456). On a realistic fork membershipManagerInstance is address(0) (only set in initializeTestingFork), so both helpers always reverted IncorrectCaller AND were never even in the fuzz selector set (dead code). Now prank the real etherFiAdminContract, drop the unused membershipManager immutable/ctor arg, add both ops to targetSelector, and bound the positive rebase to <=0.2% so it stays under MAX_POSITIVE_REBASE_BPS (0.25%) and actually applies. - WithdrawalSolvency: removed unused baseLocked/baseOutOfLp snapshots; the invariants assert the stronger construction-true absolute bound, so the baseline vars were dead. - ValidatorStateMachine: PriorityWithdrawalQueue ctor is (lp, eETH, weETH, blacklister, roleRegistry, minDelay); the fork upgrade passed roleRegistry as blacklister and treasury as roleRegistry. Now wired with blacklisterInstance + roleRegistryInstance in their correct slots. Verified on mainnet fork: WithdrawalSolvency 7/7 pass (requestsClaimed=14, finalizeBoundChecks=7, lockBoundEnforced=10), ValidatorStateMachine 2/2, RewardsDistributor 3/3 (its non-vacuity gates were already present). --- .../ValidatorStateMachine.invariant.t.sol | 5 ++++- .../WithdrawalSolvency.invariant.t.sol | 17 +++++++---------- .../handlers/WithdrawalSolvencyHandler.sol | 13 +++++++------ 3 files changed, 18 insertions(+), 17 deletions(-) diff --git a/test/invariant/ValidatorStateMachine.invariant.t.sol b/test/invariant/ValidatorStateMachine.invariant.t.sol index ac9fbe36..c4b9e465 100644 --- a/test/invariant/ValidatorStateMachine.invariant.t.sol +++ b/test/invariant/ValidatorStateMachine.invariant.t.sol @@ -74,9 +74,12 @@ contract ValidatorStateMachineInvariantTest is TestSetup, Deployed { // The production queue proxy on mainnet still runs the master impl which // has no receive(); initializeOnUpgradeV2 below sweeps queue-locked ETH // into the queue and would revert with SendFail. Upgrade the queue first. + // Constructor order is (liquidityPool, eETH, weETH, blacklister, roleRegistry, minDelay). + // Pass blacklisterInstance + roleRegistryInstance in their correct slots so the + // upgraded mainnet queue proxy's role checks match production wiring. address newPQ = address(new PriorityWithdrawalQueue( address(liquidityPoolInstance), address(eETHInstance), address(weEthInstance), - address(roleRegistryInstance), treasuryInstance, 1 hours + address(blacklisterInstance), address(roleRegistryInstance), 1 hours )); vm.prank(UPGRADE_TIMELOCK); PriorityWithdrawalQueue(payable(PRIORITY_WITHDRAWAL_QUEUE)).upgradeTo(newPQ); diff --git a/test/invariant/WithdrawalSolvency.invariant.t.sol b/test/invariant/WithdrawalSolvency.invariant.t.sol index 3001ca5b..7d0e4556 100644 --- a/test/invariant/WithdrawalSolvency.invariant.t.sol +++ b/test/invariant/WithdrawalSolvency.invariant.t.sol @@ -84,10 +84,6 @@ contract WithdrawalSolvencyInvariantTest is TestSetup { WithdrawalSolvencyHandler internal handler; address[5] internal handlerActors; - // Captured initial mainnet baselines (delta-aware assertions). - uint256 internal baseLocked; - uint256 internal baseOutOfLp; - function setUp() public { initializeRealisticFork(MAINNET_FORK); @@ -113,9 +109,6 @@ contract WithdrawalSolvencyInvariantTest is TestSetup { eETHInstance.approve(address(liquidityPoolInstance), type(uint256).max); } - baseLocked = uint256(withdrawRequestNFTInstance.ethAmountLockedForWithdrawal()); - baseOutOfLp = uint256(liquidityPoolInstance.totalValueOutOfLp()); - // Finalize the PRE-EXISTING mainnet pending range once, mirroring what // EtherFiAdmin does in production (lock the summed eETH of the range via // addEthAmountLockedForWithdrawal, then finalizeRequests). This clears @@ -133,7 +126,6 @@ contract WithdrawalSolvencyInvariantTest is TestSetup { eETHInstance, withdrawRequestNFTInstance, address(etherFiAdminInstance), - address(membershipManagerInstance), handlerActors ); @@ -142,12 +134,17 @@ contract WithdrawalSolvencyInvariantTest is TestSetup { // Restrict fuzzing to the action functions. Without this, the engine // also targets the handler's public getters/counters, burning call // budget on no-ops. probeOverLiquidityLock is included so the I3/P1 - // liquidity guard is positively driven on every run. - bytes4[] memory sel = new bytes4[](4); + // liquidity guard is positively driven on every run. rebasePositive/ + // rebaseNegative add rate churn (bounded so the share rate stays well + // above minAmountForShare) and are driven through the real + // etherFiAdminContract caller that LiquidityPool.rebase requires. + bytes4[] memory sel = new bytes4[](6); sel[0] = handler.requestWithdraw.selector; sel[1] = handler.finalizeRequests.selector; sel[2] = handler.claimWithdraw.selector; sel[3] = handler.probeOverLiquidityLock.selector; + sel[4] = handler.rebasePositive.selector; + sel[5] = handler.rebaseNegative.selector; targetSelector(FuzzSelector({addr: address(handler), selectors: sel})); // Pre-seed a batch of OUR OWN requests and finalize them, so every fuzz diff --git a/test/invariant/handlers/WithdrawalSolvencyHandler.sol b/test/invariant/handlers/WithdrawalSolvencyHandler.sol index 7b1e5cfb..4e4efe4f 100644 --- a/test/invariant/handlers/WithdrawalSolvencyHandler.sol +++ b/test/invariant/handlers/WithdrawalSolvencyHandler.sol @@ -65,7 +65,6 @@ contract WithdrawalSolvencyHandler is StdUtils { EETH public immutable eETH; WithdrawRequestNFT public immutable wrn; address public immutable etherFiAdminAddr; - address public immutable membershipManager; address[N_EOAS] public actors; @@ -130,14 +129,12 @@ contract WithdrawalSolvencyHandler is StdUtils { EETH _eETH, WithdrawRequestNFT _wrn, address _etherFiAdmin, - address _membershipManager, address[N_EOAS] memory _actors ) { lp = _lp; eETH = _eETH; wrn = _wrn; etherFiAdminAddr = _etherFiAdmin; - membershipManager = _membershipManager; for (uint256 i = 0; i < N_EOAS; i++) { actors[i] = _actors[i]; } @@ -369,13 +366,17 @@ contract WithdrawalSolvencyHandler is StdUtils { /// @notice Positive rebase stress — keeps the rate climbing (frozen-rate /// shield is irrelevant to solvency here, just adds state churn). + /// Bounded to <= 0.2% of TVL so it stays under the contract's + /// MAX_POSITIVE_REBASE_BPS (0.25%) cap and actually applies. + /// Caller is the etherFiAdminContract — the only address + /// `LiquidityPool.rebase` accepts (LiquidityPool.sol:456). function rebasePositive(uint128 deltaSeed) external { uint256 outOfLp = uint256(lp.totalValueOutOfLp()); - uint256 cap = outOfLp == 0 ? 1 ether : (outOfLp * 50) / 1e4; // <=0.5% + uint256 cap = outOfLp == 0 ? 1 ether : (outOfLp * 20) / 1e4; // <=0.2% if (cap == 0) cap = 1; if (cap > uint256(uint128(type(int128).max))) cap = uint256(uint128(type(int128).max)); int128 delta = int128(int256(bound(uint256(deltaSeed), 0, cap))); - vm.prank(membershipManager); + vm.prank(etherFiAdminAddr); try lp.rebase(delta, 0) { callCounts["rebase_pos"]++; } catch { callCounts["rebase_pos_revert"]++; } } @@ -398,7 +399,7 @@ contract WithdrawalSolvencyHandler is StdUtils { if (cap == 0) { callCounts["rebase_neg_skipped"]++; return; } if (cap > uint256(uint128(type(int128).max))) cap = uint256(uint128(type(int128).max)); int128 delta = -int128(int256(bound(uint256(deltaSeed), 1, cap))); - vm.prank(membershipManager); + vm.prank(etherFiAdminAddr); try lp.rebase(delta, 0) { callCounts["rebase_neg"]++; } catch { callCounts["rebase_neg_revert"]++; } }