Oracles With Different Quote Currency Can Be Used To Compute Price

Summary

The price can be computed using oracles with different quote currency causing the price returned to be wrong.

Vulnerability Detail

The getOraclePrice function will return the price between the baseToken and the quoteToken using Chainlink.

For each currency supported by Chainlink, there are usually two quote currencies (ETH and USD). For instance, based on https://docs.chain.link/docs/data-feeds/price-feeds/addresses/?network=ethereum:

USDC / ETH
USDC / USD
DAI / ETH
DAI / USD

Based on the current implementation, it is extremely important that the quote currency is the same. Otherwise, the price returned by this function will be wrong. This point has also been stressed in the source code's comments as shown below.

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/trading/TradingModule.sol#L208

File: TradingModule.sol
201:     /// @notice Returns the Chainlink oracle price between the baseToken and the quoteToken, the
202:     /// Chainlink oracles. The quote currency between the oracles must match or the conversion
203:     /// in this method does not work. Most Chainlink oracles are baseToken/USD pairs.
204:     /// @param baseToken address of the first token in the pair, i.e. USDC in USDC/DAI
205:     /// @param quoteToken address of the second token in the pair, i.e. DAI in USDC/DAI
206:     /// @return answer exchange rate in rate decimals
207:     /// @return decimals number of decimals in the rate, currently hardcoded to 1e18
208:     function getOraclePrice(address baseToken, address quoteToken)
209:         public
210:         view
211:         override
212:         returns (int256 answer, int256 decimals)
213:     {
214:         PriceOracle memory baseOracle = priceOracles[baseToken];
215:         PriceOracle memory quoteOracle = priceOracles[quoteToken];
216: 
217:         int256 baseDecimals = int256(10**baseOracle.rateDecimals);
218:         int256 quoteDecimals = int256(10**quoteOracle.rateDecimals);
219: 
220:         (/* */, int256 basePrice, /* */, uint256 bpUpdatedAt, /* */) = baseOracle.oracle.latestRoundData();
221:         require(block.timestamp - bpUpdatedAt <= maxOracleFreshnessInSeconds);
222:         require(basePrice > 0); /// @dev: Chainlink Rate Error
223: 
224:         (/* */, int256 quotePrice, /* */, uint256 qpUpdatedAt, /* */) = quoteOracle.oracle.latestRoundData();
225:         require(block.timestamp - qpUpdatedAt <= maxOracleFreshnessInSeconds);
226:         require(quotePrice > 0); /// @dev: Chainlink Rate Error
227: 
228:         answer =
229:             (basePrice * quoteDecimals * RATE_DECIMALS) /
230:             (quotePrice * baseDecimals);
231:         decimals = RATE_DECIMALS;
232:     }

If the admin configured with a wrong quote currency for a price pair (e.g. USDC/DAI pair using USDC / ETH + DAI / USD oracles), the price returned will be wrong, and all the assets within the vault will be either overvalued or undervalued leading to an array of issues. Even though it is critical to get this right, this requirement is not explicitly enforced within the codebase, and no validation is in place to ensure that quote currency between the two oracles is the same.

The following shows that the admin can set any price oracle by calling the setPriceOracle function. There is no validation in place to ensure that the quote currency of the oracles is standardized to either ETH or USD. The admin is free to configure an oracle with any quote currency.

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/trading/TradingModule.sol#L64

File: TradingModule.sol
64:     function setPriceOracle(address token, AggregatorV2V3Interface oracle) external override onlyNotionalOwner {
65:         PriceOracle storage oracleStorage = priceOracles[token];
66:         oracleStorage.oracle = oracle;
67:         oracleStorage.rateDecimals = oracle.decimals();
68: 
69:         emit PriceOracleUpdated(token, address(oracle));
70:     }

Furthermore, the getOraclePrice function did not perform any checks to ensure that the quote currency of the two oracles is the same. Even if the quote currency of the two oracles is different, the function will happily accept it and proceed to compute the price.

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/trading/TradingModule.sol#L208

File: TradingModule.sol
201:     /// @notice Returns the Chainlink oracle price between the baseToken and the quoteToken, the
202:     /// Chainlink oracles. The quote currency between the oracles must match or the conversion
203:     /// in this method does not work. Most Chainlink oracles are baseToken/USD pairs.
204:     /// @param baseToken address of the first token in the pair, i.e. USDC in USDC/DAI
205:     /// @param quoteToken address of the second token in the pair, i.e. DAI in USDC/DAI
206:     /// @return answer exchange rate in rate decimals
207:     /// @return decimals number of decimals in the rate, currently hardcoded to 1e18
208:     function getOraclePrice(address baseToken, address quoteToken)
209:         public
210:         view
211:         override
212:         returns (int256 answer, int256 decimals)
213:     {
214:         PriceOracle memory baseOracle = priceOracles[baseToken];
215:         PriceOracle memory quoteOracle = priceOracles[quoteToken];
216: 
217:         int256 baseDecimals = int256(10**baseOracle.rateDecimals);
218:         int256 quoteDecimals = int256(10**quoteOracle.rateDecimals);
219: 
220:         (/* */, int256 basePrice, /* */, uint256 bpUpdatedAt, /* */) = baseOracle.oracle.latestRoundData();
221:         require(block.timestamp - bpUpdatedAt <= maxOracleFreshnessInSeconds);
222:         require(basePrice > 0); /// @dev: Chainlink Rate Error
223: 
224:         (/* */, int256 quotePrice, /* */, uint256 qpUpdatedAt, /* */) = quoteOracle.oracle.latestRoundData();
225:         require(block.timestamp - qpUpdatedAt <= maxOracleFreshnessInSeconds);
226:         require(quotePrice > 0); /// @dev: Chainlink Rate Error
227: 
228:         answer =
229:             (basePrice * quoteDecimals * RATE_DECIMALS) /
230:             (quotePrice * baseDecimals);
231:         decimals = RATE_DECIMALS;
232:     }

Impact

If the price returned is wrong, the assets will be either overvalued or undervalued. This will lead to an array of serious issues such as over-borrowing and pre-matured liquidation.

Code Snippet

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/trading/TradingModule.sol#L208 https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/trading/TradingModule.sol#L64 https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/trading/TradingModule.sol#L208

Tool used

Manual Review

Recommendation

Consider implementing one of the following measures:

Standardize the quote currency to either ETH or USD within the Trade Module. For example, standardize the quote currency to ETH within the Trade Module. If anyone attempts to add a price oracle with a USD quote currency via the setPriceOracle function, it will revert.
Within the getOraclePrice function, reverts if the quote currency of the two oracles is different.

Currently, it really depends on the admin to get this right. However, the codebase will grow over time, developers join and leave the team, lack of communication between developers, a lack of proper internal documentation, or simply human errors, we have seen many mistakes made during deployment, configuring, and upgrading over the past few years leading to a security incident that can be avoided if proper validation has been implemented right at the start. Thus, it will be prudent for the team to implement this fix to prevent this issue.

sherlock-audit / 2022-09-notional-judging

xiaoming90 - Oracles With Different Quote Currency Can Be Used To Compute Price #51