search

code-423n4 / 2023-11-kelp-findings

13 stars 11 forks source link

`stETH/ETH`, `rETH/ETH` and `cbETH/ETH` chainlink oracles has too long of heartbeat and deviation threshold which can cause loss of funds #865

Closed c4-submissions closed 11 months ago

c4-submissions commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/oracles/ChainlinkPriceOracle.sol#L17

Vulnerability details

ChainlinkPriceOracle fetches prices from the Chainlink contracts. But the price feeds in the consideration has a very long price heartbeat and deviation rate which might lead to wrong price calculation and loss of token to the user.

Impact

According to the chainlink docs, following are the Price feeds info for the assets that will be used initially:

Feed Heartbeat Deviation Docs link
RETH / ETH 2% 86400s Docs
CBETH / ETH 1% 86400s Docs
RETH / ETH 0.5% 86400s Docs

For Info, Heartbeat and Deviation are variables on which the update of the prices for price feeds depends. That mean the current price feed price will be updated for the token only if one of them is met. That means if we take the example of RETH here, the price feed will be update only when the market price for the RETH fluctuates by 2% or time equal to 86400s(1 day) has passed since last price update. This could be a very problematic thing. Let's assume a following scenario:

-> Let's say price feeds was updated at time T1 and price for RETH / ETH is [1]. And since then 43200s(half day) has passed and price has become [1.018] (up by 1.8 %). -> Now Bob comes and decides to deposit 50 RETH by calling LSTDepositPool::depositAsset(...). -> But the amount of RSETH token he will get is equal to:

depositAmount = 500 RETH

// balances
bob = 500 RETH
LRTDepositPool = 0
EigenStrategy = 0
NodeDelegator = 0

// RETH Price
RETHPrice = 1e18 // hasn't updated because heartbeat and deviation hasn't reached

// total ETH (assuming zero for the sake of simplicity)
totalETHInPool = 0
RSETHSupply = 0

///////////////////////////////////////////////////////////////
// functions calculations according to chainlink price feeds //
///////////////////////////////////////////////////////////////

rsETHPrice(...) = if -> 
                      (supply = 0) 1 ETH // this condition will met
                  else -> 
                      totalETHInPool / rsETHTotalSupply

rsEthAmountToMint(...) = depositAmount * RETHPrice / rsETHPrice()
                  = 500 * 1e18 / 1e18 = 500 rsETH

/////////////////////////////////////////////////////////
/// functions calculations according to market price ////
/////////////////////////////////////////////////////////

rsETHPrice(...) = if -> 
                      (supply = 0) 1 ETH // this condition will met
                  else -> 
                      totalETHInPool / rsETHTotalSupply

rsEthAmountToMint(...) = depositAmount * RETHPrice / rsETHPrice()
                  = 500 * (1.018 * 1e18) / 1e18 = ~509 rsETH

As you can see according to the market price, 9 rsETH should be minted extra to the user. But it will not happen as the price feed deviation and heartbeat threshold hasn't passed. And that is why the prices will not be updated. Even if the checks are added in the future for staleness of price, this mechanism will not let anybody deposit token for a whole day because of staleness checks like maxTimestamp etc.

This is one of the valid issue founded in a sherlock audit. Here is a link: Link

Proof of Concept

Calculation and Link given in the above section.

Tools Used

Recommended Mitigation Steps

It is recommended to use price feeds with less heartbeat and deviation rate. Although the deviation can be handled the heartbeat is still a thing that needs to be considered. Since the time of writing this, Chainlink only support few price feeds for the above tokens with high heartbeat and deviation rate. It is recommended to go for other price oracles or combine more than one together to get the average price.

Assessed type

Oracle

c4-pre-sort commented 11 months ago

raymondfam marked the issue as sufficient quality report

c4-pre-sort commented 11 months ago

raymondfam marked the issue as duplicate of #609

c4-judge commented 11 months ago

fatherGoose1 marked the issue as unsatisfactory: Invalid

Aamirusmani1552 commented 11 months ago

Hey @c4-judge , Sorry but I don't agree with the decision. I have gone through the other duplicate at #609 but not satisfied with the answer. I think it's not a good idea to assume that the rate remains stable and it will never go near that deviation rate. The LST to ETH conversion rate will affect the original amount of rsETH to mint a lot even if it changes little bit. And going with 2% deviation rate is going to be a loss for the user. So if cbETH / ETH conversion is 1. And it changed to 1.01. The RSETH that user get will be equal to ~2106 usd according to current price. But it should be ~2124 usd with the right conversion rate. And that is when we consider only 1 percent deviation. I don't know why team is ready to go with this. And even bad thing is the oracle will keep sending the same price until the 24 hour heartbeat has passed.

c4-judge commented 11 months ago

fatherGoose1 marked the issue as duplicate of #584

c4-judge commented 11 months ago

fatherGoose1 marked the issue as satisfactory

c4-judge commented 10 months ago

fatherGoose1 changed the severity to 3 (High Risk)

d3e4 commented 10 months ago

It is not an issue that the user is minted more or less rsETH as described here. Whether it is 500 or 509 it will grant the depositor the right to withdraw the same amount as he deposited. It is simply a different conversion factor. The issue in #584 arsises when there are multiple underlying assets. Here no such argument is provided, and the scenario described here leads to no loss.

Aamirusmani1552 commented 10 months ago

Hey @d3e4, first of all I am aware of that there will be 3 underlying assets in the beginning. I have just given an example above for a single asset for the sake of simplicity. Secondly as you said in your issue there will be arbitrage opportunity, tell me how it would be possible. Wouldn't it be because of the price deviation? What you have shown in your issue is just little bit more detailed version of the same Issue I mentioned here. You took an example of profit from it whereas I stated loss. There is no difference here. You have given good examples of how this price deviation could be an issue and that is why your issue has been selected for the final report.

d3e4 commented 10 months ago

Why then did you not mention the fact that there will be three assets? This is critical. When there is one asset there is no loss. So this is not a valid example and as such your argument demonstrates no loss.

(Also, #584 is not my report)

Aamirusmani1552 commented 10 months ago

Dude just have a look at LRTConfig::initialize() Screenshot_2023-12-09-16-42-40-25

It's the first thing that anyone will look at. Everybody knows these will be the initial ones. I don't even have to give any other example here. But here are some others:

First of all this is the first thing they tells in the docs that these three will be the used in the beginning. So mentioning them again doesn't make any sense. Secondly all of the tests are written with these tokens. And thirdly why would i even mention the deviation and heartbeat for all three of them in the above issue. I should've just mentioned the one that is in the examples. don't know why are you so desperate to make other ones invalid. Don't want to argue anymore. I rest my case here. I am sure judges will make the right decision.

d3e4 commented 10 months ago

Even something evident must be explicitly invoked if it is to form the premise of an issue. You cannot just quote the code in retrospect and say you didn't have to mention something because it is clearly there in the code. Isn't the point of a report to point out which specific parts lead to which specific impact, and how? The argument you provided did not include, either implicitly or explicitly, this premise and is therefore not valid. You would have to both include the premise and amend your argument accordingly to show a loss.

Aamirusmani1552 commented 10 months ago

That means if we take the example of RETH here, the price feed will be update only when the market price for the RETH fluctuates by 2% or time equal to 86400s(1 day) has passed since last price update. This could be a very problematic thing. Let's assume a following scenario:

First of all read this. I didn't say that only RETH price feed will cause this issue. I have clearly mentioned that it was an example. And let's just keep this thing aside that we haven't mentioned all of the assets for now. If we just take an example of RETH here, the issue still exist. It doesn't depend on all of the assets prices. The calculation above clearly shows this as the dollar value of the assets will be used to calculate the price of RSETH to mint. And it will cause loss when the deviation and heartbeat hasn't met. And also the core issue is same. Let's just avoid this conversation further and wait for the judge's reply.

d3e4 commented 10 months ago

You calculations show the following. Let's say there are two deposits of 500 rETH. If the price is 1 but is incorrectly given as 1.018 then the first deposit will mint 509 rsETH. But the second deposit will also mint 509 rsETH because depositAmount * RETHPrice / (totalETHInPool / rsETHTotalSupply) = 500 * 1.018 / (1.018 * 500 / 509) = 509. The price cancels out. This means both depositors have equal shares of the pool, which is correct since they both deposited equal amounts. So there is no loss. It doesn't matter whether they are minted 500 or 509 rsETH when there is only a single asset. When there are multiple assets the price does not cancel out, and this must be shown.

Aamirusmani1552 commented 10 months ago

You have stated the example in wrong way. I said if the actual price 1.018 but the update of price feed will not happen due to set deviation. It will only change after 24 hours in that case. So if the old price is 1 then it is surely the loss to the user. After the heartbeat the price will become normal again and giving other users tokens at 1.018 that should be also the case for previous depositor. And I never said that this will happen during first and second deposit. It can happen anytime. Now the opposite is also true. If the price has become less and the price feed update hasn't done then the update will not happen again and user will get RSETH at less price than actual giving the opportunity for arbitrage(in your case). And this price will change after 24 hours so it surely is same issue as yours.

d3e4 commented 10 months ago

It seems you are thinking of the issue in #443 then? That is valid, but a different issue. But in that case it is critical to mention backrunning the price update.

Aamirusmani1552 commented 10 months ago

Although it doesn't make sense to again comment on the issue since rewards has been announced. I would do it one last time just to answer the question. The issue is not similar to 443 at all. In that issue he talks about getting benefits by frontrunning the price update of price oracle. First of all It's only possible when you know when exactly the price is going to change and that is the tricky part. And once it is changed there is no way to front-run it. So I don't think frontrunning is possible. What my issue is can be summarized like this:

That is all what I meant to say above. Now why I say that selected issue is similar like mine is because, in that the other auditor went with the arbitrage. That is when the actual price is low but the price feed is showing high LST prices. That mean a user can deposit more LSTs and get more RSETH than the actual price giving the opportunity for arbitrage. This arbitrage opportunity is only there until the price update hasn't happened. My example and his example are not different in any sense. They are complementary. I have shown an example of loss while he has shown for the profit.

Please refrain from commenting any further, while I and you may not have lot on our plates, but judges and other team members do have a lot. And posting these comments only sending them unnecessary notifications.

d3e4 commented 10 months ago

I see what you mean but I think what you are missing is the distinction between a direct profit and a regular trading profit. If a profit is made because of a price increase then this is a legitimate trading profit. The only thing that must not happen with regards to how much is minted is such that one could not immediately withdraw for a profit, otherwise it is just a normal trading profit, or at least not any more exploitable than the normal market mechanics.

If the price change is due to an inaccurate oracle, then this still effects everyone equally, so it is not exploitable. It does cancel out, if you also take into account the other holders who equally can choose to withdraw.

But you say in regards to #443 that it would be possible to frontrun the price update only if you know exactly when it is going to happen, and that this is not possible and that such a frontrunning is impossible. This is not how frontrunning works. The mempool is continuously monitored and the transaction will be spotted before it is executed, so one can indeed know exactly when it will happen in the sense that one makes sure one's own transaction is executed just before. The loss due to the price deviation you describe is the same as the loss described in #443 except that in your case it does not happen in connection to a front/backrunning.

What you describe is only exploitable, in the sense that anyone gets an unfair advantage, if you can perform this knowing about the impending price update. The only way this can be executed is through front/backrunning, or perhaps predicting the heartbeat update. So there may be a slight distinction in that sense between your issue and #443, but they are sufficiently similar, and still distinct from #584.

Again, and this is the main point, a price deviation does imply the loss you say, but it is indistinguishable from a genuine price change, and is thus fair and may be taken for the real price, except by means of an exploit like #443.