refactor: decreaseBurnOrRedistributableShares (#1414)

**Motivation:**

With the addition of non-arrified versions of `decreaseBurnableShares`
and `releaseSlashEscrow` we want to avoid the use of
`EnumerableSet.at()` as it reverts when attempting to retrieve indices
that do not exist (> length).

**Modifications:**

- Use `EnumerableSet.tryGet` and `EnumerableSet.keys` instead which does
not revert (but simply does nothing) if the index doesn't exist.
- Rename `decreaseBurnOrRedistributableShares` ->
`clearBurnOrRedistributableShares` to better reflect the functions
purpose.

**Result:**

Protocol guarantees are maintained, and better naming.

Author: 0xClandestine

src/contracts/core/SlashEscrowFactory.sol

@@ -100,11 +100,11 @@ contract SlashEscrowFactory is Initializable, SlashEscrowFactoryStorage, Ownable
         // Assert that the escrow delay has elapsed
         require(block.number >= getEscrowCompleteBlock(operatorSet, slashId), EscrowDelayNotElapsed());
 
-        // Calling `decreaseBurnOrRedistributableShares` will transfer the underlying tokens to the `SlashEscrow`.
-        // NOTE: While `decreaseBurnOrRedistributableShares` may have already been called, we call it again to ensure that the
+        // Calling `clearBurnOrRedistributableShares` will transfer the underlying tokens to the `SlashEscrow`.
+        // NOTE: While `clearBurnOrRedistributableShares` may have already been called, we call it again to ensure that the
         // underlying tokens are actually in escrow before processing and removing storage (which would otherwise prevent
         // the tokens from being released).
-        strategyManager.decreaseBurnOrRedistributableShares(operatorSet, slashId);
+        strategyManager.clearBurnOrRedistributableShares(operatorSet, slashId);
 
         // Release the slashEscrow. The `SlashEscrow` is deployed in `initiateSlashEscrow`.
         _processSlashEscrow(operatorSet, slashId, getSlashEscrow(operatorSet, slashId), redistributionRecipient);

src/contracts/core/StrategyManager.sol

@@ -170,41 +170,44 @@ contract StrategyManager is
     }
 
     /// @inheritdoc IStrategyManager
-    function decreaseBurnOrRedistributableShares(
+    function clearBurnOrRedistributableShares(
         OperatorSet calldata operatorSet,
         uint256 slashId
     ) external nonReentrant {
         // Iterate over burnable shares backwards. Iterating with an increasing index can cause
         // elements to be missed as the item to remove is swapped and popped with the last element.
-        uint256 length = _burnOrRedistributableShares[operatorSet.key()][slashId].length();
+        address[] memory strategies = _burnOrRedistributableShares[operatorSet.key()][slashId].keys();
+        uint256 length = strategies.length;
 
-        for (uint256 i = length; i > 0; --i) {
-            decreaseBurnOrRedistributableShares(operatorSet, slashId, i - 1);
+        for (uint256 i = 0; i < length; ++i) {
+            clearBurnOrRedistributableSharesByStrategy(operatorSet, slashId, IStrategy(strategies[i]));
         }
     }
 
     /// @inheritdoc IStrategyManager
-    function decreaseBurnOrRedistributableShares(
+    function clearBurnOrRedistributableSharesByStrategy(
         OperatorSet calldata operatorSet,
         uint256 slashId,
-        uint256 index
+        IStrategy strategy
     ) public returns (uint256) {
         EnumerableMap.AddressToUintMap storage burnOrRedistributableShares =
             _burnOrRedistributableShares[operatorSet.key()][slashId];
 
-        (address strategy, uint256 sharesToRemove) = burnOrRedistributableShares.at(index);
-
-        // Withdraw the shares to the slash escrow.
-        IStrategy(strategy).withdraw({
-            recipient: address(slashEscrowFactory.getSlashEscrow(operatorSet, slashId)),
-            token: IStrategy(strategy).underlyingToken(),
-            amountShares: sharesToRemove
-        });
+        (, uint256 sharesToRemove) = burnOrRedistributableShares.tryGet(address(strategy));
 
         burnOrRedistributableShares.remove(address(strategy));
 
-        // Emit an event to notify the that burnable shares have been decreased.
-        emit BurnOrRedistributableSharesDecreased(operatorSet, slashId, IStrategy(strategy), sharesToRemove);
+        if (sharesToRemove != 0) {
+            // Withdraw the shares to the slash escrow.
+            IStrategy(strategy).withdraw({
+                recipient: address(slashEscrowFactory.getSlashEscrow(operatorSet, slashId)),
+                token: IStrategy(strategy).underlyingToken(),
+                amountShares: sharesToRemove
+            });
+
+            // Emit an event to notify the that burnable shares have been decreased.
+            emit BurnOrRedistributableSharesDecreased(operatorSet, slashId, strategy, sharesToRemove);
+        }
 
         return sharesToRemove;
     }

src/contracts/interfaces/IStrategyManager.sol

@@ -140,18 +140,19 @@ interface IStrategyManager is IStrategyManagerErrors, IStrategyManagerEvents, IS
      * @param operatorSet The operator set to burn shares in.
      * @param slashId The slash ID to burn shares in.
      */
-    function decreaseBurnOrRedistributableShares(OperatorSet calldata operatorSet, uint256 slashId) external;
+    function clearBurnOrRedistributableShares(OperatorSet calldata operatorSet, uint256 slashId) external;
 
     /**
      * @notice Removes a single strategy's shares from storage and transfers the underlying tokens for the slashId to the slash escrow.
      * @param operatorSet The operator set to burn shares in.
      * @param slashId The slash ID to burn shares in.
-     * @param index The index of the strategy to burn shares in. Returns the amount of shares that were burned.
+     * @param strategy The strategy to burn shares in.
+     * @return The amount of shares that were burned.
      */
-    function decreaseBurnOrRedistributableShares(
+    function clearBurnOrRedistributableSharesByStrategy(
         OperatorSet calldata operatorSet,
         uint256 slashId,
-        uint256 index
+        IStrategy strategy
     ) external returns (uint256);
 
     /**

src/test/mocks/StrategyManagerMock.sol

@@ -109,7 +109,7 @@ contract StrategyManagerMock is Test {
         return (existingShares, addedShares);
     }
 
-    function decreaseBurnOrRedistributableShares(IStrategy strategy, uint sharesToBurn) external {}
+    function clearBurnOrRedistributableShares(IStrategy strategy, uint sharesToBurn) external {}
 
     function getBurnOrRedistributableCount(OperatorSet calldata operatorSet, uint slashId) external view returns (uint) {
         return _burnOrRedistributableCount;

src/test/unit/StrategyManagerUnit.t.sol

@@ -1190,7 +1190,7 @@ contract StrategyManagerUnitTests_increaseBurnOrRedistributableShares is Strateg
     }
 }
 
-contract StrategyManagerUnitTests_decreaseBurnOrRedistributableShares is StrategyManagerUnitTests {
+contract StrategyManagerUnitTests_clearBurnOrRedistributableShares is StrategyManagerUnitTests {
     function _increaseBurnOrRedistributableShares(IStrategy strategy, uint sharesToAdd) internal {
         cheats.prank(address(delegationManagerMock));
         strategyManager.increaseBurnOrRedistributableShares(defaultOperatorSet, defaultSlashId, strategy, sharesToAdd);
@@ -1209,7 +1209,7 @@ contract StrategyManagerUnitTests_decreaseBurnOrRedistributableShares is Strateg
 
         cheats.expectEmit(true, true, true, true, address(strategyManager));
         emit BurnOrRedistributableSharesDecreased(defaultOperatorSet, defaultSlashId, strategy, shares);
-        strategyManager.decreaseBurnOrRedistributableShares(defaultOperatorSet, defaultSlashId);
+        strategyManager.clearBurnOrRedistributableShares(defaultOperatorSet, defaultSlashId);
 
         (IStrategy[] memory escrowStrats, uint[] memory escrowShares) =
             strategyManager.getBurnOrRedistributableShares(defaultOperatorSet, defaultSlashId);
@@ -1231,7 +1231,7 @@ contract StrategyManagerUnitTests_decreaseBurnOrRedistributableShares is Strateg
 
         cheats.expectEmit(true, true, true, true, address(strategyManager));
         emit BurnOrRedistributableSharesDecreased(defaultOperatorSet, defaultSlashId, strategy, shares);
-        strategyManager.decreaseBurnOrRedistributableShares(defaultOperatorSet, defaultSlashId, 0);
+        strategyManager.clearBurnOrRedistributableSharesByStrategy(defaultOperatorSet, defaultSlashId, strategy);
 
         (IStrategy[] memory escrowStrats, uint[] memory escrowShares) =
             strategyManager.getBurnOrRedistributableShares(defaultOperatorSet, defaultSlashId);
@@ -1260,7 +1260,7 @@ contract StrategyManagerUnitTests_decreaseBurnOrRedistributableShares is Strateg
 
         _increaseBurnOrRedistributableShares(strategies, sharesToAdd);
 
-        strategyManager.decreaseBurnOrRedistributableShares(defaultOperatorSet, defaultSlashId);
+        strategyManager.clearBurnOrRedistributableShares(defaultOperatorSet, defaultSlashId);
 
         (IStrategy[] memory escrowStrats, uint[] memory escrowShares) =
             strategyManager.getBurnOrRedistributableShares(defaultOperatorSet, defaultSlashId);
@@ -1292,11 +1292,13 @@ contract StrategyManagerUnitTests_decreaseBurnOrRedistributableShares is Strateg
         _increaseBurnOrRedistributableShares(strategies, sharesToAdd);
 
         // Remove shares in random order
-        for (uint i = 0; i < strategies.length; ++i) {
-            // Create a random index to remove shares
-            uint index = r.Uint256(0, strategies.length - i - 1);
+        uint[] memory indices = new uint[](3);
+        indices[0] = 1; // dummyStrat2
+        indices[1] = 0; // dummyStrat
+        indices[2] = 2; // dummyStrat3
 
-            strategyManager.decreaseBurnOrRedistributableShares(defaultOperatorSet, defaultSlashId, index);
+        for (uint i = 0; i < strategies.length; ++i) {
+            strategyManager.clearBurnOrRedistributableSharesByStrategy(defaultOperatorSet, defaultSlashId, strategies[indices[i]]);
 
             (IStrategy[] memory strats, uint[] memory sharesToBurnOrRedistribute) =
                 strategyManager.getBurnOrRedistributableShares(defaultOperatorSet, defaultSlashId);
@@ -1340,7 +1342,7 @@ contract StrategyManagerUnitTests_decreaseBurnOrRedistributableShares is Strateg
 
         cheats.expectRevert("SafeERC20: low-level call failed");
         cheats.prank(address(delegationManagerMock));
-        strategyManager.decreaseBurnOrRedistributableShares(defaultOperatorSet, defaultSlashId);
+        strategyManager.clearBurnOrRedistributableShares(defaultOperatorSet, defaultSlashId);
 
         assertEq(
             strategyManager.getBurnOrRedistributableShares(defaultOperatorSet, defaultSlashId, strategy),