sherlock-admin2 commented 7 months ago

iamandreiski

medium

Lack of slippage parameters can affect the LRT / share amounts during deposits / withdrawals

Summary

During both deposits and withdrawals, the amount that the user receives in LRT tokens (during deposits) or amount of shares (during withdrawals) Is dependent on mainly the correlation between the supply of the given asset, the TVL (total value locked) and the price fetched from an oracle.

Since the total value locked is dependent on the amount of shares recorded in the accounting system for the underlying asset in-question / all assets (updated during rebalances) + the balance of the assets in the contract (updated whenever the deposit is sent) this means that the value a user receives in return when depositing is dependent on the of the amount of shares + the current balance recorded in the protocol.

During stepwise jumps in rewards accumulation and/or large deposits/withdrawals to/from the system, the TVL can drastically change, affecting the price ratio between the underlying assets and LRT. Users can get receive less LRT/shares than expected their deposit/withdrawal transaction was frontrun by a very large transaction, the protocol was rebalanced, etc.

Vulnerability Detail

When deposits are made, the calculations for the amount of LRT tokens the user will get in return versus the number of shares is calculated by using the following formulas. Let's take a look at the deposit flow:

Deposits:

Once amountOut is calculated, this will determine the amount of LRT tokens that the user will receive in exchange for their deposit in the underlying token (for e.g. reETH, cbETH, etc.)

amountOut = convertFromAssetToRestakingTokens(asset, amountIn);

  function convertFromAssetToRestakingTokens(address asset, uint256 amount) public view returns (uint256) {
        uint256 value = assetRegistry().convertToUnitOfAccountFromAsset(asset, amount);
        return convertFromUnitOfAccountToRestakingTokens(value);
    }

From the above the value parameter will be calculated by fetching the price from the price oracle:

function convertToUnitOfAccountFromAsset(address asset, uint256 amount) public view returns (uint256) {
        if (asset == ETH_ADDRESS) {
            return amount;
        }
        address priceFeed = assetInfo[asset].priceFeed;
        uint256 price = getPrice(priceFeed);

        return _normalizeDecimals(price * amount / priceScale, assetInfo[asset].decimals, priceFeedDecimals);
    }

After value is returned using the above calculations, then we will call the convertFromUnitOfAccountToRestakingTokens() with the value parameter as a function argument. This function will output the amount of LRT tokens that the user should receive based on the calculations below:

function convertFromUnitOfAccountToRestakingTokens(uint256 value) public view returns (uint256) {
        uint256 tvl = getTVL();
        uint256 supply = token.totalSupply();

        if (supply == 0) {
            return value;
        }
        return value * supply / tvl;
    }

As we can see, the above function utilizes the TVL (Total Value Locked) in the protocol (of all underlying assets) to come up with the price. The TVL is greatly dependent on the amount of shares currently accounted for in the system:

function getTVLForAsset(address asset) public view returns (uint256) {
        uint256 balance = getTotalBalanceForAsset(asset);
        if (asset == ETH_ADDRESS) {
            return balance;
        }
        return convertToUnitOfAccountFromAsset(asset, balance);

The way that balance is calculated, since it's crucial for the TVL which is based on balance * oraclePriceForAsset; it takes into consideration all of the asset shares held in Rio: assetInfo[asset].shares + the balanceOf(asset):

  uint256 sharesHeld = getAssetSharesHeld(asset);
        uint256 tokensInRio = IERC20(asset).balanceOf(depositPool_);
        uint256 tokensInEigenLayer = convertFromSharesToAsset(getAssetStrategy(asset), sharesHeld);

        return tokensInRio + tokensInEigenLayer;

PoC

For simpler calculations, let's say that Alice wants to deposit ETH through the depositETH() function:

        // Convert deposited ETH to restaking tokens and mint to the caller.
        amountOut = convertFromUnitOfAccountToRestakingTokens(msg.value);

        // Forward ETH to the deposit pool.
        address(depositPool()).transferETH(msg.value);

Alice wants to deposit 1 ETH (1e18), the current protocol TVL is 35e18, and the total supply of ETH is 12e25.

To calculate the amountOut:

function convertFromUnitOfAccountToRestakingTokens(uint256 value) public view returns (uint256) {
    uint256 tvl = getTVL();
    uint256 supply = token.totalSupply();

    if (supply == 0) {
        return value;
    }
    return value * supply / tvl;
}

1e18 * 12e25 / 1500e18.
The amount of shares she will receive is 1.2e24
If she was frontrun by a large transaction of let's say 200 ETH
The amount would be: 1e18 * 12e25 / 1700e18 = 7e22.

Impact

LRT tokens received in exchange for the underlying assets can vary and lead to unwanted outcomes due to the price dependency on the TVL as well as the amount of tokens received by the user is determined by an interaction with an oracle, meaning that the amount received in return may vary indefinitely while the request is waiting to be executed. This is due to a lack of slippage control on any of the deposit / withdrawal functions.

Code Snippet

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

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/main/rio-sherlock-audit/contracts/restaking/RioLRTCoordinator.sol#L162-L169

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

Tool used

Manual Review

Recommendation

Include minimumOut parameters and maybe a deadline as well to enforce slippage control to the deposit/withdraw transactions in order to prevent unwanted outcomes.

nevillehuang commented 7 months ago

Borderline medium/low leaving open for discussion.

solimander commented 7 months ago

Consider low severity - oracle updates of underlying assets and reward distributions are unlikely to cause a meaningful change to the output amount.

nevillehuang commented 6 months ago

I believe this should have been a known consideration stated in the contest details, so leaving as medium severity

KupiaSecAdmin commented 6 months ago

Escalate

This should be considered as Invalid, because:

The rate between shares and TVL is not affected much during the front-running period.
From above PoC, after 200ETH is deposited by front-running, the submitter didn't consider that total share amount is also increased before Alice tries to deposit 1ETH, which results in significant change in calculation.

sherlock-admin2 commented 6 months ago

Escalate

This should be considered as Invalid, because:

The rate between shares and TVL is not affected much during the front-running period.

From above PoC, after 200ETH is deposited by front-running, the submitter didn't consider that total share amount is also increased before Alice tries to deposit 1ETH, which results in significant change in calculation.

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.

ammarvoloder commented 6 months ago

@KupiaSecAdmin I think you are wrong. It does not have to be the front-running attack at all.

The amount of RioLRT tokens (amountOut) received by the user is determined by interacting with an oracle meaning that the amount received in return may vary indefinitely while the request is waiting to be executed (in the mempool). So the price fluctuation alone can cause loss for the user.

Besides, minShares checks are also commonly implemented in staking protocols.

0xcats commented 6 months ago

Agree with the said above, front-running does not need to take place at all.

pronobis4 commented 6 months ago

As said above, it's not about front running, it's about the lack of slippage and the amount that may be significantly different than expected.

nevillehuang commented 6 months ago

I agree with watsons, front-running is not required, and since price fluctuations is out of users control, and if significant, can cause users to receive lesser shares than intended.

Czar102 commented 6 months ago

Why is there an expectation for the deposit to be according to any certain exchange rate?

If the price changes, so should the number of shares. If the user uses an asset that is not stable, and doesn't limit the slippage externally, or doesn't ensure timely execution, it would be their fault. If it can't be exploited, I don't see how would this be an issue. See the sponsor's comment:

Consider low severity - oracle updates of underlying assets and reward distributions are unlikely to cause a meaningful change to the output amount.

cc @ammarvoloder @0xcats @pronobis4 @nevillehuang

0xcats commented 6 months ago

Why is there an expectation for the deposit to be according to any certain exchange rate?

If the price changes, so should the number of shares. If the user uses an asset that is not stable, and doesn't limit the slippage externally, or doesn't ensure timely execution, it would be their fault. If it can't be exploited, I don't see how would this be an issue. See the sponsor's comment:

Consider low severity - oracle updates of underlying assets and reward distributions are unlikely to cause a meaningful change to the output amount.

cc @ammarvoloder @0xcats @pronobis4 @nevillehuang

I raised the issue since there is currently no protection for the user, while 'minShares' checks are commonly implemented in staking contracts for slippage control. Also in the past has been a valid issue in contests.

solimander commented 6 months ago

I raised the issue since there is currently no protection for the user, while 'minShares' checks are commonly implemented in staking contracts for slippage control. Also in the past has been a valid issue in contests.

minShares checks would have been implemented if this were an AMM (or similar), but they weren't as other parties cannot cause higher slippage.

ammarvoloder commented 6 months ago

Although it might not cause a direct loss to the protocol itself, price manipulations can happen and these could cause a damage to the users since there is no slippage protection.

In my opinion, a slippage check should not only be relevant if there is a chance of protocol exploit but also for the user protection. According to Sherlock some slippage issues can also be considered as High and as @0xcats said this has also been a valid issue in the past and it is a valid issue on every other platform. @Czar102 Based on your argument, there is no need for slippage checks in any protocol where users define the minimum amount of shares or underlying assets because it is their fault if the asset is not stable?

Czar102 commented 6 months ago

Given that a loss can't be inflicted on users by other users, I don't see how does this report exceed Low severity.

@ammarvoloder if there are price manipulation exploits, they should have been mentioned in the report. If that's the case, please let me know.

In AMMs any user can cause other user to make a trade at a high slippage, so slippage controls are needed for user protection. Here, no user can make other user have high slippage, so there is no need to protect users from other users. Not protecting users from the protocol is not a vulnerability, an improvement at most, especially in this case.

Arguing about historical decisions or other platforms is pointless, as explicitly stated in the rules. These arguments hold no weight.

I'm planning to accept the escalation and invalidate the issue.

pronobis4 commented 6 months ago

The price returned by chainlink may fluctuate +/-2% from the price that is on the market, this is what a slippage should be for. The user has no control over how many shares he or she will receive. So, when sending his tokens, he can only assume that his tokens will be converted at a good price, but if the price from Oracle is, for example, 2% different from the one available everywhere else, he will lose this 2% and he will have no control over it. The front end will probably show the price at which it will be converted, but this does not mean that the price will still be the same when the tokens are sent. See an example here: https://github.com/Kelp-DAO/LRT-rsETH/blob/e75e9ef168a7b192abf76869977cd2ac8134849c/contracts/LRTDepositPool.sol#L168

Czar102 commented 6 months ago

A default 2% slippage setting is not a vulnerability, especially when not exploitable by any user.

Planning to accept the escalation and consider this issue a valid Low.

Czar102 commented 6 months ago

Result: Low Has duplicates

sherlock-admin3 commented 6 months ago

Escalations have been resolved successfully!

Escalation status:

KupiaSecAdmin: accepted

sherlock-audit / 2024-02-rio-network-core-protocol-judging

iamandreiski - Lack of slippage parameters can affect the LRT / share amounts during deposits / withdrawals #214

Lack of slippage parameters can affect the LRT / share amounts during deposits / withdrawals

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation