TwoTokenPoolUtils's _getOraclePairPrice produces incorrect oraclePairPrice when balancerOracleWeight is set to be bigger than BALANCER_ORACLE_WEIGHT_PRECISION

Summary

TwoTokenPoolUtils's _getOraclePairPrice() will produce bloated oraclePairPrice as long as oracleContext.balancerOracleWeight is bigger than BALANCER_ORACLE_WEIGHT_PRECISION.

As TwoTokenPoolUtils is a helper contract, it accepts any settings from a Vault. However, _getOraclePairPrice() logic breaks up when balancerOracleWeight > BALANCER_ORACLE_WEIGHT_PRECISION. Currently there are no controls that ensures that this will not take place.

Vulnerability Detail

Whenever balancerOracleWeight is initialized with value that exceeds BalancerConstants.BALANCER_ORACLE_WEIGHT_PRECISION, the _getOraclePairPrice() returned price becomes greater than actual price by the oracleContext.balancerOracleWeight / BalancerConstants.BALANCER_ORACLE_WEIGHT_PRECISION ratio.

In particular, it can be magnitudes higher than Oracle reported values as the most probable scenario here is using one precision instead of another. For example, if balancerOracleWeight be supplied out of 18 decimals precision, the _getOraclePairPrice() will be 10^10 higher than actual values.

Impact

_getOraclePairPrice() is used to obtain a price of a strategy in the units of the underlying token, i.e. the function supplies the marker to market strategy price for subsequent decision making in the Vault. The impact of such price being magnitudes off will be the liquidations of the healthy positions and vice versa, the prohibition of the liquidations of the healthy ones, i.e. scenarios leading to direct losses for Vaults' users.

The probability of setting oracleContext.balancerOracleWeight without regard to BalancerConstants.BALANCER_ORACLE_WEIGHT_PRECISION isn't too low as there are several precision bases used in the system that can be easily messed up (it's actually the case for one of the example Vaults in this repo as it's shown in an another issue). As this is still a precondition, setting the severity to be medium.

Code Snippet

_getOraclePairPrice() logic is based on an assumption that balancerOracleWeight is a part of the whole BalancerConstants.BALANCER_ORACLE_WEIGHT_PRECISION:

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

    /// @notice Gets the oracle price pair price between two tokens using a weighted
    /// average between a chainlink oracle and the balancer TWAP oracle.
    /// @param poolContext oracle context variables
    /// @param oracleContext oracle context variables
    /// @param tradingModule address of the trading module
    /// @return oraclePairPrice oracle price for the pair in 18 decimals
    function _getOraclePairPrice(
        TwoTokenPoolContext memory poolContext,
        OracleContext memory oracleContext, 
        ITradingModule tradingModule
    ) internal view returns (uint256 oraclePairPrice) {
        // NOTE: this balancer price is denominated in 18 decimal places
        uint256 balancerWeightedPrice;
        if (oracleContext.balancerOracleWeight > 0) {
            uint256 balancerPrice = BalancerUtils._getTimeWeightedOraclePrice(
                address(poolContext.basePool.pool),
                IPriceOracle.Variable.PAIR_PRICE,
                oracleContext.oracleWindowInSeconds
            );

            if (poolContext.primaryIndex == 1) {
                // If the primary index is the second token, we need to invert
                // the balancer price.
                balancerPrice = BalancerConstants.BALANCER_PRECISION_SQUARED / balancerPrice;
            }

            balancerWeightedPrice = balancerPrice * oracleContext.balancerOracleWeight;
        }

        uint256 chainlinkWeightedPrice;
        if (oracleContext.balancerOracleWeight < BalancerConstants.BALANCER_ORACLE_WEIGHT_PRECISION) {
            (int256 rate, int256 decimals) = tradingModule.getOraclePrice(
                poolContext.primaryToken, poolContext.secondaryToken
            );
            require(rate > 0);
            require(decimals >= 0);

            if (uint256(decimals) != BalancerConstants.BALANCER_PRECISION) {
                rate = (rate * int256(BalancerConstants.BALANCER_PRECISION)) / decimals;
            }

            // No overflow in rate conversion, checked above
            chainlinkWeightedPrice = uint256(rate) * 
                (BalancerConstants.BALANCER_ORACLE_WEIGHT_PRECISION - oracleContext.balancerOracleWeight);
        }

        oraclePairPrice = (balancerWeightedPrice + chainlinkWeightedPrice) / 
            BalancerConstants.BALANCER_ORACLE_WEIGHT_PRECISION;
    }

As BALANCER_ORACLE_WEIGHT_PRECISION = 1e8, the BalancerConstants.BALANCER_ORACLE_WEIGHT_PRECISION > 0 check is satisfied.

Different cases here are:

balancerOracleWeight == 0: ok
0 < balancerOracleWeight < BalancerConstants.BALANCER_ORACLE_WEIGHT_PRECISION: ok
balancerOracleWeight == BalancerConstants.BALANCER_ORACLE_WEIGHT_PRECISION: ok
balancerOracleWeight > BalancerConstants.BALANCER_ORACLE_WEIGHT_PRECISION: wrong as the price now is oraclePairPrice = (balancerWeightedPrice + chainlinkWeightedPrice) / BalancerConstants.BALANCER_ORACLE_WEIGHT_PRECISION while it needs to be oraclePairPrice = (balancerWeightedPrice + chainlinkWeightedPrice) / oracleContext.balancerOracleWeight as while chainlinkWeightedPrice == 0, the balancerWeightedPrice is weighted with oracleContext.balancerOracleWeight.

The latter case is possible as oracleContext.balancerOracleWeight is set via BalancerVaultStorage's setStrategyVaultSettings, that controls its value with a caller-supplied argument:

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/internal/BalancerVaultStorage.sol#L25-L47

    function setStrategyVaultSettings(
        StrategyVaultSettings memory settings, 
        uint32 maxOracleQueryWindow,
        uint16 balancerOracleWeight
    ) internal {
        require(settings.oracleWindowInSeconds <= maxOracleQueryWindow);
        require(settings.settlementCoolDownInMinutes <= BalancerConstants.MAX_SETTLEMENT_COOLDOWN_IN_MINUTES);
        require(settings.postMaturitySettlementCoolDownInMinutes <= BalancerConstants.MAX_SETTLEMENT_COOLDOWN_IN_MINUTES);
        require(settings.maxRewardTradeSlippageLimitPercent <= BalancerConstants.SLIPPAGE_LIMIT_PRECISION);
        require(settings.balancerOracleWeight <= balancerOracleWeight);
        require(settings.maxBalancerPoolShare <= BalancerConstants.VAULT_PERCENT_BASIS);
        require(settings.settlementSlippageLimitPercent <= BalancerConstants.SLIPPAGE_LIMIT_PRECISION);
        require(settings.postMaturitySettlementSlippageLimitPercent <= BalancerConstants.SLIPPAGE_LIMIT_PRECISION);
        require(settings.emergencySettlementSlippageLimitPercent <= BalancerConstants.SLIPPAGE_LIMIT_PRECISION);
        require(settings.feePercentage <= BalancerConstants.VAULT_PERCENT_BASIS);
        require(settings.oraclePriceDeviationLimitPercent <= BalancerConstants.VAULT_PERCENT_BASIS);

        mapping(uint256 => StrategyVaultSettings) storage store = _settings();
        // Hardcode to the zero slot
        store[0] = settings;

        emit BalancerEvents.StrategyVaultSettingsUpdated(settings);
    }

The value of balancerOracleWeight argument isn't controlled, it is up to Vault designer to set any value.

This way if for any reason settings.balancerOracleWeight in setStrategyVaultSettings() be set to be greater than BALANCER_ORACLE_WEIGHT_PRECISION, the _getOraclePairPrice() become up to magnitudes wrong. Say if decimals got messed up to the upside, say balancerOracleWeight can be set out of 18 decimals, while BalancerConstants.BALANCER_ORACLE_WEIGHT_PRECISION is only 8:

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/internal/BalancerConstants.sol#L7-L12

    uint256 internal constant BALANCER_ORACLE_WEIGHT_PRECISION = 1e8;
    uint32 internal constant SLIPPAGE_LIMIT_PRECISION = 1e8;

    /// @notice Precision for all percentages used by the vault
    /// 1e4 = 100% (i.e. maxBalancerPoolShare)
    uint16 internal constant VAULT_PERCENT_BASIS = 1e4;

In this case _getOraclePairPrice() price becomes circa 10^10 times greater than the actual price the oracles reported.

_getOraclePairPrice() is used for the current strategy evaluation via the following call sequences:

convertStrategyToUnderlying -> _convertStrategyToUnderlying, _executeSettlement -> _getTimeWeightedPrimaryBalance -> _getOraclePairPrice,

settleVault, settleVaultEmergency -> _executeSettlement -> _getTimeWeightedPrimaryBalance -> _getOraclePairPrice.

Tool used

Manual Review

Recommendation

The usage of BALANCER_ORACLE_WEIGHT_PRECISION is fixed in the logic, so setting a balancerOracleWeight outside it can easily lead to the malfunction of the approach. As TwoTokenPoolUtils library logic above needs to be uniform, while the Vaults can vary, the only way to avoid this is to control the setting so it always matches the logic.

Consider controlling the balancerOracleWeight limit in setStrategyVaultSettings()

Upper limit is straightforward, while lower is a kind of useful heuristic:

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/internal/BalancerVaultStorage.sol#L25-L35

    function setStrategyVaultSettings(
        StrategyVaultSettings memory settings, 
        uint32 maxOracleQueryWindow,
        uint16 balancerOracleWeight
    ) internal {
+       require(balancerOracleWeight == 0 ||
                (balancerOracleWeight >= BalancerConstants.BALANCER_ORACLE_WEIGHT_PRECISION / 100 &&
                 balancerOracleWeight <= BalancerConstants.BALANCER_ORACLE_WEIGHT_PRECISION));
        require(settings.oracleWindowInSeconds <= maxOracleQueryWindow);
        require(settings.settlementCoolDownInMinutes <= BalancerConstants.MAX_SETTLEMENT_COOLDOWN_IN_MINUTES);
        require(settings.postMaturitySettlementCoolDownInMinutes <= BalancerConstants.MAX_SETTLEMENT_COOLDOWN_IN_MINUTES);
        require(settings.maxRewardTradeSlippageLimitPercent <= BalancerConstants.SLIPPAGE_LIMIT_PRECISION);
        require(settings.balancerOracleWeight <= balancerOracleWeight);
        require(settings.maxBalancerPoolShare <= BalancerConstants.VAULT_PERCENT_BASIS);

sherlock-audit / 2022-09-notional-judging

hyh - TwoTokenPoolUtils's _getOraclePairPrice produces incorrect oraclePairPrice when balancerOracleWeight is set to be bigger than BALANCER_ORACLE_WEIGHT_PRECISION #50