Alex/burn refactor

script/tasks/complete_withdrawal_from_strategy.s.sol

@@ -67,7 +67,7 @@ contract CompleteWithdrawFromStrategy is Script, Test {
 
         // Get scaled shares for the given amount
         uint256[] memory scaledShares = new uint256[](1);
-        scaledShares[0] = SlashingLib.scaleSharesForQueuedWithdrawal({
+        scaledShares[0] = SlashingLib.scaleForQueueWithdrawal({
             sharesToWithdraw: sharesToWithdraw,
             slashingFactor: slashingFactor
         });

src/contracts/core/DelegationManager.sol

@@ -349,13 +349,19 @@ contract DelegationManager is
         require(newMaxMagnitude < prevMaxMagnitude, MaxMagnitudeCantIncrease());
 
         /// forgefmt: disable-next-item
-        (uint256 sharesToDecrement, uint256 sharesToBurn) = SlashingLib.calcSlashedAmount({
+        uint256 sharesToDecrement = SlashingLib.calcSlashedAmount({
             operatorShares: operatorShares[operator][strategy],
-            slashableSharesInQueue: _getSlashedSharesInQueue(operator, strategy, prevMaxMagnitude, newMaxMagnitude),
             prevMaxMagnitude: prevMaxMagnitude,
             newMaxMagnitude: newMaxMagnitude
         });
 
+        // While `sharesToDecrement` describes the amount we should directly remove from the operator's delegated
+        // shares, `sharesToBurn` also includes any shares that have been queued for withdrawal and are still
+        // slashable given the withdrawal delay.
+        uint256 sharesToBurn =
+            sharesToDecrement + _getSlashedSharesInQueue(operator, strategy, prevMaxMagnitude, newMaxMagnitude);
+
+        // Remove shares from operator
         _decreaseDelegation({
             operator: operator,
             staker: address(0), // we treat this as a decrease for the zero address staker
@@ -493,7 +499,7 @@ contract DelegationManager is
             IShareManager shareManager = _getShareManager(withdrawal.strategies[i]);
 
             // Calculate how much slashing to apply, as well as shares to withdraw
-            uint256 sharesToWithdraw = SlashingLib.scaleSharesForCompleteWithdrawal({
+            uint256 sharesToWithdraw = SlashingLib.scaleForCompleteWithdrawal({
                 scaledShares: withdrawal.scaledShares[i],
                 slashingFactor: prevSlashingFactors[i]
             });
@@ -641,7 +647,7 @@ contract DelegationManager is
             sharesToWithdraw[i] = dsf.calcWithdrawable(depositSharesToWithdraw[i], slashingFactors[i]);
 
             // Apply slashing. If the staker or operator has been fully slashed, this will return 0
-            scaledShares[i] = SlashingLib.scaleSharesForQueuedWithdrawal({
+            scaledShares[i] = SlashingLib.scaleForQueueWithdrawal({
                 sharesToWithdraw: sharesToWithdraw[i],
                 slashingFactor: slashingFactors[i]
             });
@@ -763,16 +769,23 @@ contract DelegationManager is
         uint64 prevMaxMagnitude,
         uint64 newMaxMagnitude
     ) internal view returns (uint256) {
-        uint256 currCumulativeScaledShares = _cumulativeScaledSharesHistory[operator][strategy].latest();
-        // Note: this will simply return 0 if no history exists, same for latest() as well
-        uint256 pastCumulativeScaledShares = _cumulativeScaledSharesHistory[operator][strategy].upperLookup({
+        // Fetch the cumulative scaled shares sitting in the withdrawal queue both now and before
+        // the withdrawal delay.
+        uint256 curCumulativeScaledShares = _cumulativeScaledSharesHistory[operator][strategy].latest();
+        uint256 prevCumulativeScaledShares = _cumulativeScaledSharesHistory[operator][strategy].upperLookup({
             key: uint32(block.number) - MIN_WITHDRAWAL_DELAY_BLOCKS
         });
-        uint256 slashableShareInQueue = SlashingLib.scaleSharesForBurning(
-            pastCumulativeScaledShares, currCumulativeScaledShares, prevMaxMagnitude, newMaxMagnitude
-        );
 
-        return slashableShareInQueue;
+        // 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;
+
+        return SlashingLib.scaleForBurning({
+            scaledShares: slashableScaledShares,
+            prevMaxMagnitude: prevMaxMagnitude,
+            newMaxMagnitude: newMaxMagnitude
+        });
     }
 
     /// @dev Add to the cumulative withdrawn scaled shares from an operator for a given strategy
@@ -952,7 +965,7 @@ contract DelegationManager is
             uint256[] memory slashingFactors = _getSlashingFactors(staker, operator, withdrawals[i].strategies);
 
             for (uint256 j; j < withdrawals[i].strategies.length; ++j) {
-                shares[i][j] = SlashingLib.scaleSharesForCompleteWithdrawal({
+                shares[i][j] = SlashingLib.scaleForCompleteWithdrawal({
                     scaledShares: withdrawals[i].scaledShares[j],
                     slashingFactor: slashingFactors[i]
                 });

src/contracts/libraries/SlashingLib.sol

@@ -62,7 +62,7 @@ library SlashingLib {
         return dsf._scalingFactor == 0 ? WAD : dsf._scalingFactor;
     }
 
-    function scaleSharesForQueuedWithdrawal(
+    function scaleForQueueWithdrawal(
         uint256 sharesToWithdraw,
         uint256 slashingFactor
     ) internal pure returns (uint256) {
@@ -73,25 +73,22 @@ library SlashingLib {
         return sharesToWithdraw.divWad(slashingFactor);
     }
 
-    function scaleSharesForCompleteWithdrawal(
-        uint256 scaledShares,
-        uint256 slashingFactor
-    ) internal pure returns (uint256) {
+    function scaleForCompleteWithdrawal(uint256 scaledShares, uint256 slashingFactor) internal pure returns (uint256) {
         return scaledShares.mulWad(slashingFactor);
     }
 
     /**
-     * @notice Scales the share difference between two cumulative scaled shares. This is used
-     * to read the total slashable/burnable shares that are queued for withdrawal from an operator.
-     * This is because shares are still slashable for the duration they are sitting in the withdrawal queue.
+     * @notice Scales shares according to the difference in an operator's magnitude before and
+     * after being slashed. This is used to calculate the number of slashable shares in the
+     * withdrawal queue.
+     * NOTE: max magnitude is guaranteed to only ever decrease.
      */
-    function scaleSharesForBurning(
-        uint256 pastCumulativeScaledShares,
-        uint256 currCumulativeScaledShares,
+    function scaleForBurning(
+        uint256 scaledShares,
         uint64 prevMaxMagnitude,
         uint64 newMaxMagnitude
     ) internal pure returns (uint256) {
-        return (currCumulativeScaledShares - pastCumulativeScaledShares).mulWad(prevMaxMagnitude - newMaxMagnitude);
+        return scaledShares.mulWad(prevMaxMagnitude - newMaxMagnitude);
     }
 
     function update(
@@ -157,12 +154,10 @@ library SlashingLib {
 
     function calcSlashedAmount(
         uint256 operatorShares,
-        uint256 slashableSharesInQueue,
         uint256 prevMaxMagnitude,
         uint256 newMaxMagnitude
-    ) internal pure returns (uint256 sharesToDecrement, uint256 sharesToBurn) {
-        // round up mulDiv so we don't round up sharesToDecrement and overslash
-        sharesToDecrement = operatorShares - operatorShares.mulDiv(newMaxMagnitude, prevMaxMagnitude, Math.Rounding.Up);
-        sharesToBurn = sharesToDecrement + slashableSharesInQueue;
+    ) internal pure returns (uint256) {
+        // round up mulDiv so we don't overslash
+        return operatorShares - operatorShares.mulDiv(newMaxMagnitude, prevMaxMagnitude, Math.Rounding.Up);
     }
 }

src/contracts/libraries/Snapshots.sol

@@ -3,7 +3,6 @@
 pragma solidity ^0.8.0;
 
 import "@openzeppelin-upgrades/contracts/utils/math/MathUpgradeable.sol";
-import "@openzeppelin-upgrades/contracts/utils/math/SafeCastUpgradeable.sol";
 
 import "./SlashingLib.sol";
 
@@ -24,8 +23,6 @@ import "./SlashingLib.sol";
  * _Available since v4.5._
  */
 library Snapshots {
-    using SafeCastUpgradeable for uint256;
-
     struct DefaultWadHistory {
         Snapshot[] _snapshots;
     }
@@ -43,22 +40,17 @@ library Snapshots {
 
     /**
      * @dev Pushes a (`key`, `value`) pair into a DefaultWadHistory so that it is stored as the snapshot.
-     *
-     * Returns previous value and new value.
      */
-    function push(DefaultWadHistory storage self, uint32 key, uint64 value) internal returns (uint64, uint64) {
-        (uint224 prevValue, uint224 currValue) = _insert(self._snapshots, key, value);
-        return (uint64(prevValue), uint64(currValue));
+    function push(DefaultWadHistory storage self, uint32 key, uint64 value) internal {
+        _insert(self._snapshots, key, value);
     }
 
     /**
      * @dev Pushes a (`key`, `value`) pair into a DefaultZeroHistory so that it is stored as the snapshot.
-     * Peforms a safecast to uint224 for the value.
-     *
-     * Returns previous value and new value.
+     * `value` is cast to uint224. Responsibility for the safety of this operation falls outside of this library.
      */
-    function push(DefaultZeroHistory storage self, uint32 key, uint256 value) internal returns (uint256, uint256) {
-        return _insert(self._snapshots, key, value.toUint224());
+    function push(DefaultZeroHistory storage self, uint32 key, uint256 value) internal {
+        _insert(self._snapshots, key, uint224(value));
     }
 
     /**
@@ -77,19 +69,6 @@ library Snapshots {
         return _upperLookup(self._snapshots, key, 0);
     }
 
-    /**
-     * @dev Returns the value in the last (most recent) snapshot with key lower or equal than the search key, or `defaultValue` if there is none.
-     */
-    function _upperLookup(
-        Snapshot[] storage snapshots,
-        uint32 key,
-        uint224 defaultValue
-    ) private view returns (uint224) {
-        uint256 len = snapshots.length;
-        uint256 pos = _upperBinaryLookup(snapshots, key, 0, len);
-        return pos == 0 ? defaultValue : _unsafeAccess(snapshots, pos - 1)._value;
-    }
-
     /**
      * @dev Returns the value in the most recent snapshot, or WAD if there are no snapshots.
      */
@@ -108,14 +87,6 @@ library Snapshots {
         return uint256(_latest(self._snapshots, 0));
     }
 
-    /**
-     * @dev Returns the value in the most recent snapshot, or `defaultValue` if there are no snapshots.
-     */
-    function _latest(Snapshot[] storage snapshots, uint224 defaultValue) private view returns (uint224) {
-        uint256 pos = snapshots.length;
-        return pos == 0 ? defaultValue : _unsafeAccess(snapshots, pos - 1)._value;
-    }
-
     /**
      * @dev Returns the number of snapshots.
      */
@@ -138,27 +109,44 @@ library Snapshots {
      * @dev Pushes a (`key`, `value`) pair into an ordered list of snapshots, either by inserting a new snapshot,
      * or by updating the last one.
      */
-    function _insert(Snapshot[] storage self, uint32 key, uint224 value) private returns (uint224, uint224) {
+    function _insert(Snapshot[] storage self, uint32 key, uint224 value) private {
         uint256 pos = self.length;
 
         if (pos > 0) {
-            // Copying to memory is important here.
+            // Validate that inserted keys are always >= the previous key
             Snapshot memory last = _unsafeAccess(self, pos - 1);
-
-            // Snapshot keys must be non-decreasing.
             require(last._key <= key, InvalidSnapshotOrdering());
 
-            // Update or push new snapshot
+            // Update existing snapshot if `key` matches
             if (last._key == key) {
                 _unsafeAccess(self, pos - 1)._value = value;
-            } else {
-                self.push(Snapshot({_key: key, _value: value}));
+                return;
             }
-            return (last._value, value);
-        } else {
-            self.push(Snapshot({_key: key, _value: value}));
-            return (0, value);
         }
+
+        // `key` was not in the list; push as a new entry
+        self.push(Snapshot({_key: key, _value: value}));
+    }
+
+    /**
+     * @dev Returns the value in the last (most recent) snapshot with key lower or equal than the search key, or `defaultValue` if there is none.
+     */
+    function _upperLookup(
+        Snapshot[] storage snapshots,
+        uint32 key,
+        uint224 defaultValue
+    ) private view returns (uint224) {
+        uint256 len = snapshots.length;
+        uint256 pos = _upperBinaryLookup(snapshots, key, 0, len);
+        return pos == 0 ? defaultValue : _unsafeAccess(snapshots, pos - 1)._value;
+    }
+
+    /**
+     * @dev Returns the value in the most recent snapshot, or `defaultValue` if there are no snapshots.
+     */
+    function _latest(Snapshot[] storage snapshots, uint224 defaultValue) private view returns (uint224) {
+        uint256 pos = snapshots.length;
+        return pos == 0 ? defaultValue : _unsafeAccess(snapshots, pos - 1)._value;
     }
 
     /**

src/test/mocks/DelegationManagerMock.sol

@@ -28,13 +28,11 @@ contract DelegationManagerMock is Test {
     function burnOperatorShares(
         address operator,
         IStrategy strategy,
-        uint256 slashableSharesInQueue,
         uint64 prevMaxMagnitude,
         uint64 newMaxMagnitude
     ) external {
-        (uint256 amountSlashed, /*uint256 amountBurned*/) = SlashingLib.calcSlashedAmount({
+        uint256 amountSlashed = SlashingLib.calcSlashedAmount({
             operatorShares: operatorShares[operator][strategy],
-            slashableSharesInQueue: slashableSharesInQueue,
             prevMaxMagnitude: prevMaxMagnitude,
             newMaxMagnitude: newMaxMagnitude
         });

src/test/unit/DelegationUnit.t.sol

@@ -381,7 +381,7 @@ contract DelegationManagerUnitTests is EigenLayerUnitTestSetup, IDelegationManag
         DepositScalingFactor memory _dsf = DepositScalingFactor(delegationManager.depositScalingFactor(staker, strategy));
         uint256 sharesToWithdraw = _dsf.calcWithdrawable(depositSharesToWithdraw, slashingFactor);
 
-        uint256 scaledShares = SlashingLib.scaleSharesForQueuedWithdrawal({
+        uint256 scaledShares = SlashingLib.scaleForQueueWithdrawal({
             sharesToWithdraw: sharesToWithdraw,
             slashingFactor: slashingFactor
         });