search

sherlock-audit / 2022-09-notional-judging

4 stars 2 forks source link

xiaoming90 - Vulnerable Internal Price Oracle #77

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

xiaoming90

high

Vulnerable Internal Price Oracle

Summary

The internal price oracle within the vault does not reflect the true value of the assets. As such, the assets might be overvalued or undervalued leading to an array of issues.

Vulnerability Detail

Note: This issue affects the MetaStable2 balancer leverage vault

One of the key questions to ask when designing a price oracle is what if the price returned is absurdly small or absurdly large, and what are the mitigation controls in place if this happens?

Based on the current implementation of stETH/ETH balancer leverage vault, the price pair is computed based on a weighted average of Balancer Oracle and Chainlink as shown below.

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/internal/pool/TwoTokenPoolUtils.sol#L72

File: TwoTokenPoolUtils.sol
066:     /// @notice Gets the oracle price pair price between two tokens using a weighted
067:     /// average between a chainlink oracle and the balancer TWAP oracle.
068:     /// @param poolContext oracle context variables
069:     /// @param oracleContext oracle context variables
070:     /// @param tradingModule address of the trading module
071:     /// @return oraclePairPrice oracle price for the pair in 18 decimals
072:     function _getOraclePairPrice(
073:         TwoTokenPoolContext memory poolContext,
074:         OracleContext memory oracleContext, 
075:         ITradingModule tradingModule
076:     ) internal view returns (uint256 oraclePairPrice) {
077:         // NOTE: this balancer price is denominated in 18 decimal places
078:         uint256 balancerWeightedPrice;
079:         if (oracleContext.balancerOracleWeight > 0) {
080:             uint256 balancerPrice = BalancerUtils._getTimeWeightedOraclePrice(
081:                 address(poolContext.basePool.pool),
082:                 IPriceOracle.Variable.PAIR_PRICE,
083:                 oracleContext.oracleWindowInSeconds
084:             );
085: 
086:             if (poolContext.primaryIndex == 1) {
087:                 // If the primary index is the second token, we need to invert
088:                 // the balancer price.
089:                 balancerPrice = BalancerConstants.BALANCER_PRECISION_SQUARED / balancerPrice;
090:             }
091: 
092:             balancerWeightedPrice = balancerPrice * oracleContext.balancerOracleWeight;
093:         }
094: 
095:         uint256 chainlinkWeightedPrice;
096:         if (oracleContext.balancerOracleWeight < BalancerConstants.BALANCER_ORACLE_WEIGHT_PRECISION) {
097:             (int256 rate, int256 decimals) = tradingModule.getOraclePrice(
098:                 poolContext.primaryToken, poolContext.secondaryToken
099:             );
100:             require(rate > 0);
101:             require(decimals >= 0);
102: 
103:             if (uint256(decimals) != BalancerConstants.BALANCER_PRECISION) {
104:                 rate = (rate * int256(BalancerConstants.BALANCER_PRECISION)) / decimals;
105:             }
106: 
107:             // No overflow in rate conversion, checked above
108:             chainlinkWeightedPrice = uint256(rate) * 
109:                 (BalancerConstants.BALANCER_ORACLE_WEIGHT_PRECISION - oracleContext.balancerOracleWeight);
110:         }
111: 
112:         oraclePairPrice = (balancerWeightedPrice + chainlinkWeightedPrice) / 
113:             BalancerConstants.BALANCER_ORACLE_WEIGHT_PRECISION;
114:     }

Based on the test script, the weightage is Balancer Oracle - 60% and Chainlink - 40%.

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/scripts/BalancerEnvironment.py#L45

File: BalancerEnvironment.py
45:             "maxRewardTradeSlippageLimitPercent": 5e6,
46:             "balancerOracleWeight": 0.6e4, # 60%
47:             "settlementCoolDownInMinutes": 60 * 6, # 6 hour settlement cooldown

Therefore, it appears that the vault attempts to mitigate the risk of price fluctuation by using a weighted average between a chainlink oracle and the balancer TWAP oracle to smooth out the differences in the prices reported by the two Oracles.

However, this solution might provide the wrong valuation of an asset. Assume that the price of stETH crashes suddenly (e.g. similar to LUNC incident) and the price has fallen from 100 to 0.01 in the real world. The price of stETH is correctly reflected on Chainlink as 0.01. However, due the certain reasons (e.g. lack of updates or the update schedule has not been triggered yet), the price of stETH has not been updated on the Balancer Oracle yet, and therefore the price of stETH is still 100 on the Balancer Oracle.

In this case, the price returned by the TwoTokenPoolUtils._getOraclePairPrice will be around 60, which is obviously overvalued.

price = balancer's price * balancer's weightage + chainlink's price * chainlink's weightage
price = 100 * 0.6 + 0.01 * 0.4 = 60.004

Note: This issue is applicable for all secondary oracles, not only Balancer Oracle. It is due to the fact that it is unlikely that both oracles will reflect the correct valuation of an asset at the same time when a sudden crash happens. Depending on the oracle in use, an update usually happens every few hours or when some event is triggered.

Impact

The price provided by the function will not reflect the true value of the assets. It might be overvalued or undervalued. The affected function is being used in almost all functions within the vault. For instance, this function is part of the critical _convertStrategyToUnderlying function that computes the value of the strategy token in terms of its underlying assets. As a result, it might cause the following:

Code Snippet

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/internal/pool/TwoTokenPoolUtils.sol#L72 https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/scripts/BalancerEnvironment.py#L45

Tool used

Manual Review

Recommendation

Instead of using a weighted average between a chainlink oracle and the balancer TWAP oracle, it is recommended to implement a solution to detect a large deviation in prices. For instance, if Oracle A's price deviates from Oracle B's price by 30%, revert the transaction as it indicates that some tail risk event has happened on-chain. In this case, it will be safer for the vault to have the transactions reverted instead of having them processed with the wrong valuation. The vault should even consider automatically pausing itself to protect its assets if possible. After a certain period of time, the price of Oracle A and Oracle B will eventually converge to a price that is within the deviation threshold (e.g. the difference between the two prices is less than 30%) and stabilizes. When this happens, the vault can proceed with all the transactions (e.g. deleverage, vault settlement) as usual.

When the price difference between Oracle A and Oracle B falls within the accepted deviation threshold (e.g. the difference between the two prices is less than 30%), it is fine to use a weighted average price between the two oracles. However, if it falls outside of the accepted deviation threshold (e.g. the difference between the two prices is more than 90%), using a weighted average price between the two oracles is not going to work out as a weighted average of a correct price and incorrect price does not automatically produce a valid price.

duplicate of #67

jeffywu commented 1 year ago

@T-Woodward / @weitianjie2000

T-Woodward commented 1 year ago

Dupe of #67

jeffywu commented 1 year ago

Issue #67 and #77 are quite closely related, in the sense that the update frequency (67) causes the likelihood that 77 (using the average weights) would materialize into a problem. There is a significant overlap in the scope of the two reported issues although they do point out slightly different effects.

Whether they are duplicates or not is debatable to me, I could see it either way. I'm not sure how much of a practical effect this has on the results of the judging either way since both issues were reported by the same watson. Seems like this watson will get a pretty large share of the prize pool regardless.