Conflicting strategy can lead to reverting the whole completeQueuedWithdrawal() and temporary freeze user assets from other strategies

[code423n4]

Lines of code

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

Vulnerability details

A user might want to withdraw funds from multiple strategies at the same time via queueWithdrawal().

If any of those strategies revert when trying to complete the withdrawal via completeQueuedWithdrawal(), the whole transaction will fail, and no funds from any of the other strategies would be able to be withdrawn.

An strategy may fail because of it being malicious (as described on the doc header of slashQueuedWithdrawal()), or because it may contain a bug in the withdraw() function to prevent this user or any other user from calling it.

Impact

Users won't be able to withdraw any of their queued funds from any strategy supposed to be withdrawable after the withdraw period via completeQueuedWithdrawal().

Users will be forced to re-stake the asset shares with receiveAsTokens = false, create a new queue withdrawal without the conflicting strategy, and wait again for the withdrawal period.

This can be considered a temporary freeze of funds, as the users won't be able to dispose of their assets as they should.

A conflicting strategy should not affect the ability of a user to withdraw assets from any other strategy in the platform.

Proof of Concept

StrategyManager::_completeQueuedWithdrawal() will fail when one of the strategies fail to withdraw the tokens.

The conflicting line that can revert is: queuedWithdrawal.strategies[i].withdraw()

This will prevent tokens from other strategies to be withdrawn.

    if (receiveAsTokens) {
        require(tokens.length == queuedWithdrawal.strategies.length, "StrategyManager.completeQueuedWithdrawal: input length mismatch");
        // actually withdraw the funds
        for (uint256 i = 0; i < strategiesLength;) {
            if (queuedWithdrawal.strategies[i] == beaconChainETHStrategy) {

                // if the strategy is the beaconchaineth strat, then withdraw through the EigenPod flow
                _withdrawBeaconChainETH(queuedWithdrawal.depositor, msg.sender, queuedWithdrawal.shares[i]);
            } else {
                // tell the strategy to send the appropriate amount of funds to the depositor
                queuedWithdrawal.strategies[i].withdraw( // @audit will make the whole transaction fail if one strategy reverts
                    msg.sender, tokens[i], queuedWithdrawal.shares[i]
                );
            }
            unchecked {
                ++i;
            }
        }
    } else {
        // else increase their shares
        for (uint256 i = 0; i < strategiesLength;) {
            _addShares(msg.sender, queuedWithdrawal.strategies[i], queuedWithdrawal.shares[i]);
            unchecked {
                ++i;
            }
        }
    }

Link to code

Coded POC

This tests how only one conflicting strategy can revert the whole completeQueuedWithdrawal() operation, affecting the withdrawal of the assets from the other strategies.

Create a file src/test/mocks/ReverterWithdraw.sol like the following to simulate a conflicting strategy:

// SPDX-License-Identifier: BUSL-1.1
pragma solidity ^0.8.9;

import "@openzeppelin/contracts/token/ERC20/IERC20.sol";

contract ReverterWithdraw {
    function deposit(IERC20 token, uint256 amount) external returns (uint256) {
        return amount;
    }

    fallback() external {
        revert("ReverterWithdraw: I am a contract that reverts on withdraw");
    }
}

Add this test to src/test/StrategyManagerUnit.t.sol and run forge test -m "testCompleteQueuedWithdrawal_FailsWithConflictingStrategy".

Include import "../mocks/ReverterWithdraw.sol"; at the start of the file.

    function testCompleteQueuedWithdrawal_FailsWithConflictingStrategy() external {
        // Include `import "../mocks/ReverterWithdraw.sol";` at the start of the file

        // Create a strategy that will revert on `withdraw()`
        ReverterWithdraw reverterWithdraw = new ReverterWithdraw();
        StrategyWrapper reverterStrat = StrategyWrapper(address(reverterWithdraw));

        // Whitelist the strategy for deposit
        cheats.startPrank(strategyManager.owner());
        IStrategy[] memory strategies = new IStrategy[](1);
        strategies[0] = reverterStrat;
        strategyManager.addStrategiesToDepositWhitelist(strategies);
        cheats.stopPrank();

        uint256 depositAmount = 1e18;

        // User deposits into both the conflicting strategy and another one
        strategyManager.depositIntoStrategy(reverterStrat, dummyToken, depositAmount);
        strategyManager.depositIntoStrategy(dummyStrat, dummyToken, depositAmount);

        // Define some variables for queueing the withdrawal
        uint256 withdrawalAmount = 1e9;
        address staker = address(this);
        address withdrawer = address(this);

        uint256[] memory strategyIndexes = new uint256[](2);
        strategyIndexes[0] = 0;
        strategyIndexes[1] = 1;

        IStrategy[] memory strategyArray = new IStrategy[](2);
        strategyArray[0] = reverterStrat;
        strategyArray[1] = dummyStrat;

        IERC20[] memory tokensArray = new IERC20[](2);
        tokensArray[0] = dummyToken;
        tokensArray[1] = dummyToken;

        uint256[] memory sharesArray = new uint256[](2);
        sharesArray[0] = withdrawalAmount;
        sharesArray[1] = withdrawalAmount;

        IStrategyManager.WithdrawerAndNonce memory withdrawerAndNonce = IStrategyManager.WithdrawerAndNonce({
            withdrawer: withdrawer,
            nonce: uint96(strategyManager.numWithdrawalsQueued(staker))
        });
        IStrategyManager.QueuedWithdrawal memory queuedWithdrawal = 
            IStrategyManager.QueuedWithdrawal({
                strategies: strategyArray,
                shares: sharesArray,
                depositor: staker,
                withdrawerAndNonce: withdrawerAndNonce,
                withdrawalStartBlock: uint32(block.number),
                delegatedAddress: strategyManager.delegation().delegatedTo(staker)
            }
        );

        // User queues a withdrawal for assets in both strategies
        strategyManager.queueWithdrawal(strategyIndexes, queuedWithdrawal.strategies, queuedWithdrawal.shares, withdrawer, false);

        // When the user tries to complete the withdrawal, it reverts because of the conflicting strategy
        // The user will be forced to re-stake the whole position, call another queue withdrawal and wait another period
        vm.expectRevert("ReverterWithdraw: I am a contract that reverts on withdraw");
        strategyManager.completeQueuedWithdrawal(queuedWithdrawal, tokensArray, /*middlewareTimesIndex*/ 0, /*receiveAsTokens*/ true);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Add an uint256[] indicesToSkip parameter to _completeQueuedWithdrawal() to skip conflicting strategies (just like it is already done in slashQueuedWithdrawal()).

Skipped strategies can be re-staked to be retried again later, but the rest of the strategies will be withdrawable immediately.

+ function _completeQueuedWithdrawal(uint256[] calldata indicesToSkip, ...) ... {

+   uint256 indicesToSkipIndex = 0;
    if (receiveAsTokens) {
        require(tokens.length == queuedWithdrawal.strategies.length, "StrategyManager.completeQueuedWithdrawal: input length mismatch");
        // actually withdraw the funds
        for (uint256 i = 0; i < strategiesLength;) {
            if (queuedWithdrawal.strategies[i] == beaconChainETHStrategy) {

                // if the strategy is the beaconchaineth strat, then withdraw through the EigenPod flow
                _withdrawBeaconChainETH(queuedWithdrawal.depositor, msg.sender, queuedWithdrawal.shares[i]);
+           } else if (indicesToSkipIndex < indicesToSkip.length && indicesToSkip[indicesToSkipIndex] == i) {
+               _addShares(msg.sender, queuedWithdrawal.strategies[i], queuedWithdrawal.shares[i]);
+               unchecked {
+                   ++indicesToSkipIndex;
+               }
            } else {
                // tell the strategy to send the appropriate amount of funds to the depositor
                queuedWithdrawal.strategies[i].withdraw(
                    msg.sender, tokens[i], queuedWithdrawal.shares[i]
                );
            }
            unchecked {
                ++i;
            }
        }
    } else {
        // else increase their shares
        for (uint256 i = 0; i < strategiesLength;) {
            _addShares(msg.sender, queuedWithdrawal.strategies[i], queuedWithdrawal.shares[i]);
            unchecked {
                ++i;
            }
        }
    }

Assessed type

Token-Transfer

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #132

[c4-judge]

GalloDaSballo marked the issue as satisfactory