refactor: final slashing cleanup

docs/core/DelegationManager.md

@@ -444,17 +444,17 @@ Just as with a normal queued withdrawal, these withdrawals can be completed by t
 ```solidity
 /**
  * @param strategies The strategies to withdraw from
- * @param depositShares For each strategy, the number of deposit shares to withdraw. Deposit shares can 
- * be queried via `getDepositedShares`. 
+ * @param depositShares For each strategy, the number of deposit shares to withdraw. Deposit shares can
+ * be queried via `getDepositedShares`.
  * NOTE: The number of shares ultimately received when a withdrawal is completed may be lower depositShares
  * if the staker or their delegated operator has experienced slashing.
- * @param withdrawer The address that will ultimately complete the withdrawal and receive the shares/tokens.
- * NOTE: This MUST be msg.sender; alternate withdrawers are not supported at this time.
+ * @param __deprecated_withdrawer This field is ignored. The only party that may complete a withdrawal
+ * is the staker that originally queued it. Alternate withdrawers are not supported.
  */
 struct QueuedWithdrawalParams {
     IStrategy[] strategies;
     uint256[] depositShares;
-    address withdrawer;
+    address __deprecated_withdrawer;
 }
 
 /**
@@ -485,7 +485,7 @@ For each `QueuedWithdrawalParams` passed as input, a `Withdrawal` is created in
     * are added to the operator's `cumulativeScaledSharesHistory`, where they can be burned if slashing occurs while the withdrawal is in the queue
 * _Withdrawable shares_ are calculated by applying both the staker's _deposit scaling factor_ AND any appropriate _slashing factor_ to the staker's _deposit shares_. These "currently withdrawable shares" are removed from the operator's delegated shares (if applicable).
 
-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.
+Note that the `QueuedWithdrawalParams.__deprecated_withdrawer` field is ignored. Originally, this was used to create withdrawals that could be completed by a third party. This functionality was removed during the M2 release due to growing concerns over the phish risk this presented. Until the slashing release, this field was explicitly checked for equivalence with `msg.sender`; however, at present it is ignored. All `Withdrawals` are created with `withdrawer == staker` regardless of this field's value.
 
 *Effects*:
 * For each `QueuedWithdrawalParams` element:

script/tasks/withdraw_from_strategy.s.sol

@@ -48,7 +48,7 @@ contract WithdrawFromStrategy is Script, Test {
         queueWithdrawals[0] = IDelegationManagerTypes.QueuedWithdrawalParams({
             strategies: strategies,
             depositShares: shares,
-            withdrawer: msg.sender
+            __deprecated_withdrawer: address(0)
         });
 
         // Withdrawal roots will be returned when we queue

src/contracts/core/DelegationManager.sol

@@ -124,7 +124,7 @@ contract DelegationManager is
         address operator,
         SignatureWithExpiry memory approverSignatureAndExpiry,
         bytes32 approverSalt
-    ) external {
+    ) public {
         require(!isDelegated(msg.sender), ActivelyDelegated());
         require(isOperator(operator), OperatorNotRegistered());
 
@@ -143,22 +143,17 @@ contract DelegationManager is
     /// @inheritdoc IDelegationManager
     function undelegate(
         address staker
-    ) external returns (bytes32[] memory withdrawalRoots) {
+    ) public returns (bytes32[] memory withdrawalRoots) {
         // Check that the `staker` can undelegate
         require(isDelegated(staker), NotActivelyDelegated());
         require(!isOperator(staker), OperatorsCannotUndelegate());
 
-        // Validate caller is the staker, the operator, or the operator's `delegationApprover`
-        require(staker != address(0), InputAddressZero());
-        address operator = delegatedTo[staker];
-        require(
-            msg.sender == staker || _checkCanCall(operator)
-                || msg.sender == _operatorDetails[operator].delegationApprover,
-            CallerCannotUndelegate()
-        );
-
-        // Emit an event if this action was not initiated by the staker themselves
+        // If the action is not being initiated by the staker, validate that it is initiated
+        // by the operator or their delegationApprover.
         if (msg.sender != staker) {
+            address operator = delegatedTo[staker];
+
+            require(_checkCanCall(operator) || msg.sender == delegationApprover(operator), CallerCannotUndelegate());
             emit StakerForceUndelegated(staker, operator);
         }
 
@@ -171,24 +166,9 @@ contract DelegationManager is
         SignatureWithExpiry memory newOperatorApproverSig,
         bytes32 approverSalt
     ) external returns (bytes32[] memory withdrawalRoots) {
-        // Check that the staker can undelegate, and `newOperator` can be delegated to
-        require(isDelegated(msg.sender), NotActivelyDelegated());
-        require(!isOperator(msg.sender), OperatorsCannotUndelegate());
-        require(isOperator(newOperator), OperatorNotRegistered());
-
-        // Undelegate the staker and queue any deposited assets for withdrawal
-        withdrawalRoots = _undelegate(msg.sender);
-
-        // If the operator has a `delegationApprover`, check the provided signature
-        _checkApproverSignature({
-            staker: msg.sender,
-            operator: newOperator,
-            signature: newOperatorApproverSig,
-            salt: approverSalt
-        });
-
-        // Delegate to the new operator
-        _delegate(msg.sender, newOperator);
+        withdrawalRoots = undelegate(msg.sender);
+        // delegateTo uses msg.sender as staker
+        delegateTo(newOperator, newOperatorApproverSig, approverSalt);
     }
 
     /// @inheritdoc IDelegationManager
@@ -200,7 +180,6 @@ contract DelegationManager is
 
         for (uint256 i = 0; i < params.length; i++) {
             require(params[i].strategies.length == params[i].depositShares.length, InputArrayLengthMismatch());
-            require(params[i].withdrawer == msg.sender, WithdrawerNotStaker());
 
             uint256[] memory slashingFactors = _getSlashingFactors(msg.sender, operator, params[i].strategies);
 
@@ -309,24 +288,29 @@ contract DelegationManager is
             newMaxMagnitude: newMaxMagnitude
         });
 
-        // While `operatorSharesSlashed` describes the amount we should directly remove from the operator's delegated
-        // shares, `operatorSharesToBurn` also includes any shares that have been queued for withdrawal and are still
-        // slashable given the withdrawal delay.
-        uint256 operatorSharesToBurn =
-            operatorSharesSlashed + _getSlashedSharesInQueue(operator, strategy, prevMaxMagnitude, newMaxMagnitude);
+        uint256 scaledSharesSlashedFromQueue = _getSlashableSharesInQueue({
+            operator: operator,
+            strategy: strategy,
+            prevMaxMagnitude: prevMaxMagnitude,
+            newMaxMagnitude: newMaxMagnitude
+        });
+
+        // Calculate the total deposit shares to burn - slashed operator shares plus still-slashable
+        // shares sitting in the withdrawal queue.
+        uint256 totalDepositSharesToBurn = operatorSharesSlashed + scaledSharesSlashedFromQueue;
 
         // Remove shares from operator
         _decreaseDelegation({
             operator: operator,
-            staker: address(0), // we treat this as a decrease for the zero address staker
+            staker: address(0), // we treat this as a decrease for the 0-staker (only used for events)
             strategy: strategy,
             sharesToDecrease: operatorSharesSlashed
         });
 
         // NOTE: native ETH shares will be burned by a different mechanism in a future release
         if (strategy != beaconChainETHStrategy) {
-            strategyManager.burnShares(strategy, operatorSharesToBurn);
-            emit OperatorSharesBurned(operator, strategy, operatorSharesToBurn);
+            strategyManager.burnShares(strategy, totalDepositSharesToBurn);
+            emit OperatorSharesBurned(operator, strategy, totalDepositSharesToBurn);
         }
     }
 
@@ -465,22 +449,16 @@ contract DelegationManager is
         require(strategies.length != 0, InputArrayLengthZero());
 
         uint256[] memory scaledShares = new uint256[](strategies.length);
-        uint256[] memory sharesToWithdraw = new uint256[](strategies.length);
+        uint256[] memory withdrawableShares = new uint256[](strategies.length);
 
         // Remove shares from staker and operator
         // Each of these operations fail if we attempt to remove more shares than exist
         for (uint256 i = 0; i < strategies.length; ++i) {
             IShareManager shareManager = _getShareManager(strategies[i]);
             DepositScalingFactor memory dsf = _depositScalingFactor[staker][strategies[i]];
 
-            // Check withdrawing deposit shares amount doesn't exceed balance
-            require(
-                depositSharesToWithdraw[i] <= shareManager.stakerDepositShares(staker, strategies[i]),
-                WithdrawalExceedsMax()
-            );
-
             // Calculate how many shares can be withdrawn after factoring in slashing
-            sharesToWithdraw[i] = dsf.calcWithdrawable(depositSharesToWithdraw[i], slashingFactors[i]);
+            withdrawableShares[i] = dsf.calcWithdrawable(depositSharesToWithdraw[i], slashingFactors[i]);
 
             // Scale shares for queue withdrawal
             scaledShares[i] = dsf.scaleForQueueWithdrawal(depositSharesToWithdraw[i]);
@@ -497,7 +475,7 @@ contract DelegationManager is
                     operator: operator,
                     staker: staker,
                     strategy: strategies[i],
-                    sharesToDecrease: sharesToWithdraw[i]
+                    sharesToDecrease: withdrawableShares[i]
                 });
             }
 
@@ -525,7 +503,7 @@ contract DelegationManager is
         queuedWithdrawals[withdrawalRoot] = withdrawal;
         _stakerQueuedWithdrawalRoots[staker].add(withdrawalRoot);
 
-        emit SlashingWithdrawalQueued(withdrawalRoot, withdrawal, sharesToWithdraw);
+        emit SlashingWithdrawalQueued(withdrawalRoot, withdrawal, withdrawableShares);
         return withdrawalRoot;
     }
 
@@ -553,7 +531,7 @@ contract DelegationManager is
             // slashableUntil is block inclusive so we need to check if the current block is strictly greater than the slashableUntil block
             // meaning the withdrawal can be completed.
             uint32 slashableUntil = withdrawal.startBlock + MIN_WITHDRAWAL_DELAY_BLOCKS;
-            require(slashableUntil < uint32(block.number), WithdrawalDelayNotElapsed());
+            require(uint32(block.number) > slashableUntil, WithdrawalDelayNotElapsed());
 
             // Given the max magnitudes of the operator the staker was originally delegated to, calculate
             // the slashing factors for each of the withdrawal's strategies.
@@ -565,6 +543,13 @@ contract DelegationManager is
             });
         }
 
+        // Remove the withdrawal from the queue. Note that for legacy withdrawals, the removals
+        // from `_stakerQueuedWithdrawalRoots` and `queuedWithdrawals` will no-op.
+        _stakerQueuedWithdrawalRoots[withdrawal.staker].remove(withdrawalRoot);
+        delete queuedWithdrawals[withdrawalRoot];
+        delete pendingWithdrawals[withdrawalRoot];
+        emit SlashingWithdrawalCompleted(withdrawalRoot);
+
         // Given the max magnitudes of the operator the staker is now delegated to, calculate the current
         // slashing factors to apply to each withdrawal if it is received as shares.
         address newOperator = delegatedTo[withdrawal.staker];
@@ -609,13 +594,6 @@ contract DelegationManager is
                 });
             }
         }
-
-        _stakerQueuedWithdrawalRoots[withdrawal.staker].remove(withdrawalRoot);
-
-        delete queuedWithdrawals[withdrawalRoot];
-        delete pendingWithdrawals[withdrawalRoot];
-
-        emit SlashingWithdrawalCompleted(withdrawalRoot);
     }
 
     /**
@@ -756,29 +734,27 @@ contract DelegationManager is
      * Note: To get the total amount of slashable shares in the queue withdrawable, set newMaxMagnitude to 0 and prevMaxMagnitude
      * is the current maxMagnitude of the operator.
      */
-    function _getSlashedSharesInQueue(
+    function _getSlashableSharesInQueue(
         address operator,
         IStrategy strategy,
         uint64 prevMaxMagnitude,
         uint64 newMaxMagnitude
     ) internal view returns (uint256) {
-        // Fetch the cumulative scaled shares sitting in the withdrawal queue both now and before
-        // the withdrawal delay.
-        // NOTE: We want all the shares in the window [block.number - MIN_WITHDRAWAL_DELAY_BLOCKS, block.number]
-        // as this is all slashable and since prevCumulativeScaledShares is being subtracted from curCumulativeScaledShares
-        // we do a -1 on the block number to also include (block.number - MIN_WITHDRAWAL_DELAY_BLOCKS) as slashable.
-        uint256 curCumulativeScaledShares = _cumulativeScaledSharesHistory[operator][strategy].latest();
-        uint256 prevCumulativeScaledShares = _cumulativeScaledSharesHistory[operator][strategy].upperLookup({
+        // We want ALL shares added to the withdrawal queue in the window [block.number - MIN_WITHDRAWAL_DELAY_BLOCKS, block.number]
+        //
+        // To get this, we take the current shares in the withdrawal queue and subtract the number of shares
+        // that were in the queue before MIN_WITHDRAWAL_DELAY_BLOCKS.
+        uint256 curQueuedScaledShares = _cumulativeScaledSharesHistory[operator][strategy].latest();
+        uint256 prevQueuedScaledShares = _cumulativeScaledSharesHistory[operator][strategy].upperLookup({
             key: uint32(block.number) - MIN_WITHDRAWAL_DELAY_BLOCKS - 1
         });
 
-        // The difference between these values represents the number of scaled shares that entered the
-        // withdrawal queue less than `MIN_WITHDRAWAL_DELAY_BLOCKS` ago. These shares are still slashable,
-        // so we use them to calculate the number of slashable shares in the withdrawal queue.
-        uint256 slashableScaledShares = curCumulativeScaledShares - prevCumulativeScaledShares;
+        // The difference between these values is the number of scaled shares that entered the withdrawal queue
+        // less than or equal to MIN_WITHDRAWAL_DELAY_BLOCKS ago. These shares are still slashable.
+        uint256 scaledSharesAdded = curQueuedScaledShares - prevQueuedScaledShares;
 
         return SlashingLib.scaleForBurning({
-            scaledShares: slashableScaledShares,
+            scaledShares: scaledSharesAdded,
             prevMaxMagnitude: prevMaxMagnitude,
             newMaxMagnitude: newMaxMagnitude
         });
@@ -827,7 +803,7 @@ contract DelegationManager is
     /// @inheritdoc IDelegationManager
     function delegationApprover(
         address operator
-    ) external view returns (address) {
+    ) public view returns (address) {
         return _operatorDetails[operator].delegationApprover;
     }
 
@@ -862,11 +838,10 @@ contract DelegationManager is
 
     /// @inheritdoc IDelegationManager
     function getSlashableSharesInQueue(address operator, IStrategy strategy) public view returns (uint256) {
-        IStrategy[] memory strategies = new IStrategy[](1);
-        strategies[0] = strategy;
-        uint64 maxMagnitude = allocationManager.getMaxMagnitudes(operator, strategies)[0];
-        // Return amount of shares slashed if all remaining magnitude were to be slashed
-        return _getSlashedSharesInQueue({
+        uint64 maxMagnitude = allocationManager.getMaxMagnitude(operator, strategy);
+
+        // Return amount of slashable scaled shares remaining
+        return _getSlashableSharesInQueue({
             operator: operator,
             strategy: strategy,
             prevMaxMagnitude: maxMagnitude,
@@ -927,11 +902,18 @@ contract DelegationManager is
         return (strategies, shares);
     }
 
+    /// @inheritdoc IDelegationManager
+    function getQueuedWithdrawal(
+        bytes32 withdrawalRoot
+    ) external view returns (Withdrawal memory) {
+        return queuedWithdrawals[withdrawalRoot];
+    }
+
     /// @inheritdoc IDelegationManager
     function getQueuedWithdrawals(
         address staker
     ) external view returns (Withdrawal[] memory withdrawals, uint256[][] memory shares) {
-        bytes32[] memory withdrawalRoots = _stakerQueuedWithdrawalRoots[staker].values();
+        bytes32[] memory withdrawalRoots = getQueuedWithdrawalRoots(staker);
 
         uint256 totalQueued = withdrawalRoots.length;
         withdrawals = new Withdrawal[](totalQueued);
@@ -971,6 +953,13 @@ contract DelegationManager is
         }
     }
 
+    /// @inheritdoc IDelegationManager
+    function getQueuedWithdrawalRoots(
+        address staker
+    ) public view returns (bytes32[] memory) {
+        return _stakerQueuedWithdrawalRoots[staker].values();
+    }
+
     /// @inheritdoc IDelegationManager
     function convertToDepositShares(
         address staker,

src/contracts/core/DelegationManagerStorage.sol

@@ -111,7 +111,7 @@ abstract contract DelegationManagerStorage is IDelegationManager {
 
     /// @notice Returns the details of a queued withdrawal given by `withdrawalRoot`.
     /// @dev This variable only reflects withdrawals that were made after the slashing release.
-    mapping(bytes32 withdrawalRoot => Withdrawal withdrawal) public queuedWithdrawals;
+    mapping(bytes32 withdrawalRoot => Withdrawal withdrawal) internal queuedWithdrawals;
 
     /// @notice Contains history of the total cumulative staker withdrawals for an operator and a given strategy.
     /// Used to calculate burned StrategyManager shares when an operator is slashed.