fix: delegate shares

docs/core/accounting/SharesAccounting.md

@@ -162,6 +162,67 @@ See implementation in:
 * [`StrategyManager.depositIntoStrategy`](../../../src/contracts/core/StrategyManager.sol)
 * [`EigenPodManager.recordBeaconChainETHBalanceUpdate`](../../../src/contracts/pods/EigenPodManager.sol)
 
+
+---
+
+### Delegation
+
+Suppose we have an undelegated staker who decides to delegate to an operator.
+We have the following properties that should be preserved.
+
+#### Operator Level
+
+Operator shares should be increased by the amount of delegatable shares the staker has, this is synonymous to their withdrawable shares $a_n$. Therefore,
+
+$$
+op_{n+1} = op_{n} + a_n
+$$
+
+$$
+= op_{n} + s_n k_n l_n m_n
+$$
+
+
+#### Staker Level
+
+withdrawable shares should remain unchanged
+
+$$
+a_{n+1} = a_n
+$$
+
+deposit shares should remain unchanged
+
+$$
+s_{n+1} = s_n
+$$
+
+beaconChainSlashingFactor and maxMagnitude should also remain unchanged. In this case, since the staker is not delegated, then their maxMagnitude should by default be equal to 1.
+
+$$
+l_{n+1} = l_n
+$$
+
+Now the question is what is the new depositScalingFactor equal to?
+
+$$
+a_{n+1} = a_n
+$$
+
+$$
+=> s_{n+1} k_{n+1} l_{n+1} m_{n+1} = s_n k_n l_n m_n
+$$
+
+$$
+=> s_{n} k_{n+1} l_{n} m_{n+1} = s_n k_n l_n m_n
+$$
+
+$$
+=> k_{n+1} = \frac {k_n m_n} { m_{n+1} }
+$$
+
+Notice how the staker variables that update $k_{n+1}$ and $m_{n+1}$ do not affect previously queued withdrawals and shares received upon withdrawal completion. This is because the maxMagnitude that is looked up is dependent on the operator at the time of the queued withdrawal and the $k_n$ is effectively stored in the scaled shares field.
+
 ---
 
 ### Slashing
@@ -297,6 +358,8 @@ $$
 
 Note that when a withdrawal is queued, a `Withdrawal` struct is created with _scaled shares_ defined as $q_t = x_t k_t$ where $t$ is the time of the queuing. The reason we define and store scaled shares like this will be clearer in [Complete Withdrawal](#complete-withdrawal) below.
 
+Additionally, we reset the depositScalingFactor when a user queues a withdrawal for all their shares, either through un/redelegation or directly. This is because the DSF at the time of withdrawal is stored in the scaled shares, and any "new" deposits or delegations by the staker should be considered as new. Note that withdrawal completion is treated as a kind of deposit when done as shares, which again will be clearer below.
+
 See implementation in:
 * `DelegationManager.queueWithdrawals`
 * `SlashingLib.scaleForQueueWithdrawal`

src/contracts/core/DelegationManager.sol

@@ -340,24 +340,37 @@ contract DelegationManager is
      *          1) new delegations are not paused (PAUSED_NEW_DELEGATION)
      */
     function _delegate(address staker, address operator) internal onlyWhenNotPaused(PAUSED_NEW_DELEGATION) {
-        // record the delegation relation between the staker and operator, and emit an event
+        // When a staker is not delegated to an operator, their deposit shares are equal to their
+        // withdrawable shares -- except for the beaconChainETH strategy, which is handled below
+        (IStrategy[] memory strategies, uint256[] memory withdrawableShares) = getDepositedShares(staker);
+
+        // Retrieve the amount of slashing experienced by the operator in each strategy so far.
+        // When delegating, we "forgive" the staker for this slashing by adjusting their
+        // deposit scaling factor.
+        uint256[] memory operatorSlashingFactors = _getSlashingFactors(address(0), operator, strategies);
+
+        // Delegate to the operator
         delegatedTo[staker] = operator;
         emit StakerDelegated(staker, operator);
 
-        // read staker's deposited shares and strategies to add to operator's shares
-        // and also update the staker depositScalingFactor for each strategy
-        (IStrategy[] memory strategies, uint256[] memory depositedShares) = getDepositedShares(staker);
-        uint256[] memory slashingFactors = _getSlashingFactors(staker, operator, strategies);
-
         for (uint256 i = 0; i < strategies.length; ++i) {
+            // Special case for beacon chain slashing - ensure the staker's beacon chain slashing is
+            // reflected in the number of shares they delegate.
+            if (strategies[i] == beaconChainETHStrategy) {
+                uint64 stakerBeaconChainSlashing = eigenPodManager.beaconChainSlashingFactor(staker);
+
+                DepositScalingFactor memory dsf = _depositScalingFactor[staker][strategies[i]];
+                withdrawableShares[i] = dsf.calcWithdrawable(withdrawableShares[i], stakerBeaconChainSlashing);
+            }
+
             // forgefmt: disable-next-item
             _increaseDelegation({
                 operator: operator, 
                 staker: staker, 
                 strategy: strategies[i],
                 prevDepositShares: uint256(0),
-                addedShares: depositedShares[i],
-                slashingFactor: slashingFactors[i]
+                addedShares: withdrawableShares[i],
+                slashingFactor: operatorSlashingFactors[i]
             });
         }
     }
@@ -477,6 +490,11 @@ contract DelegationManager is
                 });
             }
 
+            //Reset DepositScalingFactor since deposit shares are equal to withdrawable shares after full withdrawal
+            if (depositSharesToWithdraw[i] == shareManager.stakerDepositShares(staker, strategies[i])) {
+                _depositScalingFactor[staker][strategies[i]].reset();
+            }
+
             // Remove deposit shares from EigenPodManager/StrategyManager
             shareManager.removeDepositShares(staker, strategies[i], depositSharesToWithdraw[i]);
         }
@@ -622,7 +640,8 @@ contract DelegationManager is
         require(slashingFactor != 0, FullySlashed());
 
         // Update the staker's depositScalingFactor. This only results in an update
-        // if the slashing factor has changed for this strategy.
+        // if the slashing factor has changed for this strategy. Dsf update formula
+        // is different in the on delegation case.
         DepositScalingFactor storage dsf = _depositScalingFactor[staker][strategy];
         dsf.update(prevDepositShares, addedShares, slashingFactor);
         emit DepositScalingFactorUpdated(staker, strategy, dsf.scalingFactor());

src/contracts/libraries/SlashingLib.sol

@@ -94,10 +94,11 @@ library SlashingLib {
         uint256 addedShares,
         uint256 slashingFactor
     ) internal {
-        // If this is the staker's first deposit, set the scaling factor to
-        // the inverse of slashingFactor
         if (prevDepositShares == 0) {
-            dsf._scalingFactor = uint256(WAD).divWad(slashingFactor);
+            // If this is the staker's first deposit or is delegating to an operator, set the scaling factor to
+            // the inverse of slashingFactor. In the case of delegation slashingFactor is just
+            // maxMagnitude because BCSF is already accounted for in the dsf
+            dsf._scalingFactor = dsf.scalingFactor().divWad(slashingFactor);
             return;
         }
 
@@ -136,6 +137,12 @@ library SlashingLib {
         dsf._scalingFactor = newDepositScalingFactor;
     }
 
+    function reset(
+        DepositScalingFactor storage dsf
+    ) internal {
+        dsf._scalingFactor = 0;
+    }
+
     // CONVERSION
 
     function calcWithdrawable(

src/test/integration/IntegrationBase.t.sol

@@ -188,6 +188,22 @@ abstract contract IntegrationBase is IntegrationDeployer, TypeImporter {
 
         return result;
     }
+
+    /// @dev Choose a random subset of validators (selects AT LEAST ONE but NOT ALL)
+    function _chooseSubset(uint40[] memory validators) internal returns (uint40[] memory) {
+        require(validators.length >= 2, "Need at least 2 validators to choose subset"); 
+
+        uint40[] memory result = new uint40[](validators.length);
+        uint newLen;
+
+        uint rand = _randUint({ min: 1, max: validators.length ** 2 });
+        for (uint i = 0; i < validators.length; i++) {
+            if (rand >> i & 1 == 1) {
+                result[newLen] = validators[i];
+                newLen++;
+            }
+        }
+    }
 
     function _getTokenName(IERC20 token) internal view returns (string memory) {
         if (token == NATIVE_ETH) {
@@ -1313,7 +1329,7 @@ abstract contract IntegrationBase is IntegrationDeployer, TypeImporter {
         }
     }
 
-    /// @dev Check that the staker's withdrawable shares have decreased by `removedShares`
+    /// @dev Check that the staker's withdrawable shares have increased by `addedShares`
     function assert_Snap_Added_Staker_WithdrawableShares(
         User staker, 
         IStrategy[] memory strategies, 

src/test/integration/IntegrationChecks.t.sol

@@ -231,7 +231,7 @@ contract IntegrationCheckUtils is IntegrationBase {
     ) internal {
         /// Undelegate from an operator
         //
-        // ... check that the staker is undelegated, all strategies from which the staker is deposited are unqeuued,
+        // ... check that the staker is undelegated, all strategies from which the staker is deposited are unqueued,
         //     that the returned root matches the hashes for each strategy and share amounts, and that the staker
         //     and operator have reduced shares
         assertFalse(delegationManager.isDelegated(address(staker)),
@@ -250,6 +250,35 @@ contract IntegrationCheckUtils is IntegrationBase {
             "check_QueuedWithdrawal_State: failed to remove staker withdrawable shares");
     }
 
+    function check_Redelegate_State(
+        User staker, 
+        User operator,
+        IDelegationManagerTypes.Withdrawal[] memory withdrawals,
+        bytes32[] memory withdrawalRoots,
+        IStrategy[] memory strategies,
+        uint[] memory shares 
+    ) internal {
+        /// Redelegate to a new operator
+        //
+        // ... check that the staker is delegated to new operator, all strategies from which the staker is deposited are unqueued,
+        //     that the returned root matches the hashes for each strategy and share amounts, and that the staker
+        //     and operator have reduced shares
+        assertTrue(delegationManager.isDelegated(address(staker)),
+            "check_Undelegate_State: staker should not be delegated");
+        assert_ValidWithdrawalHashes(withdrawals, withdrawalRoots,
+            "check_Undelegate_State: calculated withdrawl should match returned root");
+        assert_AllWithdrawalsPending(withdrawalRoots,
+            "check_Undelegate_State: stakers withdrawal should now be pending");
+        assert_Snap_Added_QueuedWithdrawals(staker, withdrawals,
+            "check_Undelegate_State: staker should have increased nonce by withdrawals.length");
+        assert_Snap_Removed_OperatorShares(operator, strategies, shares,
+            "check_Undelegate_State: failed to remove operator shares");
+        assert_Snap_Removed_Staker_DepositShares(staker, strategies, shares,
+            "check_Undelegate_State: failed to remove staker shares");
+        assert_Snap_Removed_Staker_WithdrawableShares(staker, strategies, shares,
+            "check_QueuedWithdrawal_State: failed to remove staker withdrawable shares");
+    }
+
     /**
      * @notice Overloaded function to check the state after a withdrawal as tokens, accepting a non-user type for the operator.
      * @param staker The staker who completed the withdrawal.
@@ -329,6 +358,28 @@ contract IntegrationCheckUtils is IntegrationBase {
         assert_Snap_Unchanged_StrategyShares(strategies, "strategies should have total shares unchanged");
     }
 
+    function check_Withdrawal_AsShares_Redelegated_State(
+        User staker,
+        User operator,
+        User newOperator,
+        IDelegationManagerTypes.Withdrawal memory withdrawal,
+        IStrategy[] memory strategies,
+        uint[] memory shares
+    ) internal {
+        /// Complete withdrawal(s):
+        // The staker will complete the withdrawal as shares
+        // 
+        // ... check that the withdrawal is not pending, that the token balances of the staker and operator are unchanged,
+        //     that the withdrawer received the expected shares, and that that the total shares of each o
+        //     strategy withdrawn remains unchanged 
+        assert_WithdrawalNotPending(delegationManager.calculateWithdrawalRoot(withdrawal), "staker withdrawal should no longer be pending");
+        assert_Snap_Unchanged_TokenBalances(staker, "staker should not have any change in underlying token balances");
+        assert_Snap_Unchanged_TokenBalances(operator, "operator should not have any change in underlying token balances");
+        assert_Snap_Added_Staker_DepositShares(staker, strategies, shares, "staker should have received expected shares");
+        assert_Snap_Unchanged_OperatorShares(operator, "operator should have shares unchanged");
+        assert_Snap_Unchanged_StrategyShares(strategies, "strategies should have total shares unchanged");
+    }
+
     /*******************************************************************************
                                 ALM - BASIC INVARIANTS
     *******************************************************************************/