sherlock-audit / 2022-09-notional-judging

4 stars 2 forks source link

xiaoming90 - Using Balancer Math Functions With Incorrect Precision #78

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

xiaoming90

high

Using Balancer Math Functions With Incorrect Precision

Summary

The vault was found to perform Balancer math functions with incorrect precision.

Vulnerability Detail

Note: This issue affects MetaStable2 and Boosted3 balancer leverage vaults

When performing Balancer math functions, all amounts need to be in BALANCER_PRECISION as per the comment at Line 105 below. Therefore, in Line 106-109 below, the code attempts to convert the precision of primaryAmount and secondaryAmount to BALANCER_PRECISION (1e18) before passing them to the StableMath._calcSpotPrice function for computation. The calculatedPairPrice return value is then passed to Stable2TokenOracleMath._checkPriceLimit for additional validation.

Assume that either primaryAmount or secondaryPrecision is USDC OR any other tokens that their precision != BALANCER_PRECISION (1e18).

USDC's precision is 1e6, while BALANCER_PRECISION is 1e18. Therefore, based on the formula in Lines 106-109 below, the USDC's precision will be converted to 1e18 before passing it to StableMath._calcSpotPrice.

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/internal/math/Stable2TokenOracleMath.sol#L89

File: Stable2TokenOracleMath.sol
089:     function _validateSpotPriceAndPairPrice(
090:         StableOracleContext calldata oracleContext,
091:         TwoTokenPoolContext calldata poolContext,
092:         StrategyContext calldata strategyContext,
093:         uint256 primaryAmount, 
094:         uint256 secondaryAmount
095:     ) internal view {
096:         // Oracle price is always specified in terms of primary, so tokenIndex == 0 for primary
097:         uint256 spotPrice = _getSpotPrice(oracleContext, poolContext, 0);
098:         _checkPriceLimit(strategyContext, poolContext, spotPrice);
099: 
100:         // We always validate in terms of the primary here so it is the first value in the _balances array
101:         uint256 invariant = StableMath._calculateInvariant(
102:             oracleContext.ampParam, StableMath._balances(primaryAmount, secondaryAmount), true // round up
103:         );
104: 
105:         /// @notice Balancer math functions expect all amounts to be in BALANCER_PRECISION
106:         uint256 primaryPrecision = 10 ** poolContext.primaryDecimals;
107:         uint256 secondaryPrecision = 10 ** poolContext.secondaryDecimals;
108:         primaryAmount = primaryAmount * BalancerConstants.BALANCER_PRECISION / primaryPrecision;
109:         secondaryAmount = secondaryAmount * BalancerConstants.BALANCER_PRECISION / secondaryPrecision;
110: 
111:         uint256 calculatedPairPrice = StableMath._calcSpotPrice({
112:             amplificationParameter: oracleContext.ampParam,
113:             invariant: invariant,
114:             balanceX: primaryAmount,
115:             balanceY: secondaryAmount
116:         });
117: 
118:         _checkPriceLimit(strategyContext, poolContext, calculatedPairPrice);
119:     }

However, this precision conversion was not consistently applied across all the functions. Within Stable2TokenOracleMath._getSpotPrice function, the balanceX and balanceX are passed directly to the StableMath._calcSpotPrice function for computation without converting them to BALANCER_PRECISION (1e18). Thus, if the precision of either balanceX or balanceY is not equal to BALANCER_PRECISION(1e18), for example USDC, then the balancer math functions will produce an incorrect result.

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/internal/math/Stable2TokenOracleMath.sol#L16

File: Stable2TokenOracleMath.sol
16:     function _getSpotPrice(
17:         StableOracleContext memory oracleContext, 
18:         TwoTokenPoolContext memory poolContext, 
19:         uint256 tokenIndex
20:     ) internal view returns (uint256 spotPrice) {
21:         // Prevents overflows, we don't expect tokens to be greater than 18 decimals, don't use
22:         // equal sign for minor gas optimization
23:         require(poolContext.primaryDecimals < 19); /// @dev primaryDecimals overflow
24:         require(poolContext.secondaryDecimals < 19); /// @dev secondaryDecimals overflow
25:         require(tokenIndex < 2); /// @dev invalid token index
26: 
27:         (uint256 balanceX, uint256 balanceY) = tokenIndex == 0 ?
28:             (poolContext.primaryBalance, poolContext.secondaryBalance) :
29:             (poolContext.secondaryBalance, poolContext.primaryBalance);
30: 
31:         uint256 invariant = StableMath._calculateInvariant(
32:             oracleContext.ampParam, StableMath._balances(balanceX, balanceY), true // round up
33:         );
34: 
35:         spotPrice = StableMath._calcSpotPrice({
36:             amplificationParameter: oracleContext.ampParam,
37:             invariant: invariant,
38:             balanceX: balanceX,
39:             balanceY: balanceY
40:         });
41:     }
42: 

Impact

The affected _getSpotPrice function was found to be in use in the following areas. As a result, the incorrect pricing returned by the balancer math functions will cause these features to wrongly validate the price and cause trades that are unfavorable to the vault to be executed.

Code Snippet

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/internal/math/Stable2TokenOracleMath.sol#L89 https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/internal/math/Stable2TokenOracleMath.sol#L16

Tool used

Manual Review

Recommendation

It is recommended to convert all the amounts to BALANCER_PRECISION before passing them to the Balancer math functions and ensure that this is consistently applied across the codebase where Balancer math functions are used.

function _getSpotPrice(
    StableOracleContext memory oracleContext, 
    TwoTokenPoolContext memory poolContext, 
    uint256 tokenIndex
) internal view returns (uint256 spotPrice) {
    // Prevents overflows, we don't expect tokens to be greater than 18 decimals, don't use
    // equal sign for minor gas optimization
    require(poolContext.primaryDecimals < 19); /// @dev primaryDecimals overflow
    require(poolContext.secondaryDecimals < 19); /// @dev secondaryDecimals overflow
    require(tokenIndex < 2); /// @dev invalid token index

    (uint256 balanceX, uint256 balanceY) = tokenIndex == 0 ?
        (poolContext.primaryBalance, poolContext.secondaryBalance) :
        (poolContext.secondaryBalance, poolContext.primaryBalance);

    uint256 invariant = StableMath._calculateInvariant(
        oracleContext.ampParam, StableMath._balances(balanceX, balanceY), true // round up
    );

+    /// @notice Balancer math functions expect all amounts to be in BALANCER_PRECISION
+    uint256 primaryPrecision = 10 ** poolContext.primaryDecimals;
+    uint256 secondaryPrecision = 10 ** poolContext.secondaryDecimals;
+    balanceX = balanceX * BalancerConstants.BALANCER_PRECISION / primaryPrecision;
+    balanceY = balanceY * BalancerConstants.BALANCER_PRECISION / secondaryPrecision;

    spotPrice = StableMath._calcSpotPrice({
        amplificationParameter: oracleContext.ampParam,
        invariant: invariant,
        balanceX: balanceX,
        balanceY: balanceY
    });
}
jeffywu commented 1 year ago

@weitianjie2000

weitianjie2000 commented 1 year ago

invalid, poolContext balances are always in BALANCER_PRECISION (1e18)