search

sherlock-audit / 2023-11-olympus-judging

9 stars 7 forks source link

KupiaSec - Price calculation can be manipulated by intentionally reverting some of price feeds. #127

Open sherlock-admin opened 6 months ago

sherlock-admin commented 6 months ago

KupiaSec

high

Price calculation can be manipulated by intentionally reverting some of price feeds.

Summary

Price calculation module iterates through available price feeds for the requested asset, gather prices of non-revert price feeds and then apply strategy on available prices to calculate final asset price. By abusing this functionality, an attacker can let some price feeds revert to get advantage from any manipulated price feed.

Vulnerability Detail

Here we have some methods that attackers can abuse to intentionally revert price feeds.

  1. UniswapV3 price feed UniswapV3Price.sol#L210-214 
    
// Get the current price of the lookup token in terms of the quote token
(, int24 currentTick, , , , , bool unlocked) = params.pool.slot0();

// Check for re-entrancy if (unlocked == false) revert UniswapV3_PoolReentrancy(address(params.pool));

In UniswapV3 price feed, it reverts if current state is re-entered.
An attacker can intentionally revert this price feed by calling it from UniswapV3's callback methods.

2. Balancer price feed
[BalancerPoolTokenPrice.sol#L388](https://github.com/sherlock-audit/2023-11-olympus/blob/9c8df76dc9820b4c6605d2e1e6d87dcfa9e50070/bophades/src/modules/PRICE/submodules/feeds/BalancerPoolTokenPrice.sol#L388)
[BalancerPoolTokenPrice.sol#487](https://github.com/sherlock-audit/2023-11-olympus/blob/9c8df76dc9820b4c6605d2e1e6d87dcfa9e50070/bophades/src/modules/PRICE/submodules/feeds/BalancerPoolTokenPrice.sol#L487)
[BalancerPoolTokenPrice.sol#599](https://github.com/sherlock-audit/2023-11-olympus/blob/9c8df76dc9820b4c6605d2e1e6d87dcfa9e50070/bophades/src/modules/PRICE/submodules/feeds/BalancerPoolTokenPrice.sol#L599)
[BalancerPoolTokenPrice.sol#748](https://github.com/sherlock-audit/2023-11-olympus/blob/9c8df76dc9820b4c6605d2e1e6d87dcfa9e50070/bophades/src/modules/PRICE/submodules/feeds/BalancerPoolTokenPrice.sol#L748)
```Solidity
// Prevent re-entrancy attacks
VaultReentrancyLib.ensureNotInVaultContext(balVault);

In BalancerPool price feed, it reverts if current state is re-entered. An attacker can intentionally revert this price feed by calling it in the middle of Balancer action.

  1. BunniToken price feed BunniPirce.sol#L155-160 
    _validateReserves(
_getBunniKey(token),
lens,
params.twapMaxDeviationsBps,
params.twapObservationWindow
);

    In BunniToken price feed, it validates reserves and reverts if it doesn't satisfy deviation. Since BunniToken uses UniswapV3, this can be intentionally reverted by calling it from UniswapV3's mint callback.

Usually for ERC20 token prices, above 3 price feeds are commonly used combined with Chainlink price feed, and optionally with averageMovingPrice. There are another two points to consider here:

  1. When average moving price is used, it is appended at the end of the price array. OlympusPrice.v2.sol#L160 
    if (asset.useMovingAverage) prices[numFeeds] = asset.cumulativeObs / asset.numObservations;
  2. In price calculation strategy, first non-zero price is used when there are 2 valid prices: getMedianPriceIfDeviation - SimplePriceFeedStrategy.sol#L246 getMedianPrice - SimplePriceFeedStrategy.sol#L313 For getAveragePrice and getAveragePriceIfDeviation, it uses average price if it deviates.

Based on the information above, here are potential attack vectors that attackers would try:

  1. When Chainlink price feed is manipulated, an attacker can disable all three above price feeds intentionally to get advantage of the price manipulation.
  2. When Chainlink price feed is not used for an asset, an attacker can manipulate one of above 3 spot price feeds and disable other ones.

When averageMovingPrice is used and average price strategy is applied, the manipulation effect becomes half: $\frac{(P + \Delta X) + (P)}{2} = P + \frac{\Delta X}{2}, P=Market Price, \Delta X=Manipulated Amount$

Impact

Attackers can disable some of price feeds as they want with ease, they can get advantage of one manipulated price feed.

Code Snippet

https://github.com/sherlock-audit/2023-11-olympus/blob/9c8df76dc9820b4c6605d2e1e6d87dcfa9e50070/bophades/src/modules/PRICE/OlympusPrice.v2.sol#L132-L184

Tool used

Manual Review

Recommendation

For the cases above that price feeds being intentionally reverted, the price calculation itself also should revert without just ignoring it.

nevillehuang commented 6 months ago

Invalid, if a user purposely revert price feeds, they are only affecting their own usage, not the usage of price feeds for other users transactions.

KupiaSecAdmin commented 6 months ago

Escalate

Hey @nevillehuang - Yes, exactly you are right. What an attacker can manipulate is a spot price using flashloans, so if an attacker purposely disable other price feeds but only leave manipulated price feed, there happens a vulnerability that an attacker can buy tokens at affected price.

sherlock-admin2 commented 6 months ago

Escalate

Hey @nevillehuang - Yes, exactly you are right. What an attacker can manipulate is a spot price using flashloans, so if an attacker purposely disable other price feeds but only leave manipulated price feed, there happens a vulnerability that an attacker can buy tokens at affected price.

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.

nevillehuang commented 6 months ago

@KupiaSecAdmin, All of your scenarios are invalid

  1. There is no point for somebody to reenter to explicity cause a revert for using the price feed himself
  2. Same reason as 1.
  3. There is no point for somebody to cause a deviation to explicity cause a revert for using the price feed himself
  4. A user cannot manipulate a chainlink price feed since there are no reserves

This is on top of the fact that price submodules are not intended to be called directly, but via the primary price module mentioned in this comment here

KupiaSecAdmin commented 6 months ago

@nevillehuang - For example, you can manipulate spot price of Uniswap. To make this work, you need to make other price feeds revert because if they are all enabled, average/median price strategy will be taken and manipulated spot price will not take effect.

nevillehuang commented 6 months ago

@KupiaSecAdmin you cannot make other feeds revert for other user, only yourself, and your submission certainly doesn't prove that it is possible. Besides, to manipulate spot price in uniswap, you will have to manipulate the reserves, which is a known issue in the contest and out of scope.

KupiaSecAdmin commented 6 months ago

@nevillehuang - I would like to add some notes and scenarios below that I think might be attack vectors. @0xJem - I would be happy to get some feedbacks from the protocol team regarding the issue.

[Notes]

  1. (I believe) This price module will be used in other parts of Olympus protocol to determine fair price of OHM(and other ERC20 tokens) at any time by integrating multiple price feeds and applying a strategy(average or median) to different prices to carry out final fair price.
  2. The carried out final price will be used to buy/sell OHM tokens using other collaterals in other modules of Olympus protocol.

[Scenario]

  1. Let's assume that an attacker can manipulate a spot price of one price feed, e.g. Uni2, Uni3, Bunni. It can not be guaranteed that all spot price feeds work correctly.
  2. As a result, we can assume that the attacker can manipulate OHM price of one price feed to $9(for example by manipulating Bunni).
  3. However, multiple price feeds are used to calculate fair OHM price, for example, 3 strategies can be used to determine fair OHM price: Chainlink, Uniswap3, Bunni. Thus assume Chainlink returns $11.1 and Uniswap3 returns $11.05 for OHM price.
  4. The price strategy takes median strategy, this means manipulating Bunni price feeds does not take effect on final OHM price determination because the median price of ($9, $11.05, $11.1) is $11.05 which could be accepted as fair OHM price.
  5. Now, the attacker can intentionally make Uniswap 3 price feed reverting using re-entrancy.
  6. When this happens, the only available price feeds are Chainlink and Bunny which are $9 and $11.1. Median price strategy is applied to these feeds thus returning $10 as OHM price, which is affected and this could result in attacker can buy more OHM tokens than expected.

[Thoughts] Price feeds can revert for any reason by accidents so it would actually make sense using try/catch to ignore reverted price feeds. However, price feeds being reverted because of re-entrancy check can not be considered as accidents because it's intentional and unusual behavior. So I think it's the right behavior to revert price calculation itself as a whole when any price feed is reverted by re-entrancy check.

[Claims] @nevillehuang - You were mentioning that I can not make other feeds revert for other users but only for myself. Yes, that's right. An attacker will let some price feeds revert only for himself(and only within a single transaction, they should work fine in other transactions), and it is to manipulate final fair price of tokens regardless of whatever strategy is taken.

nevillehuang commented 6 months ago

@KupiaSecAdmin Can you provide a coded PoC for your above scenario? I really don't see how step 5 can occur, given price feeds are utilized in separate transactions? How would one users price feed reverting affect another?

  1. Now, the attacker can intentionally make Uniswap 3 price feed reverting using re-entrancy.
KupiaSecAdmin commented 6 months ago

@nevillehuang @0xJem - Here's a PoC that shows how price can be manipulated. You can put this test file in same test directory with PRICE.v2.t.sol. https://gist.github.com/KupiaSecAdmin/fc7ef6664b191ab2b758a22ab15bf404

Running test: forge test --fork-url {{MAINNET_RPC}} --fork-block-number 19041673 --match-test testPriceManipulation -vv

Result:

[PASS] testPriceManipulation() (gas: 2299239)
Logs:
  Before: Chainklink/Olympus 6294108760000000000 6308514491323687440
  After: Chainklink/Olympus 6294108760000000000 29508079057029841191

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.69s

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

[Scenario]

  1. It calculates UNI price using mainnet's forked data.
  2. It is assumed that Olympus uses UniV2 and UniV3 price feeds for calculating UNI price.
  3. The test manipulates UniV2 price and intentionally reverts UniV3 price feed, thus the final price is same as manipulated UniV2 price.

[Focus]

  1. Even though test shows price manipulation is done via reserves, but reserve manipulation is not the only way of manipulating price, as Olympus integrates further more price feeds and based on protocols.
  2. The main point to show from the issue and PoC is that intentionally reverting some price feeds is dangerous because that can be a cause of price manipulation.
0xJem commented 6 months ago

@Oighty can you weigh in on the risk of a third-party deliberately triggering the re-entrancy lock in the UniV3 pool?

To me, this represents a misconfiguration of the asset price feeds.

If it was a single price feed (UniV3) only, it would be fine, as the price lookup would fail. It's because there's a UniV2 pool in use that this could be susceptible to the price manipulation as you described. However, this feels unlikely because:

If we were to have UNI defined as an asset, we would be more likely to do this:

Given the difficulty of manipulation both sources, and the deep liquidity of the UniV3 pool ($31.65m), we'd be confident that it would be resilient enough.

nevillehuang commented 5 months ago
Czar102 commented 5 months ago

I think this is a really nice finding if true, kudos for the thought process @KupiaSecAdmin!

Since price manipulation itself is out of scope, but the expectation of using multiple price sources should make the price more difficult to manipulate, and because of the bug, the breakdown value falls drastically. Thus I believe it deserves to be a valid Medium.

I'm not sure about the point above, @0xJem could you explain why would such setup be a misconfiguration? From my understanding, any setup using any of these 3 oracles and any other one will be susceptible to manipulation.

nevillehuang commented 5 months ago

@Czar102 Some questions:

  1. Is there anywhere it was indicated that the above uni pools would be used as price feeds? Given the watson made an assumption:

assumed that Olympus uses UniV2 and UniV3 price feeds for calculating UNI price.

  1. Isn't the additional data provided by the watson still related to manipulation of reserves and like you said out of scope? To me he still hasn't prove that there is any other cause other than manipulating reserves other than stating a possibility? Would be nice if he can prove this issues above scenario of 1 and 2 (reentrancy triggering affecting price feed of other users?)

  2. Dont Olympus use an internal MA to mitigate risk of reserve manipulation?

0xJem commented 5 months ago

I'm not sure about the point above, @0xJem could you explain why would such setup be a misconfiguration? From my understanding, any setup using any of these 3 oracles and any other one will be susceptible to manipulation.

Czar102 commented 5 months ago

@nevillehuang I believe the assumption you are mentioning in point 1 is just an example and the different price feeds could be anything, like Uni v3 + Uni v3 – one could manipulate one of these and make the other revert, for example.

Regarding point 2, I don't think the crux here is the manipulation of reserves, they may be just off with respect to each other. The point is that the attacker can selectively decide which sources of information to use, impacting the final price reading. The point of using multiple feeds is to make the price more reliable, and they are being made less reliable if you can make the readings be rejected.

Regarding point 3, I believe you could repetitively make the price pass sanity checks, making it exponentially diverge from the real price.

Regarding @0xJem's points: I believe simply not using a Uni v2 pool doesn't mitigate this. Using any of the dexes mentioned above together with any feed will have this impact. So, a Chainlink feed + Uni v3 pool could be exploited in a way that the Uni v3 reading will revert and only Chainlink feed will be used, which may benefit the attacker in a certain way.

Has the approach for creating these safe setups been shared with Watsons anywhere? Am I misunderstanding something? @0xJem @nevillehuang @KupiaSecAdmin

nevillehuang commented 5 months ago

@Czar102

  1. In case of non-obvious issues with complex vulnerabilities/attack paths, Watson must submit a valid POC for the issue to be considered valid and rewarded.

TLDR, unless the watson or YOU provide sufficient proof (best with a PoC) that it is economically possible/profitable, I’m not convinced this is a valid issue since you are just simply stating possibility. Please only consider the original submission only and see if it has sufficient information in place during the time when Im judging this.

Hash01011122 commented 5 months ago

IMO In my opinion, while the precise impact of the potential attack isn't crystal clear, the mentioned attack path, extending up to price manipulation, significantly expands the attack surface. This broader surface introduces multiple avenues for potential attacks that may not be immediately apparent. I find @nevillehuang's comment lacking in persuasiveness, on how this issue should be considered as invalid after watson submitted the PoC. With a clear attack impact, Watson's submission should be rated as High severity. Watson's failure to articulate how the identified issue could result in a loss of funds for the protocol is crucial. But the issue highlights numerous ways the core functionality of the contract could be exploited, making it a valid medium-severity concern.

nevillehuang commented 5 months ago

@Hash01011122, stating the possibility of an issue and proving it are two separate things. Can you look at the details provided in the issue and tell me with at least 80% confidence rate that it is valid without additional research by the judge to prove its validity when its not the case?

For example, the watson is simply stating "user can cause reentrancy" with a single one liner type comment without any code description/POC (there are multiple instances throughout the issue)? How am I suppose to verify that? I am a firm believer that burden of proof is on the watson not the judge, and I believe sherlock also enforces this stance.

The fact that Head of judging and sponsor has to come in and supplement the non-obvious finding of the watson certainly doesn't help too, and I believe this will be resolved in the future now that we have the request poc feature, but I believe as of contest date, the information provided in the ORIGINAL submission is insufficient to warrant its severity other than low/invalid.

Czar102 commented 5 months ago

I understood the finding when I haven't read a half or it. I think the only thing that needs to be verified is that a revert in price reading will cause the price to be computed based on other sources.

Selective manipulation of sources of information defeats the purpose of sourcing the data from many sources – instead of increasing security, the data will be pulled from potentially least safe sources.

I think it warrants Medium severity.

nevillehuang commented 5 months ago

@Czar102 ok got it I put it on myself for not having the knowledge u possess to understand this issue. I will let you decide once you decide what @0xJem considers. Again understanding and proving to issue is two separate issues for debate.

Czar102 commented 5 months ago

Result: Medium Unique

sherlock-admin2 commented 5 months ago

Escalations have been resolved successfully!

Escalation status: