Intrinsic arbitrage between assets due to price feed deviation threshold

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTOracle.sol#L66-L78

Vulnerability details

Impact

Withdrawals have not yet been implemented but I assume it will be implemented in the usual way such that the fraction of total supply of rsETH a user redeems gives him an equal fraction of total assets held, i.e. received = sharesToRedeem * totalAssets / totalShares. Thus one should be able to withdraw at most what one deposited.
The issue described here is that this invariant does not hold; the minted amount of rsETH may represent a greater value than just deposited. If we imagine that one could immediately withdraw, this means that one can deposit and immediately withdraw for profit.

Proof of Concept

We have something like an ERC4626 vault, where the minted shares are calculated as depositAmount * totalShares / totalAssets. The complication here is that we have multiple assets so we cannot use a single balance, but must assign a single value to the held balances of all assets, i.e. depositAmount * totalShares / totalETHValue.
This is indeed how the number of shares to mint is calculated in our case:

rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice();

where lrtOracle.getAssetPrice(asset) is the price given by an oracle (so far only Chainlink is implemented), and where lrtOracle.getRSETHPrice() is totalETHInPool / rsEthSupply where totalETHInPool is the sum of the value of all assets balances. Thus rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) * rsEthSupply / totalETHInPool.

This is correct in theory, but in practice the price feeds are slightly inaccurate. Chainlink has a deviation threshold of up to 2% or so, which means that it will not update the price unless the true price deviates at least 2%. Thus the price feeds may be off by up to 2%.

Note that for a normal single asset vault the price doesn't matter; it is merely a conversion factor, any discrepancy of which cancels out. It only matters that it be consistent. But when we have multiple assets the price does matter and must be carefully chosen such that one cannot profit by depositing.

Suppose there are two supported assets TIK and TOK.
The pool holds 100 TIK and 0 TOK and has minted 100 rsETH.
The true value of each is currently 1 ETH.
The oracle reports this exactly as 1 for TIK, but as 1.02 for TOK.
A user deposits 100 TOK, which he got for 100 ETH. He is thus minted 100 * 1.02 * 100 / (100 * 1 + 1.02 * 0) = 102 rsETH. However this is of course staked according to its true values.
He now owns 102 out of a total of 202 shares of the assets. If he were to withdraw he would get 102 rsETH * 200 ETH / 202 rsETH = 100.99 ETH, i.e. a profit of 0.99 ETH.

This is not simply because the price is wrong. Note what would have happened if the pool held 0 TIK but 100 TOK instead. Then he would get 100 * 1.02 * 100 / (0 * 1 + 100 * 1.02) = 100 rsETH. And redeem 100 rsETH for 100 * 200 / 200 = 100 ETH. It cancels out.

What happens is that the price discrepancy creates an arbitrage between the assets. It is as if the depositor can deposit TOK, have the protocol trade it for TIK according to its own oracle given price, but redeem 50 TIK and 50 TOK from the real market. He deposits one asset but effectively gets back a distribution of other assets.

Recommended mitigation steps

The contract cannot know the true prices, and the oracle only gives approximate prices. But we do know that the true prices fall within certain ranges. We need to reduce the mint amount, within these ranges, such that no arbitrage is possible.

Assessed type

ERC4626

[Rassska]

dup of #584

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #284

[c4-pre-sort]

raymondfam marked the issue as duplicate of #584

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-judge]

fatherGoose1 marked the issue as unsatisfactory:
Invalid

[c4-judge]

fatherGoose1 marked the issue as satisfactory