From ecf6c59bbe751844ab1234a2d1c0fa38139ca26a Mon Sep 17 00:00:00 2001 From: wadealexc Date: Tue, 13 Feb 2024 16:53:02 +0000 Subject: [PATCH] feat: prevent queuing withdrawals to other addresses --- docs/core/DelegationManager.md | 13 +++--- src/contracts/core/DelegationManager.sol | 2 +- src/test/Withdrawals.t.sol | 54 +++--------------------- src/test/unit/DelegationUnit.t.sol | 38 ++++++++--------- 4 files changed, 29 insertions(+), 78 deletions(-) diff --git a/docs/core/DelegationManager.md b/docs/core/DelegationManager.md index 40641fdc4..2b2f356a0 100644 --- a/docs/core/DelegationManager.md +++ b/docs/core/DelegationManager.md @@ -226,20 +226,18 @@ function queueWithdrawals( Allows the caller to queue one or more withdrawals of their held shares across any strategy (in either/both the `EigenPodManager` or `StrategyManager`). If the caller is delegated to an Operator, the `shares` and `strategies` being withdrawn are immediately removed from that Operator's delegated share balances. Note that if the caller is an Operator, this still applies, as Operators are essentially delegated to themselves. -`queueWithdrawals` works very similarly to `undelegate`, except that the caller is not undelegated, and also may: -* Choose which strategies and how many shares to withdraw (as opposed to ALL shares/strategies) -* Specify a `withdrawer` to receive withdrawn funds once the withdrawal is completed +`queueWithdrawals` works very similarly to `undelegate`, except that the caller is not undelegated, and also may choose which strategies and how many shares to withdraw (as opposed to ALL shares/strategies). All shares being withdrawn (whether via the `EigenPodManager` or `StrategyManager`) are removed while the withdrawals are in the queue. -Withdrawals can be completed by the `withdrawer` after max(`minWithdrawalDelayBlocks`, `strategyWithdrawalDelayBlocks[strategy]`) such that `strategy` represents the queued strategies to be withdrawn. Withdrawals do not require the `withdrawer` to "fully exit" from the system -- they may choose to receive their shares back in full once the withdrawal is completed (see [`completeQueuedWithdrawal`](#completequeuedwithdrawal) for details). +Withdrawals can be completed by the caller after max(`minWithdrawalDelayBlocks`, `strategyWithdrawalDelayBlocks[strategy]`) such that `strategy` represents the queued strategies to be withdrawn. Withdrawals do not require the caller to "fully exit" from the system -- they may choose to receive their shares back in full once the withdrawal is completed (see [`completeQueuedWithdrawal`](#completequeuedwithdrawal) for details). -Note that for any `strategy` s.t `StrategyManager.thirdPartyTransfersForbidden(strategy) == true` the `withdrawer` must be the same address as the `staker` as this setting disallows users to deposit or withdraw on behalf of other users. (see [`thirdPartyTransfersForbidden`](./StrategyManager.md) for details). +Note that the `QueuedWithdrawalParams` struct has a `withdrawer` field. Originally, this was used to specify an address that the withdrawal would be credited to once completed. However, `queueWithdrawals` now requires that `withdrawer == msg.sender`. Any other input is rejected. *Effects*: * For each withdrawal: * If the caller is delegated to an Operator, that Operator's delegated balances are decreased according to the `strategies` and `shares` being withdrawn. - * A `Withdrawal` is queued for the `withdrawer`, tracking the strategies and shares being withdrawn + * A `Withdrawal` is queued for the caller, tracking the strategies and shares being withdrawn * The caller's withdrawal nonce is increased * The hash of the `Withdrawal` is marked as "pending" * See [`EigenPodManager.removeShares`](./EigenPodManager.md#eigenpodmanagerremoveshares) @@ -250,8 +248,7 @@ Note that for any `strategy` s.t `StrategyManager.thirdPartyTransfersForbidden(s * For each withdrawal: * `strategies.length` MUST equal `shares.length` * `strategies.length` MUST NOT be equal to 0 - * The `withdrawer` MUST NOT be 0 - * For all strategies being withdrawn, the `withdrawer` MUST be the same address as the `staker` if `StrategyManager.thirdPartyTransfersForbidden(strategy) == true` + * The `withdrawer` MUST equal `msg.sender` * See [`EigenPodManager.removeShares`](./EigenPodManager.md#eigenpodmanagerremoveshares) * See [`StrategyManager.removeShares`](./StrategyManager.md#removeshares) diff --git a/src/contracts/core/DelegationManager.sol b/src/contracts/core/DelegationManager.sol index e9ae3ef4e..068370733 100644 --- a/src/contracts/core/DelegationManager.sol +++ b/src/contracts/core/DelegationManager.sol @@ -272,7 +272,7 @@ contract DelegationManager is Initializable, OwnableUpgradeable, Pausable, Deleg for (uint256 i = 0; i < queuedWithdrawalParams.length; i++) { require(queuedWithdrawalParams[i].strategies.length == queuedWithdrawalParams[i].shares.length, "DelegationManager.queueWithdrawal: input length mismatch"); - require(queuedWithdrawalParams[i].withdrawer != address(0), "DelegationManager.queueWithdrawal: must provide valid withdrawal address"); + require(queuedWithdrawalParams[i].withdrawer == msg.sender, "DelegationManager.queueWithdrawal: withdrawer must be staker"); // Remove shares from staker's strategies and place strategies/shares in queue. // If the staker is delegated to an operator, the operator's delegated shares are also reduced diff --git a/src/test/Withdrawals.t.sol b/src/test/Withdrawals.t.sol index 2f07da4e1..567822f70 100644 --- a/src/test/Withdrawals.t.sol +++ b/src/test/Withdrawals.t.sol @@ -22,16 +22,17 @@ contract WithdrawalTests is EigenLayerTestHelper { function testWithdrawalWrapper( address operator, address depositor, - address withdrawer, uint96 ethAmount, uint96 eigenAmount, bool withdrawAsTokens, bool RANDAO - ) public fuzzedAddress(operator) fuzzedAddress(depositor) fuzzedAddress(withdrawer) { + ) public fuzzedAddress(operator) fuzzedAddress(depositor) { cheats.assume(depositor != operator); cheats.assume(ethAmount >= 1 && ethAmount <= 1e18); cheats.assume(eigenAmount >= 1 && eigenAmount <= 1e18); + address withdrawer = depositor; + if (RANDAO) { _testWithdrawalAndDeregistration(operator, depositor, withdrawer, ethAmount, eigenAmount, withdrawAsTokens); } else { @@ -244,14 +245,13 @@ contract WithdrawalTests is EigenLayerTestHelper { function testRedelegateAfterWithdrawal( address operator, address depositor, - address withdrawer, uint96 ethAmount, uint96 eigenAmount, bool withdrawAsShares - ) public fuzzedAddress(operator) fuzzedAddress(depositor) fuzzedAddress(withdrawer) { + ) public fuzzedAddress(operator) fuzzedAddress(depositor) { cheats.assume(depositor != operator); //this function performs delegation and subsequent withdrawal - testWithdrawalWrapper(operator, depositor, withdrawer, ethAmount, eigenAmount, withdrawAsShares, true); + testWithdrawalWrapper(operator, depositor, ethAmount, eigenAmount, withdrawAsShares, true); cheats.prank(depositor); delegation.undelegate(depositor); @@ -261,50 +261,6 @@ contract WithdrawalTests is EigenLayerTestHelper { _initiateDelegation(operator, depositor, ethAmount, eigenAmount); } - // onlyNotFrozen modifier is not used in current DelegationManager implementation. - // commented out test case for now - // /// @notice test to see if an operator who is slashed/frozen - // /// cannot be undelegated from by their stakers. - // /// @param operator is the operator being delegated to. - // /// @param staker is the staker delegating stake to the operator. - // function testSlashedOperatorWithdrawal(address operator, address staker, uint96 ethAmount, uint96 eigenAmount) - // public - // fuzzedAddress(operator) - // fuzzedAddress(staker) - // { - // cheats.assume(staker != operator); - // testDelegation(operator, staker, ethAmount, eigenAmount); - - // { - // address slashingContract = slasher.owner(); - - // cheats.startPrank(operator); - // slasher.optIntoSlashing(address(slashingContract)); - // cheats.stopPrank(); - - // cheats.startPrank(slashingContract); - // slasher.freezeOperator(operator); - // cheats.stopPrank(); - // } - - // (IStrategy[] memory updatedStrategies, uint256[] memory updatedShares) = - // strategyManager.getDeposits(staker); - - // uint256[] memory strategyIndexes = new uint256[](2); - // strategyIndexes[0] = 0; - // strategyIndexes[1] = 1; - - // IERC20[] memory tokensArray = new IERC20[](2); - // tokensArray[0] = weth; - // tokensArray[0] = eigenToken; - - // //initiating queued withdrawal - // cheats.expectRevert( - // bytes("StrategyManager.onlyNotFrozen: staker has been frozen and may be subject to slashing") - // ); - // _testQueueWithdrawal(staker, strategyIndexes, updatedStrategies, updatedShares, staker); - // } - // Helper function to begin a delegation function _initiateDelegation( address operator, diff --git a/src/test/unit/DelegationUnit.t.sol b/src/test/unit/DelegationUnit.t.sol index 45634d114..2e71bfe17 100644 --- a/src/test/unit/DelegationUnit.t.sol +++ b/src/test/unit/DelegationUnit.t.sol @@ -2828,14 +2828,16 @@ contract DelegationManagerUnitTests_queueWithdrawals is DelegationManagerUnitTes delegationManager.queueWithdrawals(queuedWithdrawalParams); } - function test_Revert_WhenZeroAddressWithdrawer() public { + function test_Revert_WhenNotStakerWithdrawer(address withdrawer) public { + cheats.assume(withdrawer != defaultStaker); + (IDelegationManager.QueuedWithdrawalParams[] memory queuedWithdrawalParams, , ) = _setUpQueueWithdrawalsSingleStrat({ staker: defaultStaker, - withdrawer: address(0), + withdrawer: withdrawer, strategy: strategyMock, withdrawalAmount: 100 }); - cheats.expectRevert("DelegationManager.queueWithdrawal: must provide valid withdrawal address"); + cheats.expectRevert("DelegationManager.queueWithdrawal: withdrawer must be staker"); delegationManager.queueWithdrawals(queuedWithdrawalParams); } @@ -2977,6 +2979,7 @@ contract DelegationManagerUnitTests_queueWithdrawals is DelegationManagerUnitTes uint256 randSalt ) public filterFuzzedAddressInputs(staker){ cheats.assume(depositAmounts.length > 0 && depositAmounts.length <= 32); + cheats.assume(staker != defaultOperator); uint256[] memory withdrawalAmounts = _fuzzWithdrawalAmounts(depositAmounts); IStrategy[] memory strategies = _deployAndDepositIntoStrategies(staker, depositAmounts); @@ -3033,7 +3036,7 @@ contract DelegationManagerUnitTests_queueWithdrawals is DelegationManagerUnitTes uint256[] memory depositAmounts, uint256 randSalt ) public filterFuzzedAddressInputs(staker) { - cheats.assume(staker != withdrawer && withdrawer != address(0)); + cheats.assume(staker != withdrawer); cheats.assume(depositAmounts.length > 0 && depositAmounts.length <= 32); uint256[] memory withdrawalAmounts = _fuzzWithdrawalAmounts(depositAmounts); @@ -3052,8 +3055,11 @@ contract DelegationManagerUnitTests_queueWithdrawals is DelegationManagerUnitTes }); // queueWithdrawals + // NOTE: Originally, you could queue a withdrawal to a different address, which would fail with a specific error + // if third party transfers were forbidden. Now, withdrawing to a different address is forbidden regardless + // of third party transfer status. cheats.expectRevert( - "DelegationManager._removeSharesAndQueueWithdrawal: withdrawer must be same address as staker if thirdPartyTransfersForbidden are set" + "DelegationManager.queueWithdrawal: withdrawer must be staker" ); cheats.prank(staker); delegationManager.queueWithdrawals(queuedWithdrawalParams); @@ -3247,12 +3253,10 @@ contract DelegationManagerUnitTests_completeQueuedWithdrawal is DelegationManage */ function test_completeQueuedWithdrawal_SingleStratWithdrawAsTokens( address staker, - address withdrawer, uint256 depositAmount, uint256 withdrawalAmount ) public filterFuzzedAddressInputs(staker) { cheats.assume(staker != defaultOperator); - cheats.assume(withdrawer != address(0)); cheats.assume(withdrawalAmount > 0 && withdrawalAmount <= depositAmount); _registerOperatorWithBaseDetails(defaultOperator); ( @@ -3261,7 +3265,7 @@ contract DelegationManagerUnitTests_completeQueuedWithdrawal is DelegationManage bytes32 withdrawalRoot ) = _setUpCompleteQueuedWithdrawalSingleStrat({ staker: staker, - withdrawer: withdrawer, + withdrawer: staker, depositAmount: depositAmount, withdrawalAmount: withdrawalAmount }); @@ -3271,7 +3275,7 @@ contract DelegationManagerUnitTests_completeQueuedWithdrawal is DelegationManage // completeQueuedWithdrawal cheats.roll(block.number + delegationManager.getWithdrawalDelay(withdrawal.strategies)); - cheats.prank(withdrawer); + cheats.prank(staker); cheats.expectEmit(true, true, true, true, address(delegationManager)); emit WithdrawalCompleted(withdrawalRoot); delegationManager.completeQueuedWithdrawal(withdrawal, tokens, 0 /* middlewareTimesIndex */, true); @@ -3291,21 +3295,20 @@ contract DelegationManagerUnitTests_completeQueuedWithdrawal is DelegationManage */ function test_completeQueuedWithdrawal_SingleStratWithdrawAsShares( address staker, - address withdrawer, uint256 depositAmount, uint256 withdrawalAmount ) public filterFuzzedAddressInputs(staker) { cheats.assume(staker != defaultOperator); - cheats.assume(withdrawer != defaultOperator && withdrawer != address(0)); cheats.assume(withdrawalAmount > 0 && withdrawalAmount <= depositAmount); _registerOperatorWithBaseDetails(defaultOperator); + ( IDelegationManager.Withdrawal memory withdrawal, IERC20[] memory tokens, bytes32 withdrawalRoot ) = _setUpCompleteQueuedWithdrawalSingleStrat({ staker: staker, - withdrawer: withdrawer, + withdrawer: staker, depositAmount: depositAmount, withdrawalAmount: withdrawalAmount }); @@ -3315,19 +3318,14 @@ contract DelegationManagerUnitTests_completeQueuedWithdrawal is DelegationManage // completeQueuedWithdrawal cheats.roll(block.number + delegationManager.getWithdrawalDelay(withdrawal.strategies)); - cheats.prank(withdrawer); + cheats.prank(staker); cheats.expectEmit(true, true, true, true, address(delegationManager)); emit WithdrawalCompleted(withdrawalRoot); delegationManager.completeQueuedWithdrawal(withdrawal, tokens, 0 /* middlewareTimesIndex */, false); uint256 operatorSharesAfter = delegationManager.operatorShares(defaultOperator, withdrawal.strategies[0]); - if (staker == withdrawer) { - // Since staker is delegated, operatorShares get incremented - assertEq(operatorSharesAfter, operatorSharesBefore + withdrawalAmount, "operator shares not increased correctly"); - } else { - // Since withdrawer is not the staker and isn't delegated, staker's oeprator shares are unchanged - assertEq(operatorSharesAfter, operatorSharesBefore, "operator shares should be unchanged"); - } + // Since staker is delegated, operatorShares get incremented + assertEq(operatorSharesAfter, operatorSharesBefore + withdrawalAmount, "operator shares not increased correctly"); assertFalse(delegationManager.pendingWithdrawals(withdrawalRoot), "withdrawalRoot should be completed and marked false now"); } } \ No newline at end of file