Users might receive less funds from the queued withdrawals

Summary

Users will receive less funds when claiming withdrawals from an epoch that was queued into EigenLayer through queueCurrentEpochSettlement if other users request withdrawals in that same epoch.

Vulnerability Detail

For a user to withdraw funds from the protocol, they must first request a withdrawal using the requestWithdrawal function, which will queue the withdrawal in the current epoch by calling withdrawalQueue().queueWithdrawal.

All the queued withdrawals will be processed when the rebalance function is called. There are two ways to do so (after invoking _processUserWithdrawalsForCurrentEpoch under the hood):

The deposit pool has enough funds to cover all the withdrawals, so RioLRTWithdrawalQueue.settleCurrentEpoch is called.
Or the deposit pool can only cover a part of the withdrawal shares, so we must request a withdrawal from EigenLayer by calling RioLRTWithdrawalQueue.queueCurrentEpochSettlement. The withdrawal can later be completed (after EigenLayer 7 days delay) by calling RioLRTWithdrawalQueue.settleEpochFromEigenLayer to receive the funds from EigenLayer.

The _processUserWithdrawalsForCurrentEpoch function will handle all the withdrawal logic in the rebalancing call:

function _processUserWithdrawalsForCurrentEpoch(address asset, uint256 sharesOwed) internal {
    IRioLRTWithdrawalQueue withdrawalQueue_ = withdrawalQueue();
    (uint256 assetsSent, uint256 sharesSent) = depositPool().transferMaxAssetsForShares(
        asset,
        sharesOwed,
        address(withdrawalQueue_)
    );
    uint256 sharesRemaining = sharesOwed - sharesSent;

    // Exit early if all pending withdrawals were paid from the deposit pool.
    if (sharesRemaining == 0) {
        withdrawalQueue_.settleCurrentEpoch(asset, assetsSent, sharesSent);
        return;
    }

    address strategy = assetRegistry().getAssetStrategy(asset);
    bytes32 aggregateRoot = OperatorOperations.queueWithdrawalFromOperatorsForUserSettlement(
        operatorRegistry(),
        strategy,
        sharesRemaining
    );

    withdrawalQueue_.queueCurrentEpochSettlement(asset, assetsSent, sharesSent, aggregateRoot);
}

In this rebalance process, there is a scenario in which users may receive less funds than expected for their queued withdrawal shares. Let's illustrate that scenario:

We are currently in epoch 8 (i.e., getCurrentEpoch() = 8).
When rebalance gets called, the deposit Pool hasn't enough funds to cover the full withdrawal, so the function must queue withdrawal from operators, and both RioLRTOperatorDelegator.queueWithdrawalForUserSettlement and RioLRTWithdrawalQueue.queueCurrentEpochSettlement will be invoked.
The queueCurrentEpochSettlement will store the asset received from the deposit pool and their corresponding shares value into epochWithdrawals.assetsReceived and epochWithdrawals.shareValueOfAssetsReceived respectively and will decrement the amount to burn epochWithdrawals.amountToBurnAtSettlement.
After the rebalance, a user requests another withdrawal, and because we are still in the same epoch (epoch was not settled), this withdrawal will get recorded with the currently queued epoch, and the user shares will be added to epochWithdrawals.sharesOwed.
After the rebalanceDelay has passed, the rebalance function gets called again, and suppose this time also the deposit Pool hasn't enough funds to cover the full withdrawal, the rebalance call will revert because the epoch was already queued for withdrawal from EigenLayer in the last rebalance, and thus queueCurrentEpochSettlement function will revert because of the following check:

if (epochWithdrawals.aggregateRoot != bytes32(0)) revert WITHDRAWALS_ALREADY_QUEUED_FOR_EPOCH();

So the request user shares were added to epoch owed shares, but the contract can't request withdrawal for them.
The settleCurrentEpoch function will override the previously set epochWithdrawals.assetsReceived and epochWithdrawals.shareValueOfAssetsReceived and will burn the remaining epochWithdrawals.amountToBurnAtSettlement and will settle the epoch by setting epochWithdrawals.settled = true.
When the epoch is finally settled from EigenLayer by calling settleEpochFromEigenLayer function, all shares will be diluted as that added user share didn't get their corresponding asset withdrawn from EigenLayer.
The result is that all users will receive less funds than expected because they must share the received asset funds with that user who was added after the epoch was already queued, this is illustrated by the amountout which will be lower as the sharesOwed will be more than what was expected:

amountOut = userSummary.sharesOwed.mulDiv(epochWithdrawals.assetsReceived, epochWithdrawals.sharesOwed);

Note that the received asset will be further diluted as more users request a withdrawal after the epoch was already queued.

Impact

Users will receive less funds when claiming withdrawals from an epoch that was queued into EigenLayer through queueCurrentEpochSettlement if other users request withdrawals in that same epoch after it was queued.

Code Snippet

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/main/rio-sherlock-audit/contracts/restaking/RioLRTCoordinator.sol#L245-L267

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/main/rio-sherlock-audit/contracts/restaking/RioLRTWithdrawalQueue.sol#L130-L145

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/main/rio-sherlock-audit/contracts/restaking/RioLRTWithdrawalQueue.sol#L177-L209

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/main/rio-sherlock-audit/contracts/restaking/RioLRTWithdrawalQueue.sol#L104

Tool used

Manual Review

Recommendation

To address this issue, the simplest method is to increment the epoch currentEpochsByAsset when RioLRTWithdrawalQueue.queueCurrentEpochSettlement is called. Requests for withdrawals made after queuing will be recorded in the next epoch.

Duplicate of #4

sherlock-audit / 2024-02-rio-network-core-protocol-judging

Aymen0909 - Users might receive less funds from the queued withdrawals #364