code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

Users can avoid getting their queuedWithdrawal slashed because of the wrong implementation.

Proof of Concept

Let's take a look at the following code snippet from StrategyManager#slashQueuedWithdrawal.

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

Now let's say we want to skip only the first strategy because it always reverts on withdraw. We will do this be calling slashQueuedWithdrawal with indicesToSkip = [0]. If we follow the code we can see that in the first iteration i=0 and indicesToSkipIndex=0. We enter the if part since the condition is true (indicesToSkip[0] == 0). This will increase the indicesToSkipIndex to 1.

Now in second iteration we have indicesToSkipIndex = 1 and i = 0. The if part is not true in this iteration so we enter the else part of the conditional statement and essentially try to withdraw from the strategy[0] that we wanted to skip.

User can set such an array of strategies when queueing withdrawal that withdrawing from the first strategy always fails. This way his queued withdrawal cannot get slashed.

Tools Used

Recommended Mitigation Steps

Put the ++i statement outside of else part.

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

+   unchecked {
+       ++i;
+   }
}

Assessed type

DoS

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as duplicate of #205

c4-judge commented 1 year ago

GalloDaSballo changed the severity to 3 (High Risk)