code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/StrategyManager.sol#L370-L374 https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/StrategyManager.sol#L501-L505 https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/StrategyManager.sol#L701 https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/StrategyManager.sol#L715-L740

Vulnerability details

Proof of Concept

strategyIndexes is used to indicate which strategies the caller will withdraw 100% of his shares, but it can contain any value when calling StrategyManager.queueWithdrawal() and StrategyManager.slashShares().

These two functions will reuse StrategyManager._removeShares(), which calls StrategyManager._removeStrategyFromStakerStrategyList() which does not enforce for the index and the strategy to match.

if (stakerStrategyList[depositor][strategyIndex] == strategy) {
    // replace the strategy with the last strategy in the list
    stakerStrategyList[depositor][strategyIndex] =
        stakerStrategyList[depositor][stakerStrategyList[depositor].length - 1];
} else {
    //loop through all of the strategies, find the right one, then replace
    ...

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

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

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

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

Impact

The caller might pass some indexes that are not going to be removed if they don't match the strategy and this can result in silent failures, since no revert will take place.

Removing strategies incorrectly or in an unexpected way might break the shares accounting and withdrawals logic.

Tools Used

Manual review.

Recommendation

I would recommend to not reuse StrategyManager._removeStrategyFromStakerStrategyList() when calling StrategyManager.queueWithdrawal() and StrategyManager.slashShares() , and create a new function that enforces the correct index to match, e.g. revert if the index being pass doesn't match the strategy.

If strategyIndexes was intended to be optional, an alternative can be two create two versions for StrategyManager.queueWithdrawal() and StrategyManager.slashShares() , where the first version receives and validates strategyIndexes and the second version doesn't receive strateIndexes and scan for the indexes manually to match the strategy to withdraw all the shares.