mstpr-brainbot - If the operators receives or tries to deposit dust amount of shares then the rebalance will not be possible

[sherlock-admin]

mstpr-brainbot

medium

If the operators receives or tries to deposit dust amount of shares then the rebalance will not be possible

Summary

Operators has caps when depositing to the strategies of EigenLayer. If the deposited amount is dust then the round down of the shares minted can be "0" which the tx would revert in EigenLayer side. This would brick the deposit and rebalance flow completely.

Vulnerability Detail

When the deposit pool has excess balance, the balance will be distributed to operators respecting their utilizations and caps. Assume the least utilized operator has only a few remaining spots left, which is a dust amount like 1e3. Now, let's see what would happen in the allocation flow:

function allocateStrategyShares(address strategy, uint256 sharesToAllocate) external onlyDepositPool returns (uint256 sharesAllocated, OperatorStrategyAllocation[] memory allocations) {
        .
        while (remainingShares > 0) {
            .
            .
           -> uint256 newShareAllocation = FixedPointMathLib.min(operatorShares.cap - operatorShares.allocation, remainingShares);
            -> uint256 newTokenAllocation = IStrategy(strategy).sharesToUnderlyingView(newShareAllocation);
            allocations[allocationIndex] = OperatorStrategyAllocation(
                operator.delegator,
                newShareAllocation,
                newTokenAllocation
            );
            .
            .
        }
       .
    }

As we can observe in the above snippet, if the amount is dust, then uint256 newTokenAllocation = IStrategy(strategy).sharesToUnderlyingView(newShareAllocation); can be rounded down to "0" since EigenLayer rounds down when calculating the underlying tokens needed. Then, the newTokenAllocation will be equal to "0", and the delegation operator deposits the amount to EigenLayer as follows:

function stakeERC20(address strategy, address token_, uint256 amount) external onlyDepositPool returns (uint256 shares) {
        if (IERC20(token_).allowance(address(this), address(strategyManager)) < amount) {
            IERC20(token_).forceApprove(address(strategyManager), type(uint256).max);
        }
        -> shares = strategyManager.depositIntoStrategy(strategy, token_, amount);
    }

From the above snippet, we can see that the strategyManager.depositIntoStrategy will be called with an amount of "0". Now, let's examine how EigenLayer handles the "0" amount deposits:

function deposit(
        IERC20 token,
        uint256 amount
    ) external virtual override onlyWhenNotPaused(PAUSED_DEPOSITS) onlyStrategyManager returns (uint256 newShares) {
        .
        .
        // account for virtual shares and balance
        uint256 virtualShareAmount = priorTotalShares + SHARES_OFFSET;
        uint256 virtualTokenBalance = _tokenBalance() + BALANCE_OFFSET;
        // calculate the prior virtual balance to account for the tokens that were already transferred to this contract
        uint256 virtualPriorTokenBalance = virtualTokenBalance - amount;
        newShares = (amount * virtualShareAmount) / virtualPriorTokenBalance;

        // extra check for correctness / against edge case where share rate can be massively inflated as a 'griefing' sort of attack
        -> require(newShares != 0, "StrategyBase.deposit: newShares cannot be zero");
        .
    }

As we can see, if the shares to be minted are "0," which will be the case since we try to deposit "0" amount of tokens, then the transaction will revert, hence, the entire deposit flow will be halted.

Malicious Scenario: Assume that at an epoch, there are "N" assets requested for withdrawal, and there are no deposits to the LRT token. The attacker can donate 1 wei to the deposit pool just before the rebalance call. Subsequently, the rebalance would attempt to withdraw the "N" tokens as normal. However, when it tries to deposit the excess back to operators, which is only "1 wei," the transaction can revert since it's a dust amount.

Impact

The above scenarios can happen in normal flow or can be triggered by a malicious user. There is a DoS threat and it needs donations or owner manually lowering the caps. Hence, I will label this as medium.

Code Snippet

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/restaking/RioLRTOperatorDelegator.sol#L174-L179

https://github.com/Layr-Labs/eigenlayer-contracts/blob/5c192e1a780c22e027f6861f958db90fb9ae263c/src/contracts/strategies/StrategyBase.sol#L96-L123

Tool used

Manual Review

Recommendation

If the amount is dust then skip to the next operator and ignore the amount, don't deposit.

[nevillehuang]

request poc

Given there is a sacrificial deposit, wouldn't this be not possible?

[sherlock-admin3]

PoC requested from @mstpr

Requests remaining: 18

[mstpr]

PoC:

// forge test --match-contract RioLRTDepositPoolTest --match-test test_RoundDownInEigenLayerStrategy -vv
 function test_RoundDownInEigenLayerStrategy() public {
        uint8 operatorId = addOperatorDelegator(reLST.operatorRegistry, address(reLST.rewardDistributor));
        address operatorDelegator = reLST.operatorRegistry.getOperatorDetails(operatorId).delegator;

        // initiate the strategy with some funds first
        uint bootstrapAm = 100 * 1e18;
        cbETH.mint(EOA, bootstrapAm);
        uint256 AMOUNT = 1;
        vm.prank(EOA);
        cbETH.approve(address(strategyManager), type(uint256).max);
        vm.prank(EOA);
        strategyManager.depositIntoStrategy(address(CBETH_STRATEGY), CBETH_ADDRESS, bootstrapAm);
        // add some more cbETH to mock yield on the strategy
        cbETH.mint(address(CBETH_STRATEGY), 1e5);

        uint sharess = IStrategy(CBETH_STRATEGY).underlyingToSharesView(AMOUNT);
        // round down to "0" as we expected!
        assertEq(sharess, 0);

        // Allocate to cbETH strategy.small amount such that the amount round down in EigenLayer strategy and it reverts
        cbETH.approve(address(reLST.coordinator), type(uint256).max);
        reLST.coordinator.deposit(CBETH_ADDRESS, AMOUNT);
        console.log("SHARES HELD", reLST.assetRegistry.getAssetSharesHeld(CBETH_ADDRESS));

        // Push funds into EigenLayer, it will fail because of the 
        vm.startPrank(EOA, EOA);
        vm.expectRevert();
        reLST.coordinator.rebalance(CBETH_ADDRESS);
        vm.stopPrank();
    }

[mstpr]

request poc

Given there is a sacrificial deposit, wouldn't this be not possible?

request poc

Given there is a sacrificial deposit, wouldn't this be not possible?

Not really. This round down issue happens in EigenLayer strategy side not in Rio's.

[solimander]

I don't believe this is feasible when there are actually pending withdrawals.

e.g.

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

    // initiate the strategy with some funds first
    uint256 bootstrapAm = 100 * 1e18;
    cbETH.mint(EOA, bootstrapAm);
    uint256 AMOUNT = 1;
    vm.prank(EOA);
    cbETH.approve(address(strategyManager), type(uint256).max);
    vm.prank(EOA);

    strategyManager.depositIntoStrategy(address(CBETH_STRATEGY), CBETH_ADDRESS, bootstrapAm);

    cbETH.approve(address(reLST.coordinator), type(uint256).max);

    // Deposit to create some reLST to withdraw
    cbETH.mint(address(CBETH_STRATEGY), 1e18);
    uint256 amountOut = reLST.coordinator.deposit(CBETH_ADDRESS, 1e18);

    // Rebalance to push into EigenLayer
    skip(reLST.coordinator.rebalanceDelay());

    vm.prank(EOA, EOA);
    reLST.coordinator.rebalance(CBETH_ADDRESS);

    reLST.coordinator.requestWithdrawal(CBETH_ADDRESS, amountOut);

    // add some more cbETH to mock yield on the strategy
    cbETH.mint(address(CBETH_STRATEGY), 1e5);

    uint256 sharess = IStrategy(CBETH_STRATEGY).underlyingToSharesView(AMOUNT);
    // round down to "0" as we expected!
    assertEq(sharess, 0);

    // Allocate to cbETH strategy.small amount such that the amount round down in EigenLayer strategy and it reverts
    reLST.coordinator.deposit(CBETH_ADDRESS, AMOUNT);
    console.log('SHARES HELD', reLST.assetRegistry.getAssetSharesHeld(CBETH_ADDRESS));

    skip(reLST.coordinator.rebalanceDelay());

    vm.startPrank(EOA, EOA);
    vm.expectRevert();
    reLST.coordinator.rebalance(CBETH_ADDRESS);
    vm.stopPrank();
}

[KupiaSecAdmin]

Escalate

This should be considered as Invalid, because: Share value is compared to actual asset value, it's at least equal or bigger, that's why it is called 'share'. When share value is converted to actual asset amount, it must become bigger than the share value provided even though it's rounded down in math. Thus, whether 1wei or 1e3wei is passed to EigenLayer, more than the share value is returned, as a result, returned value literally can not be zero.

Hope this helps making final judgement. 👍

[sherlock-admin2]

Escalate

This should be considered as Invalid, because: Share value is compared to actual asset value, it's at least equal or bigger, that's why it is called 'share'. When share value is converted to actual asset amount, it must become bigger than the share value provided even though it's rounded down in math. Thus, whether 1wei or 1e3wei is passed to EigenLayer, more than the share value is returned, as a result, returned value literally can not be zero.

Hope this helps making final judgement. 👍

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.

[10xhash]

The dust amount will result in 0 shares. And hence it should not make any calls to eigenlayer to deposit. The revert in above poc occurs due to the condition kept in Rio that there is no rebalance needed when there are 0 shares to allocate which is a feature

[Almur100]

Rebalance will revert when the address pool has less balance than the epoch’s shareowed.this is not a feature.

[mstpr]

@nevillehuang Agreed, looks invalid to me aswell now.

[nevillehuang]

Agree that this issue is invalid

[Almur100]

Issue 179 and issue 181 are valid. those are not feature.

[Czar102]

I agree that this report (#18) is invalid. Planning to invalidate it.

@nevillehuang can you look into @Almur100's claim?

[Almur100]

Issue 179 and 181 are different from issue 18.

[nevillehuang]

@Almur100 both sharesReceived and sharesOwed needs to be 0 for rebalance to revert. I believe your claims are incorrect.

Please provided a coded PoC for #179 and #181 if you feel otherwise.

[Almur100]

Yes, thanks. Those are invalid

[Czar102]

Result: Invalid Has duplicates

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• KupiaSecAdmin: accepted