EigenLayer Contest - Ruhum's results

Lines of code

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

Vulnerability details

Impact

When slashing a queued withdrawal, the owner can specify an array of strategies that are supposed to be skipped, i.e. not withdrawn from. But, the implementation has a bug causing none of the strategies to be skipped.

The feature was implemented in case

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.

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

Proof of Concept

When you loop over the strategies to withdraw the funds, the function checks whether the current strategy is supposed to be skipped. If that's the case it doesn't execute the withdrawal:

        // keeps track of the index in the `indicesToSkip` array
        uint256 indicesToSkipIndex = 0;

        uint256 strategiesLength = queuedWithdrawal.strategies.length;
        for (uint256 i = 0; i < strategiesLength;) {
            // check if the index i matches one of the indices specified in the `indicesToSkip` array
            if (indicesToSkipIndex < indicesToSkip.length && indicesToSkip[indicesToSkipIndex] == i) {
                unchecked {
                    ++indicesToSkipIndex;
                }
            } else {
                if (queuedWithdrawal.strategies[i] == beaconChainETHStrategy){
                     //withdraw the beaconChainETH to the recipient
                    _withdrawBeaconChainETH(queuedWithdrawal.depositor, recipient, queuedWithdrawal.shares[i]);
                } else {
                    // 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;
                }
            }
        }

The issue is that it only increments indiciesToSkipIndex and not i. Meaning, loop will be executed again with the same value for i but with a different value for inidicesToSkipIndex. In that execution indicesToSkip[indicesToSkipIndex] != i thus, the funds will be withdrawn from it.

Here's a PoC:

// StrategyManagerUnit.t.sol
    function testIssue() external {
        uint depositAmount = 1e18;
        address recipient = address(333);

        // add a deposit so we can withdraw it later on
        testDepositIntoStrategySuccessfully(/*staker*/ address(this), depositAmount);

        IStrategy[] memory strategyArray = new IStrategy[](2);
        IERC20[] memory tokensArray = new IERC20[](2);
        uint256[] memory shareAmounts = new uint256[](2);
        strategyArray[0] = dummyStrat;
        tokensArray[0] = dummyToken;
        shareAmounts[0] = 5e17;
        strategyArray[1] = dummyStrat;
        tokensArray[1] = dummyToken;
        shareAmounts[1] = 5e17;

        IStrategyManager.WithdrawerAndNonce memory withdrawerAndNonce = IStrategyManager.WithdrawerAndNonce({
            withdrawer: recipient,
            nonce: uint96(strategyManager.numWithdrawalsQueued(address(this)))
        });

        IStrategyManager.QueuedWithdrawal memory queuedWithdrawal = 
            IStrategyManager.QueuedWithdrawal({
                strategies: strategyArray,
                shares: shareAmounts,
                depositor: address(this),
                withdrawerAndNonce: withdrawerAndNonce,
                withdrawalStartBlock: uint32(block.number),
                delegatedAddress: strategyManager.delegation().delegatedTo(address(this))
            }
        );
        // calculate the withdrawal root
        bytes32 withdrawalRoot = strategyManager.calculateWithdrawalRoot(queuedWithdrawal);
        
        uint256[] memory strategyIndexes = new uint256[](2);
        strategyIndexes[0] = 0;
        strategyIndexes[1] = 0;

        strategyManager.queueWithdrawal(
            strategyIndexes,
            queuedWithdrawal.strategies,
            queuedWithdrawal.shares,
            /*withdrawer*/ recipient,
            false 
        );
        uint256 balanceBefore = dummyToken.balanceOf(address(recipient));

        slasherMock.freezeOperator(queuedWithdrawal.delegatedAddress);

        cheats.startPrank(strategyManager.owner());
        uint[] memory toSkip = new uint[](1);
        // skip first strategy
        toSkip[0] = 0;
        strategyManager.slashQueuedWithdrawal(recipient, queuedWithdrawal, tokensArray, toSkip);
        cheats.stopPrank();

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

        assertEq(balanceAfter, balanceBefore + 5e17, "balanceAfter != balanceBefore + withdrawalAmount");
    }
 

Tools Used

Recommended Mitigation Steps

Increment i as well:

    if (indicesToSkipIndex < indicesToSkip.length && indicesToSkip[indicesToSkipIndex] == i) {
                unchecked {
                    ++i;
                    ++indicesToSkipIndex;
                }

Assessed type

Loop