sherlock-audit / 2022-09-notional-judging

4 stars 2 forks source link

ctf_sec - getGetAmplificationParameter() precision is not used, which result in accounting issue in MetaStable2TokenAuraHelper.sol and in Boosted3TokenAuraHelper.sol #124

Open sherlock-admin opened 1 year ago

sherlock-admin commented 1 year ago

ctf_sec

high

getGetAmplificationParameter() precision is not used, which result in accounting issue in MetaStable2TokenAuraHelper.sol and in Boosted3TokenAuraHelper.sol

Summary

getGetAmplificationParameter() precision is not used, which result in accounting issue in MetaStable2TokenAuraHelper.sol and in Boosted3TokenAuraHelper.sol

Vulnerability Detail

This report has two part,

part one trace the accounting issue in MetaStable2TokenAuraHelper.sol,

part two trace the accounting issue in Boosted3TokenAuraHelper.sol,

both issue rooted in not handling the getGetAmplificationParameter() precision

According to the Balancer documentation

https://dev.balancer.fi/resources/pool-interfacing/stable-pool#amplification-parameter

pool.getGetAmplificationParameter()

returns something resembling

value : 620000 isUpdating : False precision : 1000

where the amplification parameter is 620000 / 1000 = 620

but in the code, the isUpdating and precision returned is ignored and not used.

Part One

Let's trace the function reinvestReward in MetaStable2TokenAuraHelper.sol

    function reinvestReward(
        MetaStable2TokenAuraStrategyContext calldata context,
        ReinvestRewardParams calldata params
    )

It calls

// Make sure we are joining with the right proportion to minimize slippage
        oracleContext._validateSpotPriceAndPairPrice({
            poolContext: poolContext,
            strategyContext: strategyContext,
            primaryAmount: primaryAmount,
            secondaryAmount: secondaryAmount
        });

then it calls

uint256 spotPrice = _getSpotPrice(oracleContext, poolContext, 0);

then it calls

Insite the function

        (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
        );

        spotPrice = StableMath._calcSpotPrice({
            amplificationParameter: oracleContext.ampParam,
            invariant: invariant,
            balanceX: balanceX,
            balanceY: balanceY
        });

What's wrong with this, I believe the precision has issue for ampParam

Because When we get the oracleContext.ampParam from MetaStable2TokenVaultMixin.sol

We did not use the precision returned from the pool

      (
            uint256 value,
            /* bool isUpdating */,
            /* uint256 precision */
        ) = IMetaStablePool(address(BALANCER_POOL_TOKEN)).getAmplificationParameter();

According to the Balancer documentation

https://dev.balancer.fi/resources/pool-interfacing/stable-pool#amplification-parameter

pool.getGetAmplificationParameter()

returns something resembling

value : 620000 isUpdating : False precision : 1000

where the amplification parameter is 620000 / 1000 = 620

The formula that calculate the spot price is

   /**************************************************************************************************************
    //                                                                                                           //
    //                             2.a.x.y + a.y^2 + b.y                                                         //
    // spot price Y/X = - dx/dy = -----------------------                                                        //
    //                             2.a.x.y + a.x^2 + b.x                                                         //
    //                                                                                                           //
    // n = 2                                                                                                     //
    // a = amp param * n                                                                                         //
    // b = D + a.(S - D)                                                                                         //
    // D = invariant                                                                                             //
    // S = sum of balances but x,y = 0 since x  and y are the only tokens                                        //
    **************************************************************************************************************/

the function _calcSpotPrice hardcode the amp precision to 1e3;

   uint256 internal constant _AMP_PRECISION = 1e3;

and implement

uint256 a = (amplificationParameter * 2) / _AMP_PRECISION;

if the pool's ampParameter is not equal to _AMP_PRECISION, the math will break.

Part Two

Let's trace the call in Boosted3TokenPoolUtils.sol

First the function reinvestReward in Boosted3TokenAuraHelper.sol is called

    function reinvestReward(
        Boosted3TokenAuraStrategyContext calldata context,
        ReinvestRewardParams calldata params
    ) 

Then we call

   uint256 minBPT = context.poolContext._getMinBPT(
      oracleContext, strategyContext, primaryAmount
   );

then we call

    minBPT = StableMath._calcBptOutGivenExactTokensIn({
        amp: oracleContext.ampParam,
        balances: balances,
        amountsIn: amountsIn,
        bptTotalSupply: virtualSupply,
        swapFeePercentage: 0,
        currentInvariant: invariant
    });

then we call

 // Get current and new invariants, taking swap fees into account
    uint256 newInvariant = _calculateInvariant(amp, newBalances, false);
    uint256 invariantRatio = newInvariant.divDown(currentInvariant);

then we call

  uint256 ampTimesTotal = amplificationParameter * numTokens;

we just use the amplificationParameter without handling the precision.

The amplificationParameter comes from BoostedTokenPoolMixin.sol

    (
        uint256 value,
        /* bool isUpdating */,
        /* uint256 precision */
    ) = pool.getAmplificationParameter();

the isUpdating and precision is not used,

however, according to the documentation

According to the Balancer documentation

https://dev.balancer.fi/resources/pool-interfacing/stable-pool#amplification-parameter

pool.getGetAmplificationParameter()

returns something resembling

value : 620000 isUpdating : False precision : 1000

where the amplification parameter is 620000 / 1000 = 620

Impact

The amplificationParameter has precision, ignoring the precision will result in accounting issue.

If the precision of the amplificationParameter is not equal to hardcoded 1e3, the spot price is invalid.

the code

   uint256 ampTimesTotal = amplificationParameter * numTokens;

will be overvalued because we did not divide the value by the precision.

Code Snippet

For part one

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/external/MetaStable2TokenAuraHelper.sol#L114-L153

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

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/mixins/MetaStable2TokenVaultMixin.sol#L22-L33

For part two

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

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

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

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/mixins/Boosted3TokenPoolMixin.sol#L103-L117

Tool used

Manual Review

Recommendation

We recommend the project use the precision returned from getGetAmplificationParameter()

      (
            uint256 value,
            bool isUpdating */,
            uint256 precision */
        ) = IMetaStablePool(address(BALANCER_POOL_TOKEN)).getAmplificationParameter();
        return value / precision;
jeffywu commented 1 year ago

@weitianjie2000, although I do believe in the meta stable vaults the AMP precision is hardcoded to 1e3 in practice. We should go with the value that is returned from the method.

rayn731 commented 1 year ago

Looks good, it checks the returned precision should be equal to StableMath._AMP_PRECISION.