EigenLayer Contest - SpicyMeatball's results

Lines of code

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

Vulnerability details

Impact

As described in comments we use indicesToSkip array to ignore malicious strategies which revert on withdrawal effectively preventing queued withdrawal from slashing. Unfortunately current implementation of the skip feature doesn't work, it doesn't matter which indices we include in indicesToSkip - all strategies withdraw methods will still be called.

Proof of Concept

Let's try the for loop from slashQueuedWithdrawal in Remix

// SPDX-License-Identifier: MIT

pragma solidity 0.8.6;

contract SkipBadStrategies{
    string[] public goodStrategies;

    function skipArray(string[] memory strategies, uint256[] memory indicesToSkip) public {
        uint256 indicesToSkipIndex = 0;
        uint256 strategiesLength = 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 (strategies[i] == "eth"){
                //    goodStrategies.push(strategies[i]);
                //} else {
                    goodStrategies.push(strategies[i]);
                //}
                unchecked {
                    ++i;
                }
            }
        }
    }
    function getGoodStrategies() public view returns(string[] memory) {
        return goodStrategies;
    }
}

We input strategies: ["bad0", "bad1", "good0", "good1", "bad3"] indicesToSkip: [0, 1, 4] Result in goodStrategies will be: ["bad0", "bad1", "good0", "good1", "bad3"] Bad strategies weren't skipped

Tools Used

Remix IDE

Recommended Mitigation Steps

Place ++i outside of the else block

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;
            }
        }