Aymen0909 - `queueOperatorStrategyExit` doesn't decrease the operator shares allocation

[sherlock-admin2]

Aymen0909

medium

queueOperatorStrategyExit doesn't decrease the operator shares allocation

Summary

The queueOperatorStrategyExit function doesn't decrease the operator shares allocation when removing them, potentially causing issues with future allocations and heap organization when the operator is included again.

Vulnerability Detail

The queueOperatorStrategyExit function is utilized to queue a complete exit from the specified strategy for a given operator. It's called when changing the operator cap to 0 using setOperatorStrategyCap.

In the setOperatorStrategyCap function, when the new cap is set to 0, the operator is removed from the heap, and if the operator had an allocation, queueOperatorStrategyExit function is invoked:

if (currentShareDetails.cap > 0 && newShareCap.cap == 0) {
    // If the operator has allocations, queue them for exit.
    if (currentShareDetails.allocation > 0) {
        operatorDetails.queueOperatorStrategyExit(operatorId, newShareCap.strategy);
    }
    // Remove the operator from the utilization heap.
    utilizationHeap.removeByID(operatorId);
}

The queueOperatorStrategyExit function is responsible for calculating the shares to exit and queuing the withdrawal:

uint256 sharesToExit;
    if (strategy == BEACON_CHAIN_STRATEGY) {
        // Queues an exit for verified validators only. Unverified validators must be exited once verified,
        // and ETH must be scraped into the deposit pool. Exits are rounded to the nearest Gwei. It is not
        // possible to exit ETH with precision less than 1 Gwei. We do not populate `sharesToExit` if the
        // Eigen Pod shares are not greater than 0.
        int256 eigenPodShares = delegator.getEigenPodShares();
        if (eigenPodShares > 0) {
            sharesToExit = uint256(eigenPodShares).reducePrecisionToGwei();
        }
    } else {
        sharesToExit = operator.shareDetails[strategy].allocation;
        //@audit operatorDetails[operatorId].shareDetails[strategy].allocation was not decreased
    }
    if (sharesToExit == 0) revert IRioLRTOperatorRegistry.CANNOT_EXIT_ZERO_SHARES();

    // Queues a withdrawal to the deposit pool.
    bytes32 withdrawalRoot = delegator.queueWithdrawalForOperatorExit(strategy, sharesToExit);

As observed, the function doesn't decrease the operator allocation at all but queues the shares for withdrawal directly.Thus, even if the operator was removed and all their allocation was withdrawn, in the protocol's internal accounting, they still have an allocation.

This can lead to problems later on if that operator is added again (if their shares cap is increased again > 0), as major protocol functions like allocateStrategyShares or deallocateStrategyShares rely on the value of operatorDetails[operatorId].shareDetails[strategy].allocation. However, this value is not correct as it still indicates the old operator allocation, potentially causing incorrect behavior of those protocol functions (unable to allocate or deallocate new shares) and a wrong ordering in the heap structure.

Impact

The queueOperatorStrategyExit function doesn't decrease the operator shares allocation when removing them, potentially causing issues with future allocations and heap organization when the operator is included again.

Code Snippet

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/main/rio-sherlock-audit/contracts/utils/OperatorRegistryV1Admin.sol#L144-L165

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/main/rio-sherlock-audit/contracts/utils/OperatorRegistryV1Admin.sol#L248-L255

Tool used

Manual Review

Recommendation

The queueOperatorStrategyExit function must reset the removed operator allocation:

function queueOperatorStrategyExit(IRioLRTOperatorRegistry.OperatorDetails storage operator, uint8 operatorId, address strategy) internal {
    IRioLRTOperatorDelegator delegator = IRioLRTOperatorDelegator(operator.delegator);

    uint256 sharesToExit;
    if (strategy == BEACON_CHAIN_STRATEGY) {
        // Queues an exit for verified validators only. Unverified validators must be exited once verified,
        // and ETH must be scraped into the deposit pool. Exits are rounded to the nearest Gwei. It is not
        // possible to exit ETH with precision less than 1 Gwei. We do not populate `sharesToExit` if the
        // Eigen Pod shares are not greater than 0.
        int256 eigenPodShares = delegator.getEigenPodShares();
        if (eigenPodShares > 0) {
            sharesToExit = uint256(eigenPodShares).reducePrecisionToGwei();
        }
    } else {
        sharesToExit = operator.shareDetails[strategy].allocation;
        //@audit reset operator allocation
        delete operatorDetails[operatorId].shareDetails[strategy].allocation;
    }
    if (sharesToExit == 0) revert IRioLRTOperatorRegistry.CANNOT_EXIT_ZERO_SHARES();

    // Queues a withdrawal to the deposit pool.
    bytes32 withdrawalRoot = delegator.queueWithdrawalForOperatorExit(strategy, sharesToExit);
    emit IRioLRTOperatorRegistry.OperatorStrategyExitQueued(operatorId, strategy, sharesToExit, withdrawalRoot);
}

Duplicate of #10

[sherlock-admin4]

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

[10xhash]

The protocol team fixed this issue in the following PRs/commits: rio-org/rio-sherlock-audit#14

Fixed Sets allocation to 0

[sherlock-admin4]

The Lead Senior Watson signed off on the fix.