kennedy1030 - In RioLRTCoordinator.requestWithdrawal, availableShares doesn't consider previous queued withdrawals and users can request withrawal more than available shares.

[sherlock-admin2]

kennedy1030

high

In RioLRTCoordinator.requestWithdrawal, availableShares doesn't consider previous queued withdrawals and users can request withrawal more than available shares.

kennedy1030

high

Summary

If there is not sufficient shares for users withdrawal request, it should be reverted because of the insufficient shares for withdrawal
availableShares contains all assets in the deposit pool and eigen layer, it also contains the queued withdrawals but only reduce the current epoch shares owed, hence the user can request withdrawal without limit.

Vulnerability Detail

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

    function requestWithdrawal(address asset, uint256 amountIn) external checkWithdrawal(asset, amountIn) returns (uint256 sharesOwed) {
        // Determine the amount of shares owed to the withdrawer using the current exchange rate.
        sharesOwed = convertToSharesFromRestakingTokens(asset, amountIn);

        // If requesting ETH, reduce the precision of the shares owed to the nearest Gwei,
        // which is the smallest unit of account supported by EigenLayer.
        if (asset == ETH_ADDRESS) sharesOwed = sharesOwed.reducePrecisionToGwei();

        // Pull restaking tokens from the sender to the withdrawal queue.
        token.safeTransferFrom(msg.sender, address(withdrawalQueue()), amountIn);

        // Ensure there are enough shares to cover the withdrawal request, and queue the withdrawal.
        uint256 availableShares = assetRegistry().convertToSharesFromAsset(asset, assetRegistry().getTotalBalanceForAsset(asset));
112     if (sharesOwed > availableShares - withdrawalQueue().getSharesOwedInCurrentEpoch(asset)) {
            revert INSUFFICIENT_SHARES_FOR_WITHDRAWAL();
        }
        withdrawalQueue().queueWithdrawal(msg.sender, asset, sharesOwed, amountIn);
    }

At [L112] from availableShares previous queued withdrawals should be removed but the only current epoch shares owed is removed. Hence the check can bypass and the users request withdrawal without limit.

Impact

Users can request withdrawal and queue it with no check or limit and this is against the check at [L112] and this may break the protocol.

Code Snippet

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

Tool used

Manual Review

Recommendation

Previous queued withdrawal should be removed also at [L112]

For this, a state variable totalSharesOwed and a view function getTotalSharesOwed() has to be added into RioLRTWithdrawalQueue.sol.

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

    function requestWithdrawal(address asset, uint256 amountIn) external checkWithdrawal(asset, amountIn) returns (uint256 sharesOwed) {
        // Determine the amount of shares owed to the withdrawer using the current exchange rate.
        sharesOwed = convertToSharesFromRestakingTokens(asset, amountIn);

        // If requesting ETH, reduce the precision of the shares owed to the nearest Gwei,
        // which is the smallest unit of account supported by EigenLayer.
        if (asset == ETH_ADDRESS) sharesOwed = sharesOwed.reducePrecisionToGwei();

        // Pull restaking tokens from the sender to the withdrawal queue.
        token.safeTransferFrom(msg.sender, address(withdrawalQueue()), amountIn);

        // Ensure there are enough shares to cover the withdrawal request, and queue the withdrawal.
        uint256 availableShares = assetRegistry().convertToSharesFromAsset(asset, assetRegistry().getTotalBalanceForAsset(asset));

-       if (sharesOwed > availableShares - withdrawalQueue().getSharesOwedInCurrentEpoch(asset)) {

+       if (sharesOwed > availableShares - withdrawalQueue().getTotalSharesOwed(asset)) {
            revert INSUFFICIENT_SHARES_FOR_WITHDRAWAL();
        }
        withdrawalQueue().queueWithdrawal(msg.sender, asset, sharesOwed, amountIn);
    }

Duplicate of #361