code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/StrategyManager.sol#L536

Vulnerability details

Impact

In src/contracts/core/StrategyManager.sol#L536 parameter indicesToSkip per documentation: """exists so that, e.g., if the slashed QueuedWithdrawal contains a malicious strategy in the strategies array which always reverts on calls to its 'withdraw' function, then the malicious strategy can be skipped (with the shares in effect "burned"), while the non-malicious strategies are still called as normal""".

However, the maliciuos strategies are not skipped because the loop index is incremented incorrectly, which allows one malicious strategy to effectively block withdrawal from all staker's strategies.

Proof of Concept

The following test (modified testSlashQueuedWithdrawalNotBeaconChainETH with non-empty indicesToSkip, added at src/test/unit/StrategyManagerUnit.t.sol#L1575) passes, although it should not:

function testSlashQueuedWithdrawalWithSkipping() external {
    address recipient = address(333);
    uint256 depositAmount = 1e18;
    uint256 withdrawalAmount = depositAmount;
    bool undelegateIfPossible = false;

    (IStrategyManager.QueuedWithdrawal memory queuedWithdrawal, /*IERC20[] memory tokensArray*/, bytes32 withdrawalRoot) =
        testQueueWithdrawal_ToSelf_NotBeaconChainETH(depositAmount, withdrawalAmount, undelegateIfPossible);

    uint256 balanceBefore = dummyToken.balanceOf(address(recipient));

    // slash the delegatedOperator
    slasherMock.freezeOperator(queuedWithdrawal.delegatedAddress);

    uint256[] memory indicesToSkip = new uint256[](1);
    indicesToSkip[0] = 0;

    cheats.startPrank(strategyManager.owner());
    strategyManager.slashQueuedWithdrawal(recipient, queuedWithdrawal, _arrayWithJustDummyToken(), indicesToSkip);
    cheats.stopPrank();

    uint256 balanceAfter = dummyToken.balanceOf(address(recipient));

    require(balanceAfter == balanceBefore + withdrawalAmount, "balanceAfter != balanceBefore + withdrawalAmount");
    require(!strategyManager.withdrawalRootPending(withdrawalRoot), "withdrawalRootPendingAfter is true!");
}

Recommended Mitigation Steps

Increment the loop index in every loop cycle:

diff --git a/src/contracts/core/StrategyManager.sol b/src/contracts/core/StrategyManager.sol
index fdf224a..58404a1 100644
--- a/src/contracts/core/StrategyManager.sol
+++ b/src/contracts/core/StrategyManager.sol
@@ -571,9 +571,9 @@ contract StrategyManager is
                     // tell the strategy to send the appropriate amount of funds to the recipient
                     queuedWithdrawal.strategies[i].withdraw(recipient, tokens[i], queuedWithdrawal.shares[i]);
                 }
-                unchecked {
-                    ++i;
-                }
+            }
+            unchecked {
+                ++i;
             }
         }
     }

Assessed type

Loop

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as duplicate of #205

c4-judge commented 1 year ago

GalloDaSballo marked the issue as satisfactory