Possible arbitrage from Chainlink price discrepancy

[c4-submissions]

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:

• N/A

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

[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 not a duplicate

[c4-pre-sort]

raymondfam marked the issue as high quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-sponsor]

gus-staderlabs (sponsor) disputed

[gus-stdr]

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.

[c4-judge]

fatherGoose1 marked the issue as unsatisfactory:
Invalid

[c4-judge]

fatherGoose1 marked the issue as satisfactory

[c4-judge]

fatherGoose1 marked the issue as selected for report

[fatherGoose1]

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.

[PlamenTSV]

Would issues like
#127
and
#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.

[d3e4]

@fatherGoose1 Please take what is said in this comment into account as well: #584 (comment). 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.

[PKbubu]

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.

[Rassska]

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!

[PKbubu]

@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!

[d3e4]

@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:

1. The price oracle might deviate from the true price.
2. The price deviation leads to an incorrect amount of rsETH minted.
3. The price update can be backrun to make a profitable trade (as if predicting the market).
4. 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.

[0xnirlin]

@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.

[d3e4]

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.