Accounting issue in MarketCore.sol#_rebalancePoolsAndExecuteBatchedAction

Summary

MarketCore.sol#_rebalancePoolsAndExecuteBatchedAction has accounting issue.

Vulnerability Detail

Let us check the implementation below:

        if (poolType != FLOAT_TYPE) {
          // To correctly apportion funding owed for the underblananced tiers, we need to remove the float liquidity contribution
          int256 actualTotalEffectiveLiquidityForPoolType = int256(
            (uint256(totalEffectiveLiquidityPoolType[poolType]) -
              (poolType == params.underBalancedSide ? uint256(pools[PoolType.FLOAT][0].value).mul(floatPoolLeverage.abs()) : 0))
          );

          // Long and short pools both pay funding
          poolValue +=
            (((poolValue * poolFixedConfig.leverage * params.valueChange) / int128(totalEffectiveLiquidityPoolType[poolType])) -
              ((poolValue * poolFixedConfig.leverage * params.fundingAmount[poolType]) / (actualTotalEffectiveLiquidityForPoolType))) /
            1e18;
        } else {
          // Float pool recieves all funding and fees.
          poolValue +=
            ((poolValue * floatPoolLeverage * params.valueChange) /
              (int256(uint256(totalEffectiveLiquidityPoolType[params.underBalancedSide])) * 1e18)) +
            -params.fundingAmount[SHORT_TYPE] + // funding value is negative for short side (double negative to add it)
            params.fundingAmount[LONG_TYPE] +
            int256(feesToDistribute[epochIndex & 1]);

          feesToDistribute[epochIndex & 1] = 0;
        }

note these two line, one is not * 10e18

    poolValue +=
      (((poolValue * poolFixedConfig.leverage * params.valueChange) / int128(totalEffectiveLiquidityPoolType[poolType])) -

one is * 1e18,

  ((poolValue * floatPoolLeverage * params.valueChange) /
     (int256(uint256(totalEffectiveLiquidityPoolType[params.underBalancedSide])) * 1e18)) +

Also division math use two division in a row, resulting in value truncation.

   ((poolValue * poolFixedConfig.leverage * params.fundingAmount[poolType]) / (actualTotalEffectiveLiquidityForPoolType))) /
   1e18;

same truncation happens in

    ((poolValue * floatPoolLeverage * params.valueChange) /
       (int256(uint256(totalEffectiveLiquidityPoolType[params.underBalancedSide])) * 1e18)) +

Impact

If the pool value is not accurately calculated because of the truncation in divisoin, then the funding rate would be inaccurate.

Code Snippet

https://github.com/sherlock-audit/2022-11-float-capital/blob/main/contracts/market/template/MarketCore.sol#L144-L161

Tool used

Manual Review

Recommendation

We recommend the make sure the pool value addition or subtraction value is in the same scale and making sure the pool liquidity is not truncated by division. Make sure that multiplication comes before division and never use division by multiplication.

I think we should always favor multiplication over division if we need scaling.

For example, we can change from

          // Long and short pools both pay funding
          poolValue +=
            (((poolValue * poolFixedConfig.leverage * params.valueChange) / int128(totalEffectiveLiquidityPoolType[poolType])) -
              ((poolValue * poolFixedConfig.leverage * params.fundingAmount[poolType]) / (actualTotalEffectiveLiquidityForPoolType))) /
            1e18;

to

          // Long and short pools both pay funding
          poolValue +=
            (((poolValue * poolFixedConfig.leverage * params.valueChange) / int128(totalEffectiveLiquidityPoolType[poolType] * 1e18)) -
              ((poolValue * poolFixedConfig.leverage * params.fundingAmount[poolType]) / (actualTotalEffectiveLiquidityForPoolType)))

JasoonS commented 1 year ago

Hmm - in his example I'd argue that it worsened the precision.

(also I think he forgot one of the second / 1e18, or the / (actualTotalEffectiveLiquidityForPoolType * 1e18))

For example (asume we are working with decimals of 2 precision:

(1.25e2 + 1.8e2) / 1e3 = 305 / 100 = 3

But
(1.25e2 / 1e3) + (1.8e2 / 1e3) / 1e3 = 1 + 1 = 2

So precision is worsened by following this example.

Stentonian commented 1 year ago

Seems like either way will give you some good rounding and some bad rounding. Code:

def test(_fundingAmount,_actualTotalEffectiveLiquidityForPoolType,_valueChange,_totalEffectiveLiquidityPoolType,_leverage,_poolValue):
    prec = 1e10
    poolValue = _poolValue * prec
    leverage = _leverage * prec
    totalEffectiveLiquidityPoolType = _totalEffectiveLiquidityPoolType * prec
    valueChange = _valueChange * prec
    actualTotalEffectiveLiquidityForPoolType = _actualTotalEffectiveLiquidityForPoolType * prec
    fundingAmount = _fundingAmount * prec

    us = poolValue
    them = poolValue
    perfect = poolValue

    us += (((poolValue * leverage * valueChange) // (totalEffectiveLiquidityPoolType)) - ((poolValue * leverage * fundingAmount) // (actualTotalEffectiveLiquidityForPoolType))) // prec

    them += (((poolValue * leverage * valueChange) // (totalEffectiveLiquidityPoolType * prec)) - ((poolValue * leverage * fundingAmount) // (actualTotalEffectiveLiquidityForPoolType * prec)))

    perfect += (((poolValue * leverage * valueChange) / (totalEffectiveLiquidityPoolType)) - ((poolValue * leverage * fundingAmount) / (actualTotalEffectiveLiquidityForPoolType))) / prec

    return us, them, perfect

good = 0
bad = 0
same = 0

for a in range(7, 1000, 101):
    for b in range(5, 1000, 111):
        for c in range(11, 1000, 107):
            for d in range(13, 1000, 121):
                for e in range(17, 1000, 101):
                    for f in range(19, 1000, 131):
                        us, them, actual = test(a,b,c,d,e,f)
                        if actual < 0:
                            continue
                        if round(actual) == them and round(actual) != us:
                            bad += 1
                        elif round(actual) != them and round(actual) == us:
                            good += 1
                        else:
                            same += 1

print("same", same)
print("bad", bad)
print("good", good)

Results:

same 155070                                                                                                  
bad 21039                                                                           
good 139627