Queued withdrawals are not treated correctly when a slash occurs, leading to loss of user funds

[code423n4]

Lines of code

https://github.com/code-423n4/2023-04-eigenlayer/blob/5e4872358cd2bda1936c29f460ece2308af4def6/src/contracts/core/StrategyManager.sol#L523
https://github.com/code-423n4/2023-04-eigenlayer/blob/5e4872358cd2bda1936c29f460ece2308af4def6/src/contracts/core/StrategyManager.sol#L567-L573

Vulnerability details

Description

According to the documentation, the EigenLayer protocol allows for partial slashes. If an operator is slashed, all stakers who have delegated to the operator will be subject to the same percentage slash. This means that if a delegate is slashed by $X$%, all stakers who trusted the operator will also be slashed by $X$%. However, there is an issue with queued withdrawals during partial slashes. Currently, the implementation reduces the queued withdrawal to 100%, resulting in a loss of funds for the user.

Impact

Despite intending to implement a partial slash, the current issue has resulted in the complete cancellation of queued withdrawals and the loss of funds for the user, therefore we evaluate the impact of this issue as HIGH.

Proof of Concept

The scenario below explains the issue:

Context:

• Alice is a staker who has delegated their shares
• Bob is the operator
• Alice and Bob have staked 100 shares each

Scenario:

1. Alice initiates a withdrawal of 100 shares using the queueWithdrawal function, which sets her shares in the stakerStrategyShares mapping to 0.
2. A day later, Bob is slashed by governance for 30 shares due to misbehavior, using the StategyManager::slashShares function.
3. To slash Alice's shares that do not correspond to the queued withdrawal, calling the StategyManager::slashShares function does not make sense, as the stakerStrategyShares element in the mapping was previously set to 0. Doing so would cause the StategyManager::slashShares transaction to revert.
4. Instead, the StategyManager owner calls the StategyManager::slashQueuedWithdrawals function to slash Alice's queued withdrawal. Alice should only be slashed for 30% of her shares in the strategy. BUT, the amount to withdraw from the strategy corresponds to all the shares in the queuedWithdrawal.share[i] element, which results in Alice being fully slashed.
5. As a result, Alice loses all her shares, instead of just 30.

Recommended Mitigation steps

The issue stems from the failure to account for the percentage of shares of the operator that are cut during a slash.
We propose two options to mitigate this issue:

Option A

Disallow partial slashes and only allow full slashes.

Option B

When an operator is slashed, take into account the percentage that is slashed and slash the queued withdrawals accordingly.
This can be done by including the amount of shares to slash in the function StrategyManager::slashQueuedWithdrawal.

    function slashQueuedWithdrawal(
        address recipient,
        QueuedWithdrawal calldata queuedWithdrawal,
        IERC20[] calldata tokens,
        uint256[] calldata indicesToSkip
+       uint256[] calldata sharesToSlash,
    )
        external
        onlyOwner
        onlyFrozen(queuedWithdrawal.delegatedAddress)
        nonReentrant
    {
        require(tokens.length == queuedWithdrawal.strategies.length, "StrategyManager.slashQueuedWithdrawal: input length mismatch");

+       require(sharesToSlash.length == queuedWithdrawal.strategies.length, "StrategyManager.slashQueuedWithdrawal: input length mismatch");

        // find the withdrawalRoot
        bytes32 withdrawalRoot = calculateWithdrawalRoot(queuedWithdrawal);

        // verify that the queued withdrawal is pending
        require(
            withdrawalRootPending[withdrawalRoot],
            "StrategyManager.slashQueuedWithdrawal: withdrawal is not pending"
        );

        // reset the storage slot in mapping of queued withdrawals
        withdrawalRootPending[withdrawalRoot] = false;

        // keeps track of the index in the `indicesToSkip` array
        uint256 indicesToSkipIndex = 0;

        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 (queuedWithdrawal.strategies[i] == beaconChainETHStrategy){
                     //withdraw the beaconChainETH to the recipient
-                   _withdrawBeaconChainETH(queuedWithdrawal.depositor, recipient, queuedWithdrawal.shares[i]);
+                   _withdrawBeaconChainETH(queuedWithdrawal.depositor, recipient,sharesToSlash[i]);
                } else {
                    // tell the strategy to send the appropriate amount of funds to the recipient
-                   queuedWithdrawal.strategies[i].withdraw(recipient, tokens[i], queuedWithdrawal.shares[i]);
+                   queuedWithdrawal.strategies[i].withdraw(recipient, tokens[i], sharesToSlash[i]);
                }
                unchecked {
                    ++i;
                }
            }
        }

Assessed type

Other

[0xSorryNotSorry]

Very well demonstration including the mitigation. Marking as HQ.

[c4-pre-sort]

0xSorryNotSorry marked the issue as high quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-sponsor]

Sidu28 marked the issue as disagree with severity

[Sidu28]

We believe this is informational severity, and the suggested mitigation would result in unrecoverable/burnt funds. Agreed the function in question currently only fully slashes QueuedWithdrawals, by design. We could consider adding more complex functionality to this function in the future.

[GalloDaSballo]

@Sidu28 Is it realistic for a partial withdrawal to be necessary at this time?
e.g.
-> 3 depositors delegate to an operator
-> Operator is slashed
-> Does the 3 depositor need to be slashed for 1/3 each, how would the system handle this scenario without partial slashing?

[ChaoticWalrus]

@Sidu28 Is it realistic for a partial withdrawal to be necessary at this time? e.g. -> 3 depositors delegate to an operator -> Operator is slashed -> Does the 3 depositor need to be slashed for 1/3 each, how would the system handle this scenario without partial slashing?

Based on our intended usage, this is a wholly unnecessary feature at this time.
We do indeed already have support for partial slashing of funds that are not already part of a queued withdrawal, so I understand noting the discrepancy.
However, even in the case where support for partial slashing of queued withdrawals in particular is needed, it would still be possible to achieve effectively the same result by fully slashing a queued withdrawal followed immediately by 'returning' a portion of the slashed funds.

[GalloDaSballo]

Judging the finding boils down to whether it would be reasonable for partial slashing to occur in the currently coded way
-> Missing feature as bug or not

And whether the Admin Privileges were disclosed beforehand
It seems to me that the catch-all disclosure in the Known Findings, as well as the Sponsor Provided Disclosure in the in-scope docs is sufficient to argue that the slashing is enacted by a trusted actor, as such the flow suggested by the Sponsor is reasonable although not enforced on chain

See Docs here:
https://github.com/code-423n4/2023-04-eigenlayer/blob/398cc428541b91948f717482ec973583c9e76232/docs/AVS-Guide.md#L29-L33

3. *Services Slash Only Objectively Attributable Behavior*: <br/>
    EigenLayer is built to support slashing as a result of an on-chain-checkable, objectively attributable action. An AVS SHOULD slash in EigenLayer only for such provable and attributable behavior. It is expected that operators will be very hesitant to opt-in to services that slash for other types of behavior, and other services may even choose to exclude operators who have opted-into serving one or more AVSs with such “subjective slashing conditions”, as these slashing conditions present a significant challenge for risk modeling, and may be perceived as more dangerous in general. Some examples of on-chain-checkable, objectively attributable behavior: 
    - double-signing a block in Ethereum, but NOT inactivity leak; 
    - proofs-of-custody in EigenDA, but NOT a node ceasing to serve data; 
    - a node in a light-node-bridge AVS signing an invalid block from another chain.

In terms of the lack of functionality
Per the comment by the sponsor this can still be enacted, although it is not enforced in the code

I will invite the Wardens to send some arguments around why this would break the functionality given the Sponsor provided alternative, this is fairly similar to how GogoPool was judge in that if a FSM is broken it may be worth raising to Med

That said, currently, I believe this finding should be downgraded to QA - Low Severity

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

@Cyfrin lmk if you have a counter argument to the above

[hansfriese]

While we acknowledge that slashing is performed by a trusted actor, it is essential to highlight that no documentation was provided during the auditing process regarding the handling of partial withdrawals.

As auditors, we had to consider the possibility of both partial withdrawals and partial slashing, particularly when withdrawals are already queued.

The sponsor has proposed fully slashing a queued withdrawal and promptly returning a portion of the slashed funds to achieve the intended objective. However, we have reservations about the feasibility of implementing and managing refunds on a large scale.

Given that a substantial part of the process would occur off-chain, there exists a risk of genuine users losing funds.
As auditors, we believe it is imperative to address this risk and identify an appropriate solution that ensures the security of user funds while effectively accommodating partial withdrawals.

We would like to note that we submitted this as HIGH because it can cause a loss of user funds in a reasonable scenario.

[GalloDaSballo]

@hansfriese could you link me to a specific part of documentation that directly mentions partial slashing and how they are supposed to work in relation to queued withdrawals?

[GalloDaSballo]

While there are multiple ways to look at the finding, a few things should be considered:

• 1. No clear specification on Partial Slashing
     To the best of my ability, I was not able to find documentation around the logics of partial slashing
     Anecdotally the Sponsor claimed that they are not supported at this time, which would not be sufficient to mark the finding OOS, however it makes it harder to judge due to additional hypothetical
• 2. The current state of the system (only full slashing)
     In the in-scope code only full slashing is present
• 3. The possibility of performing such slashes via multiple sidesteps
     Per the comment by the Sponsor there are ways to perform partial slashes that involve doing a full slash and re-funding funds
• 4. Further considerations around partial slashing in the context of full and partial withdrawals
     If a full withdrawal is performed, and delegation was removed, then the slashing simply cannot be performed. These withdrawals are not subject to slashing since the stake wasn't being delegate already

If a partial withdrawal is performed, slashing all of it may be appropriate, additionally, instead of slashing the partial withdrawal, the other shares (not in the queued withdrawal) could be slashed (and they could be slashed partially via slashShares)

This would leave us only with a scenario in which a partial slash had to be performed on a withdrawal that was partial and the slashing would have to be partial.
At this time, I believe that all mathematical combinations would be covered by the following logic, enforced by the Admin:
Total Slashable = Queued + Delegated
Slash Amount <= Total Slashable
If Queued < Slash Amount , then slash all of Queued and partial of Delegated
If Queued > Slash Amount , then slash Delegated

Meaning that while there is a reason to discuss the current design decisions, given the already known reliance on a Trusted Owner, the finding doesn't point to a specific risk

For the reasons above, am going to downgrade the finding to QA

[c4-judge]

GalloDaSballo marked the issue as grade-a

[hansfriese]

We are grateful for your comprehensive response and the clarification provided regarding the final decision.