hash - Incorrect price for tokens of Balancer stable pools due to fixed 1e18 input amount

[sherlock-admin]

hash

high

Incorrect price for tokens of Balancer stable pools due to fixed 1e18 input amount

Summary

Stable token rate is not considered for Balancer MetaStablePool tokens when computing the price

Vulnerability Detail

The amountIn is always provided as 1e18 for the StableMath._calcOutGivenIn function when calculating the price of stable pool token.

    function getTokenPriceFromStablePool(
        address lookupToken_,
        uint8 outputDecimals_,
        bytes calldata params_
    ) external view returns (uint256) {

        ......
                try pool.getLastInvariant() returns (uint256, uint256 ampFactor) {
                    // Upscale balances by the scaling factors
                    uint256[] memory scalingFactors = pool.getScalingFactors();
                    uint256 len = scalingFactors.length;
                    for (uint256 i; i < len; ++i) {
                        balances_[i] = FixedPoint.mulDown(balances_[i], scalingFactors[i]);
                    }

                    // Calculate the quantity of lookupTokens returned by swapping 1 destinationToken
                    lookupTokensPerDestinationToken = StableMath._calcOutGivenIn(
                        ampFactor,
                        balances_,
                        destinationTokenIndex,
                        lookupTokenIndex,
                        1e18,
                        StableMath._calculateInvariant(ampFactor, balances_) // Sometimes the fetched invariant value does not work, so calculate it
                    );
        .......

The lookupTokensPerDestinationToken is used as-is without any further division with the inputted token amount as the input amount is assumed to be 1 unit.

Although in normal stable pools 1 unit of token is upscaled to 1e18 for calculations, pools having rate proivders (MetaStablePool) upscale by also factoring in the token rate.

    function _scalingFactors() internal view virtual override returns (uint256[] memory scalingFactors) {

        .......

        scalingFactors = super._scalingFactors();
        scalingFactors[0] = scalingFactors[0].mulDown(_priceRate(_token0));
        scalingFactors[1] = scalingFactors[1].mulDown(_priceRate(_token1));
    }

Hence the input token amount will deviate from 1 unit of token and will not return the price of the token.

Example Scenario

In wstEth-cbEth pool wstEth has rate of 1.151313786310212348 Actual input to the calcAmountIn should be 1.151313786310212348e18 and not 1e18. This will give an inflated price for cbEth.

Impact

Incorrect token price calculation for MetaStablePool tokens in which lookup tokens have rates.

Code Snippet

1e18 is used as input amount always https://github.com/sherlock-audit/2023-11-olympus/blob/9c8df76dc9820b4c6605d2e1e6d87dcfa9e50070/bophades/src/modules/PRICE/submodules/feeds/BalancerPoolTokenPrice.sol#L811-L827

Tool used

Manual Review

Recommendation

Input the upscaled amount instead of 1e18

                    lookupTokensPerDestinationToken = StableMath._calcOutGivenIn(
                        ampFactor,
                        balances_,
                        destinationTokenIndex,
                        lookupTokenIndex,
=>                      FixedPoint.mulDown(1e18,scalingFactors[destinationTokenIndex]),
                        StableMath._calculateInvariant(ampFactor, balances_) // Sometimes the fetched invariant value does not work, so calculate it
                    );

[0xJem]

Please provide links to the documentation/code to prove this:

Although in normal stable pools 1 unit of token is upscaled to 1e18 for calculations, pools having rate proivders (MetaStablePool) upscale by also factoring in the token rate.

We are likely to drop both the Balancer submodules from the final version, since we no longer have any Balancer pools used for POL and don't have any assets that require price resolution via Balancer pools.

[nevillehuang]

Hi @0xJem here is additional information provided by the watson:

I will give the etherscan link of a metastablepool

https://vscode.blockscan.com/ethereum/0x1e19cf2d73a72ef1332c882f20534b6519be0276

on searching _scalingFactors() it can be seen that the token balances are multiplied with priceRate apart from the normal scalingFactor (look for the scalingFacotrs in MetaStablePool.sol)

for the swap flow, the general flow is onSwap -> _swapGivenIn() -> update balances with scaling factors -> _onSwapGivenIn() which actually does the StableMath._calcOutGivenIn calculation. While updating balances with scaling factors in _swapGivenIn() for metastable pools, 1 unit of token will be converted to 1 normalScalingFactor priceRate

[0xauditsea]

Escalate

  lookupTokensPerDestinationToken = StableMath._calcOutGivenIn(
      ampFactor,
      balances_,
      destinationTokenIndex,
      lookupTokenIndex,
      1e18,
      StableMath._calculateInvariant(ampFactor, balances_) // Sometimes the fetched invariant value does not work, so calculate it
  );

Above formula is to calculate output amount of destination token when 1 unit of lookup token is provided, so it has to be 1e18, not multiplied by scaled factor, thus invalid.

[sherlock-admin2]

Escalate

  lookupTokensPerDestinationToken = StableMath._calcOutGivenIn(
      ampFactor,
      balances_,
      destinationTokenIndex,
      lookupTokenIndex,
      1e18,
      StableMath._calculateInvariant(ampFactor, balances_) // Sometimes the fetched invariant value does not work, so calculate it
  );

Above formula is to calculate output amount of destination token when 1 unit of lookup token is provided, so it has to be 1e18, not multiplied by scaled factor, thus invalid.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[securitygrid]

I think the judger should consider whether the protocol wants to interact with pools that may cause problems. If the protocol only interacts with a specific pool, then there will be no problem. It's like we can't assume that a protocol wants to interact with any ERC20, including weird tokens.

[0xJem]

I think the judger should consider whether the protocol wants to interact with pools that may cause problems. If the protocol only interacts with a specific pool, then there will be no problem. It's like we can't assume that a protocol wants to interact with any ERC20, including weird tokens.

We don't want to have to add support for MetaStablePools (for example), but the nature of the changes made by Balancer mean that the existing code would treat a MetaStablePool the same as the StablePools that are supported, which would be problematic.

This is one of the major reasons why we won't be deploying the Balancer-related functionality in the system.

[IAm0x52]

I think the judger should consider whether the protocol wants to interact with pools that may cause problems. If the protocol only interacts with a specific pool, then there will be no problem. It's like we can't assume that a protocol wants to interact with any ERC20, including weird tokens.

This. It does not apply to any of the tokens in the expected ERC20 tokens

[nevillehuang]

@10xhash does your above mentioned issue apply to any of the supported ERC20 tokens? If not I will have to agree with @IAm0x52 and @securitygrid. Same for #178

[10xhash]

It can apply but requires some things to happen in future. For calculating the price it is not required for the smart contracts to interact with the destination token. The contract would be interacting with the Balancer pool. Hence the destination token could be anything. This issue will happen if any such token has a rate provider

Possible scenarios:

1. Internal price calculation itself being wrong: WETH-wstETH (or reth-weth ) balancer pool could be added as a feed for WETH in future if a price feed independent from WETH is available for wstETH (ie. wstEth/USD feed. This is currently available in some other chains). In such a case, the above issue will occur which will hugely alter the price of WETH (~= wstEth price ie. ~2400 will be reported as ~2800). Similar pools could be added for sDai or DAI.
2. Direct calls to BalancerPoolTokenPrice being wrong: If a dai/sDai metastable pool is created then a call to getTokenPriceFromStablePool with this pool and Dai as the look up token will return incorrect price since sDai has a rate provider (sdai rate provider : 0xc7177B6E18c1Abd725F5b75792e5F7A3bA5DBC2c)

The chances are low due to newer composable stable pools but the team has attempted to add support for MetaStablePool's following the last audit indicating they are either expecting more assets or is considering possible future interactions.

[10xhash]

Yes, #178 is applicable for normal stable pools too. This will include the DAI-USDC-USDT stable pool.

[Czar102]

@10xhash can you let me know more why do you think metastable pools were intended to work with this function in more detail?

[10xhash]

@10xhash can you let me know more why do you think metastable pools were intended to work with this function in more detail?

1. Comments explicitly mentioning only avoiding composable stable pools among stable pools.

https://github.com/sherlock-audit/2023-11-olympus/blob/9c8df76dc9820b4c6605d2e1e6d87dcfa9e50070/bophades/src/modules/PRICE/submodules/feeds/BalancerPoolTokenPrice.sol#L712

    /// @dev                    - If the pool is not a stable pool or is a composable stable pool (determined by the absence of the `getLastInvariant()` function)

https://github.com/sherlock-audit/2023-11-olympus/blob/9c8df76dc9820b4c6605d2e1e6d87dcfa9e50070/bophades/src/modules/PRICE/submodules/feeds/BalancerPoolTokenPrice.sol#L477-L479

            // Ensure that the pool is a stable pool, but not a composable stable pool.
            // Determining the LP token price using a composable stable pool is sufficiently different from other
            // stable pools, and should be added in a separate adapter/function at a later date.

2. In a previous audit, the issue Similarly, the calculated lookupTokensPerDestinationToken needs to be downscaled by the scaling factor because some pools’ scaling factors factor in the token rate, like for appreciating assets against the base token. An example of this is the rETH-WETH pool, where fetching the rETH price incorrectly returned an approximately equivalent WETH price was raised and was responded with a fix of dividing by the token's scalingFactor.

3. As an example for a stable pool, the team messaged me the reth-weth pool on discord which is a metastable pool.

[nevillehuang]

@0xJem Was reth-weth a previously interested balancer pool by olympus? Because it certainly wasn't mentioned in the contest details based on supported ERC20s or details (for rETH), so in fact the watsons making escalations has a point that this could be invalid.

[0xJem]

No it wasn't. I believe @10xhash is referring to a discussion in Discord where that particular pool might have been referred to as an example pool (without realising that it's not a "stable pool").

On this... the code documentation is very clear that only a "stable pool" is supported. Composable stable pools are explicitly mentioned as not supported, but as a "meta stable pool" is not a "stable pool", and was not intended to be supported, I don't think this is valid.

[Czar102]

I agree with the sponsor, planning to accept the escalation and make the issue invalid.

[10xhash]

My question on discord was Hey can you give a stable pool example. I could only find composable stable pools in balancer for which the reply was the reth-weth pool which is a metastable pool.

The term stable pool is used in many places throughout Balancer to encompass the various types of stable pool implementations like stable pool, metastable pool, stablepool v2 etc. The inline documentations of this codebase matches this idea.

Determining the token price using a composable stable pool is sufficiently different from other stable pools, and should be added in a separate adapter/function at a later date, this states the idea that this contract is supposed to find the token prices of all other stable pools except composable stable pools. Additionally if a stable pool is only "stable pool" why even mention explicitly about composable stable pool.

                    // Ensure that the pool is a stable pool, but not a composable stable pool.
                    // Determining the token price using a composable stable pool is sufficiently different from other
                    // stable pools, and should be added in a separate adapter/function at a later date.

Same reasoning as above, why even mention or is a composable stable pool (determined by the absence of the getLastInvariant() function)

    /// @dev                    - If the pool is not a stable pool or is a composable stable pool (determined by the absence of the `getLastInvariant()` function)

If the reasons I mentioned are not convincing enough and still gives the idea that this codebase is intended to interact with only "stable pool", I agree that this issue should be judged invalid.

[Czar102]

Did the conversation with sponsor happen on DMs or a public channel?

[10xhash]

private thread

[Czar102]

Could any parts of the code be simpler if one assumed metastable pools are not supported? I'm looking for an in-code signal that their integration was intended.

[10xhash]

https://github.com/sherlock-audit/2023-11-olympus/blob/9c8df76dc9820b4c6605d2e1e6d87dcfa9e50070/bophades/src/modules/PRICE/submodules/feeds/BalancerPoolTokenPrice.sol#L820-L847 In case of normal stable pools one can straightaway convert the returned lookupTokensPerDestinationToken to outputDecimals by multiplying with (10 ** output decimals / 10 ** 18) since Balancer always return in 18 decimals. But as highlighted in a previous audit (pls see my earlier comment), scaling factors of metastable pools will contain the rate of the asset and hence has to be divided by the scaling factor first which will convert it to the token's decimals only after which it can be converted to outputDecimals. This is how it is implemented currently following the recommendation from previous audit.

[0xJem]

Could any parts of the code be simpler if one assumed metastable pools are not supported? I'm looking for an in-code signal that their integration was intended.

They were never intended to be supported in the first place, and are not referenced in the code

[nevillehuang]

@Czar102 I believe this issue should be invalid based on the above sponsor comment here.

[Czar102]

I believe Balancer differentiates between stable pools and matastable pools like here in the folder structure.

Also, it seems MetaStableMath would be used for computations. There are no explicit mentions of usage of metastable pools in public channels. A mistake on DMs is not considered a source of truth.

Planning to invalidate the issue.

[10xhash]

1. Of course metastable pool and stable pool will be differentiated in some places since one is "metastable pool" and other is "stable pool". I am not saying stable pool == metastable pool. I am saying that with in-scope documentation/comments like this:

                   // Ensure that the pool is a stable pool, but not a composable stable pool.
                   // Determining the token price using a composable stable pool is sufficiently different from other
                   // stable pools, and should be added in a separate adapter/function at a later date.

   and the usage of "stable pool" to encompass all stable pools in Balancer, it gives the idea that the usage of stable pools in the context is the general one.

2. Both StablePool and MetaStablePool use the same math library for computations and integration wise there is no different library to be used in the stable math computation. Integration wise, like you asked before, actually things could have been done simpler if MetastablePools are not to be supported.

3. DMs in discord : I agree it should not be. But what is the source of truth here? As per me the in line comments in contest repo was indicating metastable pools, the provided past audit report was indicating metastable pools and the DM was mentioned as the last point because you asked why I thought and is not my primary reason. And the contest readme doesn't get violated.

I think this is a very bad scenario as it is not going to provide any value for the sponsor as they are not planning to use Metastable pools (stated after contest) but as I have mentioned, the information during contest doesn't reflect this.

[Czar102]

When talking about a general term for stable pools, it seems Balancer uses the term "stable-type pools" and the term "stable pools" refers to a single implementation which does not include metastable pools. See here.

Ensure that the pool is a stable pool, but not a composable stable pool.

I think it is worded that way because composable stable pools may have the same properties as stable pools, but they aren't compatible with the system.

@10xhash It seems you are right about the math library – sorry, I was wrong.

I understand your point of view and understand it was an honest mistake, likely influenced by the message on dms. Nevertheless, any other participant having what the sources they had could have understood the intentions of the sponsor correctly (not that it I'm taking into account what they were) and not submit this issue. Accepting this issue would not only be rewarding an invalid report, but also hurting all Watsons that have understood the intentions as they were publicly described.

[Czar102]

Result: Invalid Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• 0xauditsea: accepted

[10xhash]

1. I am providing the link to balancers documentation which is provided as in-line documentation in the contracts for evaluating balancer stable pool token price. https://github.com/sherlock-audit/2023-11-olympus/blob/9c8df76dc9820b4c6605d2e1e6d87dcfa9e50070/bophades/src/modules/PRICE/submodules/feeds/BalancerPoolTokenPrice.sol#L436-L439

   /// @notice                 Determines the unit price of the pool token for the Balancer stable pool specified in `params_`.
   /// @dev                    To avoid price manipulation, this function calculated the pool token price in the following manner:
   /// @dev                    - Applies a guard to protect against re-entrancy attacks on the Balancer pool
   /// @dev                    - Utilises the formula suggested by Balancer: https://docs.balancer.fi/concepts/advanced/valuing-bpt/valuing-bpt.html#on-chain-price-evaluation

https://docs.balancer.fi/concepts/advanced/valuing-bpt/valuing-bpt.html#on-chain-price-evaluation

Stable Pools

The pool.getRate() function returns the exchange rate of a BPT to the underlying base asset of the pool accounting for rate providers, if they exist. A few examples:

• The WETH/wstETH pool will return a rate relative to WETH
• bb-a-USD will return a rate relative to USD, calculated as a weighted average of the underlying stablecoins (DAI, USDC, USDT) in the nested linear pools (bb-a-DAI, bb-a-USDC, bb-a-USDT)

Stable Pools are including metastable pools and the specific example they are providing is weth/wstEth which is not a "stable pool"

2. Yes. Composable stable pools are not compatible with the system. While the other stable pools are supposed to be according to the documentation.

3. I have clarified many times the reasoning for why the information during contest points to the general usage and don't think it is a mistake from my end to come to the conclusion based on the resources provided. Yes, the dm in discord supported the belief but the primary reason that made me believe is the in-line documentation. If based on the publicly described documentation available to the watsons, any watson claims that metastable pools are excluded, the judgement would be made on the following lines of documentation:

            // Ensure that the pool is a stable pool, but not a composable stable pool.
            // Determining the LP token price using a composable stable pool is sufficiently different from other
            // stable pools, and should be added in a separate adapter/function at a later date.

    /// @dev                    - If the pool is not a stable pool or is a composable stable pool (determined by the absence of the `getLastInvariant()` function)

And the provided past audit report could be added as a supporting document

If the judgement for this is that it indeed disallows metastable pools (which I think is incorrect) ,as I stated before my view and this issue would be invalid and I would not have pursued this issue further.