From 6008dd040dd339e14bfb5d441c60aadf7735bc16 Mon Sep 17 00:00:00 2001 From: jtfirek Date: Sat, 31 Aug 2024 15:45:00 -0500 Subject: [PATCH 1/7] new withdraw flow + fixing test compilation errors --- .../WithdrawRequestNFTUpgradeScript.s.sol | 4 +- src/WithdrawRequestNFT.sol | 160 +++++++++++++----- src/interfaces/IWithdrawRequestNFT.sol | 7 +- test/LiquidityPool.t.sol | 6 +- test/MembershipManager.t.sol | 17 +- test/MembershipManagerV0.t.sol | 12 +- test/TestSetup.sol | 2 +- test/WithdrawRequestNFT.t.sol | 18 +- 8 files changed, 152 insertions(+), 74 deletions(-) diff --git a/script/upgrades/WithdrawRequestNFTUpgradeScript.s.sol b/script/upgrades/WithdrawRequestNFTUpgradeScript.s.sol index 823cf90a..83c45294 100644 --- a/script/upgrades/WithdrawRequestNFTUpgradeScript.s.sol +++ b/script/upgrades/WithdrawRequestNFTUpgradeScript.s.sol @@ -15,7 +15,7 @@ contract WithdrawRequestNFTUpgrade is Script { address addressProviderAddress = vm.envAddress("CONTRACT_REGISTRY"); addressProvider = AddressProvider(addressProviderAddress); - address proxyAddress = addressProvider.getContractAddress("WithdrawRequestNFT"); + address payable proxyAddress = payable(addressProvider.getContractAddress("WithdrawRequestNFT")); vm.startBroadcast(deployerPrivateKey); @@ -26,4 +26,4 @@ contract WithdrawRequestNFTUpgrade is Script { vm.stopBroadcast(); } -} \ No newline at end of file +} diff --git a/src/WithdrawRequestNFT.sol b/src/WithdrawRequestNFT.sol index d8976936..2374bafe 100644 --- a/src/WithdrawRequestNFT.sol +++ b/src/WithdrawRequestNFT.sol @@ -25,12 +25,14 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad uint32 public lastFinalizedRequestId; uint96 public accumulatedDustEEthShares; // to be burned or used to cover the validator churn cost + FinalizationCheckpoint[] public finalizationCheckpoints; + RoleRegistry public roleRegistry; bytes32 public constant WITHDRAW_NFT_ADMIN_ROLE = keccak256("WITHDRAW_NFT_ADMIN_ROLE"); event WithdrawRequestCreated(uint32 indexed requestId, uint256 amountOfEEth, uint256 shareOfEEth, address owner, uint256 fee); - event WithdrawRequestClaimed(uint32 indexed requestId, uint256 amountOfEEth, uint256 burntShareOfEEth, address owner, uint256 fee); + event WithdrawRequestClaimed(uint32 indexed requestId, uint256 amountOfEEth, uint256 DEPRECATED_burntShareOfEEth, address owner, uint256 fee); event WithdrawRequestInvalidated(uint32 indexed requestId); event WithdrawRequestValidated(uint32 indexed requestId); event WithdrawRequestSeized(uint32 indexed requestId); @@ -61,6 +63,9 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad // TODO: compile list of values in DEPRECATED_admins to clear out roleRegistry = RoleRegistry(_roleRegistry); + + // to avoid having to deal with the finalizationCheckpoints[index - 1] edgecase where the index is 0, we add a dummy checkpoint + finalizationCheckpoints.push(FinalizationCheckpoint(0, 0)); } /// @notice creates a withdraw request and issues an associated NFT to the recipient @@ -71,7 +76,7 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad /// @param fee fee to be subtracted from amount when recipient calls claimWithdraw /// @return uint256 id of the withdraw request function requestWithdraw(uint96 amountOfEEth, uint96 shareOfEEth, address recipient, uint256 fee) external payable onlyLiquidtyPool returns (uint256) { - uint256 requestId = nextRequestId++; + uint256 requestId = nextRequestId++; uint32 feeGwei = uint32(fee / 1 gwei); _requests[requestId] = IWithdrawRequestNFT.WithdrawRequest(amountOfEEth, shareOfEEth, true, feeGwei); @@ -81,16 +86,30 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad return requestId; } - function getClaimableAmount(uint256 tokenId) public view returns (uint256) { - require(tokenId < nextRequestId, "Request does not exist"); + function getClaimableAmount(uint256 tokenId, uint256 checkpointIndex) public view returns (uint256) { require(tokenId <= lastFinalizedRequestId, "Request is not finalized"); + require(tokenId < nextRequestId, "Request does not exist"); require(ownerOf(tokenId) != address(0), "Already Claimed"); + require( + checkpointIndex != 0 && + checkpointIndex < finalizationCheckpoints.length, + "Invalid checkpoint index" + ); + FinalizationCheckpoint memory lowerBoundCheckpoint = finalizationCheckpoints[checkpointIndex - 1]; + FinalizationCheckpoint memory checkpoint = finalizationCheckpoints[checkpointIndex]; + require( + lowerBoundCheckpoint.lastFinalizedRequestId < tokenId && + tokenId <= checkpoint.lastFinalizedRequestId, + "Invalid checkpoint" + ); IWithdrawRequestNFT.WithdrawRequest memory request = _requests[tokenId]; - // send the lesser value of the originally requested amount of eEth or the current eEth value of the shares - uint256 amountForShares = liquidityPool.amountForShare(request.shareOfEEth); - uint256 amountToTransfer = (request.amountOfEEth < amountForShares) ? request.amountOfEEth : amountForShares; + // send the lesser ether ETH value of the originally requested amount of eEth or the eEth value at the last `cachedSharePrice` update + uint256 cachedSharePrice = checkpoint.cachedShareValue; + uint256 amountForSharesCached = request.shareOfEEth * cachedSharePrice / 1 ether; + uint256 amountToTransfer = _min(request.amountOfEEth, amountForSharesCached); + uint256 fee = uint256(request.feeGwei) * 1 gwei; return amountToTransfer - fee; @@ -99,40 +118,35 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad /// @notice called by the NFT owner to claim their ETH /// @dev burns the NFT and transfers ETH from the liquidity pool to the owner minus any fee, withdraw request must be valid and finalized /// @param tokenId the id of the withdraw request and associated NFT - function claimWithdraw(uint256 tokenId) external { - return _claimWithdraw(tokenId, ownerOf(tokenId)); + function claimWithdraw(uint256 tokenId, uint256 checkpointIndex) external { + return _claimWithdraw(tokenId, ownerOf(tokenId), checkpointIndex); } - function _claimWithdraw(uint256 tokenId, address recipient) internal { + function _claimWithdraw(uint256 tokenId, address recipient, uint256 checkpointIndex) internal { + require(ownerOf(tokenId) == msg.sender, "Not the owner of the NFT"); IWithdrawRequestNFT.WithdrawRequest memory request = _requests[tokenId]; require(request.isValid, "Request is not valid"); uint256 fee = uint256(request.feeGwei) * 1 gwei; - uint256 amountToWithdraw = getClaimableAmount(tokenId); + uint256 amountToWithdraw = getClaimableAmount(tokenId, checkpointIndex); // transfer eth to recipient _burn(tokenId); delete _requests[tokenId]; - uint256 amountBurnedShare = 0; if (fee > 0) { // send fee to membership manager - amountBurnedShare += liquidityPool.withdraw(address(membershipManager), fee); + _sendFund(address(membershipManager), fee); } - amountBurnedShare += liquidityPool.withdraw(recipient, amountToWithdraw); + _sendFund(recipient, amountToWithdraw); - uint256 amountUnBurnedShare = request.shareOfEEth - amountBurnedShare; - if (amountUnBurnedShare > 0) { - accumulatedDustEEthShares += uint96(amountUnBurnedShare); - } - - emit WithdrawRequestClaimed(uint32(tokenId), amountToWithdraw + fee, amountBurnedShare, recipient, fee); + emit WithdrawRequestClaimed(uint32(tokenId), amountToWithdraw + fee, 0, recipient, fee); } - function batchClaimWithdraw(uint256[] calldata tokenIds) external { + function batchClaimWithdraw(uint256[] calldata tokenIds, uint256[] calldata checkpointIndices) external { for (uint256 i = 0; i < tokenIds.length; i++) { - _claimWithdraw(tokenIds[i], ownerOf(tokenIds[i])); + _claimWithdraw(tokenIds[i], ownerOf(tokenIds[i]), checkpointIndices[i]); } } @@ -155,7 +169,7 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad // Given an invalidated withdrawal request NFT of ID `requestId`:, // - burn the NFT // - withdraw its ETH to the `recipient` - function seizeInvalidRequest(uint256 requestId, address recipient) external onlyOwner { + function seizeInvalidRequest(uint256 requestId, address recipient, uint256 checkpointIndex) external onlyOwner { require(!_requests[requestId].isValid, "Request is valid"); require(ownerOf(requestId) != address(0), "Already Claimed"); @@ -165,14 +179,9 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad // Undo its invalidation to claim _requests[requestId].isValid = true; - // its ETH amount is not locked - // - if it was finalized when being invalidated, we revoked it via `reduceEthAmountLockedForWithdrawal` - // - if it was not finalized when being invalidated, it was not locked - uint256 ethAmount = getClaimableAmount(requestId); - liquidityPool.addEthAmountLockedForWithdrawal(uint128(ethAmount)); - // withdraw the ETH to the recipient - _claimWithdraw(requestId, recipient); + // TODO: fix this + _claimWithdraw(requestId, recipient, checkpointIndex); emit WithdrawRequestSeized(uint32(requestId)); } @@ -193,11 +202,11 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad function calculateTotalPendingAmount(uint256 lastRequestId) public view returns (uint256) { uint256 totalAmount = 0; for (uint256 i = lastFinalizedRequestId + 1; i <= lastRequestId; i++) { - if (!isValid(i)) continue; IWithdrawRequestNFT.WithdrawRequest memory request = _requests[i]; uint256 amountForShares = liquidityPool.amountForShare(request.shareOfEEth); - uint256 amount = (request.amountOfEEth < amountForShares) ? request.amountOfEEth : amountForShares; + + uint256 amount = _min(request.amountOfEEth, amountForShares); totalAmount += amount; } @@ -208,14 +217,23 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad if (!roleRegistry.hasRole(WITHDRAW_NFT_ADMIN_ROLE, msg.sender)) revert IncorrectRole(); uint128 totalAmount = uint128(calculateTotalPendingAmount(lastRequestId)); - _finalizeRequests(lastRequestId, totalAmount); + uint256 cachedSharePrice = liquidityPool.amountForShare(1 ether); + + finalizationCheckpoints.push(FinalizationCheckpoint(uint32(lastRequestId), cachedSharePrice)); + + // TODO: We can probably deprecate this variable and use the last element of the finalizationCheckpoints array + lastFinalizedRequestId = uint32(lastRequestId); + + liquidityPool.withdraw(address(this), totalAmount); + emit UpdateFinalizedRequestId(uint32(lastRequestId), totalAmount); } + // Maybe deprecated ? I should try to fix any accounting errors and delete this function // It can be used to correct the total amount of pending withdrawals. There are some accounting erros as of now function finalizeRequests(uint256 lastRequestId, uint128 totalAmount) external { if (!roleRegistry.hasRole(WITHDRAW_NFT_ADMIN_ROLE, msg.sender)) revert IncorrectRole(); - _finalizeRequests(lastRequestId, totalAmount); + return; } function invalidateRequest(uint256 requestId) external { @@ -223,11 +241,6 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad require(isValid(requestId), "Request is not valid"); - if (isFinalized(requestId)) { - uint256 ethAmount = getClaimableAmount(requestId); - liquidityPool.reduceEthAmountLockedForWithdrawal(uint128(ethAmount)); - } - _requests[requestId].isValid = false; emit WithdrawRequestInvalidated(uint32(requestId)); @@ -252,18 +265,75 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad function _authorizeUpgrade(address newImplementation) internal override onlyOwner {} - function _finalizeRequests(uint256 lastRequestId, uint128 totalAmount) internal { - emit UpdateFinalizedRequestId(uint32(lastRequestId), totalAmount); - lastFinalizedRequestId = uint32(lastRequestId); - liquidityPool.addEthAmountLockedForWithdrawal(totalAmount); - } - function getImplementation() external view returns (address) { return _getImplementation(); } + function _sendFund(address _recipient, uint256 _amount) internal { + uint256 balanace = address(this).balance; + (bool sent, ) = _recipient.call{value: _amount}(""); + require(sent && address(this).balance == balanace - _amount, "SendFail"); + } + + function _min(uint256 a, uint256 b) internal pure returns (uint256) { + return a < b ? a : b; + } + + // This contract only accepts ETH sent from the liquidity pool + receive() external payable onlyLiquidtyPool() { } + modifier onlyLiquidtyPool() { require(msg.sender == address(liquidityPool), "Caller is not the liquidity pool"); _; } + + + function getFinalizationCheckpoint(uint256 checkpointId) external view returns (FinalizationCheckpoint memory) { + return finalizationCheckpoints[checkpointId]; + } + + /// @dev View function to find a finalization checkpoint to use in `claimWithdraw()` + /// Search will be performed in the range of `[_start, _end]` over the `finalizationCheckpoints` array + /// + /// @param _requestId request id to search the checkpoint for + /// @param _start index of the left boundary of the search range + /// @param _end index of the right boundary of the search range, should be less than or equal to `finalizationCheckpoints.length` + /// + /// @return index into `finalizationCheckpoints` that the `_requestId` belongs to. + function findCheckpointIndex(uint256 _requestId, uint256 _start, uint256 _end) public view returns (uint256) { + require(_requestId <= lastFinalizedRequestId, "Request is not finalized"); + require(_start <= _end, "Invalid range"); + require(_end < finalizationCheckpoints.length, "End index out of bounds of finalizationCheckpoints"); + + // Binary search + uint256 min = _start; + uint256 max = _end; + + while (max > min) { + uint256 mid = (max + min + 1) / 2; + if (finalizationCheckpoints[mid].lastFinalizedRequestId < _requestId) { + + // if mid index in the array is less than the request id, we need to move up the array, as the index we are looking for has + // a `finalizationCheckpoints[mid].lastFinalizedRequestId` greater than the request id + + + min = mid; + } else { + // by getting here, we have a `finalizationCheckpoints[mid].lastFinalizedRequestId` greater than the request id + // to know we have found the right index, finalizationCheckpoints[mid - 1].lastFinalizedRequestId` must be less than the request id + + if (finalizationCheckpoints[mid - 1].lastFinalizedRequestId < _requestId) { + return mid; + } + + max = mid - 1; // there are values to the left of mid that are greater than the request id + } + } + } + + // Add a getter function for the length of the array + function getFinalizationCheckpointsLength() public view returns (uint256) { + return finalizationCheckpoints.length; + } + } diff --git a/src/interfaces/IWithdrawRequestNFT.sol b/src/interfaces/IWithdrawRequestNFT.sol index 99c8f195..3a211fbf 100644 --- a/src/interfaces/IWithdrawRequestNFT.sol +++ b/src/interfaces/IWithdrawRequestNFT.sol @@ -9,9 +9,14 @@ interface IWithdrawRequestNFT { uint32 feeGwei; } + struct FinalizationCheckpoint { + uint256 lastFinalizedRequestId; + uint256 cachedShareValue; + } + function initialize(address _liquidityPoolAddress, address _eEthAddress, address _membershipManager) external; function requestWithdraw(uint96 amountOfEEth, uint96 shareOfEEth, address requester, uint256 fee) external payable returns (uint256); - function claimWithdraw(uint256 requestId) external; + function claimWithdraw(uint256 requestId, uint256 checkpointIndex) external; function getRequest(uint256 requestId) external view returns (WithdrawRequest memory); function isFinalized(uint256 requestId) external view returns (bool); diff --git a/test/LiquidityPool.t.sol b/test/LiquidityPool.t.sol index 1df244ef..127e6d48 100644 --- a/test/LiquidityPool.t.sol +++ b/test/LiquidityPool.t.sol @@ -144,7 +144,7 @@ contract LiquidityPoolTest is TestSetup { _finalizeWithdrawalRequest(aliceReqId); vm.startPrank(alice); - withdrawRequestNFTInstance.claimWithdraw(aliceReqId); + withdrawRequestNFTInstance.claimWithdraw(aliceReqId, 1); assertEq(eETHInstance.balanceOf(alice), 1 ether); assertEq(alice.balance, 2 ether); vm.stopPrank(); @@ -159,7 +159,7 @@ contract LiquidityPoolTest is TestSetup { _finalizeWithdrawalRequest(bobReqId); vm.startPrank(bob); - withdrawRequestNFTInstance.claimWithdraw(bobReqId); + withdrawRequestNFTInstance.claimWithdraw(bobReqId, 1); assertEq(eETHInstance.balanceOf(bob), 0); assertEq(bob.balance, 3 ether); vm.stopPrank(); @@ -757,7 +757,7 @@ contract LiquidityPoolTest is TestSetup { _finalizeWithdrawalRequest(bobRequestId); vm.prank(bob); - withdrawRequestNFTInstance.claimWithdraw(bobRequestId); + withdrawRequestNFTInstance.claimWithdraw(bobRequestId, 1); assertEq(address(liquidityPoolInstance).balance, 0); assertEq(eETHInstance.totalSupply(), 0); diff --git a/test/MembershipManager.t.sol b/test/MembershipManager.t.sol index 40ec318a..899978bd 100644 --- a/test/MembershipManager.t.sol +++ b/test/MembershipManager.t.sol @@ -71,7 +71,7 @@ contract MembershipManagerTest is TestSetup { _finalizeWithdrawalRequest(bobTokenId); vm.prank(bob); - withdrawRequestNFTInstance.claimWithdraw(bobTokenId); + withdrawRequestNFTInstance.claimWithdraw(bobTokenId, 1); assertEq(bob.balance, 100 ether, "Bob should have 100 ether"); assertEq(membershipNftInstance.balanceOf(bob, bobToken), 0); @@ -101,7 +101,10 @@ contract MembershipManagerTest is TestSetup { uint256[] memory requestIds = new uint256[](2); requestIds[0] = requestId1; requestIds[1] = requestId2; - withdrawRequestNFTInstance.batchClaimWithdraw(requestIds); + uint256[] memory requestIdCheckpoints = new uint256[](2); + requestIdCheckpoints[0] = 1; + requestIdCheckpoints[1] = 2; + withdrawRequestNFTInstance.batchClaimWithdraw(requestIds, requestIdCheckpoints); vm.stopPrank(); assertEq(address(membershipManagerV1Instance).balance, 2 * 0.5 ether); @@ -140,7 +143,7 @@ contract MembershipManagerTest is TestSetup { _finalizeWithdrawalRequest(aliceRequestId1); vm.startPrank(alice); - withdrawRequestNFTInstance.claimWithdraw(aliceRequestId1); + withdrawRequestNFTInstance.claimWithdraw(aliceRequestId1, 1); assertEq(membershipNftInstance.loyaltyPointsOf(tokenId), 2 * kwei); assertEq(membershipNftInstance.tierPointsOf(tokenId), 0); assertEq(membershipNftInstance.valueOf(tokenId), 1 ether); @@ -162,7 +165,7 @@ contract MembershipManagerTest is TestSetup { _finalizeWithdrawalRequest(aliceRequestId2); vm.startPrank(alice); - withdrawRequestNFTInstance.claimWithdraw(aliceRequestId2); + withdrawRequestNFTInstance.claimWithdraw(aliceRequestId2, 1); assertEq(membershipNftInstance.balanceOf(alice, tokenId), 0); assertEq(alice.balance, 2 ether); vm.stopPrank(); @@ -533,7 +536,7 @@ contract MembershipManagerTest is TestSetup { _finalizeWithdrawalRequest(requestId); vm.startPrank(alice); - withdrawRequestNFTInstance.claimWithdraw(requestId); + withdrawRequestNFTInstance.claimWithdraw(requestId, 1); assertEq(eETHInstance.balanceOf(alice), 0 ether); assertEq(membershipNftInstance.valueOf(aliceToken), 1 ether); assertEq(alice.balance, 1 ether); @@ -1019,7 +1022,7 @@ contract MembershipManagerTest is TestSetup { _finalizeWithdrawalRequest(requestId); vm.startPrank(actor); - withdrawRequestNFTInstance.claimWithdraw(requestId); + withdrawRequestNFTInstance.claimWithdraw(requestId, 1); counts[3]++; } @@ -1041,7 +1044,7 @@ contract MembershipManagerTest is TestSetup { _finalizeWithdrawalRequest(requestId); vm.prank(actor); - withdrawRequestNFTInstance.claimWithdraw(requestId); + withdrawRequestNFTInstance.claimWithdraw(requestId, 1); assertLe(address(actor).balance, expectedBalanceAfterWithdrawal); assertGe(address(actor).balance, expectedBalanceAfterWithdrawal - 3); // rounding errors diff --git a/test/MembershipManagerV0.t.sol b/test/MembershipManagerV0.t.sol index 8ef92190..180ed2e0 100644 --- a/test/MembershipManagerV0.t.sol +++ b/test/MembershipManagerV0.t.sol @@ -81,7 +81,7 @@ contract MembershipManagerV0Test is TestSetup { _finalizeWithdrawalRequest(bobTokenId); vm.prank(bob); - withdrawRequestNFTInstance.claimWithdraw(bobTokenId); + withdrawRequestNFTInstance.claimWithdraw(bobTokenId, 1); assertEq(bob.balance, 100 ether, "Bob should have 100 ether"); assertEq(membershipNftInstance.balanceOf(bob, bobToken), 0); @@ -117,7 +117,7 @@ contract MembershipManagerV0Test is TestSetup { _finalizeWithdrawalRequest(aliceRequestId1); vm.startPrank(alice); - withdrawRequestNFTInstance.claimWithdraw(aliceRequestId1); + withdrawRequestNFTInstance.claimWithdraw(aliceRequestId1, 1); assertEq(membershipNftInstance.loyaltyPointsOf(tokenId), 2 * kwei); assertEq(membershipNftInstance.tierPointsOf(tokenId), 0); assertEq(membershipNftInstance.valueOf(tokenId), 1 ether); @@ -139,7 +139,7 @@ contract MembershipManagerV0Test is TestSetup { _finalizeWithdrawalRequest(aliceRequestId2); vm.startPrank(alice); - withdrawRequestNFTInstance.claimWithdraw(aliceRequestId2); + withdrawRequestNFTInstance.claimWithdraw(aliceRequestId2, 1); assertEq(membershipNftInstance.balanceOf(alice, tokenId), 0); assertEq(alice.balance, 2 ether); vm.stopPrank(); @@ -928,7 +928,7 @@ contract MembershipManagerV0Test is TestSetup { _finalizeWithdrawalRequest(reqId); vm.startPrank(alice); - withdrawRequestNFTInstance.claimWithdraw(reqId); + withdrawRequestNFTInstance.claimWithdraw(reqId, 1); assertEq(address(alice).balance, 10 ether); reqId = membershipManagerV1Instance.requestWithdrawAndBurn(aliceToken1); @@ -938,7 +938,7 @@ contract MembershipManagerV0Test is TestSetup { _finalizeWithdrawalRequest(reqId); vm.startPrank(alice); - withdrawRequestNFTInstance.claimWithdraw(reqId); + withdrawRequestNFTInstance.claimWithdraw(reqId, 1); assertEq(address(alice).balance, 200 ether); reqId = membershipManagerV1Instance.requestWithdrawAndBurn(aliceToken2); @@ -948,7 +948,7 @@ contract MembershipManagerV0Test is TestSetup { _finalizeWithdrawalRequest(reqId); vm.startPrank(alice); - withdrawRequestNFTInstance.claimWithdraw(reqId); + withdrawRequestNFTInstance.claimWithdraw(reqId, 1); assertEq(address(alice).balance, 400 ether); vm.stopPrank(); diff --git a/test/TestSetup.sol b/test/TestSetup.sol index 790dae87..956ebe3b 100644 --- a/test/TestSetup.sol +++ b/test/TestSetup.sol @@ -387,7 +387,7 @@ contract TestSetup is Test { nodeOperatorManagerInstance = NodeOperatorManager(addressProviderInstance.getContractAddress("NodeOperatorManager")); node = EtherFiNode(payable(addressProviderInstance.getContractAddress("EtherFiNode"))); earlyAdopterPoolInstance = EarlyAdopterPool(payable(addressProviderInstance.getContractAddress("EarlyAdopterPool"))); - withdrawRequestNFTInstance = WithdrawRequestNFT(addressProviderInstance.getContractAddress("WithdrawRequestNFT")); + withdrawRequestNFTInstance = WithdrawRequestNFT(payable(addressProviderInstance.getContractAddress("WithdrawRequestNFT"))); liquifierInstance = Liquifier(payable(addressProviderInstance.getContractAddress("Liquifier"))); etherFiTimelockInstance = EtherFiTimelock(payable(addressProviderInstance.getContractAddress("EtherFiTimelock"))); etherFiAdminInstance = EtherFiAdmin(payable(addressProviderInstance.getContractAddress("EtherFiAdmin"))); diff --git a/test/WithdrawRequestNFT.t.sol b/test/WithdrawRequestNFT.t.sol index d4011c78..a6986d9d 100644 --- a/test/WithdrawRequestNFT.t.sol +++ b/test/WithdrawRequestNFT.t.sol @@ -137,7 +137,7 @@ contract WithdrawRequestNFTTest is TestSetup { vm.expectRevert("Request is not finalized"); vm.prank(bob); - withdrawRequestNFTInstance.claimWithdraw(requestId); + withdrawRequestNFTInstance.claimWithdraw(requestId, 1); } function test_ClaimWithdrawOfOthers() public { @@ -157,7 +157,7 @@ contract WithdrawRequestNFTTest is TestSetup { vm.expectRevert("Not the owner of the NFT"); vm.prank(alice); - withdrawRequestNFTInstance.claimWithdraw(requestId); + withdrawRequestNFTInstance.claimWithdraw(requestId, 1); } function test_ValidClaimWithdraw1() public { @@ -196,7 +196,7 @@ contract WithdrawRequestNFTTest is TestSetup { _finalizeWithdrawalRequest(requestId); vm.prank(bob); - withdrawRequestNFTInstance.claimWithdraw(requestId); + withdrawRequestNFTInstance.claimWithdraw(requestId, 1); uint256 bobsEndingBalance = address(bob).balance; @@ -239,7 +239,7 @@ contract WithdrawRequestNFTTest is TestSetup { _finalizeWithdrawalRequest(requestId); vm.prank(bob); - withdrawRequestNFTInstance.claimWithdraw(requestId); + withdrawRequestNFTInstance.claimWithdraw(requestId, 1); uint256 bobsEndingBalance = address(bob).balance; @@ -268,7 +268,7 @@ contract WithdrawRequestNFTTest is TestSetup { uint256 bobsStartingBalance = address(bob).balance; vm.prank(bob); - withdrawRequestNFTInstance.claimWithdraw(requestId); + withdrawRequestNFTInstance.claimWithdraw(requestId, 1); uint256 bobsEndingBalance = address(bob).balance; @@ -305,7 +305,7 @@ contract WithdrawRequestNFTTest is TestSetup { vm.prank(bob); - withdrawRequestNFTInstance.claimWithdraw(requestId); + withdrawRequestNFTInstance.claimWithdraw(requestId, 1); // Within `claimWithdraw`, // - `request.amountOfEEth` is 9 // - `amountForShares` is (8 * 100) / 98 = 8.16 ---> (rounded down to) 8 @@ -392,7 +392,7 @@ contract WithdrawRequestNFTTest is TestSetup { // REVERT if not owner vm.prank(alice); vm.expectRevert("Ownable: caller is not the owner"); - withdrawRequestNFTInstance.seizeInvalidRequest(requestId, chad); + withdrawRequestNFTInstance.seizeInvalidRequest(requestId, chad, 1); } function test_InvalidatedRequestNft_seizeInvalidAndMintNew_1() public { @@ -401,7 +401,7 @@ contract WithdrawRequestNFTTest is TestSetup { uint256 chadBalance = address(chad).balance; vm.prank(owner); - withdrawRequestNFTInstance.seizeInvalidRequest(requestId, chad); + withdrawRequestNFTInstance.seizeInvalidRequest(requestId, chad, 1); assertEq(liquidityPoolInstance.ethAmountLockedForWithdrawal(), 0, "Must be withdrawn"); assertEq(address(chad).balance, chadBalance + claimableAmount, "Chad should receive the claimable amount"); @@ -413,7 +413,7 @@ contract WithdrawRequestNFTTest is TestSetup { uint256 chadBalance = address(chad).balance; vm.prank(owner); - withdrawRequestNFTInstance.seizeInvalidRequest(requestId, chad); + withdrawRequestNFTInstance.seizeInvalidRequest(requestId, chad, 1); assertEq(liquidityPoolInstance.ethAmountLockedForWithdrawal(), 0, "Must be withdrawn"); assertEq(address(chad).balance, chadBalance + claimableAmount, "Chad should receive the claimable amount"); From 72be7d70337e03bd284690d587483620593eb17f Mon Sep 17 00:00:00 2001 From: jtfirek Date: Mon, 2 Sep 2024 15:56:38 -0500 Subject: [PATCH 2/7] new dust shares + integration existing unit tests --- src/LiquidityPool.sol | 12 ++--- src/WithdrawRequestNFT.sol | 71 +++++++++++++------------- src/interfaces/IWithdrawRequestNFT.sol | 2 +- test/LiquidityPool.t.sol | 2 +- test/Liquifier.t.sol | 4 +- test/MembershipManager.t.sol | 25 +++++---- test/MembershipManagerV0.t.sol | 8 +-- test/WithdrawRequestNFT.t.sol | 6 +-- 8 files changed, 66 insertions(+), 64 deletions(-) diff --git a/src/LiquidityPool.sol b/src/LiquidityPool.sol index 2edb7028..a57cf3b6 100644 --- a/src/LiquidityPool.sol +++ b/src/LiquidityPool.sol @@ -195,13 +195,13 @@ contract LiquidityPool is Initializable, OwnableUpgradeable, UUPSUpgradeable, IL function withdraw(address _recipient, uint256 _amount) external whenNotPaused returns (uint256) { uint256 share = sharesForWithdrawalAmount(_amount); require(msg.sender == address(withdrawRequestNFT) || msg.sender == address(membershipManager) || msg.sender == address(liquifier), "Incorrect Caller"); - if (totalValueInLp < _amount || (msg.sender == address(withdrawRequestNFT) && ethAmountLockedForWithdrawal < _amount) || eETH.balanceOf(msg.sender) < _amount) revert InsufficientLiquidity(); + if (totalValueInLp < _amount || eETH.balanceOf(msg.sender) < _amount) revert InsufficientLiquidity(); if (_amount > type(uint128).max || _amount == 0 || share == 0) revert InvalidAmount(); totalValueInLp -= uint128(_amount); - if (msg.sender == address(withdrawRequestNFT)) { - ethAmountLockedForWithdrawal -= uint128(_amount); - } + // if (msg.sender == address(withdrawRequestNFT)) { + // ethAmountLockedForWithdrawal -= uint128(_amount); + // } eETH.burnShares(msg.sender, share); @@ -224,7 +224,7 @@ contract LiquidityPool is Initializable, OwnableUpgradeable, UUPSUpgradeable, IL // transfer shares to WithdrawRequestNFT contract from this contract eETH.transferFrom(msg.sender, address(withdrawRequestNFT), amount); - uint256 requestId = withdrawRequestNFT.requestWithdraw(uint96(amount), uint96(share), recipient, 0); + uint256 requestId = withdrawRequestNFT.requestWithdraw(uint96(amount), uint96(share), recipient); emit Withdraw(msg.sender, recipient, amount, SourceOfFunds.EETH); @@ -260,7 +260,7 @@ contract LiquidityPool is Initializable, OwnableUpgradeable, UUPSUpgradeable, IL // transfer shares to WithdrawRequestNFT contract eETH.transferFrom(msg.sender, address(withdrawRequestNFT), amount); - uint256 requestId = withdrawRequestNFT.requestWithdraw(uint96(amount), uint96(share), recipient, fee); + uint256 requestId = withdrawRequestNFT.requestWithdraw(uint96(amount), uint96(share), recipient); emit Withdraw(msg.sender, recipient, amount, SourceOfFunds.ETHER_FAN); diff --git a/src/WithdrawRequestNFT.sol b/src/WithdrawRequestNFT.sol index 2374bafe..f6bb75a9 100644 --- a/src/WithdrawRequestNFT.sol +++ b/src/WithdrawRequestNFT.sol @@ -23,7 +23,7 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad uint32 public nextRequestId; uint32 public lastFinalizedRequestId; - uint96 public accumulatedDustEEthShares; // to be burned or used to cover the validator churn cost + uint96 public DEPRECATED_accumulatedDustEEthShares; // to be burned or used to cover the validator churn cost FinalizationCheckpoint[] public finalizationCheckpoints; @@ -31,8 +31,8 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad bytes32 public constant WITHDRAW_NFT_ADMIN_ROLE = keccak256("WITHDRAW_NFT_ADMIN_ROLE"); - event WithdrawRequestCreated(uint32 indexed requestId, uint256 amountOfEEth, uint256 shareOfEEth, address owner, uint256 fee); - event WithdrawRequestClaimed(uint32 indexed requestId, uint256 amountOfEEth, uint256 DEPRECATED_burntShareOfEEth, address owner, uint256 fee); + event WithdrawRequestCreated(uint32 indexed requestId, uint256 amountOfEEth, uint256 shareOfEEth, address owner); + event WithdrawRequestClaimed(uint32 indexed requestId, uint256 amountOfEEth, uint256 DEPRECATED_burntShareOfEEth, address owner); event WithdrawRequestInvalidated(uint32 indexed requestId); event WithdrawRequestValidated(uint32 indexed requestId); event WithdrawRequestSeized(uint32 indexed requestId); @@ -66,6 +66,8 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad // to avoid having to deal with the finalizationCheckpoints[index - 1] edgecase where the index is 0, we add a dummy checkpoint finalizationCheckpoints.push(FinalizationCheckpoint(0, 0)); + + DEPRECATED_accumulatedDustEEthShares = 0; } /// @notice creates a withdraw request and issues an associated NFT to the recipient @@ -73,19 +75,25 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad /// @param amountOfEEth amount of eETH requested for withdrawal /// @param shareOfEEth share of eETH requested for withdrawal /// @param recipient address to recieve with WithdrawRequestNFT - /// @param fee fee to be subtracted from amount when recipient calls claimWithdraw /// @return uint256 id of the withdraw request - function requestWithdraw(uint96 amountOfEEth, uint96 shareOfEEth, address recipient, uint256 fee) external payable onlyLiquidtyPool returns (uint256) { + function requestWithdraw(uint96 amountOfEEth, uint96 shareOfEEth, address recipient) external payable onlyLiquidtyPool returns (uint256) { uint256 requestId = nextRequestId++; - uint32 feeGwei = uint32(fee / 1 gwei); - _requests[requestId] = IWithdrawRequestNFT.WithdrawRequest(amountOfEEth, shareOfEEth, true, feeGwei); + _requests[requestId] = IWithdrawRequestNFT.WithdrawRequest(amountOfEEth, shareOfEEth, true, 0); _safeMint(recipient, requestId); - emit WithdrawRequestCreated(uint32(requestId), amountOfEEth, shareOfEEth, recipient, fee); + emit WithdrawRequestCreated(uint32(requestId), amountOfEEth, shareOfEEth, recipient); return requestId; } + /// @notice returns the value of a request after finalization. The value of a request can be: + /// - nominal (when the amount of eth locked for this request are equal to the request's eETH) + /// - discounted (when the amount of eth will be lower, because the protocol share rate dropped + /// before request is finalized, so it will be equal to `request's shares` * `protocol share rate`) + /// @param tokenId the id of the withdraw request NFT + /// @param checkpointIndex the index of the `finalizationCheckpoints` that the request belongs to. + /// `checkpointIndex` can be found using `findCheckpointIndex` function + /// @return uint256 the amount of ETH that can be claimed by the owner of the NFT function getClaimableAmount(uint256 tokenId, uint256 checkpointIndex) public view returns (uint256) { require(tokenId <= lastFinalizedRequestId, "Request is not finalized"); require(tokenId < nextRequestId, "Request does not exist"); @@ -105,14 +113,10 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad ); IWithdrawRequestNFT.WithdrawRequest memory request = _requests[tokenId]; - // send the lesser ether ETH value of the originally requested amount of eEth or the eEth value at the last `cachedSharePrice` update - uint256 cachedSharePrice = checkpoint.cachedShareValue; - uint256 amountForSharesCached = request.shareOfEEth * cachedSharePrice / 1 ether; + uint256 amountForSharesCached = request.shareOfEEth * checkpoint.cachedShareValue / 1 ether; uint256 amountToTransfer = _min(request.amountOfEEth, amountForSharesCached); - - uint256 fee = uint256(request.feeGwei) * 1 gwei; - return amountToTransfer - fee; + return amountToTransfer; } /// @notice called by the NFT owner to claim their ETH @@ -128,20 +132,15 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad IWithdrawRequestNFT.WithdrawRequest memory request = _requests[tokenId]; require(request.isValid, "Request is not valid"); - uint256 fee = uint256(request.feeGwei) * 1 gwei; uint256 amountToWithdraw = getClaimableAmount(tokenId, checkpointIndex); // transfer eth to recipient _burn(tokenId); delete _requests[tokenId]; - if (fee > 0) { - // send fee to membership manager - _sendFund(address(membershipManager), fee); - } _sendFund(recipient, amountToWithdraw); - emit WithdrawRequestClaimed(uint32(tokenId), amountToWithdraw + fee, 0, recipient, fee); + emit WithdrawRequestClaimed(uint32(tokenId), amountToWithdraw, 0, recipient); } function batchClaimWithdraw(uint256[] calldata tokenIds, uint256[] calldata checkpointIndices) external { @@ -150,22 +149,6 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad } } - // Reduce the accumulated dust eEth shares by the given amount - // This is to fix the accounting error that was over-accumulating dust eEth shares due to the fee - function updateAccumulatedDustEEthShares(uint96 amount) external onlyOwner { - accumulatedDustEEthShares -= amount; - } - - // a function to transfer accumulated shares to admin - function withdrawAccumulatedDustEEthShares(address _recipient) external { - if (!roleRegistry.hasRole(WITHDRAW_NFT_ADMIN_ROLE, msg.sender)) revert IncorrectRole(); - uint256 shares = accumulatedDustEEthShares; - accumulatedDustEEthShares = 0; - - uint256 amountForShares = liquidityPool.amountForShare(shares); - eETH.transfer(_recipient, amountForShares); - } - // Given an invalidated withdrawal request NFT of ID `requestId`:, // - burn the NFT // - withdraw its ETH to the `recipient` @@ -336,4 +319,20 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad return finalizationCheckpoints.length; } + function getAccumulatedDustEEthAmount() public view returns (uint256) { + uint256 amountRequestedWithdraw = calculateTotalPendingAmount(nextRequestId - 1); + + uint256 contractEEthBalance = eETH.balanceOf(address(this)); + + return contractEEthBalance - amountRequestedWithdraw; + } + + function withdrawAccumulatedDustEEth(address _recipient) external { + if (!roleRegistry.hasRole(WITHDRAW_NFT_ADMIN_ROLE, msg.sender)) revert IncorrectRole(); + + uint256 dust = getAccumulatedDustEEthAmount(); + + // the dust amount is monotonically increasing, so we can just transfer the whole amount + eETH.transfer(_recipient, dust); + } } diff --git a/src/interfaces/IWithdrawRequestNFT.sol b/src/interfaces/IWithdrawRequestNFT.sol index 3a211fbf..9d0d5605 100644 --- a/src/interfaces/IWithdrawRequestNFT.sol +++ b/src/interfaces/IWithdrawRequestNFT.sol @@ -15,7 +15,7 @@ interface IWithdrawRequestNFT { } function initialize(address _liquidityPoolAddress, address _eEthAddress, address _membershipManager) external; - function requestWithdraw(uint96 amountOfEEth, uint96 shareOfEEth, address requester, uint256 fee) external payable returns (uint256); + function requestWithdraw(uint96 amountOfEEth, uint96 shareOfEEth, address requester) external payable returns (uint256); function claimWithdraw(uint256 requestId, uint256 checkpointIndex) external; function getRequest(uint256 requestId) external view returns (WithdrawRequest memory); diff --git a/test/LiquidityPool.t.sol b/test/LiquidityPool.t.sol index 127e6d48..b6c5d834 100644 --- a/test/LiquidityPool.t.sol +++ b/test/LiquidityPool.t.sol @@ -159,7 +159,7 @@ contract LiquidityPoolTest is TestSetup { _finalizeWithdrawalRequest(bobReqId); vm.startPrank(bob); - withdrawRequestNFTInstance.claimWithdraw(bobReqId, 1); + withdrawRequestNFTInstance.claimWithdraw(bobReqId, 2); assertEq(eETHInstance.balanceOf(bob), 0); assertEq(bob.balance, 3 ether); vm.stopPrank(); diff --git a/test/Liquifier.t.sol b/test/Liquifier.t.sol index 2ca15da9..7735548e 100644 --- a/test/Liquifier.t.sol +++ b/test/Liquifier.t.sol @@ -457,8 +457,8 @@ contract LiquifierTest is TestSetup { uint256 afterTVL = liquidityPoolInstance.getTotalPooledEther(); uint256 afterLiquifierTotalPooledEther = liquifierInstance.getTotalPooledEther(); - assertApproxEqAbs(afterTVL, beforeTVL, 1); - assertApproxEqAbs(beforeLiquifierTotalPooledEther, afterLiquifierTotalPooledEther, 1); + assertApproxEqAbs(afterTVL, beforeTVL, 2); + assertApproxEqAbs(beforeLiquifierTotalPooledEther, afterLiquifierTotalPooledEther, 2); } function test_withdrawEEth() public { diff --git a/test/MembershipManager.t.sol b/test/MembershipManager.t.sol index 899978bd..e0edfe1e 100644 --- a/test/MembershipManager.t.sol +++ b/test/MembershipManager.t.sol @@ -79,7 +79,7 @@ contract MembershipManagerTest is TestSetup { function test_batchClaimWithdraw() public { - assertEq(withdrawRequestNFTInstance.accumulatedDustEEthShares(), 0, "Accumulated dust should be 0"); + assertEq(withdrawRequestNFTInstance.getAccumulatedDustEEthAmount(), 0, "Accumulated dust should be 0"); vm.prank(alice); membershipManagerV1Instance.setFeeAmounts(0 ether, 0.5 ether, 0 ether, 30); @@ -107,10 +107,11 @@ contract MembershipManagerTest is TestSetup { withdrawRequestNFTInstance.batchClaimWithdraw(requestIds, requestIdCheckpoints); vm.stopPrank(); - assertEq(address(membershipManagerV1Instance).balance, 2 * 0.5 ether); - assertEq(address(bob).balance, 100 ether - 2 * 0.5 ether); + // fees have been deprecated in the withdraw flow so expect collected fees to always be 0 + assertEq(address(membershipManagerV1Instance).balance, 0); + assertEq(address(bob).balance, 100 ether); - assertEq(withdrawRequestNFTInstance.accumulatedDustEEthShares(), 0, "Accumulated dust should be 0"); + assertEq(withdrawRequestNFTInstance.getAccumulatedDustEEthAmount(), 0, "Accumulated dust should be 0"); } @@ -165,7 +166,7 @@ contract MembershipManagerTest is TestSetup { _finalizeWithdrawalRequest(aliceRequestId2); vm.startPrank(alice); - withdrawRequestNFTInstance.claimWithdraw(aliceRequestId2, 1); + withdrawRequestNFTInstance.claimWithdraw(aliceRequestId2, 2); assertEq(membershipNftInstance.balanceOf(alice, tokenId), 0); assertEq(alice.balance, 2 ether); vm.stopPrank(); @@ -1021,8 +1022,9 @@ contract MembershipManagerTest is TestSetup { _finalizeWithdrawalRequest(requestId); + uint256 requestCheckpointIndex = withdrawRequestNFTInstance.findCheckpointIndex(requestId, 0, withdrawRequestNFTInstance.getFinalizationCheckpointsLength() - 1); vm.startPrank(actor); - withdrawRequestNFTInstance.claimWithdraw(requestId, 1); + withdrawRequestNFTInstance.claimWithdraw(requestId, requestCheckpointIndex); counts[3]++; } @@ -1043,11 +1045,11 @@ contract MembershipManagerTest is TestSetup { _finalizeWithdrawalRequest(requestId); + uint256 requestCheckpointIndex = withdrawRequestNFTInstance.findCheckpointIndex(requestId, 0, withdrawRequestNFTInstance.getFinalizationCheckpointsLength() - 1); vm.prank(actor); - withdrawRequestNFTInstance.claimWithdraw(requestId, 1); + withdrawRequestNFTInstance.claimWithdraw(requestId, requestCheckpointIndex); - assertLe(address(actor).balance, expectedBalanceAfterWithdrawal); - assertGe(address(actor).balance, expectedBalanceAfterWithdrawal - 3); // rounding errors + assertApproxEqAbs(address(actor).balance, expectedBalanceAfterWithdrawal, 20); totalActorsBalance += address(actor).balance; } @@ -1062,7 +1064,7 @@ contract MembershipManagerTest is TestSetup { console.log("address(liquidityPoolInstance).balance", address(liquidityPoolInstance).balance); console.log("eETHInstance.balanceOf(address(membershipManagerV1Instance))", eETHInstance.balanceOf(address(membershipManagerV1Instance))); // console.log("resting Rewards", liquidityPoolInstance.amountForShare(membershipManagerV1Instance.sharesReservedForRewards())); - assertEq(totalActorsBalance + address(liquidityPoolInstance).balance, totalMoneySupply); + assertEq(totalActorsBalance + address(liquidityPoolInstance).balance + address(withdrawRequestNFTInstance).balance, totalMoneySupply); // assertLe(membershipManagerV1Instance.sharesReservedForRewards(), eETHInstance.shares(address(membershipManagerV1Instance))); } @@ -1348,7 +1350,8 @@ contract MembershipManagerTest is TestSetup { uint256 reqId2 = membershipManagerV1Instance.requestWithdrawAndBurn(aliceToken2); assertEq(withdrawRequestNFTInstance.getRequest(reqId1).amountOfEEth, 1 ether); - assertEq(withdrawRequestNFTInstance.getRequest(reqId1).feeGwei, uint32(burnFee / 1 gwei)); + // burn fees are deprecated + assertEq(withdrawRequestNFTInstance.getRequest(reqId1).feeGwei, 0); assertEq(withdrawRequestNFTInstance.getRequest(reqId2).amountOfEEth, 1 ether); } diff --git a/test/MembershipManagerV0.t.sol b/test/MembershipManagerV0.t.sol index 180ed2e0..52d2967f 100644 --- a/test/MembershipManagerV0.t.sol +++ b/test/MembershipManagerV0.t.sol @@ -139,7 +139,7 @@ contract MembershipManagerV0Test is TestSetup { _finalizeWithdrawalRequest(aliceRequestId2); vm.startPrank(alice); - withdrawRequestNFTInstance.claimWithdraw(aliceRequestId2, 1); + withdrawRequestNFTInstance.claimWithdraw(aliceRequestId2, 2); assertEq(membershipNftInstance.balanceOf(alice, tokenId), 0); assertEq(alice.balance, 2 ether); vm.stopPrank(); @@ -861,7 +861,7 @@ contract MembershipManagerV0Test is TestSetup { assertEq(membershipNftInstance.valueOf(tokens[4]), 1 ether + 1 ether * uint256(30) / uint256(100)); } - function test_token_vault_migratino() public { + function test_token_vault_migration() public { vm.deal(alice, 100 ether); // Alice mints two NFTs with 50 ETH for each @@ -938,7 +938,7 @@ contract MembershipManagerV0Test is TestSetup { _finalizeWithdrawalRequest(reqId); vm.startPrank(alice); - withdrawRequestNFTInstance.claimWithdraw(reqId, 1); + withdrawRequestNFTInstance.claimWithdraw(reqId, 2); assertEq(address(alice).balance, 200 ether); reqId = membershipManagerV1Instance.requestWithdrawAndBurn(aliceToken2); @@ -948,7 +948,7 @@ contract MembershipManagerV0Test is TestSetup { _finalizeWithdrawalRequest(reqId); vm.startPrank(alice); - withdrawRequestNFTInstance.claimWithdraw(reqId, 1); + withdrawRequestNFTInstance.claimWithdraw(reqId, 3); assertEq(address(alice).balance, 400 ether); vm.stopPrank(); diff --git a/test/WithdrawRequestNFT.t.sol b/test/WithdrawRequestNFT.t.sol index a6986d9d..bae52a01 100644 --- a/test/WithdrawRequestNFT.t.sol +++ b/test/WithdrawRequestNFT.t.sol @@ -179,7 +179,7 @@ contract WithdrawRequestNFTTest is TestSetup { vm.prank(bob); uint256 requestId = liquidityPoolInstance.requestWithdraw(bob, 1 ether); - assertEq(withdrawRequestNFTInstance.accumulatedDustEEthShares(), 0, "Accumulated dust should be 0"); + assertEq(withdrawRequestNFTInstance.getAccumulatedDustEEthAmount(), 0, "Accumulated dust should be 0"); assertEq(eETHInstance.balanceOf(bob), 9 ether); assertEq(eETHInstance.balanceOf(address(withdrawRequestNFTInstance)), 1 ether, "eETH balance should be 1 ether"); @@ -202,10 +202,10 @@ contract WithdrawRequestNFTTest is TestSetup { assertEq(bobsEndingBalance, bobsStartingBalance + 1 ether, "Bobs balance should be 1 ether higher"); assertEq(eETHInstance.balanceOf(address(withdrawRequestNFTInstance)), 1 ether, "eETH balance should be 1 ether"); - assertEq(liquidityPoolInstance.amountForShare(withdrawRequestNFTInstance.accumulatedDustEEthShares()), 1 ether); + assertEq(withdrawRequestNFTInstance.getAccumulatedDustEEthAmount(), 1 ether); vm.prank(alice); - withdrawRequestNFTInstance.withdrawAccumulatedDustEEthShares(bob); + withdrawRequestNFTInstance.withdrawAccumulatedDustEEth(bob); assertEq(eETHInstance.balanceOf(address(withdrawRequestNFTInstance)), 0 ether, "eETH balance should be 0 ether"); assertEq(eETHInstance.balanceOf(bob), 18 ether + 1 ether); // 1 ether eETH in `withdrawRequestNFT` contract is sent to Bob } From 1fceb707044f561016162303f7cad796d5dbfd46 Mon Sep 17 00:00:00 2001 From: jtfirek Date: Tue, 3 Sep 2024 14:35:28 -0500 Subject: [PATCH 3/7] re-organizing functions + adding comments + consistent usage of uint32 --- src/EtherFiAdmin.sol | 4 +- src/LiquidityPool.sol | 39 +-- src/WithdrawRequestNFT.sol | 341 +++++++++++++------------ src/interfaces/IEtherFiOracle.sol | 4 +- src/interfaces/ILiquidityPool.sol | 6 +- src/interfaces/IWithdrawRequestNFT.sol | 13 +- test/EtherFiAdminUpgrade.t.sol | 4 +- test/LiquidityPool.t.sol | 6 +- test/MembershipManager.t.sol | 32 +-- test/MembershipManagerV0.t.sol | 14 +- test/TestSetup.sol | 6 +- test/WithdrawRequestNFT.t.sol | 36 ++- test/eethPayoutUpgrade.t.sol | 4 +- 13 files changed, 254 insertions(+), 255 deletions(-) diff --git a/src/EtherFiAdmin.sol b/src/EtherFiAdmin.sol index 9a69ce40..d00ee870 100644 --- a/src/EtherFiAdmin.sol +++ b/src/EtherFiAdmin.sol @@ -248,7 +248,7 @@ contract EtherFiAdmin is Initializable, OwnableUpgradeable, UUPSUpgradeable { for (uint256 i = 0; i < _report.withdrawalRequestsToInvalidate.length; i++) { withdrawRequestNft.invalidateRequest(_report.withdrawalRequestsToInvalidate[i]); } - withdrawRequestNft.finalizeRequests(_report.lastFinalizedWithdrawalRequestId, _report.finalizedWithdrawalAmount); + withdrawRequestNft.finalizeRequests(_report.lastFinalizedWithdrawalRequestId); } function slotForNextReportToProcess() public view returns (uint32) { @@ -274,4 +274,4 @@ contract EtherFiAdmin is Initializable, OwnableUpgradeable, UUPSUpgradeable { } function _authorizeUpgrade(address newImplementation) internal override onlyOwner {} -} \ No newline at end of file +} diff --git a/src/LiquidityPool.sol b/src/LiquidityPool.sol index a57cf3b6..0962a64c 100644 --- a/src/LiquidityPool.sol +++ b/src/LiquidityPool.sol @@ -65,7 +65,7 @@ contract LiquidityPool is Initializable, OwnableUpgradeable, UUPSUpgradeable, IL mapping(address => BnftHoldersIndex) public validatorSpawner; bool public restakeBnftDeposits; - uint128 public ethAmountLockedForWithdrawal; + uint128 public DEPRECATED_ethAmountLockedForWithdrawal; bool public paused; IAuctionManager public auctionManager; ILiquifier public liquifier; @@ -75,7 +75,7 @@ contract LiquidityPool is Initializable, OwnableUpgradeable, UUPSUpgradeable, IL RoleRegistry public roleRegistry; //-------------------------------------------------------------------------------------- - //------------------------------------- ROLES --------------------------------------- + //------------------------------------- ROLES ---------------------------------------- //-------------------------------------------------------------------------------------- bytes32 public constant LIQUIDITY_POOL_ADMIN_ROLE = keccak256("LIQUIDITY_POOL_ADMIN_ROLE"); @@ -136,7 +136,7 @@ contract LiquidityPool is Initializable, OwnableUpgradeable, UUPSUpgradeable, IL tNft = ITNFT(_tNftAddress); paused = true; restakeBnftDeposits = false; - ethAmountLockedForWithdrawal = 0; + DEPRECATED_ethAmountLockedForWithdrawal = 0; etherFiAdminContract = _etherFiAdminContract; withdrawRequestNFT = IWithdrawRequestNFT(_withdrawRequestNFT); DEPRECATED_admins[_etherFiAdminContract] = true; @@ -152,8 +152,8 @@ contract LiquidityPool is Initializable, OwnableUpgradeable, UUPSUpgradeable, IL function initializeV2dot5(address _roleRegistry) external onlyOwner { require(address(roleRegistry) == address(0x00), "already initialized"); - - // TODO: compile list of values in DEPRECATED_admins to clear out + + DEPRECATED_ethAmountLockedForWithdrawal = 0; roleRegistry = RoleRegistry(_roleRegistry); } @@ -199,9 +199,6 @@ contract LiquidityPool is Initializable, OwnableUpgradeable, UUPSUpgradeable, IL if (_amount > type(uint128).max || _amount == 0 || share == 0) revert InvalidAmount(); totalValueInLp -= uint128(_amount); - // if (msg.sender == address(withdrawRequestNFT)) { - // ethAmountLockedForWithdrawal -= uint128(_amount); - // } eETH.burnShares(msg.sender, share); @@ -216,15 +213,15 @@ contract LiquidityPool is Initializable, OwnableUpgradeable, UUPSUpgradeable, IL /// @dev Transfers the amount of eETH from msg.senders account to the WithdrawRequestNFT contract & mints an NFT to the msg.sender /// @param recipient address that will be issued the NFT /// @param amount requested amount to withdraw from contract - /// @return uint256 requestId of the WithdrawRequestNFT - function requestWithdraw(address recipient, uint256 amount) public whenNotPaused returns (uint256) { + /// @return uint32 requestId of the WithdrawRequestNFT + function requestWithdraw(address recipient, uint256 amount) public whenNotPaused returns (uint32) { uint256 share = sharesForAmount(amount); if (amount > type(uint96).max || amount == 0 || share == 0) revert InvalidAmount(); // transfer shares to WithdrawRequestNFT contract from this contract eETH.transferFrom(msg.sender, address(withdrawRequestNFT), amount); - uint256 requestId = withdrawRequestNFT.requestWithdraw(uint96(amount), uint96(share), recipient); + uint32 requestId = withdrawRequestNFT.requestWithdraw(uint96(amount), uint96(share), recipient); emit Withdraw(msg.sender, recipient, amount, SourceOfFunds.EETH); @@ -236,11 +233,11 @@ contract LiquidityPool is Initializable, OwnableUpgradeable, UUPSUpgradeable, IL /// @param _owner address that will be issued the NFT /// @param _amount requested amount to withdraw from contract /// @param _permit signed permit data to approve transfer of eETH - /// @return uint256 requestId of the WithdrawRequestNFT + /// @return uint32 requestId of the WithdrawRequestNFT function requestWithdrawWithPermit(address _owner, uint256 _amount, PermitInput calldata _permit) external whenNotPaused - returns (uint256) + returns (uint32) { try eETH.permit(msg.sender, address(this), _permit.value, _permit.deadline, _permit.v, _permit.r, _permit.s) {} catch {} return requestWithdraw(_owner, _amount); @@ -251,7 +248,7 @@ contract LiquidityPool is Initializable, OwnableUpgradeable, UUPSUpgradeable, IL /// @param recipient address that will be issued the NFT /// @param amount requested amount to withdraw from contract /// @param fee the burn fee to be paid by the recipient when the withdrawal is claimed (WithdrawRequestNFT.claimWithdraw) - /// @return uint256 requestId of the WithdrawRequestNFT + /// @return uint32 requestId of the WithdrawRequestNFT function requestMembershipNFTWithdraw(address recipient, uint256 amount, uint256 fee) public whenNotPaused returns (uint256) { if (msg.sender != address(membershipManager)) revert IncorrectCaller(); uint256 share = sharesForAmount(amount); @@ -260,7 +257,7 @@ contract LiquidityPool is Initializable, OwnableUpgradeable, UUPSUpgradeable, IL // transfer shares to WithdrawRequestNFT contract eETH.transferFrom(msg.sender, address(withdrawRequestNFT), amount); - uint256 requestId = withdrawRequestNFT.requestWithdraw(uint96(amount), uint96(share), recipient); + uint32 requestId = withdrawRequestNFT.requestWithdraw(uint96(amount), uint96(share), recipient); emit Withdraw(msg.sender, recipient, amount, SourceOfFunds.ETHER_FAN); @@ -473,12 +470,6 @@ contract LiquidityPool is Initializable, OwnableUpgradeable, UUPSUpgradeable, IL isLpBnftHolder = _isLpBnftHolder; } - function addEthAmountLockedForWithdrawal(uint128 _amount) external { - if (msg.sender != address(withdrawRequestNFT)) revert IncorrectCaller(); - - ethAmountLockedForWithdrawal += _amount; - } - // This function can't change the TVL // but used only to correct the errors in tracking {totalValueOutOfLp} and {totalValueInLp} function updateTvlSplits(int128 _diffTotalValueOutOfLp, int128 _diffTotalValueInLp) external onlyOwner { @@ -490,12 +481,6 @@ contract LiquidityPool is Initializable, OwnableUpgradeable, UUPSUpgradeable, IL if(tvl != getTotalPooledEther()) revert(); } - function reduceEthAmountLockedForWithdrawal(uint128 _amount) external { - if (msg.sender != address(withdrawRequestNFT)) revert IncorrectCaller(); - - ethAmountLockedForWithdrawal -= _amount; - } - //-------------------------------------------------------------------------------------- //------------------------------ INTERNAL FUNCTIONS ---------------------------------- //-------------------------------------------------------------------------------------- diff --git a/src/WithdrawRequestNFT.sol b/src/WithdrawRequestNFT.sol index f6bb75a9..93788b7f 100644 --- a/src/WithdrawRequestNFT.sol +++ b/src/WithdrawRequestNFT.sol @@ -13,6 +13,9 @@ import "./RoleRegistry.sol"; contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgradeable, IWithdrawRequestNFT { + //-------------------------------------------------------------------------------------- + //--------------------------------- STATE-VARIABLES ---------------------------------- + //-------------------------------------------------------------------------------------- ILiquidityPool public liquidityPool; IeETH public eETH; @@ -29,17 +32,29 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad RoleRegistry public roleRegistry; + //-------------------------------------------------------------------------------------- + //------------------------------------- ROLES ---------------------------------------- + //-------------------------------------------------------------------------------------- + bytes32 public constant WITHDRAW_NFT_ADMIN_ROLE = keccak256("WITHDRAW_NFT_ADMIN_ROLE"); + //-------------------------------------------------------------------------------------- + //------------------------------------- EVENTS --------------------------------------- + //-------------------------------------------------------------------------------------- + event WithdrawRequestCreated(uint32 indexed requestId, uint256 amountOfEEth, uint256 shareOfEEth, address owner); event WithdrawRequestClaimed(uint32 indexed requestId, uint256 amountOfEEth, uint256 DEPRECATED_burntShareOfEEth, address owner); event WithdrawRequestInvalidated(uint32 indexed requestId); event WithdrawRequestValidated(uint32 indexed requestId); event WithdrawRequestSeized(uint32 indexed requestId); - event UpdateFinalizedRequestId(uint32 indexed requestId, uint128 finalizedAmount); + event UpdateFinalizedRequestId(uint32 indexed requestId, uint256 finalizedAmount); error IncorrectRole(); + //-------------------------------------------------------------------------------------- + //---------------------------- STATE-CHANGING FUNCTIONS ------------------------------ + //-------------------------------------------------------------------------------------- + /// @custom:oz-upgrades-unsafe-allow constructor constructor() { _disableInitializers(); @@ -62,12 +77,10 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad require(address(roleRegistry) == address(0x00), "already initialized"); // TODO: compile list of values in DEPRECATED_admins to clear out - roleRegistry = RoleRegistry(_roleRegistry); + DEPRECATED_accumulatedDustEEthShares = 0; - // to avoid having to deal with the finalizationCheckpoints[index - 1] edgecase where the index is 0, we add a dummy checkpoint + roleRegistry = RoleRegistry(_roleRegistry); finalizationCheckpoints.push(FinalizationCheckpoint(0, 0)); - - DEPRECATED_accumulatedDustEEthShares = 0; } /// @notice creates a withdraw request and issues an associated NFT to the recipient @@ -76,8 +89,8 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad /// @param shareOfEEth share of eETH requested for withdrawal /// @param recipient address to recieve with WithdrawRequestNFT /// @return uint256 id of the withdraw request - function requestWithdraw(uint96 amountOfEEth, uint96 shareOfEEth, address recipient) external payable onlyLiquidtyPool returns (uint256) { - uint256 requestId = nextRequestId++; + function requestWithdraw(uint96 amountOfEEth, uint96 shareOfEEth, address recipient) external payable onlyLiquidityPool returns (uint32) { + uint32 requestId = nextRequestId++; _requests[requestId] = IWithdrawRequestNFT.WithdrawRequest(amountOfEEth, shareOfEEth, true, 0); _safeMint(recipient, requestId); @@ -86,73 +99,32 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad return requestId; } - /// @notice returns the value of a request after finalization. The value of a request can be: - /// - nominal (when the amount of eth locked for this request are equal to the request's eETH) - /// - discounted (when the amount of eth will be lower, because the protocol share rate dropped - /// before request is finalized, so it will be equal to `request's shares` * `protocol share rate`) - /// @param tokenId the id of the withdraw request NFT + /// @notice called by the NFT owner of a finalized request to claim their ETH + /// @dev `checkpointIndex` can be found using `findCheckpointIndex` function + /// @param requestId the id of the withdraw request and associated NFT /// @param checkpointIndex the index of the `finalizationCheckpoints` that the request belongs to. - /// `checkpointIndex` can be found using `findCheckpointIndex` function - /// @return uint256 the amount of ETH that can be claimed by the owner of the NFT - function getClaimableAmount(uint256 tokenId, uint256 checkpointIndex) public view returns (uint256) { - require(tokenId <= lastFinalizedRequestId, "Request is not finalized"); - require(tokenId < nextRequestId, "Request does not exist"); - require(ownerOf(tokenId) != address(0), "Already Claimed"); - - require( - checkpointIndex != 0 && - checkpointIndex < finalizationCheckpoints.length, - "Invalid checkpoint index" - ); - FinalizationCheckpoint memory lowerBoundCheckpoint = finalizationCheckpoints[checkpointIndex - 1]; - FinalizationCheckpoint memory checkpoint = finalizationCheckpoints[checkpointIndex]; - require( - lowerBoundCheckpoint.lastFinalizedRequestId < tokenId && - tokenId <= checkpoint.lastFinalizedRequestId, - "Invalid checkpoint" - ); - IWithdrawRequestNFT.WithdrawRequest memory request = _requests[tokenId]; - - uint256 amountForSharesCached = request.shareOfEEth * checkpoint.cachedShareValue / 1 ether; - uint256 amountToTransfer = _min(request.amountOfEEth, amountForSharesCached); - - return amountToTransfer; + function claimWithdraw(uint32 requestId, uint32 checkpointIndex) external { + return _claimWithdraw(requestId, ownerOf(requestId), checkpointIndex); } - /// @notice called by the NFT owner to claim their ETH - /// @dev burns the NFT and transfers ETH from the liquidity pool to the owner minus any fee, withdraw request must be valid and finalized - /// @param tokenId the id of the withdraw request and associated NFT - function claimWithdraw(uint256 tokenId, uint256 checkpointIndex) external { - return _claimWithdraw(tokenId, ownerOf(tokenId), checkpointIndex); + /// @notice Batch version of `claimWithdraw` + function batchClaimWithdraw(uint32[] calldata requestIds, uint32[] calldata checkpointIndices) external { + for (uint32 i = 0; i < requestIds.length; i++) { + _claimWithdraw(requestIds[i], ownerOf(requestIds[i]), checkpointIndices[i]); + } } - - function _claimWithdraw(uint256 tokenId, address recipient, uint256 checkpointIndex) internal { - require(ownerOf(tokenId) == msg.sender, "Not the owner of the NFT"); - IWithdrawRequestNFT.WithdrawRequest memory request = _requests[tokenId]; - require(request.isValid, "Request is not valid"); - - uint256 amountToWithdraw = getClaimableAmount(tokenId, checkpointIndex); - - // transfer eth to recipient - _burn(tokenId); - delete _requests[tokenId]; - - _sendFund(recipient, amountToWithdraw); + /// @notice allows the admin to withdraw the accumulated dust eETH + function withdrawAccumulatedDustEEth(address _recipient) external { + if (!roleRegistry.hasRole(WITHDRAW_NFT_ADMIN_ROLE, msg.sender)) revert IncorrectRole(); - emit WithdrawRequestClaimed(uint32(tokenId), amountToWithdraw, 0, recipient); - } + uint256 dust = getAccumulatedDustEEthAmount(); - function batchClaimWithdraw(uint256[] calldata tokenIds, uint256[] calldata checkpointIndices) external { - for (uint256 i = 0; i < tokenIds.length; i++) { - _claimWithdraw(tokenIds[i], ownerOf(tokenIds[i]), checkpointIndices[i]); - } + // the dust amount is monotonically increasing, so we can just transfer the whole amount + eETH.transfer(_recipient, dust); } - // Given an invalidated withdrawal request NFT of ID `requestId`:, - // - burn the NFT - // - withdraw its ETH to the `recipient` - function seizeInvalidRequest(uint256 requestId, address recipient, uint256 checkpointIndex) external onlyOwner { + function seizeInvalidRequest(uint32 requestId, address recipient, uint32 checkpointIndex) external onlyOwner { require(!_requests[requestId].isValid, "Request is valid"); require(ownerOf(requestId) != address(0), "Already Claimed"); @@ -162,117 +134,81 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad // Undo its invalidation to claim _requests[requestId].isValid = true; - // withdraw the ETH to the recipient - // TODO: fix this _claimWithdraw(requestId, recipient, checkpointIndex); - emit WithdrawRequestSeized(uint32(requestId)); - } - - function getRequest(uint256 requestId) external view returns (IWithdrawRequestNFT.WithdrawRequest memory) { - return _requests[requestId]; - } - - function isFinalized(uint256 requestId) public view returns (bool) { - return requestId <= lastFinalizedRequestId; - } - - function isValid(uint256 requestId) public view returns (bool) { - require(_exists(requestId), "Request does not exist"); - return _requests[requestId].isValid; - } - - function calculateTotalPendingAmount(uint256 lastRequestId) public view returns (uint256) { - uint256 totalAmount = 0; - for (uint256 i = lastFinalizedRequestId + 1; i <= lastRequestId; i++) { - - IWithdrawRequestNFT.WithdrawRequest memory request = _requests[i]; - uint256 amountForShares = liquidityPool.amountForShare(request.shareOfEEth); - - uint256 amount = _min(request.amountOfEEth, amountForShares); - - totalAmount += amount; - } - return totalAmount; + emit WithdrawRequestSeized(requestId); } - function finalizeRequests(uint256 lastRequestId) external { + function finalizeRequests(uint32 lastRequestId) external { if (!roleRegistry.hasRole(WITHDRAW_NFT_ADMIN_ROLE, msg.sender)) revert IncorrectRole(); - uint128 totalAmount = uint128(calculateTotalPendingAmount(lastRequestId)); + uint256 totalAmount = uint256(calculateTotalPendingAmount(lastRequestId)); uint256 cachedSharePrice = liquidityPool.amountForShare(1 ether); finalizationCheckpoints.push(FinalizationCheckpoint(uint32(lastRequestId), cachedSharePrice)); - // TODO: We can probably deprecate this variable and use the last element of the finalizationCheckpoints array - lastFinalizedRequestId = uint32(lastRequestId); - - liquidityPool.withdraw(address(this), totalAmount); - emit UpdateFinalizedRequestId(uint32(lastRequestId), totalAmount); - } + lastFinalizedRequestId = lastRequestId; - // Maybe deprecated ? I should try to fix any accounting errors and delete this function - // It can be used to correct the total amount of pending withdrawals. There are some accounting erros as of now - function finalizeRequests(uint256 lastRequestId, uint128 totalAmount) external { - if (!roleRegistry.hasRole(WITHDRAW_NFT_ADMIN_ROLE, msg.sender)) revert IncorrectRole(); + if (totalAmount > 0) { + liquidityPool.withdraw(address(this), totalAmount); + } - return; + emit UpdateFinalizedRequestId(lastRequestId, totalAmount); } - function invalidateRequest(uint256 requestId) external { + function invalidateRequest(uint32 requestId) external { if (!roleRegistry.hasRole(WITHDRAW_NFT_ADMIN_ROLE, msg.sender)) revert IncorrectRole(); - require(isValid(requestId), "Request is not valid"); _requests[requestId].isValid = false; - emit WithdrawRequestInvalidated(uint32(requestId)); + emit WithdrawRequestInvalidated(requestId); } - function validateRequest(uint256 requestId) external { + function validateRequest(uint32 requestId) external { if (!roleRegistry.hasRole(WITHDRAW_NFT_ADMIN_ROLE, msg.sender)) revert IncorrectRole(); - require(!_requests[requestId].isValid, "Request is valid"); + _requests[requestId].isValid = true; - emit WithdrawRequestValidated(uint32(requestId)); + emit WithdrawRequestValidated(requestId); } - // invalid NFTs is non-transferable except for the case they are being burnt by the owner via `seizeInvalidRequest` - function _beforeTokenTransfer(address from, address to, uint256 firstTokenId, uint256 batchSize) internal override { - for (uint256 i = 0; i < batchSize; i++) { - uint256 tokenId = firstTokenId + i; - require(_requests[tokenId].isValid || msg.sender == owner(), "INVALID_REQUEST"); - } - } - - function _authorizeUpgrade(address newImplementation) internal override onlyOwner {} - - function getImplementation() external view returns (address) { - return _getImplementation(); - } + //-------------------------------------------------------------------------------------- + //--------------------------------- VIEW FUNCTIONS ----------------------------------- + //-------------------------------------------------------------------------------------- - function _sendFund(address _recipient, uint256 _amount) internal { - uint256 balanace = address(this).balance; - (bool sent, ) = _recipient.call{value: _amount}(""); - require(sent && address(this).balance == balanace - _amount, "SendFail"); - } - - function _min(uint256 a, uint256 b) internal pure returns (uint256) { - return a < b ? a : b; - } - - // This contract only accepts ETH sent from the liquidity pool - receive() external payable onlyLiquidtyPool() { } + /// @notice returns the value of a request after finalization. The value of a request can be: + /// - nominal (when the amount of eth locked for this request are equal to the request's eETH) + /// - discounted (when the amount of eth will be lower, because the protocol share rate dropped + /// before request is finalized, so it will be equal to `request's shares` * `protocol share rate`) + /// @param requestId the id of the withdraw request NFT + /// @param checkpointIndex the index of the `finalizationCheckpoints` that the request belongs to. + /// `checkpointIndex` can be found using `findCheckpointIndex` function + /// @return uint256 the amount of ETH that can be claimed by the owner of the NFT + function getClaimableAmount(uint32 requestId, uint32 checkpointIndex) public view returns (uint256) { + require(requestId <= lastFinalizedRequestId, "Request is not finalized"); + require(requestId < nextRequestId, "Request does not exist"); + require(ownerOf(requestId) != address(0), "Already claimed"); - modifier onlyLiquidtyPool() { - require(msg.sender == address(liquidityPool), "Caller is not the liquidity pool"); - _; - } + require( + checkpointIndex != 0 && + checkpointIndex < finalizationCheckpoints.length, + "Invalid checkpoint index" + ); + FinalizationCheckpoint memory lowerBoundCheckpoint = finalizationCheckpoints[checkpointIndex - 1]; + FinalizationCheckpoint memory checkpoint = finalizationCheckpoints[checkpointIndex]; + require( + lowerBoundCheckpoint.lastFinalizedRequestId < requestId && + requestId <= checkpoint.lastFinalizedRequestId, + "Checkpoint does not contain the request" + ); + IWithdrawRequestNFT.WithdrawRequest memory request = _requests[requestId]; + uint256 amountForSharesCached = request.shareOfEEth * checkpoint.cachedShareValue / 1 ether; + uint256 amountToTransfer = _min(request.amountOfEEth, amountForSharesCached); - function getFinalizationCheckpoint(uint256 checkpointId) external view returns (FinalizationCheckpoint memory) { - return finalizationCheckpoints[checkpointId]; + return amountToTransfer; } /// @dev View function to find a finalization checkpoint to use in `claimWithdraw()` @@ -283,23 +219,21 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad /// @param _end index of the right boundary of the search range, should be less than or equal to `finalizationCheckpoints.length` /// /// @return index into `finalizationCheckpoints` that the `_requestId` belongs to. - function findCheckpointIndex(uint256 _requestId, uint256 _start, uint256 _end) public view returns (uint256) { + function findCheckpointIndex(uint32 _requestId, uint32 _start, uint32 _end) public view returns (uint32) { require(_requestId <= lastFinalizedRequestId, "Request is not finalized"); require(_start <= _end, "Invalid range"); require(_end < finalizationCheckpoints.length, "End index out of bounds of finalizationCheckpoints"); // Binary search - uint256 min = _start; - uint256 max = _end; + uint32 min = _start; + uint32 max = _end; while (max > min) { - uint256 mid = (max + min + 1) / 2; + uint32 mid = (max + min + 1) / 2; if (finalizationCheckpoints[mid].lastFinalizedRequestId < _requestId) { // if mid index in the array is less than the request id, we need to move up the array, as the index we are looking for has // a `finalizationCheckpoints[mid].lastFinalizedRequestId` greater than the request id - - min = mid; } else { // by getting here, we have a `finalizationCheckpoints[mid].lastFinalizedRequestId` greater than the request id @@ -314,11 +248,12 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad } } - // Add a getter function for the length of the array - function getFinalizationCheckpointsLength() public view returns (uint256) { - return finalizationCheckpoints.length; - } - + /// @notice The excess eETH balance of this contract beyond what is needed to fulfill withdrawal requests. + /// This excess accumulates due to: + /// - eETH requested for withdrawal accruing staking rewards until the withdrawal is finalized. + /// Any remaining positive rebase rewards stay in the contract after finalization + /// - eETH balance calculation includes integer division, and there is a common case when the whole eETH + /// balance can't be transferred from the account while leaving the last 1-2 wei on the sender's account function getAccumulatedDustEEthAmount() public view returns (uint256) { uint256 amountRequestedWithdraw = calculateTotalPendingAmount(nextRequestId - 1); @@ -327,12 +262,96 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad return contractEEthBalance - amountRequestedWithdraw; } - function withdrawAccumulatedDustEEth(address _recipient) external { - if (!roleRegistry.hasRole(WITHDRAW_NFT_ADMIN_ROLE, msg.sender)) revert IncorrectRole(); + /// @notice The amount of eETH that needed to fulfill the pending withdrawal requests up to and including `lastRequestId` + function calculateTotalPendingAmount(uint32 lastRequestId) public view returns (uint256) { + uint256 totalAmount = 0; + for (uint32 i = lastFinalizedRequestId + 1; i <= lastRequestId; i++) { - uint256 dust = getAccumulatedDustEEthAmount(); + IWithdrawRequestNFT.WithdrawRequest memory request = _requests[i]; + uint256 amountForShares = liquidityPool.amountForShare(request.shareOfEEth); + uint256 amount = _min(request.amountOfEEth, amountForShares); - // the dust amount is monotonically increasing, so we can just transfer the whole amount - eETH.transfer(_recipient, dust); + totalAmount += amount; + } + return totalAmount; + } + + function getFinalizationCheckpoint(uint32 checkpointId) external view returns (FinalizationCheckpoint memory) { + return finalizationCheckpoints[checkpointId]; + } + + function getFinalizationCheckpointsLength() public view returns (uint32) { + return uint32(finalizationCheckpoints.length); + } + + function getRequest(uint32 requestId) external view returns (IWithdrawRequestNFT.WithdrawRequest memory) { + return _requests[requestId]; + } + + + function isFinalized(uint32 requestId) public view returns (bool) { + return requestId <= lastFinalizedRequestId; + } + + function isValid(uint32 requestId) public view returns (bool) { + require(_exists(requestId), "Request does not exist"); + return _requests[requestId].isValid; + } + + function getImplementation() external view returns (address) { + return _getImplementation(); + } + + //-------------------------------------------------------------------------------------- + //------------------------------ INTERNAL FUNCTIONS ---------------------------------- + //-------------------------------------------------------------------------------------- + + function _claimWithdraw(uint32 requestId, address recipient, uint32 checkpointIndex) internal { + + require(ownerOf(requestId) == msg.sender, "Not the owner of the NFT"); + IWithdrawRequestNFT.WithdrawRequest memory request = _requests[requestId]; + require(request.isValid, "Request is not valid"); + + uint256 amountToWithdraw = getClaimableAmount(requestId, checkpointIndex); + + // transfer eth to recipient + _burn(requestId); + delete _requests[requestId]; + + _sendFund(recipient, amountToWithdraw); + + emit WithdrawRequestClaimed(requestId, amountToWithdraw, 0, recipient); + } + + // invalid NFTs is non-transferable except for the case they are being burnt by the owner via `seizeInvalidRequest` + function _beforeTokenTransfer(address /*from*/, address /*to*/, uint256 firstTokenId, uint256 batchSize) internal view override { + for (uint256 i = 0; i < batchSize; i++) { + uint256 tokenId = firstTokenId + i; + require(_requests[tokenId].isValid || msg.sender == owner(), "INVALID_REQUEST"); + } + } + + function _authorizeUpgrade(address newImplementation) internal override onlyOwner {} + + function _sendFund(address _recipient, uint256 _amount) internal { + uint256 balanace = address(this).balance; + (bool sent, ) = _recipient.call{value: _amount}(""); + require(sent && address(this).balance == balanace - _amount, "SendFail"); + } + + function _min(uint256 a, uint256 b) internal pure returns (uint256) { + return a < b ? a : b; + } + + //-------------------------------------------------------------------------------------- + //----------------------------------- MODIFIERS -------------------------------------- + //-------------------------------------------------------------------------------------- + + // This contract only accepts ETH sent from the liquidity pool + receive() external payable onlyLiquidityPool() { } + + modifier onlyLiquidityPool() { + require(msg.sender == address(liquidityPool), "Caller is not the liquidity pool"); + _; } } diff --git a/src/interfaces/IEtherFiOracle.sol b/src/interfaces/IEtherFiOracle.sol index ed40761e..de8603d5 100644 --- a/src/interfaces/IEtherFiOracle.sol +++ b/src/interfaces/IEtherFiOracle.sol @@ -15,7 +15,7 @@ interface IEtherFiOracle { uint256[] exitedValidators; uint32[] exitedValidatorsExitTimestamps; uint256[] slashedValidators; - uint256[] withdrawalRequestsToInvalidate; + uint32[] withdrawalRequestsToInvalidate; uint32 lastFinalizedWithdrawalRequestId; uint32 eEthTargetAllocationWeight; uint32 etherFanTargetAllocationWeight; @@ -60,4 +60,4 @@ interface IEtherFiOracle { function setOracleReportPeriod(uint32 _reportPeriodSlot) external; function setConsensusVersion(uint32 _consensusVersion) external; function setEtherFiAdmin(address _etherFiAdminAddress) external; -} \ No newline at end of file +} diff --git a/src/interfaces/ILiquidityPool.sol b/src/interfaces/ILiquidityPool.sol index ff25bfaf..6693005e 100644 --- a/src/interfaces/ILiquidityPool.sol +++ b/src/interfaces/ILiquidityPool.sol @@ -56,8 +56,8 @@ interface ILiquidityPool { function deposit(address _user, address _referral) external payable returns (uint256); function depositToRecipient(address _recipient, uint256 _amount, address _referral) external returns (uint256); function withdraw(address _recipient, uint256 _amount) external returns (uint256); - function requestWithdraw(address recipient, uint256 amount) external returns (uint256); - function requestWithdrawWithPermit(address _owner, uint256 _amount, PermitInput calldata _permit) external returns (uint256); + function requestWithdraw(address recipient, uint256 amount) external returns (uint32); + function requestWithdrawWithPermit(address _owner, uint256 _amount, PermitInput calldata _permit) external returns (uint32); function requestMembershipNFTWithdraw(address recipient, uint256 amount, uint256 fee) external returns (uint256); function batchDeposit(uint256[] calldata _candidateBidIds, uint256 _numberOfValidators) external payable returns (uint256[] memory); @@ -69,6 +69,4 @@ interface ILiquidityPool { function rebase(int128 _accruedRewards) external; function payProtocolFees(uint128 _protocolFees) external; - function addEthAmountLockedForWithdrawal(uint128 _amount) external; - function reduceEthAmountLockedForWithdrawal(uint128 _amount) external; } diff --git a/src/interfaces/IWithdrawRequestNFT.sol b/src/interfaces/IWithdrawRequestNFT.sol index 9d0d5605..96a3fd07 100644 --- a/src/interfaces/IWithdrawRequestNFT.sol +++ b/src/interfaces/IWithdrawRequestNFT.sol @@ -15,13 +15,12 @@ interface IWithdrawRequestNFT { } function initialize(address _liquidityPoolAddress, address _eEthAddress, address _membershipManager) external; - function requestWithdraw(uint96 amountOfEEth, uint96 shareOfEEth, address requester) external payable returns (uint256); - function claimWithdraw(uint256 requestId, uint256 checkpointIndex) external; + function requestWithdraw(uint96 amountOfEEth, uint96 shareOfEEth, address requester) external payable returns (uint32); + function claimWithdraw(uint32 requestId, uint32 checkpointIndex) external; - function getRequest(uint256 requestId) external view returns (WithdrawRequest memory); - function isFinalized(uint256 requestId) external view returns (bool); + function getRequest(uint32 requestId) external view returns (WithdrawRequest memory); + function isFinalized(uint32 requestId) external view returns (bool); - function invalidateRequest(uint256 requestId) external; - function finalizeRequests(uint256 upperBound) external; - function finalizeRequests(uint256 lastRequestId, uint128 totalAmount) external; + function invalidateRequest(uint32 requestId) external; + function finalizeRequests(uint32 lastRequestId) external; } diff --git a/test/EtherFiAdminUpgrade.t.sol b/test/EtherFiAdminUpgrade.t.sol index 056e74bc..63d2db6a 100644 --- a/test/EtherFiAdminUpgrade.t.sol +++ b/test/EtherFiAdminUpgrade.t.sol @@ -74,7 +74,7 @@ contract EtherFiAdminUpgradeTest is TestSetup { exitedValidators: new uint256[](0), exitedValidatorsExitTimestamps: new uint32[](0), slashedValidators: new uint256[](0), - withdrawalRequestsToInvalidate: new uint256[](0), + withdrawalRequestsToInvalidate: new uint32[](0), lastFinalizedWithdrawalRequestId: 21696, eEthTargetAllocationWeight: 0, etherFanTargetAllocationWeight: 0, @@ -230,7 +230,7 @@ contract EtherFiAdminUpgradeTest is TestSetup { uint256 postTotalPooledEth = liquidityPoolInstance.getTotalPooledEther(); uint256 boost = membershipManagerV1Instance.fanBoostThresholdEthAmount(); - assert(preTotalPooledEth + accruedRewards + boost == postTotalPooledEth); + assertEq(preTotalPooledEth + accruedRewards + boost, postTotalPooledEth + report.finalizedWithdrawalAmount); } //0xab30d861d075d595fdff4dc100568722047230ceea4916e4d7eceff3804c50c4 admin diff --git a/test/LiquidityPool.t.sol b/test/LiquidityPool.t.sol index b6c5d834..3889e6be 100644 --- a/test/LiquidityPool.t.sol +++ b/test/LiquidityPool.t.sol @@ -138,7 +138,7 @@ contract LiquidityPoolTest is TestSetup { uint256 aliceNonce = eETHInstance.nonces(alice); // alice priv key = 2 ILiquidityPool.PermitInput memory permitInputAlice = createPermitInput(2, address(liquidityPoolInstance), 2 ether, aliceNonce, 2**256 - 1, eETHInstance.DOMAIN_SEPARATOR()); - uint256 aliceReqId = liquidityPoolInstance.requestWithdrawWithPermit(alice, 2 ether, permitInputAlice); + uint32 aliceReqId = liquidityPoolInstance.requestWithdrawWithPermit(alice, 2 ether, permitInputAlice); vm.stopPrank(); _finalizeWithdrawalRequest(aliceReqId); @@ -153,7 +153,7 @@ contract LiquidityPoolTest is TestSetup { uint256 bobNonce = eETHInstance.nonces(bob); // bob priv key = 3 ILiquidityPool.PermitInput memory permitInputBob = createPermitInput(3, address(liquidityPoolInstance), 2 ether, bobNonce, 2**256 - 1, eETHInstance.DOMAIN_SEPARATOR()); - uint256 bobReqId = liquidityPoolInstance.requestWithdrawWithPermit(bob, 2 ether, permitInputBob); + uint32 bobReqId = liquidityPoolInstance.requestWithdrawWithPermit(bob, 2 ether, permitInputBob); vm.stopPrank(); _finalizeWithdrawalRequest(bobReqId); @@ -751,7 +751,7 @@ contract LiquidityPoolTest is TestSetup { vm.startPrank(bob); eETHInstance.approve(address(liquidityPoolInstance), eEthTVL); - uint256 bobRequestId = liquidityPoolInstance.requestWithdraw(bob, eEthTVL); + uint32 bobRequestId = liquidityPoolInstance.requestWithdraw(bob, eEthTVL); vm.stopPrank(); _finalizeWithdrawalRequest(bobRequestId); diff --git a/test/MembershipManager.t.sol b/test/MembershipManager.t.sol index e0edfe1e..95aa263b 100644 --- a/test/MembershipManager.t.sol +++ b/test/MembershipManager.t.sol @@ -65,10 +65,10 @@ contract MembershipManagerTest is TestSetup { membershipManagerV1Instance.requestWithdrawAndBurn(aliceToken); // Bob burns the NFT extracting remaining value - uint256 bobTokenId = membershipManagerV1Instance.requestWithdrawAndBurn(bobToken); + uint32 bobTokenId = uint32(membershipManagerV1Instance.requestWithdrawAndBurn(bobToken)); vm.stopPrank(); - _finalizeWithdrawalRequest(bobTokenId); + _finalizeWithdrawalRequest(uint32(bobTokenId)); vm.prank(bob); withdrawRequestNFTInstance.claimWithdraw(bobTokenId, 1); @@ -90,18 +90,18 @@ contract MembershipManagerTest is TestSetup { uint256 bobToken1 = membershipManagerV1Instance.wrapEth{value: 10 ether}(10 ether, 0); uint256 bobToken2 = membershipManagerV1Instance.wrapEth{value: 10 ether}(10 ether, 0); - uint256 requestId1 = membershipManagerV1Instance.requestWithdrawAndBurn(bobToken1); - uint256 requestId2 = membershipManagerV1Instance.requestWithdrawAndBurn(bobToken2); + uint32 requestId1 = uint32(membershipManagerV1Instance.requestWithdrawAndBurn(bobToken1)); + uint32 requestId2 = uint32(membershipManagerV1Instance.requestWithdrawAndBurn(bobToken2)); vm.stopPrank(); _finalizeWithdrawalRequest(requestId1); _finalizeWithdrawalRequest(requestId2); vm.startPrank(bob); - uint256[] memory requestIds = new uint256[](2); + uint32[] memory requestIds = new uint32[](2); requestIds[0] = requestId1; requestIds[1] = requestId2; - uint256[] memory requestIdCheckpoints = new uint256[](2); + uint32[] memory requestIdCheckpoints = new uint32[](2); requestIdCheckpoints[0] = 1; requestIdCheckpoints[1] = 2; withdrawRequestNFTInstance.batchClaimWithdraw(requestIds, requestIdCheckpoints); @@ -138,7 +138,7 @@ contract MembershipManagerTest is TestSetup { assertEq(membershipNftInstance.tierPointsOf(tokenId), 24); // Alice's NFT unwraps 1 membership points to 1 ETH - uint256 aliceRequestId1 = membershipManagerV1Instance.requestWithdraw(tokenId, 1 ether); + uint32 aliceRequestId1 = uint32(membershipManagerV1Instance.requestWithdraw(tokenId, 1 ether)); vm.stopPrank(); _finalizeWithdrawalRequest(aliceRequestId1); @@ -160,7 +160,7 @@ contract MembershipManagerTest is TestSetup { assertEq(membershipNftInstance.tierPointsOf(tokenId), 24 * 2); // Alice's NFT unwraps all her remaining membership points, burning the NFT - uint256 aliceRequestId2 = membershipManagerV1Instance.requestWithdrawAndBurn(tokenId); + uint32 aliceRequestId2 = uint32(membershipManagerV1Instance.requestWithdrawAndBurn(tokenId)); vm.stopPrank(); _finalizeWithdrawalRequest(aliceRequestId2); @@ -531,7 +531,7 @@ contract MembershipManagerTest is TestSetup { assertEq(membershipNftInstance.valueOf(aliceToken), 2 ether); // Alice burns membership points directly for ETH - uint256 requestId = membershipManagerV1Instance.requestWithdraw(aliceToken, 1 ether); + uint32 requestId = uint32(membershipManagerV1Instance.requestWithdraw(aliceToken, 1 ether)); vm.stopPrank(); _finalizeWithdrawalRequest(requestId); @@ -1017,12 +1017,12 @@ contract MembershipManagerTest is TestSetup { counts[2]++; } if (random % 3 == 0 && i % 4 != 0) { - uint256 requestId = membershipManagerV1Instance.requestWithdraw(token, withdrawalAmount); + uint32 requestId = uint32(membershipManagerV1Instance.requestWithdraw(token, withdrawalAmount)); vm.stopPrank(); _finalizeWithdrawalRequest(requestId); - uint256 requestCheckpointIndex = withdrawRequestNFTInstance.findCheckpointIndex(requestId, 0, withdrawRequestNFTInstance.getFinalizationCheckpointsLength() - 1); + uint32 requestCheckpointIndex = withdrawRequestNFTInstance.findCheckpointIndex(requestId, 0, withdrawRequestNFTInstance.getFinalizationCheckpointsLength() - 1); vm.startPrank(actor); withdrawRequestNFTInstance.claimWithdraw(requestId, requestCheckpointIndex); counts[3]++; @@ -1041,11 +1041,11 @@ contract MembershipManagerTest is TestSetup { uint256 expectedBalanceAfterWithdrawal = address(actor).balance + tokenValue; vm.prank(actor); - uint256 requestId = membershipManagerV1Instance.requestWithdrawAndBurn(token); + uint32 requestId = uint32(membershipManagerV1Instance.requestWithdrawAndBurn(token)); _finalizeWithdrawalRequest(requestId); - uint256 requestCheckpointIndex = withdrawRequestNFTInstance.findCheckpointIndex(requestId, 0, withdrawRequestNFTInstance.getFinalizationCheckpointsLength() - 1); + uint32 requestCheckpointIndex = withdrawRequestNFTInstance.findCheckpointIndex(requestId, 0, withdrawRequestNFTInstance.getFinalizationCheckpointsLength() - 1); vm.prank(actor); withdrawRequestNFTInstance.claimWithdraw(requestId, requestCheckpointIndex); @@ -1340,14 +1340,14 @@ contract MembershipManagerTest is TestSetup { // Alice burns one NFT paying for the burn fee assertEq(membershipManagerV1Instance.hasMetBurnFeeWaiverPeriod(aliceToken1), false); - uint256 reqId1 = membershipManagerV1Instance.requestWithdrawAndBurn(aliceToken1); + uint32 reqId1 = uint32(membershipManagerV1Instance.requestWithdrawAndBurn(aliceToken1)); // 16 days passed vm.warp(block.timestamp + uint256(16 * 24 * 60 * 60)); // Alice burns the other NFT not paying for the burn fee since the stkaing period passed 30 days assertEq(membershipManagerV1Instance.hasMetBurnFeeWaiverPeriod(aliceToken2), true); - uint256 reqId2 = membershipManagerV1Instance.requestWithdrawAndBurn(aliceToken2); + uint32 reqId2 = uint32(membershipManagerV1Instance.requestWithdrawAndBurn(aliceToken2)); assertEq(withdrawRequestNFTInstance.getRequest(reqId1).amountOfEEth, 1 ether); // burn fees are deprecated @@ -1368,7 +1368,7 @@ contract MembershipManagerTest is TestSetup { assertEq(eETHInstance.balanceOf(alice), 0 ether); // Alice mints an NFT with 1 ETH - uint256 aliceToken = membershipManagerV1Instance.wrapEth{value: 1 ether}(1 ether, 0 ether); + uint32 aliceToken = uint32(membershipManagerV1Instance.wrapEth{value: 1 ether}(1 ether, 0 ether)); assertEq(alice.balance, 0 ether); assertEq(address(liquidityPoolInstance).balance, 1 ether); diff --git a/test/MembershipManagerV0.t.sol b/test/MembershipManagerV0.t.sol index 52d2967f..75d45c8e 100644 --- a/test/MembershipManagerV0.t.sol +++ b/test/MembershipManagerV0.t.sol @@ -75,7 +75,7 @@ contract MembershipManagerV0Test is TestSetup { membershipManagerInstance.requestWithdrawAndBurn(aliceToken); // Bob burns the NFT extracting remaining value - uint256 bobTokenId = membershipManagerInstance.requestWithdrawAndBurn(bobToken); + uint32 bobTokenId = uint32(membershipManagerInstance.requestWithdrawAndBurn(bobToken)); vm.stopPrank(); _finalizeWithdrawalRequest(bobTokenId); @@ -111,7 +111,7 @@ contract MembershipManagerV0Test is TestSetup { assertEq(membershipNftInstance.tierPointsOf(tokenId), 24); // Alice's NFT unwraps 1 membership points to 1 ETH - uint256 aliceRequestId1 = membershipManagerInstance.requestWithdraw(tokenId, 1 ether); + uint32 aliceRequestId1 = uint32(membershipManagerInstance.requestWithdraw(tokenId, 1 ether)); vm.stopPrank(); _finalizeWithdrawalRequest(aliceRequestId1); @@ -133,7 +133,7 @@ contract MembershipManagerV0Test is TestSetup { assertEq(membershipNftInstance.tierPointsOf(tokenId), 24 * 2); // Alice's NFT unwraps all her remaining membership points, burning the NFT - uint256 aliceRequestId2 = membershipManagerInstance.requestWithdrawAndBurn(tokenId); + uint32 aliceRequestId2 = uint32(membershipManagerInstance.requestWithdrawAndBurn(tokenId)); vm.stopPrank(); _finalizeWithdrawalRequest(aliceRequestId2); @@ -868,7 +868,7 @@ contract MembershipManagerV0Test is TestSetup { vm.startPrank(alice); uint256 aliceToken1 = membershipManagerInstance.wrapEth{value: 50 ether}(50 ether, 0 ether); uint256 aliceToken2 = membershipManagerInstance.wrapEth{value: 50 ether}(50 ether, 0 ether); - uint256 reqId; + uint32 reqId; skip(1 days); @@ -919,7 +919,7 @@ contract MembershipManagerV0Test is TestSetup { vm.startPrank(alice); // Alice requests to withdraw 10 ETH - reqId = membershipManagerV1Instance.requestWithdraw(aliceToken1, 10 ether); + reqId = uint32(membershipManagerV1Instance.requestWithdraw(aliceToken1, 10 ether)); assertEq(membershipNftInstance.valueOf(aliceToken1), 190 ether); assertEq(membershipNftInstance.loyaltyPointsOf(aliceToken1), 50 * kwei); assertEq(membershipNftInstance.tierPointsOf(aliceToken1), 0); @@ -931,7 +931,7 @@ contract MembershipManagerV0Test is TestSetup { withdrawRequestNFTInstance.claimWithdraw(reqId, 1); assertEq(address(alice).balance, 10 ether); - reqId = membershipManagerV1Instance.requestWithdrawAndBurn(aliceToken1); + reqId = uint32(membershipManagerV1Instance.requestWithdrawAndBurn(aliceToken1)); assertEq(membershipNftInstance.valueOf(aliceToken1), 0 ether); vm.stopPrank(); @@ -941,7 +941,7 @@ contract MembershipManagerV0Test is TestSetup { withdrawRequestNFTInstance.claimWithdraw(reqId, 2); assertEq(address(alice).balance, 200 ether); - reqId = membershipManagerV1Instance.requestWithdrawAndBurn(aliceToken2); + reqId = uint32(membershipManagerV1Instance.requestWithdrawAndBurn(aliceToken2)); assertEq(membershipNftInstance.valueOf(aliceToken2), 0 ether); vm.stopPrank(); diff --git a/test/TestSetup.sol b/test/TestSetup.sol index 956ebe3b..4268989b 100644 --- a/test/TestSetup.sol +++ b/test/TestSetup.sol @@ -814,7 +814,7 @@ contract TestSetup is Test { uint256[] memory exitedValidators = new uint256[](0); uint32[] memory exitTimestamps = new uint32[](0); uint256[] memory slashedValidators = new uint256[](0); - uint256[] memory withdrawalRequestsToInvalidate = new uint256[](0); + uint32[] memory withdrawalRequestsToInvalidate = new uint32[](0); reportAtPeriod2A = IEtherFiOracle.OracleReport(1, 0, 1024 - 1, 0, 1024 - 1, 1, 0,validatorsToApprove, validatorsToExit, exitedValidators, exitTimestamps, slashedValidators, withdrawalRequestsToInvalidate, 1, 80, 20, 0, 0); reportAtPeriod2B = IEtherFiOracle.OracleReport(1, 0, 1024 - 1, 0, 1024 - 1, 1, 0,validatorsToApprove, validatorsToExit, exitedValidators, exitTimestamps, slashedValidators, withdrawalRequestsToInvalidate, 1, 81, 19, 0, 0); reportAtPeriod2C = IEtherFiOracle.OracleReport(2, 0, 1024 - 1, 0, 1024 - 1, 1, 0, validatorsToApprove, validatorsToExit, exitedValidators, exitTimestamps, slashedValidators, withdrawalRequestsToInvalidate, 1, 79, 21, 0, 0); @@ -1019,7 +1019,7 @@ contract TestSetup is Test { uint256[] memory emptyVals = new uint256[](0); uint32[] memory emptyVals32 = new uint32[](0); uint32 consensusVersion = etherFiOracleInstance.consensusVersion(); - report = IEtherFiOracle.OracleReport(consensusVersion, 0, 0, 0, 0, 0, 0, emptyVals, emptyVals, emptyVals, emptyVals32, emptyVals, emptyVals, 0, 0, 0, 0, 0); + report = IEtherFiOracle.OracleReport(consensusVersion, 0, 0, 0, 0, 0, 0, emptyVals, emptyVals, emptyVals, emptyVals32, emptyVals, emptyVals32, 0, 0, 0, 0, 0); } function calculatePermitDigest(address _owner, address spender, uint256 value, uint256 nonce, uint256 deadline, bytes32 domainSeparator) public pure returns (bytes32) { @@ -1231,7 +1231,7 @@ contract TestSetup is Test { return newValidators; } - function _finalizeWithdrawalRequest(uint256 _requestId) internal { + function _finalizeWithdrawalRequest(uint32 _requestId) internal { vm.startPrank(alice); withdrawRequestNFTInstance.finalizeRequests(_requestId); uint128 amount = withdrawRequestNFTInstance.getRequest(_requestId).amountOfEEth; diff --git a/test/WithdrawRequestNFT.t.sol b/test/WithdrawRequestNFT.t.sol index bae52a01..d61fdc99 100644 --- a/test/WithdrawRequestNFT.t.sol +++ b/test/WithdrawRequestNFT.t.sol @@ -31,7 +31,7 @@ contract WithdrawRequestNFTTest is TestSetup { eETHInstance.approve(address(liquidityPoolInstance), amountOfEEth); vm.prank(bob); - uint256 requestId = liquidityPoolInstance.requestWithdraw(bob, amountOfEEth); + uint32 requestId = liquidityPoolInstance.requestWithdraw(bob, amountOfEEth); WithdrawRequestNFT.WithdrawRequest memory request = withdrawRequestNFTInstance.getRequest(requestId); @@ -53,7 +53,7 @@ contract WithdrawRequestNFTTest is TestSetup { eETHInstance.approve(address(liquidityPoolInstance), amountOfEEth); vm.prank(bob); - uint256 requestId1 = liquidityPoolInstance.requestWithdraw(bob, amountOfEEth); + uint32 requestId1 = liquidityPoolInstance.requestWithdraw(bob, amountOfEEth); assertEq(requestId1, 1, "Request id should be 1"); @@ -61,7 +61,7 @@ contract WithdrawRequestNFTTest is TestSetup { eETHInstance.approve(address(liquidityPoolInstance), amountOfEEth); vm.prank(bob); - uint256 requestId2 = liquidityPoolInstance.requestWithdraw(bob, amountOfEEth); + uint32 requestId2 = liquidityPoolInstance.requestWithdraw(bob, amountOfEEth); assertEq(requestId2, 2, "Request id should be 2"); } @@ -79,7 +79,7 @@ contract WithdrawRequestNFTTest is TestSetup { eETHInstance.approve(address(liquidityPoolInstance), amountOfEEth); vm.prank(bob); - uint256 requestId = liquidityPoolInstance.requestWithdraw(bob, amountOfEEth); + uint32 requestId = liquidityPoolInstance.requestWithdraw(bob, amountOfEEth); bool earlyRequestIsFinalized = withdrawRequestNFTInstance.isFinalized(requestId); assertFalse(earlyRequestIsFinalized, "Request should not be Finalized"); @@ -106,7 +106,7 @@ contract WithdrawRequestNFTTest is TestSetup { eETHInstance.approve(address(liquidityPoolInstance), amountOfEEth); vm.prank(bob); - uint256 requestId = liquidityPoolInstance.requestWithdraw(bob, amountOfEEth); + uint32 requestId = liquidityPoolInstance.requestWithdraw(bob, amountOfEEth); bool requestIsFinalized = withdrawRequestNFTInstance.isFinalized(requestId); assertFalse(requestIsFinalized, "Request should not be finalized"); @@ -130,7 +130,7 @@ contract WithdrawRequestNFTTest is TestSetup { eETHInstance.approve(address(liquidityPoolInstance), amountOfEEth); vm.prank(bob); - uint256 requestId = liquidityPoolInstance.requestWithdraw(bob, amountOfEEth); + uint32 requestId = liquidityPoolInstance.requestWithdraw(bob, amountOfEEth); bool requestIsFinalized = withdrawRequestNFTInstance.isFinalized(requestId); assertFalse(requestIsFinalized, "Request should not be finalized"); @@ -153,7 +153,7 @@ contract WithdrawRequestNFTTest is TestSetup { eETHInstance.approve(address(liquidityPoolInstance), amountOfEEth); vm.prank(bob); - uint256 requestId = liquidityPoolInstance.requestWithdraw(bob, amountOfEEth); + uint32 requestId = liquidityPoolInstance.requestWithdraw(bob, amountOfEEth); vm.expectRevert("Not the owner of the NFT"); vm.prank(alice); @@ -177,7 +177,7 @@ contract WithdrawRequestNFTTest is TestSetup { eETHInstance.approve(address(liquidityPoolInstance), 1 ether); vm.prank(bob); - uint256 requestId = liquidityPoolInstance.requestWithdraw(bob, 1 ether); + uint32 requestId = liquidityPoolInstance.requestWithdraw(bob, 1 ether); assertEq(withdrawRequestNFTInstance.getAccumulatedDustEEthAmount(), 0, "Accumulated dust should be 0"); assertEq(eETHInstance.balanceOf(bob), 9 ether); @@ -228,7 +228,7 @@ contract WithdrawRequestNFTTest is TestSetup { eETHInstance.approve(address(liquidityPoolInstance), 1 ether); vm.prank(bob); - uint256 requestId = liquidityPoolInstance.requestWithdraw(bob, 1 ether); + uint32 requestId = liquidityPoolInstance.requestWithdraw(bob, 1 ether); vm.prank(address(membershipManagerInstance)); liquidityPoolInstance.rebase(-35 ether); @@ -255,7 +255,7 @@ contract WithdrawRequestNFTTest is TestSetup { // bob requests withdrawal vm.prank(bob); - uint256 requestId = liquidityPoolInstance.requestWithdraw(bob, 60 ether); + uint32 requestId = liquidityPoolInstance.requestWithdraw(bob, 60 ether); // Somehow, LP gets some ETH // For example, alice deposits 100 ETH :D @@ -295,7 +295,7 @@ contract WithdrawRequestNFTTest is TestSetup { vm.prank(bob); // Withdraw request for 9 wei eETH amount (= 8.82 wei eETH share) // 8 wei eETH share is transfered to `withdrawRequestNFT` contract - uint256 requestId = liquidityPoolInstance.requestWithdraw(bob, 9); + uint32 requestId = liquidityPoolInstance.requestWithdraw(bob, 9); assertEq(eETHInstance.balanceOf(address(withdrawRequestNFTInstance)), 8); // Within `LP.requestWithdraw` // - `share` is calculated by `sharesForAmount` as (9 * 98) / 100 = 8.82 ---> (rounded down to) 8 @@ -327,7 +327,7 @@ contract WithdrawRequestNFTTest is TestSetup { // It depicts the scenario where bob's WithdrawalRequest NFT is stolen by alice. // The owner invalidates the request - function test_InvalidatedRequestNft_after_finalization() public returns (uint256 requestId) { + function test_InvalidatedRequestNft_after_finalization() public returns (uint32 requestId) { startHoax(bob); liquidityPoolInstance.deposit{value: 10 ether}(); vm.stopPrank(); @@ -353,7 +353,7 @@ contract WithdrawRequestNFTTest is TestSetup { withdrawRequestNFTInstance.invalidateRequest(requestId); } - function test_InvalidatedRequestNft_before_finalization() public returns (uint256 requestId) { + function test_InvalidatedRequestNft_before_finalization() public returns (uint32 requestId) { startHoax(bob); liquidityPoolInstance.deposit{value: 10 ether}(); vm.stopPrank(); @@ -378,7 +378,7 @@ contract WithdrawRequestNFTTest is TestSetup { } function test_InvalidatedRequestNft_NonTransferrable() public { - uint256 requestId = test_InvalidatedRequestNft_after_finalization(); + uint32 requestId = test_InvalidatedRequestNft_after_finalization(); vm.prank(alice); vm.expectRevert("INVALID_REQUEST"); @@ -386,7 +386,7 @@ contract WithdrawRequestNFTTest is TestSetup { } function test_seizeInvalidAndMintNew_revert_if_not_owner() public { - uint256 requestId = test_InvalidatedRequestNft_after_finalization(); + uint32 requestId = test_InvalidatedRequestNft_after_finalization(); uint256 claimableAmount = withdrawRequestNFTInstance.getRequest(requestId).amountOfEEth; // REVERT if not owner @@ -396,26 +396,24 @@ contract WithdrawRequestNFTTest is TestSetup { } function test_InvalidatedRequestNft_seizeInvalidAndMintNew_1() public { - uint256 requestId = test_InvalidatedRequestNft_after_finalization(); + uint32 requestId = test_InvalidatedRequestNft_after_finalization(); uint256 claimableAmount = withdrawRequestNFTInstance.getRequest(requestId).amountOfEEth; uint256 chadBalance = address(chad).balance; vm.prank(owner); withdrawRequestNFTInstance.seizeInvalidRequest(requestId, chad, 1); - assertEq(liquidityPoolInstance.ethAmountLockedForWithdrawal(), 0, "Must be withdrawn"); assertEq(address(chad).balance, chadBalance + claimableAmount, "Chad should receive the claimable amount"); } function test_InvalidatedRequestNft_seizeInvalidAndMintNew_2() public { - uint256 requestId = test_InvalidatedRequestNft_before_finalization(); + uint32 requestId = test_InvalidatedRequestNft_before_finalization(); uint256 claimableAmount = withdrawRequestNFTInstance.getRequest(requestId).amountOfEEth; uint256 chadBalance = address(chad).balance; vm.prank(owner); withdrawRequestNFTInstance.seizeInvalidRequest(requestId, chad, 1); - assertEq(liquidityPoolInstance.ethAmountLockedForWithdrawal(), 0, "Must be withdrawn"); assertEq(address(chad).balance, chadBalance + claimableAmount, "Chad should receive the claimable amount"); } } diff --git a/test/eethPayoutUpgrade.t.sol b/test/eethPayoutUpgrade.t.sol index 657b7fe0..0cf8174b 100644 --- a/test/eethPayoutUpgrade.t.sol +++ b/test/eethPayoutUpgrade.t.sol @@ -54,7 +54,7 @@ contract eethPayoutUpgradeTest is TestSetup { exitedValidators: new uint256[](0), exitedValidatorsExitTimestamps: new uint32[](0), slashedValidators: new uint256[](0), - withdrawalRequestsToInvalidate: new uint256[](0), + withdrawalRequestsToInvalidate: new uint32[](0), lastFinalizedWithdrawalRequestId: 30403, eEthTargetAllocationWeight: 0, etherFanTargetAllocationWeight: 0, @@ -100,4 +100,4 @@ contract eethPayoutUpgradeTest is TestSetup { liquidityPoolInstance.depositToRecipient(treasury, 10 ether, address(0)); vm.stopPrank(); } -} \ No newline at end of file +} From bf9dcca765367602bc426445cf2813541a23ec62 Mon Sep 17 00:00:00 2001 From: jtfirek Date: Tue, 3 Sep 2024 19:37:01 -0500 Subject: [PATCH 4/7] additional testing + precision base for cached share value --- src/WithdrawRequestNFT.sol | 85 ++++++++++++++++----------- test/MembershipManager.t.sol | 4 +- test/TestSetup.sol | 1 - test/WithdrawRequestNFT.t.sol | 105 ++++++++++++++++++++++++++++------ 4 files changed, 143 insertions(+), 52 deletions(-) diff --git a/src/WithdrawRequestNFT.sol b/src/WithdrawRequestNFT.sol index 93788b7f..f129a8a5 100644 --- a/src/WithdrawRequestNFT.sol +++ b/src/WithdrawRequestNFT.sol @@ -11,6 +11,7 @@ import "./interfaces/IWithdrawRequestNFT.sol"; import "./interfaces/IMembershipManager.sol"; import "./RoleRegistry.sol"; +import "forge-std/console.sol"; contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgradeable, IWithdrawRequestNFT { //-------------------------------------------------------------------------------------- @@ -28,8 +29,16 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad uint32 public lastFinalizedRequestId; uint96 public DEPRECATED_accumulatedDustEEthShares; // to be burned or used to cover the validator churn cost + /// Stores the cached share price at the time of finalization for each batch + /// For a checkpoint index i, this batch includes requests: + /// `finalizationCheckpoints[i-1].lastFinalizedRequestId < requestId <= finalizationCheckpoints[i].lastFinalizedRequestId` FinalizationCheckpoint[] public finalizationCheckpoints; + /// precision base for cached share value + uint256 internal constant E27_PRECISION_BASE = 1e27; + + uint32 internal constant NOT_FOUND = 0; + RoleRegistry public roleRegistry; //-------------------------------------------------------------------------------------- @@ -76,8 +85,8 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad function initializeV2dot5(address _roleRegistry) external onlyOwner { require(address(roleRegistry) == address(0x00), "already initialized"); - // TODO: compile list of values in DEPRECATED_admins to clear out DEPRECATED_accumulatedDustEEthShares = 0; + // TODO: compile list of values in DEPRECATED_admins to clear out roleRegistry = RoleRegistry(_roleRegistry); finalizationCheckpoints.push(FinalizationCheckpoint(0, 0)); @@ -100,14 +109,14 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad } /// @notice called by the NFT owner of a finalized request to claim their ETH - /// @dev `checkpointIndex` can be found using `findCheckpointIndex` function /// @param requestId the id of the withdraw request and associated NFT - /// @param checkpointIndex the index of the `finalizationCheckpoints` that the request belongs to. + /// @param checkpointIndex the index of the `finalizationCheckpoints` that the request belongs to. + /// can be found with `findCheckpointIndex(_requestIds, 1, getLastCheckpointIndex())` function claimWithdraw(uint32 requestId, uint32 checkpointIndex) external { return _claimWithdraw(requestId, ownerOf(requestId), checkpointIndex); } - /// @notice Batch version of `claimWithdraw` + /// @notice Batch version of `claimWithdraw()` function batchClaimWithdraw(uint32[] calldata requestIds, uint32[] calldata checkpointIndices) external { for (uint32 i = 0; i < requestIds.length; i++) { _claimWithdraw(requestIds[i], ownerOf(requestIds[i]), checkpointIndices[i]); @@ -143,7 +152,7 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad if (!roleRegistry.hasRole(WITHDRAW_NFT_ADMIN_ROLE, msg.sender)) revert IncorrectRole(); uint256 totalAmount = uint256(calculateTotalPendingAmount(lastRequestId)); - uint256 cachedSharePrice = liquidityPool.amountForShare(1 ether); + uint256 cachedSharePrice = liquidityPool.amountForShare(E27_PRECISION_BASE); finalizationCheckpoints.push(FinalizationCheckpoint(uint32(lastRequestId), cachedSharePrice)); @@ -180,11 +189,11 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad /// @notice returns the value of a request after finalization. The value of a request can be: /// - nominal (when the amount of eth locked for this request are equal to the request's eETH) - /// - discounted (when the amount of eth will be lower, because the protocol share rate dropped - /// before request is finalized, so it will be equal to `request's shares` * `protocol share rate`) + /// - discounted (when the amount of eth will be lower, because the protocol share rate dropped before the + /// request is finalized, so it will be equal to `request's shares` * `protocol share rate at finalization`) /// @param requestId the id of the withdraw request NFT /// @param checkpointIndex the index of the `finalizationCheckpoints` that the request belongs to. - /// `checkpointIndex` can be found using `findCheckpointIndex` function + /// `checkpointIndex` can be found using `findCheckpointIndex()` function /// @return uint256 the amount of ETH that can be claimed by the owner of the NFT function getClaimableAmount(uint32 requestId, uint32 checkpointIndex) public view returns (uint256) { require(requestId <= lastFinalizedRequestId, "Request is not finalized"); @@ -205,7 +214,7 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad ); IWithdrawRequestNFT.WithdrawRequest memory request = _requests[requestId]; - uint256 amountForSharesCached = request.shareOfEEth * checkpoint.cachedShareValue / 1 ether; + uint256 amountForSharesCached = request.shareOfEEth * checkpoint.cachedShareValue / E27_PRECISION_BASE; uint256 amountToTransfer = _min(request.amountOfEEth, amountForSharesCached); return amountToTransfer; @@ -213,39 +222,48 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad /// @dev View function to find a finalization checkpoint to use in `claimWithdraw()` /// Search will be performed in the range of `[_start, _end]` over the `finalizationCheckpoints` array + /// Usage: findCheckpointIndex(_requestIds, 1, getLastCheckpointIndex()) /// /// @param _requestId request id to search the checkpoint for - /// @param _start index of the left boundary of the search range - /// @param _end index of the right boundary of the search range, should be less than or equal to `finalizationCheckpoints.length` + /// @param _start index of the left boundary of the search range, should be greater than 0, because list is 1-based array + /// @param _end index of the right boundary of the search range, should be less than or equal to `getLastCheckpointIndex` /// - /// @return index into `finalizationCheckpoints` that the `_requestId` belongs to. + /// @return index into `finalizationCheckpoints` that the `_requestId` belongs to or 0 if not found function findCheckpointIndex(uint32 _requestId, uint32 _start, uint32 _end) public view returns (uint32) { require(_requestId <= lastFinalizedRequestId, "Request is not finalized"); require(_start <= _end, "Invalid range"); - require(_end < finalizationCheckpoints.length, "End index out of bounds of finalizationCheckpoints"); - - // Binary search - uint32 min = _start; - uint32 max = _end; - - while (max > min) { - uint32 mid = (max + min + 1) / 2; - if (finalizationCheckpoints[mid].lastFinalizedRequestId < _requestId) { + require (_start != 0 && _end <= getLastCheckpointIndex(), "Range is out of bounds"); - // if mid index in the array is less than the request id, we need to move up the array, as the index we are looking for has - // a `finalizationCheckpoints[mid].lastFinalizedRequestId` greater than the request id - min = mid; + // Left boundary + if (_requestId <= finalizationCheckpoints[_start].lastFinalizedRequestId) { + // either fits in at the left boundary or is out of this range + if (_requestId > finalizationCheckpoints[_start - 1].lastFinalizedRequestId) { + return _start; } else { - // by getting here, we have a `finalizationCheckpoints[mid].lastFinalizedRequestId` greater than the request id - // to know we have found the right index, finalizationCheckpoints[mid - 1].lastFinalizedRequestId` must be less than the request id + return NOT_FOUND; + } + } - if (finalizationCheckpoints[mid - 1].lastFinalizedRequestId < _requestId) { - return mid; - } + // Right boundary + if (_requestId > finalizationCheckpoints[_end].lastFinalizedRequestId) { + return NOT_FOUND; + } - max = mid - 1; // there are values to the left of mid that are greater than the request id + // Binary search + uint256 min = _start + 1; + uint256 max = _end; + + while (min < max) { + uint256 mid = (max + min) / 2; + if (_requestId > finalizationCheckpoints[mid].lastFinalizedRequestId) { + // Request was finalized after the batch at mid + min = mid + 1; + } else { + // Request was finalized in or before the batch at mid + max = mid; } } + return uint32(max); } /// @notice The excess eETH balance of this contract beyond what is needed to fulfill withdrawal requests. @@ -276,13 +294,14 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad return totalAmount; } + function getLastCheckpointIndex() public view returns (uint32) { + return uint32(finalizationCheckpoints.length) - 1; + } + function getFinalizationCheckpoint(uint32 checkpointId) external view returns (FinalizationCheckpoint memory) { return finalizationCheckpoints[checkpointId]; } - function getFinalizationCheckpointsLength() public view returns (uint32) { - return uint32(finalizationCheckpoints.length); - } function getRequest(uint32 requestId) external view returns (IWithdrawRequestNFT.WithdrawRequest memory) { return _requests[requestId]; diff --git a/test/MembershipManager.t.sol b/test/MembershipManager.t.sol index 95aa263b..ad5e3e38 100644 --- a/test/MembershipManager.t.sol +++ b/test/MembershipManager.t.sol @@ -1022,7 +1022,7 @@ contract MembershipManagerTest is TestSetup { _finalizeWithdrawalRequest(requestId); - uint32 requestCheckpointIndex = withdrawRequestNFTInstance.findCheckpointIndex(requestId, 0, withdrawRequestNFTInstance.getFinalizationCheckpointsLength() - 1); + uint32 requestCheckpointIndex = withdrawRequestNFTInstance.findCheckpointIndex(requestId, 1, withdrawRequestNFTInstance.getLastCheckpointIndex()); vm.startPrank(actor); withdrawRequestNFTInstance.claimWithdraw(requestId, requestCheckpointIndex); counts[3]++; @@ -1045,7 +1045,7 @@ contract MembershipManagerTest is TestSetup { _finalizeWithdrawalRequest(requestId); - uint32 requestCheckpointIndex = withdrawRequestNFTInstance.findCheckpointIndex(requestId, 0, withdrawRequestNFTInstance.getFinalizationCheckpointsLength() - 1); + uint32 requestCheckpointIndex = withdrawRequestNFTInstance.findCheckpointIndex(requestId, 1, withdrawRequestNFTInstance.getLastCheckpointIndex()); vm.prank(actor); withdrawRequestNFTInstance.claimWithdraw(requestId, requestCheckpointIndex); diff --git a/test/TestSetup.sol b/test/TestSetup.sol index 4268989b..15ade65f 100644 --- a/test/TestSetup.sol +++ b/test/TestSetup.sol @@ -258,7 +258,6 @@ contract TestSetup is Test { uint256 delay; } - // initialize a fork in which fresh contracts are deployed // and initialized to the same state as the unit tests. function initializeTestingFork(uint8 forkEnum) public { diff --git a/test/WithdrawRequestNFT.t.sol b/test/WithdrawRequestNFT.t.sol index d61fdc99..b38dfa63 100644 --- a/test/WithdrawRequestNFT.t.sol +++ b/test/WithdrawRequestNFT.t.sol @@ -8,6 +8,8 @@ import "./TestSetup.sol"; contract WithdrawRequestNFTTest is TestSetup { + address[] public users; + function setUp() public { setUpTests(); } @@ -212,38 +214,48 @@ contract WithdrawRequestNFTTest is TestSetup { function test_ValidClaimWithdrawWithNegativeRebase() public { launch_validator(); - + startHoax(bob); - liquidityPoolInstance.deposit{value: 10 ether}(); + liquidityPoolInstance.deposit{value: 11 ether}(); vm.stopPrank(); uint256 bobsStartingBalance = address(bob).balance; - assertEq(liquidityPoolInstance.getTotalPooledEther(), 10 ether + 60 ether); + // 71 eth in the protocol, but 1 will be removed by the finalization before the rebase + assertEq(liquidityPoolInstance.getTotalPooledEther(), 11 ether + 60 ether); // Case 2. - // After the rebase with negative rewards (loss of 35 eth among 70 eth), - // the withdrawal amount is reduced from 1 ether to 0.5 ether - vm.prank(bob); - eETHInstance.approve(address(liquidityPoolInstance), 1 ether); + // After the rebase with negative rewards + // - withdrawal finalized before the rebase should be processed as usual + // - withdrawal finalized after the rebase is reduced from 1 ether to 0.5 ether (loss of 35 eth among 70 eth) + vm.startPrank(bob); + eETHInstance.approve(address(liquidityPoolInstance), 2 ether); + uint32 requestId1 = liquidityPoolInstance.requestWithdraw(bob, 1 ether); + uint32 requestId2 = liquidityPoolInstance.requestWithdraw(bob, 1 ether); + vm.stopPrank(); - vm.prank(bob); - uint32 requestId = liquidityPoolInstance.requestWithdraw(bob, 1 ether); + _finalizeWithdrawalRequest(requestId1); vm.prank(address(membershipManagerInstance)); liquidityPoolInstance.rebase(-35 ether); - assertEq(withdrawRequestNFTInstance.balanceOf(bob), 1, "Bobs balance should be 1"); - assertEq(withdrawRequestNFTInstance.ownerOf(requestId), bob, "Bobs should own the NFT"); - - _finalizeWithdrawalRequest(requestId); + assertEq(withdrawRequestNFTInstance.balanceOf(bob), 2, "Bobs balance should be 1"); + assertEq(withdrawRequestNFTInstance.ownerOf(requestId2), bob, "Bobs should own the NFT"); + uint32 requestId1Checkpoint = withdrawRequestNFTInstance.findCheckpointIndex(requestId1, 1, withdrawRequestNFTInstance.getLastCheckpointIndex()); vm.prank(bob); - withdrawRequestNFTInstance.claimWithdraw(requestId, 1); + withdrawRequestNFTInstance.claimWithdraw(requestId1, requestId1Checkpoint); + uint256 bobBalanceAfterFirstWithdraw = address(bob).balance; + assertEq(bobBalanceAfterFirstWithdraw, bobsStartingBalance + 1 ether, "Bobs balance should be 1 ether higher"); + + _finalizeWithdrawalRequest(requestId2); - uint256 bobsEndingBalance = address(bob).balance; + uint32 requestId2Checkpoint = withdrawRequestNFTInstance.findCheckpointIndex(requestId2, 1, withdrawRequestNFTInstance.getLastCheckpointIndex()); + vm.prank(bob); + withdrawRequestNFTInstance.claimWithdraw(requestId2, requestId2Checkpoint); + uint256 bobBalanceAfterSecondWithdraw = address(bob).balance; - assertEq(bobsEndingBalance, bobsStartingBalance + 0.5 ether, "Bobs balance should be 1 ether higher"); + assertEq(bobBalanceAfterSecondWithdraw, bobBalanceAfterFirstWithdraw + 0.5 ether, "Bobs balance should be 0.5 ether higher"); } function test_withdraw_with_zero_liquidity() public { @@ -416,4 +428,65 @@ contract WithdrawRequestNFTTest is TestSetup { assertEq(address(chad).balance, chadBalance + claimableAmount, "Chad should receive the claimable amount"); } + + function test_updated_checkpoint_logic() public payable { + for (uint256 i = 0; i < 100; i++) { + address user = vm.addr(i + 1); + users.push(user); + vm.deal(user, 15 ether); + } + + // first 50 users deposit + for (uint256 i = 0; i < 50; i++) { + vm.prank(users[i]); + liquidityPoolInstance.deposit{value: 1 ether}(); + } + + // first users request withdrawal + for (uint256 i = 0; i < 25; i++) { + vm.startPrank(users[i]); + eETHInstance.approve(address(liquidityPoolInstance), 1 ether); + liquidityPoolInstance.requestWithdraw(users[i], 1 ether); + vm.stopPrank(); + } + + // rebase + vm.prank(address(membershipManagerInstance)); + // eETH value doubles + liquidityPoolInstance.rebase(50 ether); + + // finalize the requests in multiple batches + _finalizeWithdrawalRequest(5); + _finalizeWithdrawalRequest(11); + _finalizeWithdrawalRequest(17); + _finalizeWithdrawalRequest(23); + _finalizeWithdrawalRequest(withdrawRequestNFTInstance.nextRequestId() - 1); + + // claim all but 1 request + for (uint32 i = 0; i < 24; i++) { + uint32 requestId = i + 1; + uint32 requestCheckpointIndex = withdrawRequestNFTInstance.findCheckpointIndex(requestId, 1, withdrawRequestNFTInstance.getLastCheckpointIndex()); + vm.prank(users[i]); + withdrawRequestNFTInstance.claimWithdraw(requestId, requestCheckpointIndex); + } + + // claim excess rewards for all requests even the unclaimed one + assertEq(withdrawRequestNFTInstance.getAccumulatedDustEEthAmount(), 25 ether); + + uint256 aliceBalanceBefore = eETHInstance.balanceOf(alice); + vm.prank(alice); + withdrawRequestNFTInstance.withdrawAccumulatedDustEEth(alice); + uint256 aliceBalanceAfter = eETHInstance.balanceOf(alice); + assertEq(aliceBalanceAfter - aliceBalanceBefore, 25 ether); + + // claim the last request + uint32 lastRequestId = 25; + uint32 lastRequestCheckpointIndex = withdrawRequestNFTInstance.findCheckpointIndex(lastRequestId, 1, withdrawRequestNFTInstance.getLastCheckpointIndex()); + vm.prank(users[24]); + withdrawRequestNFTInstance.claimWithdraw(lastRequestId, lastRequestCheckpointIndex); + + for (uint256 i = 0; i < 25; i++) { + assertEq(users[i].balance, 15 ether); + } + } } From 33de83824e2ae107b1f71a7515bb36ca6702f91c Mon Sep 17 00:00:00 2001 From: jtfirek Date: Wed, 4 Sep 2024 21:35:08 -0500 Subject: [PATCH 5/7] finalizeRequests with specific totalAmount --- src/EtherFiAdmin.sol | 2 +- src/WithdrawRequestNFT.sol | 37 ++++++++++++++++++-------- src/interfaces/IWithdrawRequestNFT.sol | 1 + test/WithdrawRequestNFT.t.sol | 8 ++---- 4 files changed, 30 insertions(+), 18 deletions(-) diff --git a/src/EtherFiAdmin.sol b/src/EtherFiAdmin.sol index d00ee870..bd407633 100644 --- a/src/EtherFiAdmin.sol +++ b/src/EtherFiAdmin.sol @@ -248,7 +248,7 @@ contract EtherFiAdmin is Initializable, OwnableUpgradeable, UUPSUpgradeable { for (uint256 i = 0; i < _report.withdrawalRequestsToInvalidate.length; i++) { withdrawRequestNft.invalidateRequest(_report.withdrawalRequestsToInvalidate[i]); } - withdrawRequestNft.finalizeRequests(_report.lastFinalizedWithdrawalRequestId); + withdrawRequestNft.finalizeRequests(_report.lastFinalizedWithdrawalRequestId, _report.finalizedWithdrawalAmount); } function slotForNextReportToProcess() public view returns (uint32) { diff --git a/src/WithdrawRequestNFT.sol b/src/WithdrawRequestNFT.sol index f129a8a5..8086ff30 100644 --- a/src/WithdrawRequestNFT.sol +++ b/src/WithdrawRequestNFT.sol @@ -85,8 +85,11 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad function initializeV2dot5(address _roleRegistry) external onlyOwner { require(address(roleRegistry) == address(0x00), "already initialized"); - DEPRECATED_accumulatedDustEEthShares = 0; // TODO: compile list of values in DEPRECATED_admins to clear out + DEPRECATED_accumulatedDustEEthShares = 0; + + // All requests must be refinalized with the new withdrawal flow + lastFinalizedRequestId = 0; roleRegistry = RoleRegistry(_roleRegistry); finalizationCheckpoints.push(FinalizationCheckpoint(0, 0)); @@ -148,21 +151,21 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad emit WithdrawRequestSeized(requestId); } + /// @notice finalizes a batch of requests and locks the corresponding ETH to be withdrawn + /// @dev called by the `EtherFiAdmin` contract to finalize a batch of requests based on the last oracle report function finalizeRequests(uint32 lastRequestId) external { if (!roleRegistry.hasRole(WITHDRAW_NFT_ADMIN_ROLE, msg.sender)) revert IncorrectRole(); uint256 totalAmount = uint256(calculateTotalPendingAmount(lastRequestId)); - uint256 cachedSharePrice = liquidityPool.amountForShare(E27_PRECISION_BASE); - - finalizationCheckpoints.push(FinalizationCheckpoint(uint32(lastRequestId), cachedSharePrice)); - - lastFinalizedRequestId = lastRequestId; + _finalizeRequests(lastRequestId, totalAmount); + } - if (totalAmount > 0) { - liquidityPool.withdraw(address(this), totalAmount); - } + /// @notice `finalizeRequests` with the ability to specify the total amount of ETH to be locked + /// @dev The oracle calculates the amount of ETH that is needed to fulfill the pending withdrawal off-chain + function finalizeRequests(uint256 lastRequestId, uint256 totalAmount) external { + if (!roleRegistry.hasRole(WITHDRAW_NFT_ADMIN_ROLE, msg.sender)) revert IncorrectRole(); - emit UpdateFinalizedRequestId(lastRequestId, totalAmount); + _finalizeRequests(lastRequestId, totalAmount); } function invalidateRequest(uint32 requestId) external { @@ -302,7 +305,6 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad return finalizationCheckpoints[checkpointId]; } - function getRequest(uint32 requestId) external view returns (IWithdrawRequestNFT.WithdrawRequest memory) { return _requests[requestId]; } @@ -342,6 +344,19 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad emit WithdrawRequestClaimed(requestId, amountToWithdraw, 0, recipient); } + function _finalizeRequests(uint256 lastRequestId, uint256 totalAmount) internal { + uint256 cachedSharePrice = liquidityPool.amountForShare(E27_PRECISION_BASE); + finalizationCheckpoints.push(FinalizationCheckpoint(uint32(lastRequestId), cachedSharePrice)); + + lastFinalizedRequestId = uint32(lastRequestId); + + if (totalAmount > 0) { + liquidityPool.withdraw(address(this), totalAmount); + } + + emit UpdateFinalizedRequestId(uint32(lastRequestId), totalAmount); + } + // invalid NFTs is non-transferable except for the case they are being burnt by the owner via `seizeInvalidRequest` function _beforeTokenTransfer(address /*from*/, address /*to*/, uint256 firstTokenId, uint256 batchSize) internal view override { for (uint256 i = 0; i < batchSize; i++) { diff --git a/src/interfaces/IWithdrawRequestNFT.sol b/src/interfaces/IWithdrawRequestNFT.sol index 96a3fd07..5268c8d6 100644 --- a/src/interfaces/IWithdrawRequestNFT.sol +++ b/src/interfaces/IWithdrawRequestNFT.sol @@ -23,4 +23,5 @@ interface IWithdrawRequestNFT { function invalidateRequest(uint32 requestId) external; function finalizeRequests(uint32 lastRequestId) external; + function finalizeRequests(uint256 lastRequestId, uint256 totalAmount) external; } diff --git a/test/WithdrawRequestNFT.t.sol b/test/WithdrawRequestNFT.t.sol index b38dfa63..5ae5ba36 100644 --- a/test/WithdrawRequestNFT.t.sol +++ b/test/WithdrawRequestNFT.t.sol @@ -429,15 +429,11 @@ contract WithdrawRequestNFTTest is TestSetup { assertEq(address(chad).balance, chadBalance + claimableAmount, "Chad should receive the claimable amount"); } - function test_updated_checkpoint_logic() public payable { - for (uint256 i = 0; i < 100; i++) { + function test_updated_checkpoint_logic() public { + for (uint256 i = 0; i < 50; i++) { address user = vm.addr(i + 1); users.push(user); vm.deal(user, 15 ether); - } - - // first 50 users deposit - for (uint256 i = 0; i < 50; i++) { vm.prank(users[i]); liquidityPoolInstance.deposit{value: 1 ether}(); } From 9d220ab3b4f10575515b1ac0539171eb8e4e3b94 Mon Sep 17 00:00:00 2001 From: jtfirek Date: Thu, 19 Sep 2024 12:49:08 -0500 Subject: [PATCH 6/7] I-06 --- src/WithdrawRequestNFT.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/WithdrawRequestNFT.sol b/src/WithdrawRequestNFT.sol index 8086ff30..7ad7f5a9 100644 --- a/src/WithdrawRequestNFT.sol +++ b/src/WithdrawRequestNFT.sol @@ -199,7 +199,7 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad /// `checkpointIndex` can be found using `findCheckpointIndex()` function /// @return uint256 the amount of ETH that can be claimed by the owner of the NFT function getClaimableAmount(uint32 requestId, uint32 checkpointIndex) public view returns (uint256) { - require(requestId <= lastFinalizedRequestId, "Request is not finalized"); + require(isFinalized(tokenId), "Request is not finalized"); require(requestId < nextRequestId, "Request does not exist"); require(ownerOf(requestId) != address(0), "Already claimed"); From 71f70ae2978b43b437d6f0180e1812ccb5661d60 Mon Sep 17 00:00:00 2001 From: jtfirek Date: Thu, 3 Oct 2024 14:42:29 -0500 Subject: [PATCH 7/7] additional safeguards and tests --- src/LiquidityPool.sol | 1 + src/WithdrawRequestNFT.sol | 42 +++++++++++++++++++++++------------ test/WithdrawRequestNFT.t.sol | 20 +++++++++++++++-- 3 files changed, 47 insertions(+), 16 deletions(-) diff --git a/src/LiquidityPool.sol b/src/LiquidityPool.sol index 0962a64c..2510caf2 100644 --- a/src/LiquidityPool.sol +++ b/src/LiquidityPool.sol @@ -423,6 +423,7 @@ contract LiquidityPool is Initializable, OwnableUpgradeable, UUPSUpgradeable, IL emit Rebase(getTotalPooledEther(), eETH.totalShares()); } + /// @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/WithdrawRequestNFT.sol b/src/WithdrawRequestNFT.sol index 7ad7f5a9..efccbff3 100644 --- a/src/WithdrawRequestNFT.sol +++ b/src/WithdrawRequestNFT.sol @@ -11,8 +11,6 @@ import "./interfaces/IWithdrawRequestNFT.sol"; import "./interfaces/IMembershipManager.sol"; import "./RoleRegistry.sol"; -import "forge-std/console.sol"; - contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgradeable, IWithdrawRequestNFT { //-------------------------------------------------------------------------------------- //--------------------------------- STATE-VARIABLES ---------------------------------- @@ -113,7 +111,7 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad /// @notice called by the NFT owner of a finalized request to claim their ETH /// @param requestId the id of the withdraw request and associated NFT - /// @param checkpointIndex the index of the `finalizationCheckpoints` that the request belongs to. + /// @param checkpointIndex the index of the `finalizationCheckpoints` that the request belongs to /// can be found with `findCheckpointIndex(_requestIds, 1, getLastCheckpointIndex())` function claimWithdraw(uint32 requestId, uint32 checkpointIndex) external { return _claimWithdraw(requestId, ownerOf(requestId), checkpointIndex); @@ -153,8 +151,13 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad /// @notice finalizes a batch of requests and locks the corresponding ETH to be withdrawn /// @dev called by the `EtherFiAdmin` contract to finalize a batch of requests based on the last oracle report + /// @param lastRequestId the id of the last request to finalize in this batch, will update `lastFinalizedRequestId` value function finalizeRequests(uint32 lastRequestId) external { if (!roleRegistry.hasRole(WITHDRAW_NFT_ADMIN_ROLE, msg.sender)) revert IncorrectRole(); + require(lastRequestId >= lastFinalizedRequestId, "Invalid lastRequestId submitted"); + + // No new requests have been finalized since the last oracle report + if (lastRequestId == lastFinalizedRequestId) { return; } uint256 totalAmount = uint256(calculateTotalPendingAmount(lastRequestId)); _finalizeRequests(lastRequestId, totalAmount); @@ -162,8 +165,14 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad /// @notice `finalizeRequests` with the ability to specify the total amount of ETH to be locked /// @dev The oracle calculates the amount of ETH that is needed to fulfill the pending withdrawal off-chain + /// @param lastRequestId the id of the last request to finalize in this batch, will update `lastFinalizedRequestId` value + /// @param totalAmount the total amount of ETH to be locked for the requests in this batch function finalizeRequests(uint256 lastRequestId, uint256 totalAmount) external { if (!roleRegistry.hasRole(WITHDRAW_NFT_ADMIN_ROLE, msg.sender)) revert IncorrectRole(); + require(lastRequestId >= lastFinalizedRequestId, "Invalid lastRequestId submitted"); + + // No new requests have been finalized since the last oracle report + if (lastRequestId == lastFinalizedRequestId) { return; } _finalizeRequests(lastRequestId, totalAmount); } @@ -199,7 +208,7 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad /// `checkpointIndex` can be found using `findCheckpointIndex()` function /// @return uint256 the amount of ETH that can be claimed by the owner of the NFT function getClaimableAmount(uint32 requestId, uint32 checkpointIndex) public view returns (uint256) { - require(isFinalized(tokenId), "Request is not finalized"); + require(isFinalized(requestId), "Request is not finalized"); require(requestId < nextRequestId, "Request does not exist"); require(ownerOf(requestId) != address(0), "Already claimed"); @@ -235,7 +244,7 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad function findCheckpointIndex(uint32 _requestId, uint32 _start, uint32 _end) public view returns (uint32) { require(_requestId <= lastFinalizedRequestId, "Request is not finalized"); require(_start <= _end, "Invalid range"); - require (_start != 0 && _end <= getLastCheckpointIndex(), "Range is out of bounds"); + require(_start != 0 && _end <= getLastCheckpointIndex(), "Range is out of bounds"); // Left boundary if (_requestId <= finalizationCheckpoints[_start].lastFinalizedRequestId) { @@ -269,9 +278,9 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad return uint32(max); } - /// @notice The excess eETH balance of this contract beyond what is needed to fulfill withdrawal requests. + /// @notice The excess eETH balance of this contract beyond what is needed to fulfill withdrawal requests /// This excess accumulates due to: - /// - eETH requested for withdrawal accruing staking rewards until the withdrawal is finalized. + /// - eETH requested for withdrawal accruing staking rewards until the withdrawal is finalized /// Any remaining positive rebase rewards stay in the contract after finalization /// - eETH balance calculation includes integer division, and there is a common case when the whole eETH /// balance can't be transferred from the account while leaving the last 1-2 wei on the sender's account @@ -286,10 +295,14 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad /// @notice The amount of eETH that needed to fulfill the pending withdrawal requests up to and including `lastRequestId` function calculateTotalPendingAmount(uint32 lastRequestId) public view returns (uint256) { uint256 totalAmount = 0; + uint256 preciseSharePrice = liquidityPool.amountForShare(E27_PRECISION_BASE); + for (uint32 i = lastFinalizedRequestId + 1; i <= lastRequestId; i++) { IWithdrawRequestNFT.WithdrawRequest memory request = _requests[i]; - uint256 amountForShares = liquidityPool.amountForShare(request.shareOfEEth); + + // Use the precise share price calculation to maintain conistency with how amount is calculated in `getClaimableAmount` + uint256 amountForShares = request.shareOfEEth * preciseSharePrice / E27_PRECISION_BASE; uint256 amount = _min(request.amountOfEEth, amountForShares); totalAmount += amount; @@ -350,18 +363,19 @@ contract WithdrawRequestNFT is ERC721Upgradeable, UUPSUpgradeable, OwnableUpgrad lastFinalizedRequestId = uint32(lastRequestId); - if (totalAmount > 0) { - liquidityPool.withdraw(address(this), totalAmount); - } + liquidityPool.withdraw(address(this), totalAmount); emit UpdateFinalizedRequestId(uint32(lastRequestId), totalAmount); } // invalid NFTs is non-transferable except for the case they are being burnt by the owner via `seizeInvalidRequest` function _beforeTokenTransfer(address /*from*/, address /*to*/, uint256 firstTokenId, uint256 batchSize) internal view override { - for (uint256 i = 0; i < batchSize; i++) { - uint256 tokenId = firstTokenId + i; - require(_requests[tokenId].isValid || msg.sender == owner(), "INVALID_REQUEST"); + if (msg.sender != owner()) { + // if not called by the contract owner, only allow transfers of valid NFTs + for (uint256 i = 0; i < batchSize; i++) { + uint256 tokenId = firstTokenId + i; + require(_requests[tokenId].isValid, "INVALID_REQUEST"); + } } } diff --git a/test/WithdrawRequestNFT.t.sol b/test/WithdrawRequestNFT.t.sol index 5ae5ba36..bb9b0b18 100644 --- a/test/WithdrawRequestNFT.t.sol +++ b/test/WithdrawRequestNFT.t.sol @@ -311,13 +311,15 @@ contract WithdrawRequestNFTTest is TestSetup { assertEq(eETHInstance.balanceOf(address(withdrawRequestNFTInstance)), 8); // Within `LP.requestWithdraw` // - `share` is calculated by `sharesForAmount` as (9 * 98) / 100 = 8.82 ---> (rounded down to) 8 - + + vm.prank(address(membershipManagerInstance)); + liquidityPoolInstance.rebase(2); _finalizeWithdrawalRequest(requestId); - vm.prank(bob); withdrawRequestNFTInstance.claimWithdraw(requestId, 1); + // Within `claimWithdraw`, // - `request.amountOfEEth` is 9 // - `amountForShares` is (8 * 100) / 98 = 8.16 ---> (rounded down to) 8 @@ -452,8 +454,22 @@ contract WithdrawRequestNFTTest is TestSetup { liquidityPoolInstance.rebase(50 ether); // finalize the requests in multiple batches + + _finalizeWithdrawalRequest(5); + + uint256 dustShares1 = withdrawRequestNFTInstance.getAccumulatedDustEEthAmount(); + + // no new NFTs where finalized during this period _finalizeWithdrawalRequest(5); + // dust should remain the same amount + assertEq(dustShares1, withdrawRequestNFTInstance.getAccumulatedDustEEthAmount()); + + vm.expectRevert("Invalid lastRequestId submitted"); + _finalizeWithdrawalRequest(4); + _finalizeWithdrawalRequest(11); + _finalizeWithdrawalRequest(12); + _finalizeWithdrawalRequest(13); _finalizeWithdrawalRequest(17); _finalizeWithdrawalRequest(23); _finalizeWithdrawalRequest(withdrawRequestNFTInstance.nextRequestId() - 1);