Potential flash loan attack vulnerability in `get_price_v1` functions of CurveOracle

[hats-bug-reporter[bot]]

Github username: -- Submission hash (on-chain): 0x2c4be5daeef51c5489f6a3dc8bcd7bf127846e08e1a164b30ec4add731f25943 Severity: high severity

Description:

summary

During a security review of the getPrice function in the CurveOracle, a potential flash loan attack vulnerability was identified.

Vulnerability Detail

The get_price_v1 functions retrieves the spot price of each token in a Curve LP pool, calculates the minimum price among them, and multiplies it by the virtual price of the LP token to determine the USD value of the LP token. If the price of one or more tokens in the pool is manipulated, this can cause the minimum price calculation to be skewed, leading to an incorrect USD value for the LP token. This can be exploited by attackers to make a profit at the expense of other users.

Impact

This vulnerability could potentially allow attackers to manipulate the price of tokens in Curve LP pools and profit at the expense of other users. If exploited, this vulnerability could result in significant financial losses for affected users.

Code Snippet

https://github.com/hats-finance/VMEX-0x41547b88e8d46bfdb6327c0c3ab4b5a1cffb11cd/blob/4bc577756e15418b919a49a1bdec0a98fd39bdbd/packages/contracts/contracts/protocol/oracles/CurveOracle.sol#L98-L117

https://github.com/hats-finance/VMEX-0x41547b88e8d46bfdb6327c0c3ab4b5a1cffb11cd/blob/4bc577756e15418b919a49a1bdec0a98fd39bdbd/packages/contracts/contracts/protocol/oracles/CurveOracle.sol#L135-L149

Recommendation

use TWAP to determine the prices of the underlying assets in the pool.

[ksyao2002]

The proposed attack would require manipulation of Chainlink prices (as all curve underlying prices are Chainlink prices rather than Uniswap prices, so they are resistant to flashloan attacks), which is assumed to be secure without further modification. We had initially used a TWAP but had removed it due to redundancy. Check the peer review notes at https://mint-salesman-909.notion.site/VMEX-Peer-Review-6d3783a5c72c446cab7caf4d739f0889.

[Nabeel-javaid]

The proposed attack would require manipulation of Chainlink prices (as all curve underlying prices are Chainlink prices rather than Uniswap prices, so they are resistant to flashloan attacks), which is assumed to be secure without further modification. We had initially used a TWAP but had removed it due to redundancy. Check the peer review notes at https://mint-salesman-909.notion.site/VMEX-Peer-Review-6d3783a5c72c446cab7caf4d739f0889.

Hey, please correct me If i'm wrong:

So the function get_price_v1 is being used in getCurveAssetPrice() of VMEXOracle.sol which is then further being used in getAssetPrice() and then getAssetPrice() function is being used in VMEXOracle.sol till now we have the price from curve finance as we never interacted with chainLink price feeds. Then if we check the function getAssetsPrices() we can see that getAssetPrice() is used there and all of this is done without ever interacting with any chainLink aggregator

[ksyao2002]

The underlying tokens of the Curve pool are tokens such as USDT, USDC, BTC, etc all with chainlink price feeds. If you look here: https://github.com/hats-finance/VMEX-0x41547b88e8d46bfdb6327c0c3ab4b5a1cffb11cd/blob/4bc577756e15418b919a49a1bdec0a98fd39bdbd/packages/contracts/contracts/protocol/oracles/VMEXOracle.sol#L253 It gets the price of the underlying by calling getAssetPrice which would then call the chainlink aggregator to fetch those prices.

[Nabeel-javaid]

getAssetPrice is calling checkSequencerUp to check if the chainlink sequencer is down or not. Then it is calling getCurveAssetPrice which is then calling get_price_v1