From ce71ac52542ccc3b9997265cdbcf333540462c80 Mon Sep 17 00:00:00 2001 From: sandybradley Date: Tue, 25 Jul 2023 14:55:24 +0200 Subject: [PATCH 01/18] chore: remove processWithdrawQueue from grantRewards and grantValidatorWithdraw --- src/MevEth.sol | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/MevEth.sol b/src/MevEth.sol index 09b3f5f..7177555 100644 --- a/src/MevEth.sol +++ b/src/MevEth.sol @@ -321,8 +321,6 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { /// @dev Before updating the fraction, the withdraw queue is processed, which pays out any pending withdrawals. /// @dev This function is only callable by the MevEthShareVault. function grantRewards() external payable { - // Process the withdrawal queue, paying out any pending withdrawal tickets before updating the fraction. - processWithdrawalQueue(); if (!(msg.sender == address(stakingModule) || msg.sender == mevEthShareVault)) revert MevEthErrors.InvalidSender(); /// @dev Note that while a small possiblity, it is possible for the MevEthShareVault rewards + fraction.elastic to overflow a uint128. @@ -346,9 +344,6 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { revert MevEthErrors.ZeroValue(); } - // Process the withdrawal queue, paying out any pending withdrawal tickets before updating the fraction balance. - processWithdrawalQueue(); - // Emit an event to notify offchain listeners that a validator has withdrawn funds. emit ValidatorWithdraw(msg.sender, msg.value); From c423be995c9c8674d9e7721d1e30f574fd756cd4 Mon Sep 17 00:00:00 2001 From: sandybradley Date: Tue, 25 Jul 2023 14:56:41 +0200 Subject: [PATCH 02/18] chore: change processWithdrawQueue to onlyOperator --- src/MevEth.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/MevEth.sol b/src/MevEth.sol index 7177555..7d109d4 100644 --- a/src/MevEth.sol +++ b/src/MevEth.sol @@ -388,7 +388,7 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { mapping(uint256 ticketNumber => WithdrawalTicket ticket) public withdrawalQueue; /// @notice Processes the withdrawal queue, paying out any pending withdrawals with the contract's available balance. - function processWithdrawalQueue() public { + function processWithdrawalQueue() external onlyOperator { // Get the current length of the queue uint256 length = queueLength; From 1227dfcb8b64510b77f680091fa5bb80814e9da0 Mon Sep 17 00:00:00 2001 From: sandybradley Date: Tue, 25 Jul 2023 15:01:45 +0200 Subject: [PATCH 03/18] chore: set min withdraw amount --- src/MevEth.sol | 6 ++++++ src/interfaces/Errors.sol | 1 + 2 files changed, 7 insertions(+) diff --git a/src/MevEth.sol b/src/MevEth.sol index 7d109d4..ead53b0 100644 --- a/src/MevEth.sol +++ b/src/MevEth.sol @@ -598,6 +598,9 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { /// @param owner The address of the owner of the mevEth /// @return shares The amount of shares burned function withdraw(uint256 assets, address receiver, address owner) external returns (uint256 shares) { + // If withdraw is less than the minimum deposit / withdraw amount, revert + if (assets < MIN_DEPOSIT) revert MevEthErrors.WithdrawTooSmall(); + // Convert the assets to shares and check if the owner has the allowance to withdraw the shares. shares = convertToShares(assets); @@ -644,6 +647,9 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { /// @param owner The address of the owner of the mevEth /// @return assets The amount of assets withdrawn function redeem(uint256 shares, address receiver, address owner) external returns (uint256 assets) { + // If withdraw is less than the minimum deposit / withdraw amount, revert + if (convertToAssets(shares) < MIN_DEPOSIT) revert MevEthErrors.WithdrawTooSmall(); + // Convert the shares to assets and check if the owner has the allowance to withdraw the shares. assets = convertToAssets(shares); diff --git a/src/interfaces/Errors.sol b/src/interfaces/Errors.sol index 9b8d722..0d486c7 100644 --- a/src/interfaces/Errors.sol +++ b/src/interfaces/Errors.sol @@ -39,4 +39,5 @@ interface MevEthErrors { error FeesTooHigh(); error WrongDepositAmount(); error UnAuthorizedCaller(); + error WithdrawTooSmall(); } From 4a1cbfed7410f1510e9661b379aa5c088abcc8f2 Mon Sep 17 00:00:00 2001 From: sandybradley Date: Tue, 25 Jul 2023 18:21:22 +0200 Subject: [PATCH 04/18] feat: upgrade withdraw queue --- src/MevEth.sol | 103 +++++++++++++++--------------- src/interfaces/Errors.sol | 2 + test/attacks/ReentrancyAttack.sol | 5 +- test/unit/ERC4626.t.sol | 30 +++++++++ test/unit/Queue.t.sol | 27 +++++--- 5 files changed, 103 insertions(+), 64 deletions(-) diff --git a/src/MevEth.sol b/src/MevEth.sol index ead53b0..8137e7b 100644 --- a/src/MevEth.sol +++ b/src/MevEth.sol @@ -45,8 +45,6 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { bool public stakingPaused; /// @notice Indicates if contract is initialized. bool public initialized; - /// @notice Amount of ETH to retain on contract for withdrawls as a percent numerator. - uint8 public bufferPercentNumerator; /// @notice Timestamp when pending staking module update can be finalized. uint64 public pendingStakingModuleCommittedTimestamp; /// @notice Timestamp when pending mevEthShareVault update can be finalized. @@ -95,14 +93,13 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { OFTWithFee("Mev Liquid Staked Ether", "mevETH", 18, 8, authority, layerZeroEndpoint) { WETH = IWETH(weth); - bufferPercentNumerator = 2; // set at 2 % } /// @notice Calculate the needed Ether buffer required when creating a new validator. /// @return uint256 The required Ether buffer. function calculateNeededEtherBuffer() public view returns (uint256) { unchecked { - return max((uint256(fraction.elastic) * uint256(bufferPercentNumerator)) / 100, 31 ether); + return max(withdrawlAmountQueued, 31 ether); } } @@ -139,11 +136,6 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { emit MevEthInitialized(initialShareVault, initialStakingModule); } - //TODO: FIXME: never used - function updateBufferPercentNumerator(uint8 newBufferPercentNumerator) external onlyAdmin { - bufferPercentNumerator = newBufferPercentNumerator; - } - /// @notice Emitted when staking is paused. event StakingPaused(); /// @notice Emitted when staking is unpaused. @@ -368,54 +360,74 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { WITHDRAWAL QUEUE //////////////////////////////////////////////////////////////*/ - /// @notice Struct representing a withdrawal ticket which is added to the withdrawal queue if there is not enough ETH in the contract - /// to pay out the withdrawal immediately. + /// @notice Struct representing a withdrawal ticket which is added to the withdrawal queue. + /// @custom:field claimed True if this receiver has received ticket funds. /// @custom:field receiver The receiever of the ETH specified in the WithdrawalTicket. /// @custom:field amount The amount of ETH to send to the receiver when the ticket is processed. struct WithdrawalTicket { + bool claimed; address receiver; uint256 amount; } /// @notice Event emitted when a withdrawal ticket is added to the queue. - event WithdrawalQueueOpened(address indexed receipient, uint256 indexed assets); - event WithdrawalQueueClosed(address indexed receipient, uint256 indexed assets); + event WithdrawalQueueOpened(address indexed receipient, uint256 indexed withdrawlId, uint256 assets); + event WithdrawalQueueClosed(address indexed receipient, uint256 indexed withdrawlId, uint256 assets); /// @notice The length of the withdrawal queue. uint256 queueLength; + + /// @notice mark the latest withdrawal request that was finalised + uint256 requestsFinalisedUntil; + + /// @notice Withdrawl amount queued + uint256 withdrawlAmountQueued; + /// @notice The mapping representing the withdrawal queue. /// @dev The index in the queue is the key, and the value is the WithdrawalTicket. mapping(uint256 ticketNumber => WithdrawalTicket ticket) public withdrawalQueue; - /// @notice Processes the withdrawal queue, paying out any pending withdrawals with the contract's available balance. + /// @notice Claim Finalised Withdrawl Ticket + /// @param withdrawlId Unique ID of the withdrawl ticket + function claim(uint256 withdrawlId) external { + if (withdrawlId > requestsFinalisedUntil) revert MevEthErrors.NotFinalised(); + WithdrawalTicket memory ticket = withdrawalQueue[withdrawlId]; + if (ticket.claimed) revert MevEthErrors.AlreadyClaimed(); + withdrawalQueue[withdrawlId].claimed = true; + withdrawlAmountQueued -= ticket.amount; + emit WithdrawalQueueClosed(ticket.receiver, withdrawlId, ticket.amount); + WETH.deposit{ value: ticket.amount }(); + ERC20(address(WETH)).safeTransfer(ticket.receiver, ticket.amount); + } + + /// @notice Processes the withdrawal queue, reserving any pending withdrawals with the contract's available balance. function processWithdrawalQueue() external onlyOperator { + uint256 balance = address(this).balance; + if (withdrawlAmountQueued >= balance) revert MevEthErrors.NotEnoughEth(); + + uint256 available = balance - withdrawlAmountQueued; + // Get the current length of the queue uint256 length = queueLength; + uint256 finalised = requestsFinalisedUntil; + // While the queue is not empty, process the next ticket in the queue - while (length != 0) { + while (finalised < length) { // Get the next ticket in the queue - WithdrawalTicket memory currentTicket = withdrawalQueue[length - 1]; + WithdrawalTicket memory currentTicket = withdrawalQueue[finalised]; uint256 assetsOwed = currentTicket.amount; - address receipient = currentTicket.receiver; - - // If the balance of the contract has enough ETH to pay the ticket, pay the ticket and remove it from the queue. - if (address(this).balance >= assetsOwed) { - // While not strictly neccessary persay, important for - // added safety - delete withdrawalQueue[length-1]; - length--; - emit WithdrawalQueueClosed(receipient, assetsOwed); - WETH.deposit{ value: assetsOwed }(); - // SafeTransfer not needed because we know the impl - ERC20(address(WETH)).safeTransfer(receipient, assetsOwed); + + // If the balance of the contract has enough ETH to pay the ticket, reserve amount for ticket. + if (available >= assetsOwed) { + finalised += 1; + available -= assetsOwed; } else { - // If the balance of the contract does not have enough ETH to pay the ticket, exit the loop. - queueLength = length; - return; + break; } } - queueLength = 0; + requestsFinalisedUntil = finalised; + withdrawlAmountQueued += balance - available; } /*////////////////////////////////////////////////////////////// @@ -571,26 +583,11 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { ///@notice Function to withdraw assets from the mevEth contract /// @param receiver The address user whom should receive the mevEth out - /// @param owner The address of the owner of the mevEth /// @param assets The amount of assets that should be withdrawn - /// @param shares The amount of shares corresponding to assets withdrawn - function _withdraw(address receiver, address owner, uint256 assets, uint256 shares) internal { - if (address(this).balance >= assets) { - emit Withdraw(msg.sender, owner, receiver, assets, shares); - WETH.deposit{ value: assets }(); - ERC20(address(WETH)).safeTransfer(receiver, assets); - } else { - uint256 availableBalance = address(this).balance; - uint256 amountOwed = assets - availableBalance; - emit WithdrawalQueueOpened(receiver, amountOwed); - withdrawalQueue[queueLength] = WithdrawalTicket({ receiver: receiver, amount: amountOwed }); - queueLength++; - if (!_isZero(availableBalance)) { - emit Withdraw(msg.sender, owner, receiver, availableBalance, convertToShares(availableBalance)); - WETH.deposit{ value: availableBalance }(); - ERC20(address(WETH)).safeTransfer(receiver, availableBalance); - } - } + function _withdraw(address receiver, uint256 assets) internal { + withdrawalQueue[queueLength] = WithdrawalTicket({ claimed: false, receiver: receiver, amount: assets }); + emit WithdrawalQueueOpened(receiver, queueLength, assets); + queueLength++; } /// @param assets The amount of assets that should be withdrawn @@ -624,7 +621,7 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { _burn(owner, shares); // Withdraw the assets from the Mevth contract - _withdraw(receiver, owner, assets, shares); + _withdraw(receiver, assets); } ///@notice Function to simulate the maximum amount of shares that can be redeemed by the owner. @@ -673,7 +670,7 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { _burn(owner, shares); // Withdraw the assets from the Mevth contract - _withdraw(receiver, owner, assets, shares); + _withdraw(receiver, assets); } /*////////////////////////////////////////////////////////////// diff --git a/src/interfaces/Errors.sol b/src/interfaces/Errors.sol index 0d486c7..2707b3f 100644 --- a/src/interfaces/Errors.sol +++ b/src/interfaces/Errors.sol @@ -40,4 +40,6 @@ interface MevEthErrors { error WrongDepositAmount(); error UnAuthorizedCaller(); error WithdrawTooSmall(); + error NotFinalised(); + error AlreadyClaimed(); } diff --git a/test/attacks/ReentrancyAttack.sol b/test/attacks/ReentrancyAttack.sol index a99aa53..bb921a8 100644 --- a/test/attacks/ReentrancyAttack.sol +++ b/test/attacks/ReentrancyAttack.sol @@ -13,7 +13,7 @@ contract ReentrancyAttackTest is MevEthTest { // Fallback is called when MevEth sends Ether to this contract. fallback() external payable { - mevEth.withdraw(msg.value, address(this), address(this)); + mevEth.claim(0); } receive() external payable { } @@ -25,6 +25,9 @@ contract ReentrancyAttackTest is MevEthTest { uint256 bal = address(this).balance; mevEth.deposit{ value: amount }(amount, address(this)); mevEth.withdraw(amount, address(this), address(this)); + vm.prank(Operator01); + mevEth.processWithdrawalQueue(); + mevEth.claim(0); assertEq(weth.balanceOf(address(this)), amount); assertEq(address(this).balance, bal - amount); } diff --git a/test/unit/ERC4626.t.sol b/test/unit/ERC4626.t.sol index aeced6b..43a3592 100644 --- a/test/unit/ERC4626.t.sol +++ b/test/unit/ERC4626.t.sol @@ -184,6 +184,11 @@ contract ERC4626Test is MevEthTest { // Withdraw 1 mevETH mevEth.withdraw(0.75 ether, User01, User01); + vm.stopPrank(); + vm.prank(Operator01); + mevEth.processWithdrawalQueue(); + vm.prank(User01); + mevEth.claim(0); assertEq(mevEth.balanceOf(User01), 0.25 ether); assertEq(weth.balanceOf(User01), 0.75 ether); } @@ -222,6 +227,11 @@ contract ERC4626Test is MevEthTest { // Withdraw 1 mevETH mevEth.withdraw(0.75 ether, User02, User01); + vm.stopPrank(); + vm.prank(Operator01); + mevEth.processWithdrawalQueue(); + vm.prank(User01); + mevEth.claim(0); assertEq(mevEth.balanceOf(User01), 0.25 ether); assertEq(weth.balanceOf(User02), 0.75 ether); @@ -245,6 +255,11 @@ contract ERC4626Test is MevEthTest { // Withdraw 1 mevETH mevEth.withdraw(amount, User01, User01); + vm.stopPrank(); + vm.prank(Operator01); + mevEth.processWithdrawalQueue(); + vm.prank(User01); + mevEth.claim(0); assertEq(mevEth.balanceOf(User01), 0 ether); assertEq(weth.balanceOf(User01), amount); } @@ -344,6 +359,11 @@ contract ERC4626Test is MevEthTest { // Redeem 1 mevETH mevEth.redeem(0.75 ether, User01, User01); + vm.stopPrank(); + vm.prank(Operator01); + mevEth.processWithdrawalQueue(); + vm.prank(User01); + mevEth.claim(0); assertEq(mevEth.balanceOf(User01), 0.25 ether); assertEq(weth.balanceOf(User01), 0.75 ether); @@ -385,6 +405,11 @@ contract ERC4626Test is MevEthTest { // Withdraw 0.75 mevETH mevEth.redeem(0.75 ether, User02, User01); + vm.stopPrank(); + vm.prank(Operator01); + mevEth.processWithdrawalQueue(); + vm.prank(User01); + mevEth.claim(0); assertEq(mevEth.balanceOf(User01), 0.25 ether); assertEq(weth.balanceOf(User02), 0.75 ether); @@ -406,6 +431,11 @@ contract ERC4626Test is MevEthTest { // Redeem 1 mevETH mevEth.redeem(amountToRedeem, User01, User01); + vm.stopPrank(); + vm.prank(Operator01); + mevEth.processWithdrawalQueue(); + vm.prank(User01); + mevEth.claim(0); assertEq(mevEth.balanceOf(User01), amountLeftOver); assertEq(weth.balanceOf(User01), amountToRedeem); diff --git a/test/unit/Queue.t.sol b/test/unit/Queue.t.sol index 733c193..0f2d70e 100644 --- a/test/unit/Queue.t.sol +++ b/test/unit/Queue.t.sol @@ -33,10 +33,10 @@ contract QueueTest is MevEthTest { mevEth.withdraw(63 ether, User01, User01); Vm.Log[] memory entries = vm.getRecordedLogs(); - assertEq(entries[1].topics[0], keccak256("WithdrawalQueueOpened(address,uint256)")); - assertEq(uint256(entries[1].topics[2]), 31 ether); + assertEq(entries[1].topics[0], keccak256("WithdrawalQueueOpened(address,uint256,uint256)")); + assertEq(abi.decode(entries[1].data, (uint256)), 63 ether); - assertEq(weth.balanceOf(User01), 32 ether); + assertEq(weth.balanceOf(User01), 0); // Now that an unprocessed withdrawal has been created // time to ensure it can be properly processed back @@ -46,11 +46,14 @@ contract QueueTest is MevEthTest { vm.deal(stakingModuleAddress, 32 ether); vm.prank(SamBacha); IStakingModule(stakingModuleAddress).payValidatorWithdraw(32 ether); + vm.prank(Operator01); + mevEth.processWithdrawalQueue(); + mevEth.claim(0); Vm.Log[] memory entries2 = vm.getRecordedLogs(); - assertEq(entries2[0].topics[0], keccak256("WithdrawalQueueClosed(address,uint256)")); - assertEq(uint256(entries2[0].topics[2]), 31 ether); + assertEq(entries2[2].topics[0], keccak256("WithdrawalQueueClosed(address,uint256,uint256)")); + assertEq(abi.decode(entries2[2].data, (uint256)), 63 ether); assertEq(weth.balanceOf(User01), 63 ether); } @@ -83,10 +86,10 @@ contract QueueTest is MevEthTest { mevEth.redeem(mevEth.convertToShares(63 ether), User01, User01); Vm.Log[] memory entries = vm.getRecordedLogs(); - assertEq(entries[1].topics[0], keccak256("WithdrawalQueueOpened(address,uint256)")); - assertEq(uint256(entries[1].topics[2]), 31 ether); + assertEq(entries[1].topics[0], keccak256("WithdrawalQueueOpened(address,uint256,uint256)")); + assertEq(abi.decode(entries[1].data, (uint256)), 63 ether); - assertEq(weth.balanceOf(User01), 32 ether); + assertEq(weth.balanceOf(User01), 0); // Now that an unprocessed withdrawal has been created // time to ensure it can be properly processed back @@ -96,10 +99,14 @@ contract QueueTest is MevEthTest { vm.deal(stakingModuleAddress, 32 ether); vm.prank(SamBacha); IStakingModule(stakingModuleAddress).payValidatorWithdraw(32 ether); + vm.prank(Operator01); + mevEth.processWithdrawalQueue(); + + mevEth.claim(0); Vm.Log[] memory entries2 = vm.getRecordedLogs(); - assertEq(entries2[0].topics[0], keccak256("WithdrawalQueueClosed(address,uint256)")); - assertEq(uint256(entries2[0].topics[2]), 31 ether); + assertEq(entries2[2].topics[0], keccak256("WithdrawalQueueClosed(address,uint256,uint256)")); + assertEq(abi.decode(entries2[2].data, (uint256)), 63 ether); assertEq(weth.balanceOf(User01), 63 ether); } From a324717bab4bf6ca9904eaa3d33dcda7f7313542 Mon Sep 17 00:00:00 2001 From: sandybradley Date: Wed, 26 Jul 2023 17:47:37 +0200 Subject: [PATCH 05/18] feat: re-enable instant withdraws if balance available --- src/MevEth.sol | 33 +++++++++++++---- test/attacks/ReentrancyAttack.sol | 5 +-- test/unit/ERC4626.t.sol | 60 +++++++++++++++---------------- test/unit/Queue.t.sol | 12 +++---- 4 files changed, 64 insertions(+), 46 deletions(-) diff --git a/src/MevEth.sol b/src/MevEth.sol index 8137e7b..02de007 100644 --- a/src/MevEth.sol +++ b/src/MevEth.sol @@ -583,11 +583,32 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { ///@notice Function to withdraw assets from the mevEth contract /// @param receiver The address user whom should receive the mevEth out + /// @param owner The address of the owner of the mevEth /// @param assets The amount of assets that should be withdrawn - function _withdraw(address receiver, uint256 assets) internal { - withdrawalQueue[queueLength] = WithdrawalTicket({ claimed: false, receiver: receiver, amount: assets }); - emit WithdrawalQueueOpened(receiver, queueLength, assets); - queueLength++; + /// @param shares The amount of shares corresponding to assets withdrawn + function _withdraw(address receiver, address owner, uint256 assets, uint256 shares) internal { + uint256 availableBalance = address(this).balance; // available balance will be adjusted + if (availableBalance > withdrawlAmountQueued) { + availableBalance = availableBalance - withdrawlAmountQueued; + } else { + availableBalance = 0; + } + + if (availableBalance >= assets) { + emit Withdraw(msg.sender, owner, receiver, assets, shares); + WETH.deposit{ value: assets }(); + ERC20(address(WETH)).safeTransfer(receiver, assets); + } else { + uint256 amountOwed = assets - availableBalance; + withdrawalQueue[queueLength] = WithdrawalTicket({ claimed: false, receiver: receiver, amount: amountOwed }); + emit WithdrawalQueueOpened(receiver, queueLength, amountOwed); + queueLength++; + if (!_isZero(availableBalance)) { + emit Withdraw(msg.sender, owner, receiver, availableBalance, convertToShares(availableBalance)); + WETH.deposit{ value: availableBalance }(); + ERC20(address(WETH)).safeTransfer(receiver, availableBalance); + } + } } /// @param assets The amount of assets that should be withdrawn @@ -621,7 +642,7 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { _burn(owner, shares); // Withdraw the assets from the Mevth contract - _withdraw(receiver, assets); + _withdraw(receiver, owner, assets, shares); } ///@notice Function to simulate the maximum amount of shares that can be redeemed by the owner. @@ -670,7 +691,7 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { _burn(owner, shares); // Withdraw the assets from the Mevth contract - _withdraw(receiver, assets); + _withdraw(receiver, owner, assets, shares); } /*////////////////////////////////////////////////////////////// diff --git a/test/attacks/ReentrancyAttack.sol b/test/attacks/ReentrancyAttack.sol index bb921a8..a99aa53 100644 --- a/test/attacks/ReentrancyAttack.sol +++ b/test/attacks/ReentrancyAttack.sol @@ -13,7 +13,7 @@ contract ReentrancyAttackTest is MevEthTest { // Fallback is called when MevEth sends Ether to this contract. fallback() external payable { - mevEth.claim(0); + mevEth.withdraw(msg.value, address(this), address(this)); } receive() external payable { } @@ -25,9 +25,6 @@ contract ReentrancyAttackTest is MevEthTest { uint256 bal = address(this).balance; mevEth.deposit{ value: amount }(amount, address(this)); mevEth.withdraw(amount, address(this), address(this)); - vm.prank(Operator01); - mevEth.processWithdrawalQueue(); - mevEth.claim(0); assertEq(weth.balanceOf(address(this)), amount); assertEq(address(this).balance, bal - amount); } diff --git a/test/unit/ERC4626.t.sol b/test/unit/ERC4626.t.sol index 43a3592..3da02c2 100644 --- a/test/unit/ERC4626.t.sol +++ b/test/unit/ERC4626.t.sol @@ -184,11 +184,11 @@ contract ERC4626Test is MevEthTest { // Withdraw 1 mevETH mevEth.withdraw(0.75 ether, User01, User01); - vm.stopPrank(); - vm.prank(Operator01); - mevEth.processWithdrawalQueue(); - vm.prank(User01); - mevEth.claim(0); + // vm.stopPrank(); + // vm.prank(Operator01); + // mevEth.processWithdrawalQueue(); + // vm.prank(User01); + // mevEth.claim(0); assertEq(mevEth.balanceOf(User01), 0.25 ether); assertEq(weth.balanceOf(User01), 0.75 ether); } @@ -227,11 +227,11 @@ contract ERC4626Test is MevEthTest { // Withdraw 1 mevETH mevEth.withdraw(0.75 ether, User02, User01); - vm.stopPrank(); - vm.prank(Operator01); - mevEth.processWithdrawalQueue(); - vm.prank(User01); - mevEth.claim(0); + // vm.stopPrank(); + // vm.prank(Operator01); + // mevEth.processWithdrawalQueue(); + // vm.prank(User01); + // mevEth.claim(0); assertEq(mevEth.balanceOf(User01), 0.25 ether); assertEq(weth.balanceOf(User02), 0.75 ether); @@ -255,11 +255,11 @@ contract ERC4626Test is MevEthTest { // Withdraw 1 mevETH mevEth.withdraw(amount, User01, User01); - vm.stopPrank(); - vm.prank(Operator01); - mevEth.processWithdrawalQueue(); - vm.prank(User01); - mevEth.claim(0); + // vm.stopPrank(); + // vm.prank(Operator01); + // mevEth.processWithdrawalQueue(); + // vm.prank(User01); + // mevEth.claim(0); assertEq(mevEth.balanceOf(User01), 0 ether); assertEq(weth.balanceOf(User01), amount); } @@ -359,11 +359,11 @@ contract ERC4626Test is MevEthTest { // Redeem 1 mevETH mevEth.redeem(0.75 ether, User01, User01); - vm.stopPrank(); - vm.prank(Operator01); - mevEth.processWithdrawalQueue(); - vm.prank(User01); - mevEth.claim(0); + // vm.stopPrank(); + // vm.prank(Operator01); + // mevEth.processWithdrawalQueue(); + // vm.prank(User01); + // mevEth.claim(0); assertEq(mevEth.balanceOf(User01), 0.25 ether); assertEq(weth.balanceOf(User01), 0.75 ether); @@ -405,11 +405,11 @@ contract ERC4626Test is MevEthTest { // Withdraw 0.75 mevETH mevEth.redeem(0.75 ether, User02, User01); - vm.stopPrank(); - vm.prank(Operator01); - mevEth.processWithdrawalQueue(); - vm.prank(User01); - mevEth.claim(0); + // vm.stopPrank(); + // vm.prank(Operator01); + // mevEth.processWithdrawalQueue(); + // vm.prank(User01); + // mevEth.claim(0); assertEq(mevEth.balanceOf(User01), 0.25 ether); assertEq(weth.balanceOf(User02), 0.75 ether); @@ -431,11 +431,11 @@ contract ERC4626Test is MevEthTest { // Redeem 1 mevETH mevEth.redeem(amountToRedeem, User01, User01); - vm.stopPrank(); - vm.prank(Operator01); - mevEth.processWithdrawalQueue(); - vm.prank(User01); - mevEth.claim(0); + // vm.stopPrank(); + // vm.prank(Operator01); + // mevEth.processWithdrawalQueue(); + // vm.prank(User01); + // mevEth.claim(0); assertEq(mevEth.balanceOf(User01), amountLeftOver); assertEq(weth.balanceOf(User01), amountToRedeem); diff --git a/test/unit/Queue.t.sol b/test/unit/Queue.t.sol index 0f2d70e..a352233 100644 --- a/test/unit/Queue.t.sol +++ b/test/unit/Queue.t.sol @@ -34,9 +34,9 @@ contract QueueTest is MevEthTest { Vm.Log[] memory entries = vm.getRecordedLogs(); assertEq(entries[1].topics[0], keccak256("WithdrawalQueueOpened(address,uint256,uint256)")); - assertEq(abi.decode(entries[1].data, (uint256)), 63 ether); + assertEq(abi.decode(entries[1].data, (uint256)), 31 ether); - assertEq(weth.balanceOf(User01), 0); + assertEq(weth.balanceOf(User01), 32 ether); // Now that an unprocessed withdrawal has been created // time to ensure it can be properly processed back @@ -53,7 +53,7 @@ contract QueueTest is MevEthTest { Vm.Log[] memory entries2 = vm.getRecordedLogs(); assertEq(entries2[2].topics[0], keccak256("WithdrawalQueueClosed(address,uint256,uint256)")); - assertEq(abi.decode(entries2[2].data, (uint256)), 63 ether); + assertEq(abi.decode(entries2[2].data, (uint256)), 31 ether); assertEq(weth.balanceOf(User01), 63 ether); } @@ -87,9 +87,9 @@ contract QueueTest is MevEthTest { Vm.Log[] memory entries = vm.getRecordedLogs(); assertEq(entries[1].topics[0], keccak256("WithdrawalQueueOpened(address,uint256,uint256)")); - assertEq(abi.decode(entries[1].data, (uint256)), 63 ether); + assertEq(abi.decode(entries[1].data, (uint256)), 31 ether); - assertEq(weth.balanceOf(User01), 0); + assertEq(weth.balanceOf(User01), 32 ether); // Now that an unprocessed withdrawal has been created // time to ensure it can be properly processed back @@ -106,7 +106,7 @@ contract QueueTest is MevEthTest { Vm.Log[] memory entries2 = vm.getRecordedLogs(); assertEq(entries2[2].topics[0], keccak256("WithdrawalQueueClosed(address,uint256,uint256)")); - assertEq(abi.decode(entries2[2].data, (uint256)), 63 ether); + assertEq(abi.decode(entries2[2].data, (uint256)), 31 ether); assertEq(weth.balanceOf(User01), 63 ether); } From f00309890e0afbc178024c8dc70963bf38a71b68 Mon Sep 17 00:00:00 2001 From: sandybradley Date: Wed, 26 Jul 2023 20:21:35 +0200 Subject: [PATCH 06/18] chore: reduce contract size --- src/MevEth.sol | 67 ++++++++++++++++++++------------------------------ 1 file changed, 27 insertions(+), 40 deletions(-) diff --git a/src/MevEth.sol b/src/MevEth.sol index 02de007..cf2aed2 100644 --- a/src/MevEth.sol +++ b/src/MevEth.sol @@ -420,14 +420,14 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { // If the balance of the contract has enough ETH to pay the ticket, reserve amount for ticket. if (available >= assetsOwed) { - finalised += 1; + ++finalised; available -= assetsOwed; } else { break; } } requestsFinalisedUntil = finalised; - withdrawlAmountQueued += balance - available; + withdrawlAmountQueued = balance - available; } /*////////////////////////////////////////////////////////////// @@ -585,28 +585,31 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { /// @param receiver The address user whom should receive the mevEth out /// @param owner The address of the owner of the mevEth /// @param assets The amount of assets that should be withdrawn - /// @param shares The amount of shares corresponding to assets withdrawn - function _withdraw(address receiver, address owner, uint256 assets, uint256 shares) internal { - uint256 availableBalance = address(this).balance; // available balance will be adjusted - if (availableBalance > withdrawlAmountQueued) { - availableBalance = availableBalance - withdrawlAmountQueued; - } else { - availableBalance = 0; - } + function _withdraw(address receiver, address owner, uint256 assets) internal { + uint256 availableBalance = address(this).balance - withdrawlAmountQueued; // available balance will be adjusted - if (availableBalance >= assets) { - emit Withdraw(msg.sender, owner, receiver, assets, shares); - WETH.deposit{ value: assets }(); - ERC20(address(WETH)).safeTransfer(receiver, assets); - } else { + if (availableBalance < assets) { uint256 amountOwed = assets - availableBalance; withdrawalQueue[queueLength] = WithdrawalTicket({ claimed: false, receiver: receiver, amount: amountOwed }); emit WithdrawalQueueOpened(receiver, queueLength, amountOwed); - queueLength++; - if (!_isZero(availableBalance)) { - emit Withdraw(msg.sender, owner, receiver, availableBalance, convertToShares(availableBalance)); - WETH.deposit{ value: availableBalance }(); - ERC20(address(WETH)).safeTransfer(receiver, availableBalance); + ++queueLength; + assets = availableBalance; + } + if (!_isZero(assets)) { + emit Withdraw(msg.sender, owner, receiver, assets, convertToShares(assets)); + WETH.deposit{ value: assets }(); + ERC20(address(WETH)).safeTransfer(receiver, assets); + } + } + + /// @dev internal function to update allowance for withdraws if necessary + /// @param owner owner of tokens + /// @param shares amount of shares to update + function _updateAllowance(address owner, uint256 shares) internal { + if (owner != msg.sender) { + if (allowance[owner][msg.sender] < shares) revert MevEthErrors.TransferExceedsAllowance(); + unchecked { + allowance[owner][msg.sender] -= shares; } } } @@ -622,12 +625,7 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { // Convert the assets to shares and check if the owner has the allowance to withdraw the shares. shares = convertToShares(assets); - if (owner != msg.sender) { - if (allowance[owner][msg.sender] < shares) revert MevEthErrors.TransferExceedsAllowance(); - unchecked { - allowance[owner][msg.sender] -= shares; - } - } + _updateAllowance(owner, shares); // Update the elastic and base fraction.elastic -= uint128(assets); @@ -642,7 +640,7 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { _burn(owner, shares); // Withdraw the assets from the Mevth contract - _withdraw(receiver, owner, assets, shares); + _withdraw(receiver, owner, assets); } ///@notice Function to simulate the maximum amount of shares that can be redeemed by the owner. @@ -671,12 +669,7 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { // Convert the shares to assets and check if the owner has the allowance to withdraw the shares. assets = convertToAssets(shares); - if (owner != msg.sender) { - if (allowance[owner][msg.sender] < shares) revert MevEthErrors.TransferExceedsAllowance(); - unchecked { - allowance[owner][msg.sender] -= shares; - } - } + _updateAllowance(owner, shares); // Update the elastic and base fraction.elastic -= uint128(assets); @@ -691,7 +684,7 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { _burn(owner, shares); // Withdraw the assets from the Mevth contract - _withdraw(receiver, owner, assets, shares); + _withdraw(receiver, owner, assets); } /*////////////////////////////////////////////////////////////// @@ -722,10 +715,4 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { receive() external payable { if (msg.sender != address(WETH)) revert MevEthErrors.InvalidSender(); } - - /// @dev Only Weth withdraw is defined for the behaviour. Deposits should be directed to deposit / mint. Rewards via grantRewards and validator withdraws - /// via grantValidatorWithdraw. - fallback() external payable { - if (msg.sender != address(WETH)) revert MevEthErrors.InvalidSender(); - } } From e2b29fe567d6a8a9229a94e98c697d7fa37acf7e Mon Sep 17 00:00:00 2001 From: sandybradley Date: Wed, 26 Jul 2023 21:24:56 +0200 Subject: [PATCH 07/18] feat: add withdrawlQueueDOS attack --- test/attacks/WithdrawlQueueDOS.t.sol | 50 ++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 test/attacks/WithdrawlQueueDOS.t.sol diff --git a/test/attacks/WithdrawlQueueDOS.t.sol b/test/attacks/WithdrawlQueueDOS.t.sol new file mode 100644 index 0000000..c2a2be9 --- /dev/null +++ b/test/attacks/WithdrawlQueueDOS.t.sol @@ -0,0 +1,50 @@ +/// SPDX: License-Identifier: GPL-3.0-only +pragma solidity 0.8.19; + +import "../MevEthTest.sol"; + +contract WithdrawlQueueAttackTest is MevEthTest { + function testDosAttackQueueLength() public { + vm.deal(User01, 63 ether); + vm.startPrank(User01); + + address stakingModuleAddress = address(mevEth.stakingModule()); + IStakingModule.ValidatorData memory validatorData = mockValidatorData(User01, 32 ether / 1 gwei); + + // 1. Attacker deposits 63 Eth to MevEth + weth.deposit{ value: 63 ether }(); + weth.approve(address(mevEth), 63 ether); + mevEth.deposit(63 ether, User01); + assertEq(address(mevEth).balance, 63 ether); + + // 2. `createValidator()` is called -> MevEth balance 31 Ether + vm.stopPrank(); + vm.startPrank(Operator01); + mevEth.createValidator(validatorData); + assertEq(address(mevEth).balance, 31 ether); + + vm.stopPrank(); + vm.startPrank(User01); + // 3. Attackers withdraws 31 ETH -> MevEth balance 0 Ether + mevEth.withdraw(31 ether, User01, User01); + assertEq(address(mevEth).balance, 0 ether); + + // 4. Attackers shares are still worth 32 ether + assertEq(mevEth.convertToAssets(mevEth.balanceOf(User01)), 32 ether); + + // ~10 blocks worth of txs + for (uint256 i = 0; i < 1000; i++) { + // Attacker withdraws 0.01 ETH worth of shares, but because contract has no balance left the queueLength increases by 1. + // Attacker repeats step above many times. + mevEth.withdraw(0.011 ether, User01, User01); + } + + vm.stopPrank(); + vm.startPrank(Operator01); + // Give mevEth enough ether to process all withdrawals + vm.deal(address(mevEth), 100 ether); + + // Block gas limit + mevEth.processWithdrawalQueue(); + } +} From b99f1c295cefb4fb86da5082def558715b4366a0 Mon Sep 17 00:00:00 2001 From: sandybradley Date: Wed, 26 Jul 2023 21:55:37 +0200 Subject: [PATCH 08/18] chore: update withdraw attack test --- test/attacks/WithdrawlQueueDOS.t.sol | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/attacks/WithdrawlQueueDOS.t.sol b/test/attacks/WithdrawlQueueDOS.t.sol index c2a2be9..469dfce 100644 --- a/test/attacks/WithdrawlQueueDOS.t.sol +++ b/test/attacks/WithdrawlQueueDOS.t.sol @@ -8,7 +8,6 @@ contract WithdrawlQueueAttackTest is MevEthTest { vm.deal(User01, 63 ether); vm.startPrank(User01); - address stakingModuleAddress = address(mevEth.stakingModule()); IStakingModule.ValidatorData memory validatorData = mockValidatorData(User01, 32 ether / 1 gwei); // 1. Attacker deposits 63 Eth to MevEth @@ -46,5 +45,9 @@ contract WithdrawlQueueAttackTest is MevEthTest { // Block gas limit mevEth.processWithdrawalQueue(); + for (uint256 i = 0; i < 1000; i++) { + mevEth.claim(i); + } + assertEq(weth.balanceOf(User01), 42 ether); } } From 09d30a42eaf2f08f1ccd1c0e8a0ca19227cd00f8 Mon Sep 17 00:00:00 2001 From: sandybradley Date: Thu, 27 Jul 2023 14:41:45 +0200 Subject: [PATCH 09/18] chore: public withdraw queue params, reduced contract size by changing stakingUnpaused from modifier to function and removing fraction base check --- src/MevEth.sol | 39 ++++++++++------------------ test/attacks/WithdrawlQueueDOS.t.sol | 3 +++ 2 files changed, 17 insertions(+), 25 deletions(-) diff --git a/src/MevEth.sol b/src/MevEth.sol index cf2aed2..5f524f3 100644 --- a/src/MevEth.sol +++ b/src/MevEth.sol @@ -142,12 +142,9 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { event StakingUnpaused(); /// @notice Ensures that staking is not paused when invoking a specific function. - /// @dev This modifier is used on the createValidator, deposit and mint functions. - modifier stakingUnpaused() { - if (stakingPaused) { - revert MevEthErrors.StakingPaused(); - } - _; + /// @dev This check is used on the createValidator, deposit and mint functions. + function _stakingUnpaused() internal view { + if (stakingPaused) revert MevEthErrors.StakingPaused(); } /// @notice Pauses staking on the MevEth contract. @@ -290,7 +287,8 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { /// @notice This function passes through the needed Ether to the Staking module, and the assosiated credentials with it /// @param newData The data needed to create a new validator /// @dev This function is only callable by addresses with the operator role and if staking is unpaused - function createValidator(IStakingModule.ValidatorData calldata newData) external onlyOperator stakingUnpaused { + function createValidator(IStakingModule.ValidatorData calldata newData) external onlyOperator { + _stakingUnpaused(); IStakingModule _stakingModule = stakingModule; // Determine how big deposit is for the validator // *Note this will change if Rocketpool or similar modules are used @@ -375,13 +373,13 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { event WithdrawalQueueClosed(address indexed receipient, uint256 indexed withdrawlId, uint256 assets); /// @notice The length of the withdrawal queue. - uint256 queueLength; + uint256 public queueLength; /// @notice mark the latest withdrawal request that was finalised - uint256 requestsFinalisedUntil; + uint256 public requestsFinalisedUntil; /// @notice Withdrawl amount queued - uint256 withdrawlAmountQueued; + uint256 public withdrawlAmountQueued; /// @notice The mapping representing the withdrawal queue. /// @dev The index in the queue is the key, and the value is the WithdrawalTicket. @@ -416,12 +414,11 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { while (finalised < length) { // Get the next ticket in the queue WithdrawalTicket memory currentTicket = withdrawalQueue[finalised]; - uint256 assetsOwed = currentTicket.amount; // If the balance of the contract has enough ETH to pay the ticket, reserve amount for ticket. - if (available >= assetsOwed) { + if (available >= currentTicket.amount) { ++finalised; - available -= assetsOwed; + available -= currentTicket.amount; } else { break; } @@ -505,7 +502,8 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { /// @param assets The amount of WETH which should be deposited /// @param receiver The address user whom should receive the mevEth out /// @return shares The amount of shares minted - function deposit(uint256 assets, address receiver) external payable stakingUnpaused returns (uint256 shares) { + function deposit(uint256 assets, address receiver) external payable returns (uint256 shares) { + _stakingUnpaused(); // If the deposit is less than the minimum deposit, revert if (assets < MIN_DEPOSIT) revert MevEthErrors.DepositTooSmall(); @@ -547,7 +545,8 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { /// @param shares The amount of shares that should be minted /// @param receiver The address user whom should receive the mevEth out /// @return assets The amount of assets deposited - function mint(uint256 shares, address receiver) external payable stakingUnpaused returns (uint256 assets) { + function mint(uint256 shares, address receiver) external payable returns (uint256 assets) { + _stakingUnpaused(); // If the deposit is less than the minimum deposit, revert if (shares < MIN_DEPOSIT) revert MevEthErrors.DepositTooSmall(); @@ -631,11 +630,6 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { fraction.elastic -= uint128(assets); fraction.base -= uint128(shares); - // If the base is less than the minimum deposit, revert - if (fraction.base < MIN_DEPOSIT) { - revert MevEthErrors.BelowMinimum(); - } - // Burn the shares and emit a withdraw event for offchain listeners to know that a withdraw has occured _burn(owner, shares); @@ -675,11 +669,6 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { fraction.elastic -= uint128(assets); fraction.base -= uint128(shares); - // If the base is less than the minimum deposit, revert - if (fraction.base < MIN_DEPOSIT) { - revert MevEthErrors.BelowMinimum(); - } - // Burn the shares and emit a withdraw event for offchain listeners to know that a withdraw has occured _burn(owner, shares); diff --git a/test/attacks/WithdrawlQueueDOS.t.sol b/test/attacks/WithdrawlQueueDOS.t.sol index 469dfce..9b6a735 100644 --- a/test/attacks/WithdrawlQueueDOS.t.sol +++ b/test/attacks/WithdrawlQueueDOS.t.sol @@ -49,5 +49,8 @@ contract WithdrawlQueueAttackTest is MevEthTest { mevEth.claim(i); } assertEq(weth.balanceOf(User01), 42 ether); + assertEq(mevEth.queueLength(), 1000); + assertEq(mevEth.requestsFinalisedUntil(), 1000); + assertEq(mevEth.withdrawlAmountQueued(), 0); } } From d6663d3bbe6904545b0e456c198e025a5444e9cf Mon Sep 17 00:00:00 2001 From: sandybradley Date: Thu, 27 Jul 2023 14:42:33 +0200 Subject: [PATCH 10/18] chore: remove commented code --- test/unit/ERC4626.t.sol | 31 +------------------------------ 1 file changed, 1 insertion(+), 30 deletions(-) diff --git a/test/unit/ERC4626.t.sol b/test/unit/ERC4626.t.sol index 3da02c2..9a07ac8 100644 --- a/test/unit/ERC4626.t.sol +++ b/test/unit/ERC4626.t.sol @@ -184,11 +184,6 @@ contract ERC4626Test is MevEthTest { // Withdraw 1 mevETH mevEth.withdraw(0.75 ether, User01, User01); - // vm.stopPrank(); - // vm.prank(Operator01); - // mevEth.processWithdrawalQueue(); - // vm.prank(User01); - // mevEth.claim(0); assertEq(mevEth.balanceOf(User01), 0.25 ether); assertEq(weth.balanceOf(User01), 0.75 ether); } @@ -227,11 +222,6 @@ contract ERC4626Test is MevEthTest { // Withdraw 1 mevETH mevEth.withdraw(0.75 ether, User02, User01); - // vm.stopPrank(); - // vm.prank(Operator01); - // mevEth.processWithdrawalQueue(); - // vm.prank(User01); - // mevEth.claim(0); assertEq(mevEth.balanceOf(User01), 0.25 ether); assertEq(weth.balanceOf(User02), 0.75 ether); @@ -255,11 +245,7 @@ contract ERC4626Test is MevEthTest { // Withdraw 1 mevETH mevEth.withdraw(amount, User01, User01); - // vm.stopPrank(); - // vm.prank(Operator01); - // mevEth.processWithdrawalQueue(); - // vm.prank(User01); - // mevEth.claim(0); + assertEq(mevEth.balanceOf(User01), 0 ether); assertEq(weth.balanceOf(User01), amount); } @@ -359,11 +345,6 @@ contract ERC4626Test is MevEthTest { // Redeem 1 mevETH mevEth.redeem(0.75 ether, User01, User01); - // vm.stopPrank(); - // vm.prank(Operator01); - // mevEth.processWithdrawalQueue(); - // vm.prank(User01); - // mevEth.claim(0); assertEq(mevEth.balanceOf(User01), 0.25 ether); assertEq(weth.balanceOf(User01), 0.75 ether); @@ -405,11 +386,6 @@ contract ERC4626Test is MevEthTest { // Withdraw 0.75 mevETH mevEth.redeem(0.75 ether, User02, User01); - // vm.stopPrank(); - // vm.prank(Operator01); - // mevEth.processWithdrawalQueue(); - // vm.prank(User01); - // mevEth.claim(0); assertEq(mevEth.balanceOf(User01), 0.25 ether); assertEq(weth.balanceOf(User02), 0.75 ether); @@ -431,11 +407,6 @@ contract ERC4626Test is MevEthTest { // Redeem 1 mevETH mevEth.redeem(amountToRedeem, User01, User01); - // vm.stopPrank(); - // vm.prank(Operator01); - // mevEth.processWithdrawalQueue(); - // vm.prank(User01); - // mevEth.claim(0); assertEq(mevEth.balanceOf(User01), amountLeftOver); assertEq(weth.balanceOf(User01), amountToRedeem); From 0d0dc26fb3c5e15b289cb71586c3e2fdd8593995 Mon Sep 17 00:00:00 2001 From: sandybradley Date: Thu, 27 Jul 2023 14:55:40 +0200 Subject: [PATCH 11/18] chore: remove unused custom errors --- src/interfaces/Errors.sol | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/src/interfaces/Errors.sol b/src/interfaces/Errors.sol index 2707b3f..1e7e47a 100644 --- a/src/interfaces/Errors.sol +++ b/src/interfaces/Errors.sol @@ -4,27 +4,10 @@ pragma solidity 0.8.19; interface MevEthErrors { /// Errors - error BelowMinimum(); - error DepositFailed(); - error InsufficientBufferedEth(); - error TooManyValidatorRegistrations(); - error ExceedsStakingAllowance(); error StakingPaused(); error NotEnoughEth(); - error DepositTooLow(); - error ZeroShares(); error ZeroValue(); - error ReportedBeaconValidatorsGreaterThanTotalValidators(); - error ReportedBeaconValidatorsDecreased(); - error BeaconDepositFailed(); - error InvalidWithdrawalCredentials(); - error OperatorsNotCommitted(); - error OperatorMaxValidatorsReached(); - error OperatorNotCommitted(); - error MaxValidatorError(); error InvalidOperator(); - error ValidatorPreviouslyRegistered(); - error NotAuthorized(); error DepositTooSmall(); error InvalidSender(); error PrematureStakingModuleUpdateFinalization(); From 7adb9479db00483471f1e03305a1957f026f360b Mon Sep 17 00:00:00 2001 From: sandybradley Date: Fri, 28 Jul 2023 19:40:56 +0200 Subject: [PATCH 12/18] feat: replace process queue loop with batch update --- src/MevEth.sol | 40 +++++++++++++--------------- src/interfaces/Errors.sol | 2 ++ test/attacks/WithdrawlQueueDOS.t.sol | 4 +-- test/unit/Queue.t.sol | 16 ++++++----- 4 files changed, 32 insertions(+), 30 deletions(-) diff --git a/src/MevEth.sol b/src/MevEth.sol index 5f524f3..b715af7 100644 --- a/src/MevEth.sol +++ b/src/MevEth.sol @@ -362,10 +362,12 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { /// @custom:field claimed True if this receiver has received ticket funds. /// @custom:field receiver The receiever of the ETH specified in the WithdrawalTicket. /// @custom:field amount The amount of ETH to send to the receiver when the ticket is processed. + /// @custom:field accumulatedAmount Keep a running sum of all requested ETH struct WithdrawalTicket { bool claimed; address receiver; uint256 amount; + uint256 accumulatedAmount; } /// @notice Event emitted when a withdrawal ticket is added to the queue. @@ -399,32 +401,20 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { } /// @notice Processes the withdrawal queue, reserving any pending withdrawals with the contract's available balance. - function processWithdrawalQueue() external onlyOperator { + function processWithdrawalQueue(uint256 newRequestsFinalisedUntil) external onlyOperator { + if (newRequestsFinalisedUntil > queueLength) revert MevEthErrors.IndexExceedsQueueLength(); uint256 balance = address(this).balance; if (withdrawlAmountQueued >= balance) revert MevEthErrors.NotEnoughEth(); - uint256 available = balance - withdrawlAmountQueued; - // Get the current length of the queue - uint256 length = queueLength; - uint256 finalised = requestsFinalisedUntil; + if (newRequestsFinalisedUntil < finalised) revert MevEthErrors.AlreadyFinalised(); - // While the queue is not empty, process the next ticket in the queue - while (finalised < length) { - // Get the next ticket in the queue - WithdrawalTicket memory currentTicket = withdrawalQueue[finalised]; - - // If the balance of the contract has enough ETH to pay the ticket, reserve amount for ticket. - if (available >= currentTicket.amount) { - ++finalised; - available -= currentTicket.amount; - } else { - break; - } - } - requestsFinalisedUntil = finalised; - withdrawlAmountQueued = balance - available; + uint256 delta = withdrawalQueue[newRequestsFinalisedUntil].accumulatedAmount - withdrawalQueue[finalised].accumulatedAmount; + if (available < delta) revert MevEthErrors.NotEnoughEth(); + + requestsFinalisedUntil = newRequestsFinalisedUntil; + withdrawlAmountQueued += delta; } /*////////////////////////////////////////////////////////////// @@ -589,9 +579,15 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { if (availableBalance < assets) { uint256 amountOwed = assets - availableBalance; - withdrawalQueue[queueLength] = WithdrawalTicket({ claimed: false, receiver: receiver, amount: amountOwed }); - emit WithdrawalQueueOpened(receiver, queueLength, amountOwed); + // uint256 accumulatedAmount = _isZero(queueLength) ? 0 : withdrawalQueue[queueLength - 1].accumulatedAmount; ++queueLength; + withdrawalQueue[queueLength] = WithdrawalTicket({ + claimed: false, + receiver: receiver, + amount: amountOwed, + accumulatedAmount: withdrawalQueue[queueLength - 1].accumulatedAmount + amountOwed + }); + emit WithdrawalQueueOpened(receiver, queueLength, amountOwed); assets = availableBalance; } if (!_isZero(assets)) { diff --git a/src/interfaces/Errors.sol b/src/interfaces/Errors.sol index 1e7e47a..0da18a1 100644 --- a/src/interfaces/Errors.sol +++ b/src/interfaces/Errors.sol @@ -25,4 +25,6 @@ interface MevEthErrors { error WithdrawTooSmall(); error NotFinalised(); error AlreadyClaimed(); + error AlreadyFinalised(); + error IndexExceedsQueueLength(); } diff --git a/test/attacks/WithdrawlQueueDOS.t.sol b/test/attacks/WithdrawlQueueDOS.t.sol index 9b6a735..79c2301 100644 --- a/test/attacks/WithdrawlQueueDOS.t.sol +++ b/test/attacks/WithdrawlQueueDOS.t.sol @@ -44,8 +44,8 @@ contract WithdrawlQueueAttackTest is MevEthTest { vm.deal(address(mevEth), 100 ether); // Block gas limit - mevEth.processWithdrawalQueue(); - for (uint256 i = 0; i < 1000; i++) { + mevEth.processWithdrawalQueue(mevEth.queueLength()); + for (uint256 i = 1; i < 1001; i++) { mevEth.claim(i); } assertEq(weth.balanceOf(User01), 42 ether); diff --git a/test/unit/Queue.t.sol b/test/unit/Queue.t.sol index a352233..7d418dc 100644 --- a/test/unit/Queue.t.sol +++ b/test/unit/Queue.t.sol @@ -6,6 +6,10 @@ import "../MevEthTest.sol"; import "src/interfaces/Errors.sol"; contract QueueTest is MevEthTest { + function setUp() public override { + super.setUp(); + } + function testOverflowsDepositsToQueueWithWithdraw() public { vm.deal(User01, 64 ether); vm.startPrank(User01); @@ -46,10 +50,10 @@ contract QueueTest is MevEthTest { vm.deal(stakingModuleAddress, 32 ether); vm.prank(SamBacha); IStakingModule(stakingModuleAddress).payValidatorWithdraw(32 ether); - vm.prank(Operator01); - mevEth.processWithdrawalQueue(); + vm.startPrank(Operator01); + mevEth.processWithdrawalQueue(mevEth.queueLength()); - mevEth.claim(0); + mevEth.claim(1); Vm.Log[] memory entries2 = vm.getRecordedLogs(); assertEq(entries2[2].topics[0], keccak256("WithdrawalQueueClosed(address,uint256,uint256)")); @@ -99,10 +103,10 @@ contract QueueTest is MevEthTest { vm.deal(stakingModuleAddress, 32 ether); vm.prank(SamBacha); IStakingModule(stakingModuleAddress).payValidatorWithdraw(32 ether); - vm.prank(Operator01); - mevEth.processWithdrawalQueue(); + vm.startPrank(Operator01); + mevEth.processWithdrawalQueue(mevEth.queueLength()); - mevEth.claim(0); + mevEth.claim(1); Vm.Log[] memory entries2 = vm.getRecordedLogs(); assertEq(entries2[2].topics[0], keccak256("WithdrawalQueueClosed(address,uint256,uint256)")); From 4d2812d2c42455545523fe6068e53b1c0eb2b049 Mon Sep 17 00:00:00 2001 From: sandybradley Date: Fri, 28 Jul 2023 20:35:33 +0200 Subject: [PATCH 13/18] chore: reduce contract size --- foundry.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/foundry.toml b/foundry.toml index bae6a97..463607f 100644 --- a/foundry.toml +++ b/foundry.toml @@ -6,7 +6,7 @@ src = 'src' out = 'out' libs = ['lib'] optimizer = true -optimizer_runs = 2000 +optimizer_runs = 500 via_ir = true fuzz_runs = 500 deny_warnings = false From ebec5442899191fcde730151a0a42dd5aa9c87f4 Mon Sep 17 00:00:00 2001 From: sandybradley Date: Sun, 30 Jul 2023 19:31:57 +0200 Subject: [PATCH 14/18] chore: spelling fixes, commented code removal and refactored WithdrawalQueue sizes --- src/MevEth.sol | 49 +++++++++---------- ...ueueDOS.t.sol => WithdrawalQueueDOS.t.sol} | 4 +- 2 files changed, 26 insertions(+), 27 deletions(-) rename test/attacks/{WithdrawlQueueDOS.t.sol => WithdrawalQueueDOS.t.sol} (94%) diff --git a/src/MevEth.sol b/src/MevEth.sol index b715af7..ed300d9 100644 --- a/src/MevEth.sol +++ b/src/MevEth.sol @@ -99,7 +99,7 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { /// @return uint256 The required Ether buffer. function calculateNeededEtherBuffer() public view returns (uint256) { unchecked { - return max(withdrawlAmountQueued, 31 ether); + return max(withdrawalAmountQueued, 31 ether); } } @@ -366,13 +366,13 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { struct WithdrawalTicket { bool claimed; address receiver; - uint256 amount; - uint256 accumulatedAmount; + uint128 amount; + uint128 accumulatedAmount; } /// @notice Event emitted when a withdrawal ticket is added to the queue. - event WithdrawalQueueOpened(address indexed receipient, uint256 indexed withdrawlId, uint256 assets); - event WithdrawalQueueClosed(address indexed receipient, uint256 indexed withdrawlId, uint256 assets); + event WithdrawalQueueOpened(address indexed recipient, uint256 indexed withdrawalId, uint256 assets); + event WithdrawalQueueClosed(address indexed recipient, uint256 indexed withdrawalId, uint256 assets); /// @notice The length of the withdrawal queue. uint256 public queueLength; @@ -380,41 +380,41 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { /// @notice mark the latest withdrawal request that was finalised uint256 public requestsFinalisedUntil; - /// @notice Withdrawl amount queued - uint256 public withdrawlAmountQueued; + /// @notice Withdrawal amount queued + uint256 public withdrawalAmountQueued; /// @notice The mapping representing the withdrawal queue. /// @dev The index in the queue is the key, and the value is the WithdrawalTicket. mapping(uint256 ticketNumber => WithdrawalTicket ticket) public withdrawalQueue; - /// @notice Claim Finalised Withdrawl Ticket - /// @param withdrawlId Unique ID of the withdrawl ticket - function claim(uint256 withdrawlId) external { - if (withdrawlId > requestsFinalisedUntil) revert MevEthErrors.NotFinalised(); - WithdrawalTicket memory ticket = withdrawalQueue[withdrawlId]; + /// @notice Claim Finalised Withdrawal Ticket + /// @param withdrawalId Unique ID of the withdrawal ticket + function claim(uint256 withdrawalId) external { + if (withdrawalId > requestsFinalisedUntil) revert MevEthErrors.NotFinalised(); + WithdrawalTicket storage ticket = withdrawalQueue[withdrawalId]; if (ticket.claimed) revert MevEthErrors.AlreadyClaimed(); - withdrawalQueue[withdrawlId].claimed = true; - withdrawlAmountQueued -= ticket.amount; - emit WithdrawalQueueClosed(ticket.receiver, withdrawlId, ticket.amount); - WETH.deposit{ value: ticket.amount }(); - ERC20(address(WETH)).safeTransfer(ticket.receiver, ticket.amount); + withdrawalQueue[withdrawalId].claimed = true; + withdrawalAmountQueued -= uint256(ticket.amount); + emit WithdrawalQueueClosed(ticket.receiver, withdrawalId, uint256(ticket.amount)); + WETH.deposit{ value: uint256(ticket.amount) }(); + ERC20(address(WETH)).safeTransfer(ticket.receiver, uint256(ticket.amount)); } /// @notice Processes the withdrawal queue, reserving any pending withdrawals with the contract's available balance. function processWithdrawalQueue(uint256 newRequestsFinalisedUntil) external onlyOperator { if (newRequestsFinalisedUntil > queueLength) revert MevEthErrors.IndexExceedsQueueLength(); uint256 balance = address(this).balance; - if (withdrawlAmountQueued >= balance) revert MevEthErrors.NotEnoughEth(); - uint256 available = balance - withdrawlAmountQueued; + if (withdrawalAmountQueued >= balance) revert MevEthErrors.NotEnoughEth(); + uint256 available = balance - withdrawalAmountQueued; uint256 finalised = requestsFinalisedUntil; if (newRequestsFinalisedUntil < finalised) revert MevEthErrors.AlreadyFinalised(); - uint256 delta = withdrawalQueue[newRequestsFinalisedUntil].accumulatedAmount - withdrawalQueue[finalised].accumulatedAmount; + uint256 delta = uint256(withdrawalQueue[newRequestsFinalisedUntil].accumulatedAmount - withdrawalQueue[finalised].accumulatedAmount); if (available < delta) revert MevEthErrors.NotEnoughEth(); requestsFinalisedUntil = newRequestsFinalisedUntil; - withdrawlAmountQueued += delta; + withdrawalAmountQueued += delta; } /*////////////////////////////////////////////////////////////// @@ -575,11 +575,10 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { /// @param owner The address of the owner of the mevEth /// @param assets The amount of assets that should be withdrawn function _withdraw(address receiver, address owner, uint256 assets) internal { - uint256 availableBalance = address(this).balance - withdrawlAmountQueued; // available balance will be adjusted + uint256 availableBalance = address(this).balance - withdrawalAmountQueued; // available balance will be adjusted if (availableBalance < assets) { - uint256 amountOwed = assets - availableBalance; - // uint256 accumulatedAmount = _isZero(queueLength) ? 0 : withdrawalQueue[queueLength - 1].accumulatedAmount; + uint128 amountOwed = uint128(assets - availableBalance); ++queueLength; withdrawalQueue[queueLength] = WithdrawalTicket({ claimed: false, @@ -587,7 +586,7 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { amount: amountOwed, accumulatedAmount: withdrawalQueue[queueLength - 1].accumulatedAmount + amountOwed }); - emit WithdrawalQueueOpened(receiver, queueLength, amountOwed); + emit WithdrawalQueueOpened(receiver, queueLength, uint256(amountOwed)); assets = availableBalance; } if (!_isZero(assets)) { diff --git a/test/attacks/WithdrawlQueueDOS.t.sol b/test/attacks/WithdrawalQueueDOS.t.sol similarity index 94% rename from test/attacks/WithdrawlQueueDOS.t.sol rename to test/attacks/WithdrawalQueueDOS.t.sol index 79c2301..bc119b9 100644 --- a/test/attacks/WithdrawlQueueDOS.t.sol +++ b/test/attacks/WithdrawalQueueDOS.t.sol @@ -3,7 +3,7 @@ pragma solidity 0.8.19; import "../MevEthTest.sol"; -contract WithdrawlQueueAttackTest is MevEthTest { +contract WithdrawalQueueAttackTest is MevEthTest { function testDosAttackQueueLength() public { vm.deal(User01, 63 ether); vm.startPrank(User01); @@ -51,6 +51,6 @@ contract WithdrawlQueueAttackTest is MevEthTest { assertEq(weth.balanceOf(User01), 42 ether); assertEq(mevEth.queueLength(), 1000); assertEq(mevEth.requestsFinalisedUntil(), 1000); - assertEq(mevEth.withdrawlAmountQueued(), 0); + assertEq(mevEth.withdrawalAmountQueued(), 0); } } From ab1519c8764e00a59b3f5e781c0ab7d44189df3c Mon Sep 17 00:00:00 2001 From: Sandy Bradley Date: Thu, 3 Aug 2023 14:23:10 +0200 Subject: [PATCH 15/18] feat: rate provider test added (#122) * feat: rate provider test added * chore: fix edge case fuzz test --- src/interfaces/IStakingModule.sol | 1 + test/unit/Admin.t.sol | 1 + test/unit/MevEthRateProvider.t.sol | 31 ++++++++++++++++++++++++++++++ 3 files changed, 33 insertions(+) create mode 100644 test/unit/MevEthRateProvider.t.sol diff --git a/src/interfaces/IStakingModule.sol b/src/interfaces/IStakingModule.sol index 32ab097..38f76b2 100644 --- a/src/interfaces/IStakingModule.sol +++ b/src/interfaces/IStakingModule.sol @@ -32,6 +32,7 @@ interface IStakingModule { function VALIDATOR_DEPOSIT_SIZE() external view returns (uint256); // onlyAdmin Functions + function payRewards() external; function payValidatorWithdraw(uint256 amount) external; function recoverToken(address token, address recipient, uint256 amount) external; } diff --git a/test/unit/Admin.t.sol b/test/unit/Admin.t.sol index 1a1a28c..0c07413 100644 --- a/test/unit/Admin.t.sol +++ b/test/unit/Admin.t.sol @@ -99,6 +99,7 @@ contract MevAdminTest is MevEthTest { function testNegativeAddOperator(address newOperator) public { vm.assume(newOperator != 0x7109709ECfa91a80626fF3989D68f67F5b1DD12D); vm.assume(newOperator != 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf); + vm.assume(newOperator != Operator01); vm.assume(newOperator != address(0)); vm.expectRevert(Auth.Unauthorized.selector); mevEth.addOperator(newOperator); diff --git a/test/unit/MevEthRateProvider.t.sol b/test/unit/MevEthRateProvider.t.sol new file mode 100644 index 0000000..4cf0403 --- /dev/null +++ b/test/unit/MevEthRateProvider.t.sol @@ -0,0 +1,31 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.19; + +import "forge-std/console.sol"; +import "../MevEthTest.sol"; +import "src/interfaces/Errors.sol"; +import "src/MevEthRateProvider.sol"; +import "src/interfaces/IMevEth.sol"; + +contract MevEthRateProviderTest is MevEthTest { + MevETHRateProvider provider; + + function setUp() public override { + super.setUp(); + provider = new MevETHRateProvider(IMevEth(address(mevEth))); + } + + function testRate() public { + assertEq(provider.getRate(), 1 ether); + } + + function testRateUpdate() public { + vm.deal(address(this), 10 ether); + mevEth.deposit{ value: 10 ether }(10 ether, address(this)); + address staker = address(mevEth.stakingModule()); + vm.deal(staker, 2 ether); + vm.prank(SamBacha); + IStakingModule(staker).payRewards(); + assertGt(provider.getRate(), 1 ether); + } +} From 0aaae4c29b681c90fd70787a1d57d4e1794c1242 Mon Sep 17 00:00:00 2001 From: Sandy Bradley Date: Thu, 3 Aug 2023 14:46:52 +0200 Subject: [PATCH 16/18] feat: audit informational cleanup (#144) --- README.md | 4 ++-- src/MevEth.sol | 11 ++++++----- src/MevEthShareVault.sol | 2 +- src/libraries/Auth.sol | 16 ++++++++++------ test/MevEthTest.sol | 6 ++++-- test/attacks/ReentrancyAttack.sol | 4 +++- test/unit/Admin.t.sol | 12 ++++++------ 7 files changed, 32 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index d162cc7..2a3ed3d 100644 --- a/README.md +++ b/README.md @@ -85,11 +85,11 @@ mev-eth is has a couple core modules and functionality. ### Ownership -First, because mev-eth is a centralized LSR, and dependent on Manifold Finance to actuall stake the Ether, ownership is given, with a couple key roles. An address can be designed as an operator, operators are intended to be automated, keeper style addresses which can redirect Ether to beacon chain validators. Operators are also expected to post oracle updates from the Beacon chain to update the contract on any rewards accrued. Additionally, there is the ManifoldOwner role, which gives the rights to control key management functions such as withdrawing fees, and setting various configuration variables such as cache balance threshold, where Ether will be held to buffer withdrawls. +First, because mev-eth is a centralized LSR, and dependent on Manifold Finance to actually stake the Ether, ownership is given, with a couple key roles. An address can be designed as an operator, operators are intended to be automated, keeper style addresses which can redirect Ether to beacon chain validators. Operators are also expected to post oracle updates from the Beacon chain to update the contract on any rewards accrued. Additionally, there is the ManifoldOwner role, which gives the rights to control key management functions such as withdrawing fees, and setting various configuration variables such as cache balance threshold, where Ether will be held to buffer withdrawals. ### Token Design -mev-eth supports the ERC4626 interface to handle itself as an LSR. This allows many key integrations, such as Yearn Vault integrations, or any other protocols which require a yield source. This also means that mev-eth supports ERC20 as a base transferable token. Breaking from ERC4626, mev-eth also supports deposits via its fallback (recieve technically) function for call-data free deposits. +mev-eth supports the ERC4626 interface to handle itself as an LSR. This allows many key integrations, such as Yearn Vault integrations, or any other protocols which require a yield source. This also means that mev-eth supports ERC20 as a base transferable token. Breaking from ERC4626, mev-eth also supports deposits via its fallback (receive technically) function for call-data free deposits. The token keeps track of this by accounting with simple fractional math, which while a bit confusing, is the most simplistic approach, where each mev-eth token is a share which accumulates interest, and as interest grows will eventually be worth greater than it 1 eth per. diff --git a/src/MevEth.sol b/src/MevEth.sol index ed300d9..5fc88aa 100644 --- a/src/MevEth.sol +++ b/src/MevEth.sol @@ -50,11 +50,11 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { /// @notice Timestamp when pending mevEthShareVault update can be finalized. uint64 public pendingMevEthShareVaultCommittedTimestamp; /// @notice Time delay before staking module or share vault can be finalized. - uint64 public constant MODULE_UPDATE_TIME_DELAY = 7 days; + uint64 internal constant MODULE_UPDATE_TIME_DELAY = 7 days; /// @notice Max amount of ETH that can be deposited. - uint128 public constant MAX_DEPOSIT = 2 ** 128 - 1; + uint128 internal constant MAX_DEPOSIT = type(uint128).max; /// @notice Min amount of ETH that can be deposited. - uint128 public constant MIN_DEPOSIT = 10_000_000_000_000_000; // 0.01 eth + uint128 public constant MIN_DEPOSIT = 0.01 ether; // 0.01 eth /// @notice The address of the MevEthShareVault. address public mevEthShareVault; /// @notice The address of the pending MevEthShareVault when a new vault has been comitted but not finalized. @@ -537,12 +537,13 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { /// @return assets The amount of assets deposited function mint(uint256 shares, address receiver) external payable returns (uint256 assets) { _stakingUnpaused(); - // If the deposit is less than the minimum deposit, revert - if (shares < MIN_DEPOSIT) revert MevEthErrors.DepositTooSmall(); // Convert the shares to assets and update the fraction elastic and base assets = convertToAssets(shares); + // If the deposit is less than the minimum deposit, revert + if (assets < MIN_DEPOSIT) revert MevEthErrors.DepositTooSmall(); + fraction.elastic += uint128(assets); fraction.base += uint128(shares); diff --git a/src/MevEthShareVault.sol b/src/MevEthShareVault.sol index db9953e..c4a67b2 100644 --- a/src/MevEthShareVault.sol +++ b/src/MevEthShareVault.sol @@ -114,7 +114,7 @@ contract MevEthShareVault is Auth, IMevEthShareVault { /// Validators associated with the MevETH protocol set the block builder's address as the feeRecepient for the block. /// The block builder attaches a transaction to the end of the block sending the MEV rewards to the MevEthShareVault. /// This then emits the RewardPayment event, allowing the offchain operators to track the protocolFeesOwed. - /// This approach trusts that the operators are acting honestly and the protocolFeesOwed is accurately caculated. + /// This approach trusts that the operators are acting honestly and the protocolFeesOwed is accurately calculated. function logRewards(uint128 protocolFeesOwed) external onlyOperator { // Cahce the protocol balance diff --git a/src/libraries/Auth.sol b/src/libraries/Auth.sol index e078376..de32d20 100644 --- a/src/libraries/Auth.sol +++ b/src/libraries/Auth.sol @@ -3,18 +3,16 @@ pragma solidity 0.8.19; contract Auth { error Unauthorized(); - error WrongRole(); - - enum Roles { - OPERATOR, - ADMIN - } + error NoAdmin(); event AdminAdded(address indexed newAdmin); event AdminDeleted(address indexed oldAdmin); event OperatorAdded(address indexed newOperator); event OperatorDeleted(address indexed oldOperator); + // admin counter (assuming 255 admins to be max) + uint8 adminsCounter; + // Keeps track of all operators mapping(address => bool) public operators; @@ -23,6 +21,9 @@ contract Auth { constructor(address initialAdmin) { admins[initialAdmin] = true; + unchecked { + ++adminsCounter; + } operators[initialAdmin] = true; } @@ -48,11 +49,14 @@ contract Auth { Maintenance Functions //////////////////////////////////////////////////////////////*/ function addAdmin(address newAdmin) external onlyAdmin { + ++adminsCounter; admins[newAdmin] = true; emit AdminAdded(newAdmin); } function deleteAdmin(address oldAdmin) external onlyAdmin { + --adminsCounter; + if (adminsCounter == 0) revert NoAdmin(); admins[oldAdmin] = false; emit AdminDeleted(oldAdmin); } diff --git a/test/MevEthTest.sol b/test/MevEthTest.sol index ac917c0..08921eb 100644 --- a/test/MevEthTest.sol +++ b/test/MevEthTest.sol @@ -20,6 +20,8 @@ contract MevEthTest is Test { uint16 constant POLYGON_ID = 109; uint16 constant ARBITRUM_ID = 110; + uint64 MODULE_UPDATE_TIME_DELAY = 7 days; + // Admin account uint256 constant SAM_BACHA_PRIVATE_KEY = 0x01; @@ -141,7 +143,7 @@ contract MevEthTest is Test { // Helper function to update the staking module for testing function _updateStakingModule(IStakingModule newStakingModule) internal { // Commit update to the staking module - uint64 finalizationTimestamp = uint64(block.timestamp + mevEth.MODULE_UPDATE_TIME_DELAY()); + uint64 finalizationTimestamp = uint64(block.timestamp + MODULE_UPDATE_TIME_DELAY); vm.prank(SamBacha); mevEth.commitUpdateStakingModule(newStakingModule); @@ -159,7 +161,7 @@ contract MevEthTest is Test { // Helper function to update the share vault for testing function _updateShareVault(address newShareVault) internal { // Commit update to the staking module - uint64 finalizationTimestamp = uint64(block.timestamp + mevEth.MODULE_UPDATE_TIME_DELAY()); + uint64 finalizationTimestamp = uint64(block.timestamp + MODULE_UPDATE_TIME_DELAY); vm.prank(SamBacha); mevEth.commitUpdateMevEthShareVault(newShareVault); diff --git a/test/attacks/ReentrancyAttack.sol b/test/attacks/ReentrancyAttack.sol index a99aa53..c68a5e3 100644 --- a/test/attacks/ReentrancyAttack.sol +++ b/test/attacks/ReentrancyAttack.sol @@ -4,6 +4,8 @@ pragma solidity 0.8.19; import "../MevEthTest.sol"; contract ReentrancyAttackTest is MevEthTest { + uint128 MAX_DEPOSIT = type(uint128).max; + function setUp() public override { super.setUp(); vm.deal(User02, 0.1 ether); @@ -20,7 +22,7 @@ contract ReentrancyAttackTest is MevEthTest { function testAttack(uint128 amount) external payable { vm.assume(amount > mevEth.MIN_DEPOSIT()); - vm.assume(amount < mevEth.MAX_DEPOSIT() - 0.1 ether); + vm.assume(amount < MAX_DEPOSIT - 0.1 ether); vm.deal(address(this), amount); uint256 bal = address(this).balance; mevEth.deposit{ value: amount }(amount, address(this)); diff --git a/test/unit/Admin.t.sol b/test/unit/Admin.t.sol index 0c07413..9a7ae1d 100644 --- a/test/unit/Admin.t.sol +++ b/test/unit/Admin.t.sol @@ -252,7 +252,7 @@ contract MevAdminTest is MevEthTest { // Commit an update to the staking module and check the effects vm.expectEmit(true, true, true, false, address(mevEth)); - uint64 finalizationTimestamp = uint64(block.timestamp + mevEth.MODULE_UPDATE_TIME_DELAY()); + uint64 finalizationTimestamp = uint64(block.timestamp + MODULE_UPDATE_TIME_DELAY); emit StakingModuleUpdateCommitted(existingStakingModule, address(newModule), finalizationTimestamp); vm.prank(SamBacha); @@ -293,7 +293,7 @@ contract MevAdminTest is MevEthTest { address existingStakingModule = address(mevEth.stakingModule()); // Commit an update to the staking module - uint64 finalizationTimestamp = uint64(block.timestamp + mevEth.MODULE_UPDATE_TIME_DELAY()); + uint64 finalizationTimestamp = uint64(block.timestamp + MODULE_UPDATE_TIME_DELAY); vm.prank(SamBacha); mevEth.commitUpdateStakingModule(IStakingModule(address(newModule))); @@ -329,7 +329,7 @@ contract MevAdminTest is MevEthTest { // Commit a new staking module DepositContract newModule = new DepositContract(); address existingStakingModule = address(mevEth.stakingModule()); - uint64 finalizationTimestamp = uint64(block.timestamp + mevEth.MODULE_UPDATE_TIME_DELAY()); + uint64 finalizationTimestamp = uint64(block.timestamp + MODULE_UPDATE_TIME_DELAY); vm.prank(SamBacha); mevEth.commitUpdateStakingModule(IStakingModule(address(newModule))); @@ -417,7 +417,7 @@ contract MevAdminTest is MevEthTest { // Commit an update to the staking module and check the effects vm.expectEmit(true, true, true, false, address(mevEth)); - uint64 finalizationTimestamp = uint64(block.timestamp + mevEth.MODULE_UPDATE_TIME_DELAY()); + uint64 finalizationTimestamp = uint64(block.timestamp + MODULE_UPDATE_TIME_DELAY); emit MevEthShareVaultUpdateCommitted(existingVault, newVault, finalizationTimestamp); vm.prank(SamBacha); @@ -458,7 +458,7 @@ contract MevAdminTest is MevEthTest { address existingVault = address(mevEth.mevEthShareVault()); // Commit an update to the mev share vault - uint64 finalizationTimestamp = uint64(block.timestamp + mevEth.MODULE_UPDATE_TIME_DELAY()); + uint64 finalizationTimestamp = uint64(block.timestamp + MODULE_UPDATE_TIME_DELAY); vm.prank(SamBacha); mevEth.commitUpdateMevEthShareVault(newVault); @@ -495,7 +495,7 @@ contract MevAdminTest is MevEthTest { address existingVault = address(mevEth.mevEthShareVault()); // Commit an update to the mev share vault - uint64 finalizationTimestamp = uint64(block.timestamp + mevEth.MODULE_UPDATE_TIME_DELAY()); + uint64 finalizationTimestamp = uint64(block.timestamp + MODULE_UPDATE_TIME_DELAY); vm.prank(SamBacha); mevEth.commitUpdateMevEthShareVault(newVault); From ba0758aa809b7515ec071a8a599cd71510220a3e Mon Sep 17 00:00:00 2001 From: Sandy Bradley Date: Thu, 3 Aug 2023 15:07:03 +0200 Subject: [PATCH 17/18] chore: update solady (#147) --- .gitmodules | 4 +--- lib/solady | 1 - nix/packages.nix | 2 +- 3 files changed, 2 insertions(+), 5 deletions(-) delete mode 160000 lib/solady diff --git a/.gitmodules b/.gitmodules index 9ad5d8a..e5f02d9 100644 --- a/.gitmodules +++ b/.gitmodules @@ -15,6 +15,4 @@ [submodule "lib/openzeppelin-contracts"] path = lib/openzeppelin-contracts url = https://github.com/OpenZeppelin/openzeppelin-contracts -[submodule "lib/solady"] - path = lib/solady - url = https://github.com/Vectorized/solady + diff --git a/lib/solady b/lib/solady deleted file mode 160000 index a549dab..0000000 --- a/lib/solady +++ /dev/null @@ -1 +0,0 @@ -Subproject commit a549dab581488018fbd334871f99bb9170a2fdb9 diff --git a/nix/packages.nix b/nix/packages.nix index 898b5d0..e9bc0f4 100644 --- a/nix/packages.nix +++ b/nix/packages.nix @@ -28,7 +28,7 @@ configurePhase = '' cp -r ${inputs.forge-std} lib/forge-std cp -r ${inputs.openzeppelin-contracts} lib/openzeppelin-contracts - cp -r ${inputs.solady} lib/solady + cp -r ${inputs.pigeon} lib/pigeon cp -r ${inputs.solmate} lib/solmate ''; From c9e4f4cf7756d51906dd6ff2f3ec140426ea6b95 Mon Sep 17 00:00:00 2001 From: Sandy Bradley Date: Fri, 4 Aug 2023 08:54:35 +0200 Subject: [PATCH 18/18] feat: revert deposit mismatch (#146) * feat: refund user excess deposit * feat: revert instead of refund excess deposit --- src/MevEth.sol | 2 +- test/unit/ERC4626.t.sol | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/MevEth.sol b/src/MevEth.sol index 5fc88aa..835888a 100644 --- a/src/MevEth.sol +++ b/src/MevEth.sol @@ -484,7 +484,7 @@ contract MevEth is OFTWithFee, IERC4626, ITinyMevEth { ERC20(address(WETH)).safeTransferFrom(msg.sender, address(this), assets); WETH.withdraw(assets); } else { - if (msg.value < assets) revert MevEthErrors.DepositTooSmall(); + if (msg.value != assets) revert MevEthErrors.WrongDepositAmount(); } } diff --git a/test/unit/ERC4626.t.sol b/test/unit/ERC4626.t.sol index 9a07ac8..7e5041f 100644 --- a/test/unit/ERC4626.t.sol +++ b/test/unit/ERC4626.t.sol @@ -100,6 +100,15 @@ contract ERC4626Test is MevEthTest { assertEq(base, 1 ether); } + function testExcessDeposit() public { + vm.deal(User01, 1.1 ether); + vm.startPrank(User01); + + vm.expectRevert(MevEthErrors.WrongDepositAmount.selector); + // Deposit 1 ETH into the mevETH contract with an excess payment + mevEth.deposit{ value: 1.1 ether }(1 ether, User01); + } + function testFuzzSimpleDeposit(uint256 amount) public { vm.assume(amount > mevEth.MIN_DEPOSIT()); vm.assume(amount < 2 ** 128 - 1);