feat: prevent queuing withdrawals to other addresses (#438)

Author: wadealexc

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)
 

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

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,

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");
     }
 }