EigenLayer Contest - bytes032's results

Lines of code

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

Vulnerability details

Impact

Funds can be locked if one strategy withdrawal fails, preventing users from withdrawing their funds from other strategies in the queued withdrawal.

Proof of Concept

When queuing a withdrawal, the caller can specify a list of strategies to withdraw from.

function queueWithdrawal(
        uint256[] calldata strategyIndexes,
        IStrategy[] calldata strategies,
        uint256[] calldata shares,
        address withdrawer,
        bool undelegateIfPossible
    )
        external
        onlyWhenNotPaused(PAUSED_WITHDRAWALS)
        onlyNotFrozen(msg.sender)
        nonReentrant
        returns (bytes32)
    {

The actual withdrawal takes place in the _completeQueuedWithdrawal function when certain conditions are met.

If the staker is not withdrawing from the BeaconChain and has chosen to receive the funds as tokens, the withdrawal from strategies occurs in the StrategyManager contract.

                    // tell the strategy to send the appropriate amount of funds to the depositor
                    // @audit paused/bricked strategy underlying will throw here
                    queuedWithdrawal.strategies[i].withdraw(
                        msg.sender, tokens[i], queuedWithdrawal.shares[i]
                    );

This, in turn, calls the withdraw function of the strategy.

 function withdraw(address depositor, IERC20 token, uint256 amountShares)
        external
        virtual
        override
        onlyWhenNotPaused(PAUSED_WITHDRAWALS)
        onlyStrategyManager
    {
        require(token == underlyingToken, "StrategyBase.withdraw: Can only withdraw the strategy token");
        ...
        // @audit can dos
        underlyingToken.safeTransfer(depositor, amountToSend);
    }

However, this can be problematic because if a single strategy withdrawal fails, this means the user's funds are locked until the withdrawal is possible again. As there are multiple ERC20's with pausing functionality, this makes the scenario very likely.

Consider the following scenario:

1. User queues withdrawals from 10 strategies
2. 9 strategies are ready to withdraw, but the underlying transfers are paused in one of the strategies.
3. queuedWithdrawal.strategies[i].withdraw(...) reverts, and the user cannot withdraw from any of the strategies in the queued withdrawal.

Tools Used

Manual Review

Recommended Mitigation Steps

The slashQueuedWithdrawal function elegantly mitigates this issue with the indicesToSkip parameter, explicitly used for scenarios where a single strategy withdrawal fails. This allows malicious or problematic strategies to be skipped, while the non-malicious strategies are still called as normal.

My suggestion is to add the same possibility for users when completing withdrawals.

https://github.com/code-423n4/2023-04-eigenlayer/blob/398cc428541b91948f717482ec973583c9e76232/src/contracts/core/StrategyManager.sol#L560-L565

     * @param indicesToSkip Optional input parameter -- indices in the `strategies` array to skip (i.e. not call the 'withdraw' function on). This input 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.
     */

     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 the team is not comfortable with this solution, as alternative, they can implement functionality where the user can cancel individual withdrawals.