Missing lower boundary check on queueWithdrawal() could disrupt/deny slashQueuedWithdrawal()

[code423n4]

Lines of code

https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/StrategyManager.sol#L329-L429 https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/StrategyManager.sol#L536-L579 https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/strategies/StrategyBase.sol#L121-L156

Vulnerability details

Impact

In StrategyManager.sol, there is no minimum withdrawal amount required when a staker calls queueWithdrawal(). This could disrupt the contract owner calling slashQueuedWithdrawal) if the entire shares for the particular strategy have been split into infinitely small chunks of queue.

Proof of Concept

Here is a possible scenario:

1. Bob has delegated Alice, the operator, to operate validated services on strategies he has deposited into.
2. It is becoming apparent that Alice is unreliable in the operated task on strategy A and very likely most shares if not all are going to be slashed.
3. Bob being aware of this chooses the suicide path by repeatedly calling queueWithdrawal() that will have 1e9 - 1 shares removed each till strategy A is removed from stakerStrategyList with the last call on queueWithdrawal() entailing even smaller amount of shares.
4. Bob would undelegate Alice if this were the only strategy in stakerStrategyList, but unfortunately, he has other strategies under the operator's hands doing fairly well.
5. Bob could actually utilize the following function to input all queue data in array parameters if he ever made it to completing the withdrawals:

File: StrategyManager.sol#L456-L469

    function completeQueuedWithdrawals(
        QueuedWithdrawal[] calldata queuedWithdrawals,
        IERC20[][] calldata tokens,
        uint256[] calldata middlewareTimesIndexes,
        bool[] calldata receiveAsTokens
    ) external
        onlyWhenNotPaused(PAUSED_WITHDRAWALS)
        // check that the address that the staker *was delegated to* – at the time that they queued the withdrawal – is not frozen
        nonReentrant
    {
        for(uint256 i = 0; i < queuedWithdrawals.length; i++) {
            _completeQueuedWithdrawal(queuedWithdrawals[i], tokens[i], middlewareTimesIndexes[i], receiveAsTokens[i]);
        }
    }

6. However, Bob did not make it to qualify calling completeQueuedWithdrawal() before shares pending withdrawal become slashable.

File: StrategyManager.sol#L755-L758

        require(
            slasher.canWithdraw(queuedWithdrawal.delegatedAddress, queuedWithdrawal.withdrawalStartBlock, middlewareTimesIndex),
            "StrategyManager.completeQueuedWithdrawal: shares pending withdrawal are still slashable"
        );

7. The contract owner begins slashing everyone by targeting the bigger chunks of shares entailed first. And, by the time it reaches Bob's queuedWithdrawals, totalShares amount in StrategyBase.sol is nearing MIN_NONZERO_TOTAL_SHARES that could possibly make slashQueuedWithdrawal() fail in the last loop iteration.

File: StrategyBase.sol#L137-L140

        uint256 updatedTotalShares = priorTotalShares - amountShares;
        // check to avoid edge case where share rate can be massively inflated as a 'griefing' sort of attack
        require(updatedTotalShares >= MIN_NONZERO_TOTAL_SHARES || updatedTotalShares == 0,
            "StrategyBase.withdraw: updated totalShares amount would be nonzero but below MIN_NONZERO_TOTAL_SHARES");

Tools Used

Manual

Recommended Mitigation Steps

Consider adding a require check in queueWithdrawal() requiring shares[i] >= 1e9.

Assessed type

Decimal

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-sponsor]

Sidu28 marked the issue as disagree with severity

[Sidu28]

This is a theoretical griefing attack that is practically infeasible. The cost cannot be less to the would-be-attacker than it is to the owner of the StrategyManager. We believe it is Informational Severity

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid

[GalloDaSballo]

Agree with the sponsor, while marginal deposits are possible, the cost for slashing is lower (triggers some gas refunds), than the cost of griefing (zero to non-zero STOREs)