giraffe - queueCurrentEpochSettlement() does not advance current epoch leading to broken withdrawal accounting

[sherlock-admin2]

giraffe

high

queueCurrentEpochSettlement() does not advance current epoch leading to broken withdrawal accounting

Summary

In RioLRTWithdrawalQueue.sol, queueCurrentEpochSettlement() fails to advance the current epoch by + 1, resulting in permanent DOS and broken withdrawal accounting.

Vulnerability Detail

At the end of each rebalance, an epoch can be settled by one of two paths in RioLRTWithdrawQueue.sol: 1) settleCurrentEpoch() if assets in the deposit pool can fully offset all withdrawals requests, or 2) queueCurrentEpochSettlement() to queue a withdrawal from Eigenlayer and then settleEpochFromEigenLayer() after the waiting period.

    function _processUserWithdrawalsForCurrentEpoch(address asset, uint256 sharesOwed) internal {
        ...

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

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

In settleCurrentEpoch(), we observe currentEpochsByAsset[asset] += 1; but in queueCurrentEpochSettlement() , advancement of the epoch is missing.

Where should epoch be advanced then? An epoch is only considered settled after settleEpochFromEigenLayer(), however if we wait till this is called the epoch will be stuck for days due to the withdrawal delay. Therefore it would be better to advance the epoch in queueCurrentEpochSettlement() which would align with the idea that epochs are advanced after each rebalance is completed.

POC

Add this test case below to RioLRTWithdrawalQueue.t.sol. Observe that 1) epochs do not advance when settling via Eigenlayer, and 2) subsequent withdrawals revert due to underflow.

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

        // Deposit ETH, rebalance, verify the validator withdrawal credentials, and deposit again.
        uint256 depositAmount = ETH_DEPOSIT_SIZE - address(reETH.depositPool).balance;
        reETH.coordinator.depositETH{value: depositAmount}();

        vm.prank(EOA, EOA);
        reETH.coordinator.rebalance(ETH_ADDRESS);

        uint40[] memory validatorIndices = verifyCredentialsForValidators(reETH.operatorRegistry, 1, 1);
        reETH.coordinator.depositETH{value: ETH_DEPOSIT_SIZE}();

        // Request a withdrawal and rebalance.
        uint256 withdrawalAmount = ETH_DEPOSIT_SIZE + 1 ether;
        reETH.coordinator.requestWithdrawal(ETH_ADDRESS, withdrawalAmount);
        skip(reETH.coordinator.rebalanceDelay());

        uint256 epoch0 = reETH.withdrawalQueue.getCurrentEpoch(ETH_ADDRESS);

        vm.prank(EOA, EOA);
        reETH.coordinator.rebalance(ETH_ADDRESS);
        assertEq(epoch0, reETH.withdrawalQueue.getCurrentEpoch(ETH_ADDRESS)); //@audit epoch did not advance after rebalance

        // Validate reETH total supply and process withdrawals.
        assertApproxEqAbs(reETH.token.totalSupply(), ETH_DEPOSIT_SIZE, 100);
        verifyAndProcessWithdrawalsForValidatorIndexes(operatorDelegator, validatorIndices);

        // Settle the withdrawal epoch.
        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: uint256(1 ether).toArray()
        });
        reETH.withdrawalQueue.settleEpochFromEigenLayer(ETH_ADDRESS, epoch0, withdrawals, new uint256[](1));

        assertEq(epoch0, reETH.withdrawalQueue.getCurrentEpoch(ETH_ADDRESS)); //@audit epoch still did not advance

        // Assert epoch summary details.
        IRioLRTWithdrawalQueue.EpochWithdrawalSummary memory epochSummary =
            reETH.withdrawalQueue.getEpochWithdrawalSummary(ETH_ADDRESS, epoch0);
        assertTrue(epochSummary.settled);
        assertEq(epochSummary.assetsReceived, withdrawalAmount);
        assertEq(epochSummary.shareValueOfAssetsReceived, withdrawalAmount);

        // Trying to request another withdrawal will fail with underflow due to sharesOwedInCurrentEpoch still pointing to the previous epoch
        uint256 withdrawalAmount2 = 1 ether;
        vm.expectRevert();
        reETH.coordinator.requestWithdrawal(ETH_ADDRESS, withdrawalAmount2);
    }

Impact

Permanent DOS of subsequent withdrawals and broken accounting for withdrawals leading to stuck or loss of user funds.

Assuming the current epoch is not advanced, then that epoch's sharesOwed will keep increasing even after rebalance has processed user withdrawals for the current epoch (i.e. new withdrawals requests will be accounted to the previous epoch) which would cause reverts in subsequent withdrawals.

After settleEpochFromEigenLayer() is called for the first time, that epoch's aggregateRoot will be set to a non-zero value. When rebalance is called the next time, queueCurrentEpochSettlement() will also revert due to:

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

If withdrawals and rebalance cannot complete then user funds could be permanently stuck. The only way to move forward would be to allow epoch to increase via settleCurrentEpoch() but then the accounting could be already erroneous and broken.

Code Snippet

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

Tool used

Manual Review

Recommendation

Add currentEpochsByAsset[asset] += 1; to queueCurrentEpochSettlement().
Also, considering adding to queueWithdrawal() a check that current epoch is not settled yet.

Duplicate of #4