popular - Creating new withdrawal requests in conjunction with `settleEpochFromEigenLayer` will render system unusable

[sherlock-admin3]

popular

high

Creating new withdrawal requests in conjunction with settleEpochFromEigenLayer will render system unusable

Summary

This issue pertains to the flow where a user requests to withdraw more funds than are currently present in the depositPool and the system must withdraw from Eigenlayer.

Users are able to create new withdrawal requests for the current epoch while the Eigenlayer withdrawal request is pending, as well as after the epoch has been marked settled in settleEpochFromEigenLayer(). This is due to the fact that settleEpochFromEigenLayer() does not increment the current epoch, as well as that there is no way to fulfill withdrawal requests submitted after the 7 day waiting period has been initiated. Submitting a withdrawal request will result in an inability to progress epochs and a locking of the system.

Vulnerability Detail

Consider the system in the following state:

• We are in epoch 0
• A user submitted a withdrawal request for an amount greater than what is currently in depositPool
• rebalance() --> withdrawalQueue_.queueCurrentEpochSettlement() has been called
• The system made a request to Eigenlayer for the necessary amount and the withdrawal request is ready to be claimed
• The next step is to call RioLRTWithdrawalQueue:settleEpochFromEigenLayer() link

The function settleEpochFromEigenLayer() performs several important tasks - completing pending withdrawals from Eigenlayer, accounting for the amounts received, burning the appropriate amount of LRTs, and marking the epoch as settled. It does NOT increment the epoch counter for the asset - the only way to do that is in settleCurrentEpoch(), which is only called in rebalance() when there is enough present in the depositPool to cover withdrawals.

After calling settleEpochFromEigenLayer(), the system is in a state where the current epoch has been marked as settled. However, while waiting for the 7 day Eigenlayer delay it is possible that more users sent withdrawal requests. These withdrawal requests would be queued for epoch 0 (and increment sharesOwed for epoch 0) , but were not considered when performing the withdrawal from Eigenlayer. There is no way to process these requests, as the epoch has already been settled + we can only call queueCurrentEpochSettlement once per epoch due to the if (epochWithdrawals.aggregateRoot != bytes32(0)) revert WITHDRAWALS_ALREADY_QUEUED_FOR_EPOCH(); check

Notably, users that requested withdrawals have already sent the LRT amount to be burned and are unable to reclaim their funds.

Also note that there is no access control on settleEpochFromEigenLayer(), so as long as the provided withdrawal parameters are correct anybody can call the function.

Impact

Critical - system no longer operates, loss of users funds

Code Snippet

The following test can be dropped into RioLRTWithdrawalQueue.t.sol

 function test_lockAsset() public {
        uint8 operatorId = addOperatorDelegator(reETH.operatorRegistry, address(reETH.rewardDistributor));
        address operatorDelegator = reETH.operatorRegistry.getOperatorDetails(operatorId).delegator;

        // Deposit ETH, rebalance, and verify the validator withdrawal credentials.
        uint256 depositAmount = (ETH_DEPOSIT_SIZE - address(reETH.depositPool).balance);
        uint256 withdrawalAmount = 10 ether;
        assertGt(depositAmount, withdrawalAmount * 2); // We will be withdrawing twice
        reETH.coordinator.depositETH{value: depositAmount}();
        vm.prank(EOA, EOA);
        reETH.coordinator.rebalance(ETH_ADDRESS);
        uint40[] memory validatorIndices = verifyCredentialsForValidators(reETH.operatorRegistry, 1, 1);

        // Request a withdrawal and rebalance to kick off the Eigenlayer withdrawal process
        reETH.coordinator.requestWithdrawal(ETH_ADDRESS, withdrawalAmount);
        skip(reETH.coordinator.rebalanceDelay());
        vm.prank(EOA, EOA);
        reETH.coordinator.rebalance(ETH_ADDRESS);

        // Ensure no reETH has been burned yet and process withdrawals.
        assertEq(reETH.token.totalSupply(), ETH_DEPOSIT_SIZE);
        verifyAndProcessWithdrawalsForValidatorIndexes(operatorDelegator, validatorIndices);

        // Settle the withdrawal epoch. This marks the epoch as settled and
        // makes the requested withdrawal amount available to be claimed.
        uint256 withdrawalEpoch = reETH.withdrawalQueue.getCurrentEpoch(ETH_ADDRESS);
        IDelegationManager.Withdrawal[] memory withdrawals = new IDelegationManager.Withdrawal[](1);
        withdrawals[0] = IDelegationManager.Withdrawal({
            staker: operatorDelegator,
            delegatedTo: address(1),
            withdrawer: address(reETH.withdrawalQueue),
            nonce: 0,
            startBlock: 1,
            strategies: BEACON_CHAIN_STRATEGY.toArray(),
            shares: withdrawalAmount.toArray()
        });
        reETH.withdrawalQueue.settleEpochFromEigenLayer(ETH_ADDRESS, withdrawalEpoch, withdrawals, new uint256[](1));

        IRioLRTWithdrawalQueue.EpochWithdrawalSummary memory epochSummary =
            reETH.withdrawalQueue.getEpochWithdrawalSummary(ETH_ADDRESS, withdrawalEpoch);
        // Epoch is settled
        assertTrue(epochSummary.settled);

        // However, the epoch has not been incremented - we're still in epoch 0 even after settlement
        assertEq(reETH.withdrawalQueue.getCurrentEpoch(ETH_ADDRESS), 0);

        // We can still create new withdrawal requests for this epoch and increase sharesOwed
        uint256 sharesOwedBefore = epochSummary.sharesOwed;
        reETH.coordinator.requestWithdrawal(ETH_ADDRESS, withdrawalAmount);
        epochSummary = reETH.withdrawalQueue.getEpochWithdrawalSummary(ETH_ADDRESS, withdrawalEpoch);
        // Shares owed has increased
        assertGt(epochSummary.sharesOwed, sharesOwedBefore);

        // We've received one withdrawalAmount worth of assets from Eigenlayer
        assertEq(epochSummary.assetsReceived, withdrawalAmount);
        assertEq(epochSummary.shareValueOfAssetsReceived, withdrawalAmount);

        // Claim what was received from Eigenlayer (== one withdrawalAmount)
        uint256 balanceBefore = address(this).balance;
        uint256 amountOut = reETH.withdrawalQueue.claimWithdrawalsForEpoch(
            IRioLRTWithdrawalQueue.ClaimRequest({asset: ETH_ADDRESS, epoch: withdrawalEpoch})
        );
        IRioLRTWithdrawalQueue.UserWithdrawalSummary memory userSummary =
            reETH.withdrawalQueue.getUserWithdrawalSummary(ETH_ADDRESS, withdrawalEpoch, address(this));

        // The user has been marked as Claimed for this epoch, even though only one withdrawalAmount worth was claimed
        assertTrue(userSummary.claimed);
        assertEq(amountOut, withdrawalAmount);
        assertEq(address(this).balance - balanceBefore, withdrawalAmount);
        // sharesOwed for this epoch is 2 withdrawals worth (we're sitll missing one)
        assertEq(epochSummary.sharesOwed, withdrawalAmount * 2);

        // We can't rebalance because withdrawals have already been queued for this epoch
        // If we can't rebalance, we can't ever get to settleCurrentEpoch() to progress to the next epoch
        skip(reETH.coordinator.rebalanceDelay());
        vm.prank(EOA, EOA);
        vm.expectRevert(0x9a641da5); // WITHDRAWALS_ALREADY_QUEUED_FOR_EPOCH
        reETH.coordinator.rebalance(ETH_ADDRESS);

        // Current epoch is still 0
        assertEq(reETH.withdrawalQueue.getCurrentEpoch(ETH_ADDRESS), 0);

        // Reverts in pre-checks because the epoch has been marked as settled
        vm.expectRevert(0xad29946a); // EPOCH_ALREADY_SETTLED
        reETH.withdrawalQueue.settleEpochFromEigenLayer(ETH_ADDRESS, withdrawalEpoch, withdrawals, new uint256[](1));
    }

Tool used

Manual Review

Recommendation

Consider incrementing the current epoch as soon as the withdrawal process has been initiated, such that user withdrawal requests sent after an epoch has been queued for settlement will be considered a part of the next epoch

[solimander]

Valid bug - currentEpochsByAsset[asset] += 1; should be called in queueCurrentEpochSettlement.

[sherlock-admin4]

The protocol team fixed this issue in the following PRs/commits: https://github.com/rio-org/rio-sherlock-audit/pull/1

[10xhash]

The protocol team fixed this issue in PR/commit rio-org/rio-sherlock-audit#1.

Fixed The epoch is now incremented inside queueCurrentEpochSettlement function

[sherlock-admin4]

The Lead Senior Watson signed off on the fix.