zraxx - When `validatorCount` is greater than or equal to `fromIndex`, the function `reportOutOfOrderValidatorExits` will fail.

[sherlock-admin3]

zraxx

medium

When validatorCount is greater than or equal to fromIndex, the function reportOutOfOrderValidatorExits will fail.

Summary

When validatorCount is greater than or equal to fromIndex, the function reportOutOfOrderValidatorExits will fail.

Vulnerability Detail

In the function reportOutOfOrderValidatorExits, it will swap the position of the validators starting from the fromIndex with the validators that were next in line to be exited. However, when validators.exited is 0 and validatorCount is greater than or equal to fromIndex, function swapValidatorDetails will be reverted due to INDEXES_OVERLAP.

POC

I modified the test function test_reportOutOfOrderValidatorExits as shown below.

function test_reportOutOfOrderValidatorExits() public {
    uint40 UPLOADED_KEY_COUNT = 1_000;

    uint256 DEPOSIT_COUNT = 300;
    uint256 OOO_EXIT_STARTING_INDEX = 100;
    uint256 OOO_EXIT_COUNT = 100;

    uint8 operatorId = addOperatorDelegator(
        reETH.operatorRegistry, address(reETH.rewardDistributor), emptyStrategyShareCaps, UPLOADED_KEY_COUNT
    );

    IRioLRTOperatorRegistry.OperatorPublicDetails memory details =
        reETH.operatorRegistry.getOperatorDetails(operatorId);

    // Allocate `DEPOSIT_COUNT` deposits
    vm.prank(address(reETH.depositPool));
    reETH.operatorRegistry.allocateETHDeposits(DEPOSIT_COUNT);

    // Mark operators as withdrawn.
    vm.mockCall(
        address(IRioLRTOperatorDelegator(details.delegator).eigenPod()),
        abi.encodeWithSelector(IEigenPod.validatorStatus.selector),
        abi.encode(IEigenPod.VALIDATOR_STATUS.WITHDRAWN)
    );

    // Ensure the expected public keys are swapped.
    uint256 j = OOO_EXIT_STARTING_INDEX;
    (bytes memory expectedPublicKeys,) = TestUtils.getValidatorKeys(UPLOADED_KEY_COUNT);
    for (uint256 i = 0; i < OOO_EXIT_COUNT; i++) {
        uint256 key1Start = j * ValidatorDetails.PUBKEY_LENGTH;
        uint256 key1End = (j + 1) * ValidatorDetails.PUBKEY_LENGTH;

        uint256 key2Start = i * ValidatorDetails.PUBKEY_LENGTH;
        uint256 key2End = (i + 1) * ValidatorDetails.PUBKEY_LENGTH;

        vm.expectEmit(true, false, false, true, address(reETH.operatorRegistry));
        emit ValidatorDetails.ValidatorDetailsSwapped(
            operatorId,
            bytes(LibString.slice(string(expectedPublicKeys), key1Start, key1End)),
            bytes(LibString.slice(string(expectedPublicKeys), key2Start, key2End))
        );

        j++;
    }

    // Report the out of order exits of `OOO_EXIT_COUNT` validators starting at index `OOO_EXIT_STARTING_INDEX`.
    reETH.operatorRegistry.reportOutOfOrderValidatorExits(operatorId, OOO_EXIT_STARTING_INDEX, OOO_EXIT_COUNT);

    details = reETH.operatorRegistry.getOperatorDetails(operatorId);
    assertEq(details.validatorDetails.exited, OOO_EXIT_COUNT);
}

Finally, the execution results are as follows:

Impact

The function reportOutOfOrderValidatorExits can’t work normally.

Code Snippet

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/main/rio-sherlock-audit/contracts/restaking/RioLRTOperatorRegistry.sol#L310-L336

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/main/rio-sherlock-audit/contracts/utils/ValidatorDetails.sol#L129-L130

Tool used

Manual Review

Recommendation

Modify the overlap check logic.

[nevillehuang]

Borderline medium/low, leaving for discussion, need to understand impact better.

[solimander]

Seems like you can workaround this by splitting the report into two calls, no? If so, seems low severity. Will update regardless.

[10xhash]

Escalate

Splitting cannot be used to workaround in all cases ie. if the to be reported validator is validators.exited + 1, it can't be split

[sherlock-admin2]

Escalate

Splitting cannot be used to workaround in all cases ie. if the to be reported validator is validators.exited + 1, it can't be split

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.

[solimander]

@10xhash I believe you're correct - Splitting works in all cases EXCEPT when a single operator is exited early and it's at index validators.exited + 1.

[nevillehuang]

@solimander @10xhash What is the impact of this DoS and is it permanent/time sensitive?

[10xhash]

@solimander @10xhash What is the impact of this DoS and is it permanent/time sensitive?

Even after a validator has been exited and its associated funds are removed from the eigenpod, the rio system will incorrectly think that this validator is still active on the operator. This could lead to permanent reverts on withdrawing ETH since it will attempt to remove shares from the already exited operator.

[solimander]

Even after a validator has been exited and its associated funds are removed from the eigenpod, the rio system will incorrectly think that this validator is still active on the operator. This could lead to permanent reverts on withdrawing ETH since it will attempt to remove shares from the already exited operator.

This will recover on its own in most cases. If the next in line to be exited validator is exited from the beacon chain early, and either no ones proves the withdrawal, or the withdrawal is proven on the EigenPod and no one scrapes the excess ETH, then the system will recover following the next ETH withdrawal request through the operator.

If the ETH was scraped from the EigenPod, then withdrawals from the affected operator will revert until another validator is exited to cover the shortfall. Alternatively, the validator at the index above the validator mentioned here could be exited early so both could be reported.

[10xhash]

Even after a validator has been exited and its associated funds are removed from the eigenpod, the rio system will incorrectly think that this validator is still active on the operator. This could lead to permanent reverts on withdrawing ETH since it will attempt to remove shares from the already exited operator.

This will recover on its own in most cases. If the next in line to be exited validator is exited from the beacon chain early, and either no ones proves the withdrawal, or the withdrawal is proven on the EigenPod and no one scrapes the excess ETH, then the system will recover following the next ETH withdrawal request through the operator.

If the ETH was scraped from the EigenPod, then withdrawals from the affected operator will revert until another validator is exited to cover the shortfall. Alternatively, the validator at the index above the validator mentioned here could be exited early so both could be reported.

Yes, I agree that exiting another validator out of order could be used as a workaround for the permanent revert issue

[nevillehuang]

In that case, I believe medium severity to be appropriate for this issue.

[Czar102]

@10xhash do you agree with Medium?

I'm planning to make this issue valid and accept the escalation.

[KupiaSecAdmin]

@Czar102 - Among 3 issues, this issue describe different cause than #280 and #292. The point of this issue - swap reverts when count > fromIndex, and this issue can be resolved by splitting segments.

280 and #292 - swap reverts when two segments are adjacent(e.g. swap [1, 3] with [4,6] reverts), which can't be resolved by splitting it.

@nevillehuang @10xhash @solimander Your thoughts are welcome as well. 👍

[10xhash]

@10xhash do you agree with Medium?

I'm planning to make this issue valid and accept the escalation.

I think its borderline med / low

@KupiaSecAdmin I agree that this report fails to correctly identify the vulnerability. This report points to a scenario which includes the same vulnerability (when the count is equal to fromIndex and exited is 0, it will be an adjacent index case) fromIndex = 3, validatorCount = 3 to be exited [3,4,5] to be shifted [0,1,2] no index overlap

But the mentioned issue (when validators.exited is 0 and validatorCount is greater than or equal to fromIndex) points to reverts occurring in a different scenario, which is intended by the team (reverts when indexes overlap) fromIndex = 3, validatorCount = 4 to be exited [3,4,5.6] to be shifted [0,1,2,3] index 3 overlaps

[nevillehuang]

On second thought, doesn't this comment here, prove no core functionality is broken, given both out of order operators are intended to be exited anyways

[10xhash]

The second validator need not be exited in a normal scenario. The exit and invoking reportOutOfOrderValidatorExits on this validator would be done in order to workaround the reverting issue when withdrawing. In case such additional validators are not present (operator has a only <=3 active validators including the one exited), the workaround would be to add more validators, allocate ETH and then exit them to report them out of order in order to actually report originally exited validator

[nevillehuang]

@solimander Is there any scenario where a operator would have less than 3 validators? Also if the workaround was invoked, no loss of funds would occur correct?

[solimander]

@nevillehuang

Is there any scenario where a operator would have less than 3 validators?

Unlikely - each operator is planned to have tens to hundreds of validators.

Also if the workaround was invoked, no loss of funds would occur correct?

Correct.

[Czar102]

In that case, planning to reject the escalation and leave this and duplicates Low/Invalid.

[Czar102]

Result: Low Has Duplicates

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• 10xhash: rejected