From b0732d2ca24c5aa80c4360698a9104f4ad7eb307 Mon Sep 17 00:00:00 2001 From: ReposCollector Date: Wed, 27 May 2026 23:22:42 +0200 Subject: [PATCH] fix(auction,wrnft): forward bid revenue to treasury; guard WRNFT finalize 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) --- src/AuctionManager.sol | 16 +++++++--- src/WithdrawRequestNFT.sol | 7 +++++ test/AddressProvider.t.sol | 4 +-- test/TestSetup.sol | 11 ++++++- test/WithdrawRequestNFT.t.sol | 29 +++++++++++++++++++ .../RoleMigrationStorageIntegrity.t.sol | 2 +- test/fork-tests/validator-key-gen.t.sol | 2 +- 7 files changed, 61 insertions(+), 10 deletions(-) diff --git a/src/AuctionManager.sol b/src/AuctionManager.sol index 4568cd79c..fb5be5479 100644 --- a/src/AuctionManager.sol +++ b/src/AuctionManager.sol @@ -48,7 +48,6 @@ contract AuctionManager is IBlacklister public immutable blacklister; INodeOperatorManager public immutable nodeOperatorManager; address public immutable stakingManagerContractAddress; - address public immutable membershipManagerContractAddress; address public immutable treasury; //-------------------------------------------------------------------------------------- @@ -58,6 +57,7 @@ contract AuctionManager is event BidCreated(address indexed bidder, uint256 amountPerBid, uint256[] bidIdArray, uint64[] ipfsIndexArray); event BidCancelled(uint256 indexed bidId); event BidReEnteredAuction(uint256 indexed bidId); + event BidRevenueForwarded(uint256 indexed bidId, address indexed treasury, uint256 amount); event WhitelistDisabled(bool whitelistStatus); event WhitelistEnabled(bool whitelistStatus); @@ -85,11 +85,10 @@ contract AuctionManager is //-------------------------------------------------------------------------------------- /// @custom:oz-upgrades-unsafe-allow constructor - constructor(address _roleRegistry, address _blacklister, address _nodeOperatorManagerContract, address _stakingManagerContractAddress, address _membershipManagerContractAddress, address _treasury) RolesLibrary(_roleRegistry) { + constructor(address _roleRegistry, address _blacklister, address _nodeOperatorManagerContract, address _stakingManagerContractAddress, address _treasury) RolesLibrary(_roleRegistry) { blacklister = IBlacklister(_blacklister); nodeOperatorManager = INodeOperatorManager(_nodeOperatorManagerContract); stakingManagerContractAddress = _stakingManagerContractAddress; - membershipManagerContractAddress = _membershipManagerContractAddress; treasury = _treasury; _disableInitializers(); } @@ -193,7 +192,9 @@ contract AuctionManager is } /// @notice Updates the details of the bid which has been used in a stake match - /// @dev Called by batchDepositWithBidIds() in StakingManager.sol + /// @dev Called by batchDepositWithBidIds() in StakingManager.sol. Forwards the + /// consumed bid's ETH to `treasury` so protocol-side bid revenue is not + /// stranded in this contract. /// @param _bidId the ID of the bid being removed from the auction (since it has been selected) function updateSelectedBidInformation( uint256 _bidId @@ -203,6 +204,13 @@ contract AuctionManager is bid.isActive = false; numberOfActiveBids--; + + uint256 amount = bid.amount; + if (amount > 0) { + (bool sent, ) = treasury.call{value: amount}(""); + if (!sent) revert EtherTransferFailed(); + emit BidRevenueForwarded(_bidId, treasury, amount); + } } /// @notice Lets a bid that was matched to a cancelled stake re-enter the auction diff --git a/src/WithdrawRequestNFT.sol b/src/WithdrawRequestNFT.sol index be000c91b..063be6a0c 100644 --- a/src/WithdrawRequestNFT.sol +++ b/src/WithdrawRequestNFT.sol @@ -141,6 +141,7 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad error InvalidMinAcceptableShareRate(); error InvalidMinMaxAcceptableShareRate(); error AlreadyInitialized(); + error NotInitialized(); error InvalidLiveRate(); error BurnExceedsShares(); error InvalidEEthShares(); @@ -326,6 +327,12 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad function finalizeRequests(uint256 requestId) external { if (msg.sender != address(etherFiAdmin)) revert IncorrectCaller(); + // Defends against the upgrade-ordering trap: if `initializeShareRateFreezeUpgrade` + // has not yet pushed the sentinel, the first `finalizeRequests` would push a real + // rate as the trace's first entry, causing every legacy tokenId to look up to that + // rate instead of 0 — corrupting the live-rate fallback semantic for pre-upgrade + // requests. Block until the sentinel is in place. + if (_finalizationRates.length() == 0) revert NotInitialized(); if (requestId < lastFinalizedRequestId) revert CannotUndoFinalization(); if (requestId >= nextRequestId) revert CannotFinalizeFutureRequests(); diff --git a/test/AddressProvider.t.sol b/test/AddressProvider.t.sol index 3777cdeb7..c2d0750db 100644 --- a/test/AddressProvider.t.sol +++ b/test/AddressProvider.t.sol @@ -9,9 +9,8 @@ contract AuctionManagerV2Test is AuctionManager { address _blacklister, address _nodeOperatorManagerContract, address _stakingManagerContractAddress, - address _membershipManagerContractAddress, address _treasury - ) AuctionManager(_roleRegistry, _blacklister, _nodeOperatorManagerContract, _stakingManagerContractAddress, _membershipManagerContractAddress, _treasury) {} + ) AuctionManager(_roleRegistry, _blacklister, _nodeOperatorManagerContract, _stakingManagerContractAddress, _treasury) {} function isUpgraded() public pure returns(bool){ return true; } @@ -143,7 +142,6 @@ contract AddressProviderTest is TestSetup { address(blacklisterInstance), address(nodeOperatorManagerInstance), address(stakingManagerInstance), - address(membershipManagerInstance), address(treasuryInstance) ); auctionInstance.upgradeTo(address(auctionManagerV2Implementation)); diff --git a/test/TestSetup.sol b/test/TestSetup.sol index 888445a3c..1633d7cb5 100644 --- a/test/TestSetup.sol +++ b/test/TestSetup.sol @@ -759,6 +759,12 @@ contract TestSetup is Test, ContractCodeChecker, DepositDataGeneration { liquidityPoolInstance.initializeOnUpgradeV2(); } + // Push the share-rate-freeze sentinel checkpoint that the new WRNFT + // `finalizeRequests` guard requires post-upgrade. Idempotent: skip if + // already pushed (length != 0 reverts AlreadyInitialized). + vm.prank(_roleRegOwner); + try withdrawRequestNFTInstance.initializeShareRateFreezeUpgrade() {} catch {} + // The upgraded WeETH/EETH still have global MINT/BURN circuit-breaker // buckets (plus the new opt-in per-address buckets for transfers). // Bootstrap MINT/BURN at unbounded capacity on the fork so generic @@ -1025,7 +1031,6 @@ contract TestSetup is Test, ContractCodeChecker, DepositDataGeneration { address(blacklisterInstance), address(nodeOperatorManagerProxy), address(stakingManagerProxy), - address(membershipManagerProxy), address(treasuryInstance) ); auctionInstance.upgradeTo(address(auctionImplementation)); @@ -1322,6 +1327,10 @@ contract TestSetup is Test, ContractCodeChecker, DepositDataGeneration { membershipNftInstance.initialize("https://etherfi-cdn/{id}.json", address(membershipManagerInstance)); withdrawRequestNFTInstance.initialize(payable(address(liquidityPoolProxy)), payable(address(eETHProxy)), payable(address(membershipManagerProxy))); + // Push the share-rate-freeze sentinel checkpoint that the new finalizeRequests + // guard requires. We're inside an active `vm.startPrank(owner)` block here and + // `owner` was granted UPGRADE_TIMELOCK_ROLE in Phase 1, so no extra prank needed. + withdrawRequestNFTInstance.initializeShareRateFreezeUpgrade(); // initializeOnUpgrade now reverts (checks immutable roleRegistry == 0). Set the // share-remainder split + paused state it used to bootstrap directly. withdrawRequestNFTInstance.updateShareRemainderSplitToTreasuryInBps(1000); diff --git a/test/WithdrawRequestNFT.t.sol b/test/WithdrawRequestNFT.t.sol index 821b7a854..5b321b7fc 100644 --- a/test/WithdrawRequestNFT.t.sol +++ b/test/WithdrawRequestNFT.t.sol @@ -20,6 +20,14 @@ contract WithdrawRequestNFTIntrusive is WithdrawRequestNFT { function setLastFinalizedRequestIdForTest(uint32 _id) external { lastFinalizedRequestId = _id; } + + /// @dev Test-only: zero out the `_finalizationRates` length so tests can re-exercise + /// the `initializeShareRateFreezeUpgrade` flow from a "pre-init" state. Slot 311 + /// matches `forge inspect WithdrawRequestNFT storageLayout`; keep in sync if the + /// storage layout shifts. + function clearFinalizationRatesForTest() external { + assembly { sstore(311, 0) } + } } contract WithdrawRequestNFTTest is TestSetup { @@ -2019,9 +2027,25 @@ contract WithdrawRequestNFTTest is TestSetup { withdrawRequestNFTInstance.upgradeTo(cur_impl); } + /// @dev Test-only helper to revert the WRNFT to the "pre-init" state for `_finalizationRates`, + /// so tests can exercise the `initializeShareRateFreezeUpgrade` flow directly even + /// though `setUp` pushes the sentinel by default. + function _clearFinalizationRatesForTest() internal { + address cur_impl = withdrawRequestNFTInstance.getImplementation(); + address new_impl = address(new WithdrawRequestNFTIntrusive(address(roleRegistryInstance))); + vm.prank(withdrawRequestNFTInstance.owner()); + withdrawRequestNFTInstance.upgradeTo(new_impl); + WithdrawRequestNFTIntrusive(payable(address(withdrawRequestNFTInstance))).clearFinalizationRatesForTest(); + vm.prank(withdrawRequestNFTInstance.owner()); + withdrawRequestNFTInstance.upgradeTo(cur_impl); + } + /// @dev Requests that pre-date `initializeShareRateFreezeUpgrade()` see value 0 from /// `lowerLookup` and fall back to the live-rate path. function test_shareRateFreeze_legacySentinel_fallsBackToLiveRate() public { + // setUp pushes the sentinel by default; this test exercises the pre-init flow. + _clearFinalizationRatesForTest(); + startHoax(bob); liquidityPoolInstance.deposit{value: 10 ether}(); vm.stopPrank(); @@ -2074,6 +2098,7 @@ contract WithdrawRequestNFTTest is TestSetup { /// @dev After the upgrade init, the next finalize pushes a real snapshot and all /// requestIds strictly above the sentinel use the frozen rate. function test_shareRateFreeze_postUpgradeRequests_useFrozenRate() public { + _clearFinalizationRatesForTest(); vm.prank(withdrawRequestNFTInstance.owner()); withdrawRequestNFTInstance.initializeShareRateFreezeUpgrade(); @@ -2092,6 +2117,7 @@ contract WithdrawRequestNFTTest is TestSetup { /// @dev `initializeShareRateFreezeUpgrade` is a one-shot. function test_shareRateFreeze_initializeUpgrade_revertsIfAlreadyInitialized() public { + _clearFinalizationRatesForTest(); vm.startPrank(withdrawRequestNFTInstance.owner()); withdrawRequestNFTInstance.initializeShareRateFreezeUpgrade(); vm.expectRevert(WithdrawRequestNFT.AlreadyInitialized.selector); @@ -2228,6 +2254,9 @@ contract WithdrawRequestNFTTest is TestSetup { /// local live-rate fallback path. The claim payout must match what the pre-upgrade /// code (live `amountForShare`-based) would have computed at that moment. function test_legacyFallback_matchesPreUpgradeLiveRate() public { + // setUp pushes the sentinel by default; this test exercises the pre-init flow. + _clearFinalizationRatesForTest(); + // 1. set up a "pre-upgrade finalized request" by stepping lastFinalizedRequestId without // pushing a real snapshot — mirroring on-chain state at the moment of the upgrade. startHoax(bob); diff --git a/test/fork-tests/RoleMigrationStorageIntegrity.t.sol b/test/fork-tests/RoleMigrationStorageIntegrity.t.sol index 37f27de83..e0ee1ce49 100644 --- a/test/fork-tests/RoleMigrationStorageIntegrity.t.sol +++ b/test/fork-tests/RoleMigrationStorageIntegrity.t.sol @@ -99,7 +99,7 @@ contract RoleMigrationStorageIntegrityTest is Test, Deployed { bytes32[] memory preAM = _snapshot(AUCTION_MANAGER); bytes32[] memory preER = _snapshot(ETHERFI_RESTAKER); - address newAM = address(new AuctionManager(ROLE_REGISTRY, address(0), NODE_OPERATOR_MANAGER, STAKING_MANAGER, MEMBERSHIP_MANAGER, TREASURY)); + address newAM = address(new AuctionManager(ROLE_REGISTRY, address(0), NODE_OPERATOR_MANAGER, STAKING_MANAGER, TREASURY)); address newER = address(new EtherFiRestaker( LIQUIDITY_POOL, LIQUIFIER, diff --git a/test/fork-tests/validator-key-gen.t.sol b/test/fork-tests/validator-key-gen.t.sol index 109a609ad..34634f1ad 100644 --- a/test/fork-tests/validator-key-gen.t.sol +++ b/test/fork-tests/validator-key-gen.t.sol @@ -86,7 +86,7 @@ contract ValidatorKeyGenTest is Test, ArrayTestHelper { vm.prank(liquidityPool.owner()); liquidityPool.upgradeTo(address(liquidityPoolImpl)); - AuctionManager auctionManagerImpl = new AuctionManager(address(roleRegistry), address(blacklister), address(nodeOperatorManager), address(stakingManager), 0x3d320286E014C3e1ce99Af6d6B00f0C1D63E3000, 0x0c83EAe1FE72c390A02E426572854931EefF93BA); + AuctionManager auctionManagerImpl = new AuctionManager(address(roleRegistry), address(blacklister), address(nodeOperatorManager), address(stakingManager), 0x0c83EAe1FE72c390A02E426572854931EefF93BA); vm.prank(auctionManager.owner()); auctionManager.upgradeTo(address(auctionManagerImpl));