hash - Possible incorrect price for tokens in Balancer stable pool due to amplification parameter update

[sherlock-admin2]

hash

medium

Possible incorrect price for tokens in Balancer stable pool due to amplification parameter update

Summary

Incorrect price calculation of tokens in StablePools if amplification factor is being updated

Vulnerability Detail

The amplification parameter used to calculate the invariant can be in a state of update. In such a case, the current amplification parameter can differ from the amplificaiton parameter at the time of the last invariant calculation. The current implementaiton of getTokenPriceFromStablePool doesn't consider this and always uses the amplification factor obtained by calling getLastInvariant

https://github.com/sherlock-audit/2023-11-olympus/blob/main/bophades/src/modules/PRICE/submodules/feeds/BalancerPoolTokenPrice.sol#L811-L827

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

                .....

                try pool.getLastInvariant() returns (uint256, uint256 ampFactor) {

                   // @audit the amplification factor as of the last invariant calculation is used
                    lookupTokensPerDestinationToken = StableMath._calcOutGivenIn(
                        ampFactor,
                        balances_,
                        destinationTokenIndex,
                        lookupTokenIndex,
                        1e18,
                        StableMath._calculateInvariant(ampFactor, balances_) // Sometimes the fetched invariant value does not work, so calculate it
                    );

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

        // @audit the amplification parameter can be updated
        function startAmplificationParameterUpdate(uint256 rawEndValue, uint256 endTime) external authenticate {

        // @audit for calculating the invariant the current amplification factor is obtained by calling _getAmplificationParameter()
        function _onSwapGivenIn(
        SwapRequest memory swapRequest,
        uint256[] memory balances,
        uint256 indexIn,
        uint256 indexOut
    ) internal virtual override whenNotPaused returns (uint256) {
        (uint256 currentAmp, ) = _getAmplificationParameter();
        uint256 amountOut = StableMath._calcOutGivenIn(currentAmp, balances, indexIn, indexOut, swapRequest.amount);
        return amountOut;
    }

Impact

In case the amplification parameter of a pool is being updated by the admin, wrong price will be calculated.

Code Snippet

https://github.com/sherlock-audit/2023-11-olympus/blob/main/bophades/src/modules/PRICE/submodules/feeds/BalancerPoolTokenPrice.sol#L811-L827

Tool used

Manual Review

Recommendation

Use the latest amplification factor by callling the getAmplificationParameter function

[0xJem]

This doesn't seem valid - if the amplification factor is changed since the invariant was last calculated, wouldn't the value of the invariant also be invalid?

[nevillehuang]

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

The invariant used for calculating swap amounts in Balancer is always based on the latest amplification factor hence their calculation would be latest. If there are no join actions, the cached amplification factor which is used by Olympus will not reflect the new one and will result in a different invariant and different token price.

i am attaching a poc if required: https://gist.github.com/10xhash/8e24d0765ee98def8c6409c71a7d2b17

[0xauditsea]

Escalate

This looks like invalid. Logically thinking, using getLastInvariant is more precise because the goal of this price feed is to calculate spot price of the balancer pool. If current amplification factor is used, it doesn't represent current state of the pool.

[sherlock-admin2]

Escalate

This looks like invalid. Logically thinking, using getLastInvariant is more precise because the goal of this price feed is to calculate spot price of the balancer pool. If current amplification factor is used, it doesn't represent current state of the pool.

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.

[Czar102]

@nevillehuang what do you think?

[nevillehuang]

@Czar102 I don't quite understand what @0xauditsea is pointing to. If you want to calculate the latest spot price, shouldn't you use the latest factor as indicated by the PoC by @10xhash?

[Czar102]

@0xauditsea could you explain your reasoning in more detail?

[nevillehuang]

@10xhash does this affect ALL pools intended to be integrated during the time of contest?

[10xhash]

It has to be clarified what intended to be integrated pools at the time of contest are:

1. If only the list of tokens mentioned in the readme _can be _in a pool__ ( as mentioned in previous replies this is not required per the contest definition since all tokens are not required to interact with contracts ) : There are 0 stable pools including normal, metastable etc. The only possible stable pool of any type that can be used with the above restriction is the dai-sdai metastable pool which has to be deployed in future.
2. Else it must atleast include normal stable pools and according to balancer's documentation {search startAmplificationParameterUpdate} and testing done on the dai-usdc-usdt pool, it would be affected

[Czar102]

1. Metastable pools are not supposed to be supported.
2. This documentation seems to be for Avalanche, while the contracts will be deployed on mainnet. I believe this functionality exists on mainnet too, right?

Aside from that, the impact is that the price calculated is the price at the last pool update (trade) instead of the current price?

[10xhash]

2. The link opens up to Mainnet for me, if not you would have the option to select the chain on leftside. Yes.

The impact would be that the amplification parameter used in the price calculation will be that of the last join action (addliquidity , removeliquidity) which will be different from the actual one used in the pool calculations. This will result in an incorrect price until some user performs a join operation.

[Czar102]

Adding/removing liquidity doesn't necessarily happen often. This, together with the amplification parameter change, is a very unlikely situation, nevertheless a possible one.

It's a borderline Med/Low, but I am inclined to keep this one a valid Medium. I don't understand the point made in the escalation, and @0xauditsea hasn't elaborated when asked for additional information.

[gstoyanovbg]

In determining the impact of this report, in my opinion, it should be assessed how much the price can change in the described circumstances and whether the change is significant. I conducted a foundry test that shows the change in the price of AURA_BAL at different values of the amplification factor. The test should be added to BalancerPoolTokenPriceStable.t.sol.

function test_amp_factor_impact() public {
        bytes memory params = encodeBalancerPoolParams(mockStablePool);
        uint256 price;

        mockStablePool.setLastInvariant(INVARIANT, AMP_FACTOR);
        price = balancerSubmodule.getTokenPriceFromStablePool(
            AURA_BAL,
            PRICE_DECIMALS,
            params
        );
        console.log("%d, AMP_FACTOR = 50000", price);

        mockStablePool.setLastInvariant(INVARIANT, AMP_FACTOR + 2000);
        price = balancerSubmodule.getTokenPriceFromStablePool(
            AURA_BAL,
            PRICE_DECIMALS,
            params
        );
        console.log("%d, AMP_FACTOR = 50000 + 2000", price);

        mockStablePool.setLastInvariant(INVARIANT, AMP_FACTOR + 10000);
        price = balancerSubmodule.getTokenPriceFromStablePool(
            AURA_BAL,
            PRICE_DECIMALS,
            params
        );
        console.log("%d, AMP_FACTOR = 50000 + 10000", price);

        mockStablePool.setLastInvariant(INVARIANT, AMP_FACTOR * 2);
        price = balancerSubmodule.getTokenPriceFromStablePool(
            AURA_BAL,
            PRICE_DECIMALS,
            params
        );
        console.log("%d, AMP_FACTOR = 50000 * 2", price);

        mockStablePool.setLastInvariant(INVARIANT, AMP_FACTOR * 4);
        price = balancerSubmodule.getTokenPriceFromStablePool(
            AURA_BAL,
            PRICE_DECIMALS,
            params
        );
        console.log("%d, AMP_FACTOR = 50000 * 4", price);

        mockStablePool.setLastInvariant(INVARIANT, AMP_FACTOR * 10);
        price = balancerSubmodule.getTokenPriceFromStablePool(
            AURA_BAL,
            PRICE_DECIMALS,
            params
        );
        console.log("%d, AMP_FACTOR = 50000 * 10", price);

        mockStablePool.setLastInvariant(INVARIANT, AMP_FACTOR * 100);
        price = balancerSubmodule.getTokenPriceFromStablePool(
            AURA_BAL,
            PRICE_DECIMALS,
            params
        );
        console.log("%d, AMP_FACTOR = 50000 * 100", price);
    }

16602528871962134544, AMP_FACTOR = 50000 16606565178508667081, AMP_FACTOR = 50000 + 2000 16620074517406602667, AMP_FACTOR = 50000 + 10000 16655599693391809126, AMP_FACTOR = 50000 2 16682630482761745824, AMP_FACTOR = 50000 4 16699011129392628938, AMP_FACTOR = 50000 10 16708898633935285195, AMP_FACTOR = 50000 100

From the obtained results, it can be seen that the change in price is small. Even if we increase it 100 times to the maximum possible value of 5000 * 10^3, the change in price is around 0.1 (0.63%). For such a large increase of the amplification factor, it would take about 7 days (2x per day). Another question is what is the chance that there will be no join or exit within these 7 days.

@Czar102 I don't know if this is significant enough change in the price for Sherlock, but wanted to share it to be sure it will be taken into consideration.

[Czar102]

@gstoyanovbg Thank you for the test, it looks like this should be a low severity issue.

@10xhash Can you provide a scenario where the price would be altered by more than 5%?

[10xhash]

Place the test inside test/ and run forge test --mt testHash_AmplificationDiff5 It is asserted that the diff in price is > 5% when the current amplification parameter is divided by 6 with a 4 day period. Dividing by 6 would make the pool close to 8000 (currently 50000).

pragma solidity 0.8.15;

import "forge-std/Test.sol";
import {IStablePool} from "src/libraries/Balancer/interfaces/IStablePool.sol";
import {IVault} from "src/libraries/Balancer/interfaces/IVault.sol";
import {FullMath} from "src/libraries/FullMath.sol";
import {StableMath} from "src/libraries/Balancer/math/StableMath.sol";
import {IVault} from "src/libraries/Balancer/interfaces/IVault.sol";
import {IBasePool} from "src/libraries/Balancer/interfaces/IBasePool.sol";
import {IWeightedPool} from "src/libraries/Balancer/interfaces/IWeightedPool.sol";
import {IStablePool} from "src/libraries/Balancer/interfaces/IStablePool.sol";
import {VaultReentrancyLib} from "src/libraries/Balancer/contracts/VaultReentrancyLib.sol";
import {LogExpMath} from "src/libraries/Balancer/math/LogExpMath.sol";
import {FixedPoint} from "src/libraries/Balancer/math/FixedPoint.sol";

interface IStablePoolWithAmp is IStablePool {
    function getAmplificationParameter()
        external
        view
        returns (uint amp, bool isUpdating, uint precision);

        function startAmplificationParameterUpdate(uint256 rawEndValue, uint256 endTime) external;
}

interface IERC20 {
    function approve(address spender,uint amount) external;
} 

enum SwapKind { GIVEN_IN, GIVEN_OUT }

struct SingleSwap {
        bytes32 poolId;
        SwapKind kind;
        address assetIn;
        address assetOut;
        uint256 amount;
        bytes userData;
    }

        struct FundManagement {
        address sender;
        bool fromInternalBalance;
        address payable recipient;
        bool toInternalBalance;
    }

interface VaultWithSwap is IVault{
    function swap(
        SingleSwap memory singleSwap,
        FundManagement memory funds,
        uint256 limit,
        uint256 deadline
    ) external payable returns (uint256);
}

contract PriceTest is Test {
    using FullMath for uint256;

    function testHash_AmplificationDiff5() public {
        VaultWithSwap balVault = VaultWithSwap(0xBA12222222228d8Ba445958a75a0704d566BF2C8);
        IStablePoolWithAmp pool = IStablePoolWithAmp(0x3dd0843A028C86e0b760b1A76929d1C5Ef93a2dd);

        (, uint cachedAmpFactor) = pool.getLastInvariant();
        {
            (, bool isUpdating, ) = pool.getAmplificationParameter();
             assert(isUpdating == false);
        }

        console.log("cahced factor",cachedAmpFactor);

        {
        address mainnetFeeSetter = 0xf4A80929163C5179Ca042E1B292F5EFBBE3D89e6;

        vm.startPrank(mainnetFeeSetter);
        pool.startAmplificationParameterUpdate(cachedAmpFactor / 6 / 1e3, block.timestamp + 4 days);

        vm.warp(block.timestamp + 4 days + 100);

        // perform swaps to update the balances with latest amp factor
        {
            (uint amp,bool isUpdating , ) = pool.getAmplificationParameter();
            assert(isUpdating == false);
        }

        console.log("amp params set");
        }

        uint[] memory balances_;
        uint actualAmpFactor;
{
bytes32 poolId = pool.getPoolId();
        (actualAmpFactor, , ) = pool.getAmplificationParameter();

        (,  balances_, ) = balVault.getPoolTokens(poolId);
        uint256[] memory scalingFactors = pool.getScalingFactors();
        {

            uint256 len = scalingFactors.length;
            for (uint256 i; i < len; ++i) {
                balances_[i] = FixedPoint.mulDown(balances_[i], scalingFactors[i]);
            }
        }
}

        // lookup token auraBal and destination token lp token

        uint oldCachedPrice;
        uint newAmpFactorPrice;
        {
            uint destinationTokenIndex = 0;
        uint lookupTokenIndex = 1;
            console.log("calculation with previous amp factor");
            uint lookupTokensPerDestinationToken;
            lookupTokensPerDestinationToken = StableMath._calcOutGivenIn(
                cachedAmpFactor,
                balances_,
                destinationTokenIndex,
                lookupTokenIndex,
                1e18,
                StableMath._calculateInvariant(cachedAmpFactor, balances_)
            );

            // Downscale the amount to token decimals
        uint256[] memory scalingFactors = pool.getScalingFactors();
            lookupTokensPerDestinationToken = FixedPoint.divDown(
                lookupTokensPerDestinationToken,
                scalingFactors[lookupTokenIndex]
            );

            uint outputDecimals = 8;

            lookupTokensPerDestinationToken =
                (lookupTokensPerDestinationToken * 10 ** outputDecimals) /
                1e18;

            uint destinationTokenPrice =  1127000000;
            console.log("bal lp price", destinationTokenPrice);
            uint lookupTokenPrice;

            lookupTokenPrice = destinationTokenPrice.mulDiv(
                10 ** outputDecimals,
                lookupTokensPerDestinationToken
            );
            oldCachedPrice = lookupTokenPrice;
            console.log("aurabal price", lookupTokenPrice);
        }

        {
            uint destinationTokenIndex = 0;
        uint lookupTokenIndex = 1;
            console.log("calculation with updated amp factor");
            uint lookupTokensPerDestinationToken;
            lookupTokensPerDestinationToken = StableMath._calcOutGivenIn(
                actualAmpFactor,
                balances_,
                destinationTokenIndex,
                lookupTokenIndex,
                1e18,
                StableMath._calculateInvariant(actualAmpFactor, balances_)
            );

            // Downscale the amount to token decimals
        uint256[] memory scalingFactors = pool.getScalingFactors();
            lookupTokensPerDestinationToken = FixedPoint.divDown(
                lookupTokensPerDestinationToken,
                scalingFactors[lookupTokenIndex]
            );

            uint outputDecimals = 8;

            lookupTokensPerDestinationToken =
                (lookupTokensPerDestinationToken * 10 ** outputDecimals) /
                1e18;

            uint destinationTokenPrice = 1127000000;
            console.log("bal lp price", destinationTokenPrice);
            uint lookupTokenPrice;

            lookupTokenPrice = destinationTokenPrice.mulDiv(
                10 ** outputDecimals,
                lookupTokensPerDestinationToken
            );
            newAmpFactorPrice = lookupTokenPrice;
            console.log("aurabal price", lookupTokenPrice);
        }

        assert((oldCachedPrice -newAmpFactorPrice) * 100 * 1e18 / newAmpFactorPrice > 5 ether);

    }

}

[gstoyanovbg]

@10xhash well done, i think your test is valid and shows a significant price change.

[Czar102]

Thank you @10xhash! Planning to leave the issue as is.

[Czar102]

Result: Medium Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• 0xauditsea: rejected