mstpr-brainbot - Heap is incorrectly stores the removed operator ID which can lead to division by zero in deposit/withdrawal flow

[sherlock-admin]

mstpr-brainbot

high

Heap is incorrectly stores the removed operator ID which can lead to division by zero in deposit/withdrawal flow

Summary

An operator's strategy can be reset by the owner calling setOperatorStrategyCaps to "0". This action sets the utilization to "0" and removes the operator from the heap. Consequently, this means that the operator has unwound all its strategy shares and can no longer receive any more deposits. However, due to how the heap is organized, if an operator who had funds before is reset to "0", the heap will not successfully remove the operator. As a result, when ordering the heap, a division by "0" will occur, causing the transaction to revert on deposits and withdrawals indefinitely.

Vulnerability Detail

In order to break down the issue, let's divide the issue to 2 parts which their combination is the issue itself

1- Heap is not removing the removed ID from the heaps storage when the operator is removed

When the operator is removed, the operator will be removed from the heap as follows:

function setOperatorStrategyCap(
        RioLRTOperatorRegistryStorageV1.StorageV1 storage s,
        uint8 operatorId,
        IRioLRTOperatorRegistry.StrategyShareCap memory newShareCap
    ) internal {
        .
        OperatorUtilizationHeap.Data memory utilizationHeap = s.getOperatorUtilizationHeapForStrategy(newShareCap.strategy);
        // If the current cap is greater than 0 and the new cap is 0, remove the operator from the strategy.
        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);
        }
        .

        // Persist the updated heap to the active operators tracking.
        -> utilizationHeap.store(s.activeOperatorsByStrategyShareUtilization[newShareCap.strategy]);
         .
    }

removeByID calls the internal _remove function which is NOT removes the last element! self.count is decreased however, the index is still the previous value of the self.count

function _remove(Data memory self, uint8 i) internal pure {
        self.operators[i] = self.operators[self.count--];
    }

For example, if there are 3 operators as follows: operatorId: 1, utilization: 50% operatorId: 2, utilization: 60% operatorId: 3, utilization: 70% then, the heap.count would be 3 and the order would be: 1, 2, 3 in the heap heap.operators[1] = operatorId 1 heap.operators[2] = operatorId 2 heap.operators[3] = operatorId 3

if we remove the operator Id 2: heap.count = 2 order: 1,3 heap.operators[1] = operatorId 1 heap.operators[2] = operatorId 2 heap.operators[3] = operatorId 0 THIS SHOULD BE "0" since its removed but it is "3" in the current implementation!

As shown here, the operators[3] should be "0" since there isn't any operator3 in the heap anymore but the heap keeps the value and not resets it.

Here a test shows the above issue:

// forge test --match-contract OperatorUtilizationHeapTest --match-test test_removingDoesNotUpdatesStoredHeap -vv
    function test_removingDoesNotUpdatesStoredHeap() public {
        OperatorUtilizationHeap.Data memory heap = OperatorUtilizationHeap.initialize(5);

        heap.insert(OperatorUtilizationHeap.Operator({id: 1, utilization: 50}));
        heap.store(heapStore);

        heap.insert(OperatorUtilizationHeap.Operator({id: 2, utilization: 60}));
        heap.store(heapStore);

        heap.insert(OperatorUtilizationHeap.Operator({id: 3, utilization: 70}));
        heap.store(heapStore);

        console.log("Heaps count", heap.count);
        console.log("1st", heap.operators[1].id);
        console.log("2nd", heap.operators[2].id);
        console.log("3rd", heap.operators[3].id);

        // remove 2
        heap.removeByID(3);
        heap.store(heapStore);

        console.log("Heaps count", heap.count);
        console.log("1st", heap.operators[1].id);
        console.log("2nd", heap.operators[2].id);
        console.log("3rd", heap.operators[3].id);
    }

Logs:

2- When the operator cap is reseted the allocations/deallocations will not work due to above heap issue because of division by zero

Now, take the above example, we removed the operatorId 3 from the heap by setting its cap to "0". Now, there are only operators 1 and 2 active for that specific strategy. When there are idle funds in the deposit pool before the rebalance call, the excess funds that are not requested as withdrawals will be pushed to EigenLayer as follows:

function rebalance(address asset) external checkRebalanceDelayMet(asset) {
       .
       .
        -> (uint256 sharesReceived, bool isDepositCapped) = depositPool().depositBalanceIntoEigenLayer(asset);
        .
    }

 function depositBalanceIntoEigenLayer(address asset) external onlyCoordinator returns (uint256, bool) {
        uint256 amountToDeposit = asset.getSelfBalance();
        if (amountToDeposit == 0) return (0, false);
        .
        .
        -> return (OperatorOperations.depositTokenToOperators(operatorRegistry(), asset, strategy, sharesToAllocate), isDepositCapped);
    }

function depositTokenToOperators(
        IRioLRTOperatorRegistry operatorRegistry,
        address token,
        address strategy,
        uint256 sharesToAllocate
    ) internal returns (uint256 sharesReceived) {
       -> (uint256 sharesAllocated, IRioLRTOperatorRegistry.OperatorStrategyAllocation[] memory  allocations) = operatorRegistry.allocateStrategyShares(
            strategy, sharesToAllocate
        );
        .
        .
    }

function allocateStrategyShares(address strategy, uint256 sharesToAllocate) external onlyDepositPool returns (uint256 sharesAllocated, OperatorStrategyAllocation[] memory allocations) {
        -> OperatorUtilizationHeap.Data memory heap = s.getOperatorUtilizationHeapForStrategy(strategy);
       .
       .
       .
       .
    }

function getOperatorUtilizationHeapForStrategy(RioLRTOperatorRegistryStorageV1.StorageV1 storage s, address strategy) internal view returns (OperatorUtilizationHeap.Data memory heap) {
        uint8 numActiveOperators = s.activeOperatorCount;
        if (numActiveOperators == 0) return OperatorUtilizationHeap.Data(new OperatorUtilizationHeap.Operator[](0), 0);

        heap = OperatorUtilizationHeap.initialize(MAX_ACTIVE_OPERATOR_COUNT);
        LibMap.Uint8Map storage operators = s.activeOperatorsByStrategyShareUtilization[strategy];

        IRioLRTOperatorRegistry.OperatorShareDetails memory operatorShares;
        unchecked {
            uint8 i;
            for (i = 0; i < numActiveOperators; ++i) {
                uint8 operatorId = operators.get(i);

                // Non-existent operator ID. We've reached the end of the heap.
                if (operatorId == 0) break;

                operatorShares = s.operatorDetails[operatorId].shareDetails[strategy];
                heap.operators[i + 1] = OperatorUtilizationHeap.Operator({
                    id: operatorId,
                    -> utilization: operatorShares.allocation.divWad(operatorShares.cap)
                });
            }
            heap.count = i;
        }
    }

As we can see in one above code snippet, the numActiveOperators is 3. Since the stored heaps last element is not set to "0" it will point to operatorId 3 which has a cap of "0" after the removal. This will make the

utilization: operatorShares.allocation.divWad(operatorShares.cap)

part of the code to perform a division by zero and the function will revert.

Coded PoC:

// forge test --match-contract RioLRTOperatorRegistryTest --match-test test_Capped0ValidatorBricksFlow -vv
    function test_Capped0ValidatorBricksFlow() public {
        // Add 3 operators
        addOperatorDelegators(reLST.operatorRegistry, address(reLST.rewardDistributor), 3);

        // The caps for each operator is 1000e18, we will delete the id 2 so we need funds there
        // any number that is more than 1000 should be ok for that experiement 
        uint256 AMOUNT = 1002e18;

        // Allocate to cbETH strategy.
        cbETH.approve(address(reLST.coordinator), type(uint256).max);
        uint256 lrtAmount = reLST.coordinator.deposit(CBETH_ADDRESS, AMOUNT);

        // Push funds into EigenLayer.
        vm.prank(EOA, EOA);
        reLST.coordinator.rebalance(CBETH_ADDRESS);

        // Build the empty caps
        IRioLRTOperatorRegistry.StrategyShareCap[] memory zeroStrategyShareCaps =
            new IRioLRTOperatorRegistry.StrategyShareCap[](1);
        zeroStrategyShareCaps[0] = IRioLRTOperatorRegistry.StrategyShareCap({strategy: CBETH_STRATEGY, cap: 0});

        // Set the caps of CBETH_STRATEGY for operator 2 as "0"
        reLST.operatorRegistry.setOperatorStrategyShareCaps(2, zeroStrategyShareCaps);

        // Try an another deposit, we expect revert when we do the rebalance
        reLST.coordinator.deposit(CBETH_ADDRESS, 10e18);

        // Push funds into EigenLayer. Expect revert, due to division by "0"
        skip(reETH.coordinator.rebalanceDelay());
        vm.startPrank(EOA, EOA);
        vm.expectRevert(bytes4(keccak256("DivWadFailed()")));
        reLST.coordinator.rebalance(CBETH_ADDRESS);
        vm.stopPrank();
    }

Impact

Core logic broken, withdrawal/deposits can not be performed.

Code Snippet

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/4f01e065c1ed346875cf5b05d2b43e0bcdb4c849/rio-sherlock-audit/contracts/utils/OperatorRegistryV1Admin.sol#L231C5-L270C6

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/4f01e065c1ed346875cf5b05d2b43e0bcdb4c849/rio-sherlock-audit/contracts/utils/OperatorUtilizationHeap.sol#L94-L110

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

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

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/4f01e065c1ed346875cf5b05d2b43e0bcdb4c849/rio-sherlock-audit/contracts/utils/OperatorOperations.sol#L51-L68

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

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/4f01e065c1ed346875cf5b05d2b43e0bcdb4c849/rio-sherlock-audit/contracts/utils/OperatorRegistryV1Admin.sol#L327-L351

Tool used

Manual Review

Recommendation

When removing from the heap also remove the last element from the heap.

I am not sure of this, but this might work

function _remove(Data memory self, uint8 i) internal pure {
        self.operators[i] = self.operators[--self.count];
    }

[sherlock-admin4]

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

[nevillehuang]

Severity could be higher, given a use of the function correctly results in blocking of withdrawals. Leaving medium for now on grounds of admin error

[shaka0x]

Escalate

Leaving medium for now on grounds of admin error.

I respectfully disagree with this reasoning. I think the severity of the issue and its duplicate should be high, as there is no admin error involved. There is an error in the implementation that is produced after an admin action. Otherwise, all issues at deployment or in protected functions can technically be considered as admin errors.

[sherlock-admin2]

Escalate

Leaving medium for now on grounds of admin error.

I respectfully disagree with this reasoning. I think the severity of the issue and its duplicate should be high, as there is no admin error involved. There is an error in the implementation that is produced after an admin action. Otherwise, all issues at deployment or in protected functions can technically be considered as admin errors.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[nevillehuang]

Agree that this issue should be high severity since withdrawals can be blocked permanently

[Czar102]

@nevillehuang @mstpr can't the admin remediate the situation?

[nevillehuang]

@solimander Could you confirm if admin remediation is possible by resetting validator cap of removed operator? Given the intended admin workflow results in blocking of funds I think the impact is severe

[solimander]

@nevillehuang Remediation is possible by deactivating the operator:

// forge test --mt test_capped0ValidatorBricksFlowRecovery
function test_capped0ValidatorBricksFlowRecovery() public {
    // Add 3 operators
    addOperatorDelegators(reLST.operatorRegistry, address(reLST.rewardDistributor), 3);

    // The caps for each operator is 1000e18, we will delete the id 2 so we need funds there
    // any number that is more than 1000 should be ok for that experiement
    uint256 AMOUNT = 1002e18;

    // Allocate to cbETH strategy.
    cbETH.approve(address(reLST.coordinator), type(uint256).max);
    uint256 lrtAmount = reLST.coordinator.deposit(CBETH_ADDRESS, AMOUNT);

    // Push funds into EigenLayer.
    vm.prank(EOA, EOA);
    reLST.coordinator.rebalance(CBETH_ADDRESS);

    // Build the empty caps
    IRioLRTOperatorRegistry.StrategyShareCap[] memory zeroStrategyShareCaps =
        new IRioLRTOperatorRegistry.StrategyShareCap[](1);
    zeroStrategyShareCaps[0] = IRioLRTOperatorRegistry.StrategyShareCap({strategy: CBETH_STRATEGY, cap: 0});

    // Set the caps of CBETH_STRATEGY for operator 2 as "0"
    reLST.operatorRegistry.setOperatorStrategyShareCaps(2, zeroStrategyShareCaps);

    // Try an another deposit, we expect revert when we do the rebalance
    reLST.coordinator.deposit(CBETH_ADDRESS, 10e18);

    // Push funds into EigenLayer. Expect revert, due to division by "0"
    skip(reETH.coordinator.rebalanceDelay());
    vm.startPrank(EOA, EOA);
    vm.expectRevert(bytes4(keccak256('DivWadFailed()')));
    reLST.coordinator.rebalance(CBETH_ADDRESS);
    vm.stopPrank();

    // Deactivate the operator to recover the system
    reLST.operatorRegistry.deactivateOperator(2);

    // Rebalance succeeds
    vm.prank(EOA, EOA);
    reLST.coordinator.rebalance(CBETH_ADDRESS);
}

This acts as a temporary fix, which would unblock rebalances while the issue is patched.

[mstpr]

@nevillehuang @mstpr can't the admin remediate the situation?

not really.

The admin needs to reset the cap for the operator. However, when this happens, the operator's cap is reset to "0," allowing deposits to be made again. If the admin sets an operator's cap to "0," it's likely that the operator will not be used. To address the above issue, the admin must reset it to a value. However, this means that new deposits can be made to the operator. Although the admin can set the cap back to a value, all users must withdraw their funds before new deposits are made. Since the admin does not control all users, this is not feasible and cannot be fixed in my opinion.

If the operator is deactivated instead of its cap resetted to "0" then it is even worse. Then, the admin has to readd the operator back to system and needs to push funds to that operator such that the heap reorders correctly. Though, to do that admin needs significant amount of funds to push to system to increase the utilization.

Overall it might be possible but it is extremely hard and requires capital. What do you think @shaka0x @itsabinashb ?

[shaka0x]

@nevillehuang @mstpr can't the admin remediate the situation?

not really.

The admin needs to reset the cap for the operator. However, when this happens, the operator's cap is reset to "0," allowing deposits to be made again. If the admin sets an operator's cap to "0," it's likely that the operator will not be used. To address the above issue, the admin must reset it to a value. However, this means that new deposits can be made to the operator. Although the admin can set the cap back to a value, all users must withdraw their funds before new deposits are made. Since the admin does not control all users, this is not feasible and cannot be fixed in my opinion.

If the operator is deactivated instead of its cap resetted to "0" then it is even worse. Then, the admin has to readd the operator back to system and needs to push funds to that operator such that the heap reorders correctly. Though, to do that admin needs significant amount of funds to push to system to increase the utilization.

Overall it might be possible but it is extremely hard and requires capital. What do you think @shaka0x @itsabinashb ?

I do agree with the above comments and would like to add that the proposed solution will not work for the cases described in my PoCs (https://github.com/sherlock-audit/2024-02-rio-network-core-protocol-judging/issues/316), where the bug appears after deactivating an operator.

[Czar102]

@solimander do you agree with the above comments?

@itsabinashb please do not post unnecessarily long code/result snippets directly in a comment, it's better to put them in a gist.

If @solimander agrees, I'm planning to accept the escalation and consider this issue a valid High severity one.

[solimander]

@Czar102 After reviewing @shaka0x's POCs, I do agree with the above comments.

[Czar102]

Result: High Has duplicates

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• shaka0x: accepted

[zrax-x]

@nevillehuang Is issue#16 a duplicate? I can't seem to understand what the problem described in issue#16 is. I believe that it misses the point and has no negative impact. And issue#155, issue#127.

[itsabinashb]

Is issue#16 a duplicate? I can't seem to understand what the problem described in issue#16 is. I believe that it misses the point and has no negative impact. And issue#155.

Issue number 16 shows exact root cause which is same as this submission.

[zrax-x]

Is issue#16 a duplicate? I can't seem to understand what the problem described in issue#16 is. I believe that it misses the point and has no negative impact. And issue#155.

Issue number 16 shows exact root cause which is same as this submission.

However, you did not accurately describe the harm caused, which is "division by zero".

[solimander]

I do agree that #16 does sort of miss the point as the core issue is not mentioned. The issue is not that the removed operator ID still exists in memory, but that it's not correctly removed from storage.

[10xhash]

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

Fixed Now all operator slots greater than the last operator is set to 0

[sherlock-admin4]

The Lead Senior Watson signed off on the fix.