A malicious strategy can permanently DoS all currently pending withdrawals that contain it

[code423n4]

Lines of code

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

Vulnerability details

Impact

A malicious strategy can permanently DoS all currently pending withdrawals that contain it.

Vulnerability details

In order to withdrawal funds from the project a user has to:

1. queue a withdrawal (via queueWithdrawal)
2. complete a withdrawal (via completeQueuedWithdrawal(s))

Queuing a withdrawal, via queueWithdrawal modifies all internal accounting to reflect this:

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

        // modify delegated shares accordingly, if applicable
        delegation.decreaseDelegatedShares(msg.sender, strategies, shares);

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

    if (_removeShares(msg.sender, strategyIndexes[strategyIndexIndex], strategies[i], shares[i])) {

and saves the withdrawl hash in order to be used in completeQueuedWithdrawal(s)

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

            queuedWithdrawal = QueuedWithdrawal({
                strategies: strategies,
                shares: shares,
                depositor: msg.sender,
                withdrawerAndNonce: withdrawerAndNonce,
                withdrawalStartBlock: uint32(block.number),
                delegatedAddress: delegatedAddress
            });

        }

        // calculate the withdrawal root
        bytes32 withdrawalRoot = calculateWithdrawalRoot(queuedWithdrawal);

        // mark withdrawal as pending
        withdrawalRootPending[withdrawalRoot] = true;

In other words, it is final (as there is no cancelWithdrawl mechanism implemented).

When executingcompleteQueuedWithdrawal(s), the withdraw function of the strategy is called (if receiveAsTokens is set to true).

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

    // tell the strategy to send the appropriate amount of funds to the depositor
    queuedWithdrawal.strategies[i].withdraw(
        msg.sender, tokens[i], queuedWithdrawal.shares[i]
    );

In this case, a malicious strategy can always revert, blocking the user from retrieving his tokens.

If a user sets receiveAsTokens to false, the other case, then the tokens will be added as shares to the delegation contract

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

    for (uint256 i = 0; i < strategiesLength;) {
        _addShares(msg.sender, queuedWithdrawal.strategies[i], queuedWithdrawal.shares[i]);
        unchecked {
            ++i;

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

        // if applicable, increase delegated shares accordingly
        delegation.increaseDelegatedShares(depositor, strategy, shares);

This still poses a problem because the increaseDelegatedShares function's counterpart, decreaseDelegatedShares is only callable by the strategy manager (so as for users to indirectly get theier rewards worth back via this workaround)

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

    /**
     * @notice Decreases the `staker`'s delegated shares in each entry of `strategies` by its respective `shares[i]`, typically called when the staker withdraws from EigenLayer
     * @dev Callable only by the StrategyManager
     */
    function decreaseDelegatedShares(
        address staker,
        IStrategy[] calldata strategies,
        uint256[] calldata shares
    )
        external
        onlyStrategyManager
    {

but in StrategyManager there are only 3 cases where this is called, none of which is beneficial for the user:

• recordOvercommittedBeaconChainETH - slashes user rewards in certain conditions
• queueWithdrawal - accounting side effects have already been done, will fail in _removeShares
• slashShares - slashes user rewards in certain conditions

In other words, the workaround (of setting receiveAsTokens to false in completeQueuedWithdrawal(s)) just leaves the funds accounted and stuck in DelegationManager.

In both cases all shares/tokens associated with other strategies in the pending withdrawal are permanently blocked.

Proof of Concept

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

Tools Used

Manual analysis

Recommended Mitigation Steps

Add a cancelQueuedWithdrawl function that will undo the accounting and cancel out any dependency on the malicious strategy. Although this solution may create a front-running opportunity for when their withdrawal will be slashed via slashQueuedWithdrawal, there may exist workarounds to this.

Another possibility is to implement a similar mechanism to how slashQueuedWithdrawal treats malicious strategies: adding a list of strategy indices to skip

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

     * @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.
     */

     ...

        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 {     

Assessed type

DoS

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-sponsor]

Sidu28 marked the issue as disagree with severity

[Sidu28]

The description of lack of check here is accurate. There is zero actual impact; we think this is informational severity.

[GalloDaSballo]

@Sidu28 if we assumed the Strategy not to be malicious, but to revert for some reason (e.g. lack of liquidity, smart contract bug, etc...)

Would you consider the finding as valid since the user cannot withdraw from all others due to having one withdraw, or is there some other reason why you believe the finding to be Informational?

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)

[GalloDaSballo]

The Warden has shown how, any revert in a strategy (for example it being paused), will make queued withdrawals revert, even if the withdrawal should work for other strategies

This falls into the category of DOS and I believe is more appropriately judged as Medium

[ChaoticWalrus]

The Warden has shown how, any revert in a strategy (for example it being paused), will make queued withdrawals revert, even if the withdrawal should work for other strategies

This falls into the category of DOS and I believe is more appropriately judged as Medium

Even if a malicious strategy exists, there is no permanent delay here.

Users can mark the receiveAsTokens input to completeQueuedWithdrawal as false here https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/StrategyManager.sol#L437-L442 and then the shares will be transferred to the 'withrdrawing' user here https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/StrategyManager.sol#L795-L803 who can then queue a new withdrawal not containing the malicious strategy.

The description provided by the warden of the DelegationManager's behavior is inaccurate -- the funds do not end up 'stuck in the DelegationManager' -- they will be withdrawable as part of a new queued withdrawal which excludes the malicious strategy.

[abarbatei]

The Warden has shown how, any revert in a strategy (for example it being paused), will make queued withdrawals revert, even if the withdrawal should work for other strategies This falls into the category of DOS and I believe is more appropriately judged as Medium

Even if a malicious strategy exists, there is no permanent delay here.

Users can mark the receiveAsTokens input to completeQueuedWithdrawal as false here https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/StrategyManager.sol#L437-L442 and then the shares will be transferred to the 'withrdrawing' user here https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/StrategyManager.sol#L795-L803 who can then queue a new withdrawal not containing the malicious strategy.

The description provided by the warden of the DelegationManager's behavior is inaccurate -- the funds do not end up 'stuck in the DelegationManager' -- they will be withdrawable as part of a new queued withdrawal which excludes the malicious strategy.

Hey, https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/StrategyManager.sol#L795-L803 does not do:

shares will be transferred to the 'withrdrawing' user

it simply increments internal accounting and delegated shares associated to user in the delegation contract. But these are not withdrawn without a queueWithdrawal call

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

queue a new withdrawal not containing the malicious strategy.

a new queue can not reuse the already used balance as it was already accounted for in https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/StrategyManager.sol#L345-L346

Maybe I am missing something of course. Regardless, awaiting judge feedback.

[ChaoticWalrus]

it simply increments internal accounting and delegated shares associated to user in the delegation contract. But these are not withdrawn without a queueWithdrawal call

Yes, this is what I would describe as transferring the shares, similar to how the 'transfer in' portion of an ERC20 transfer works. But this is a semantic argument; I agree with the substance of your description.

a new queue can not reuse the already used balance as it was already accounted for in https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/StrategyManager.sol#L345-L346

Maybe I am missing something of course. Regardless, awaiting judge feedback.

I think perhaps you are missing that the internal _addShares function calls the Delegation Manager as well? See here https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/StrategyManager.sol#L646-L647

If the user marks receiveAsTokens as false, then this line will be triggered https://github.com/code-423n4/2023-04-eigenlayer/blob/5e4872358cd2bda1936c29f460ece2308af4def6/src/contracts/core/StrategyManager.sol#L798 which calls into the internal _addShares function and 're-adds' these delegated shares, so they can indeed be queued in a new withdrawal.

I can provide an example if it will help?

[ChaoticWalrus]

Basically, the 're-adding' reverses the accounting taken in the initial queueWithdrawal action, to clarify. This then allows the accounting done in the queueWithdrawal action to be performed once again, when a new withdrawal is queued.

[ChaoticWalrus]

@GalloDaSballo would appreciate you taking another look at this -- I am happy to provide additional context and/or an example, if desired.

[GalloDaSballo]

Thank you @ChaoticWalrus for the tag, will check today

[GalloDaSballo]

@ChaoticWalrus

It seems like the worst case scenario would be having to re-queue without the missing strategy which would require waiting for the withdrawal to unlock

So the question left to answer is whether an additional wait period is acceptable

NOTE: I edited this comment because as you pointed out the shares accounting is internal and doesn't trigger an interaction with the strategy

[ChaoticWalrus]

It seems like the worst case scenario would be having to re-queue without the missing strategy which would require waiting for the withdrawal to unlock

Yes, agreed this is the impact + worst-case scenario. The existing functionality was designed -- in part at least -- to address concerns around malicious Strategies.

So the question left to answer is whether an additional wait period is acceptable

I suppose so, yes. We have deemed this acceptable ourselves but I could see it being viewed differently. Regardless, the impact here is orders of magnitude less than the original claimed impact.

[GalloDaSballo]

The Warden has shown how, due to the possibility of queueing of a withdrawal with multiple strategies, in the case in which one of the strategies stops working (reverts, paused, malicious), the withdrawal would be denied

As the sponsor said, in those scenarios, the withdrawer would have to perform a second withdrawal, which would have to be re-queued

Intuitively, a withdrawer that always withdraws a separate strategy would also never see their withdrawal denied (except for the malicious strategy)

As we can see there are plenty of side steps to the risk shown, however, the functionality of the function is denied, even if temporarily, leading to it not working as intended, and for this reason I believe Medium Severity to be the most appropriate

As shown above, the finding could be nofixed, however it seems to me like most users would want to plan around the scenarios described in this finding and its duplicates

[c4-judge]

GalloDaSballo marked the issue as satisfactory

[c4-judge]

GalloDaSballo marked the issue as selected for report