search

sherlock-audit / 2022-11-float-capital-judging

2 stars 1 forks source link

WATCHPUG - `valueChange` is not applied to the `underBalancedSide` correctly in `_rebalancePoolsAndExecuteBatchedActions()` #47

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

WATCHPUG

high

valueChange is not applied to the underBalancedSide correctly in _rebalancePoolsAndExecuteBatchedActions()

Summary

valueChange should be credited to the winning side (long or short) fully if they are the overbalanced side.

But for the underbalanced side, it should only take a portion of the PNL because the float pool will take the rest.

Vulnerability Detail

In the current implementation of _rebalancePoolsAndExecuteBatchedActions() will apply all the valueChange to both sides as both the long side and the short side will go into the branch of L138-150.

At L146-147, params.valueChange is applied fully regardless if the current poolType is underBalancedSide.

Impact

The part of the PNL that belongs to the exposure provided by the float pool (floatPoolLeverage) of the underbalanced side will be double counted.

Code Snippet

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

Tool used

Manual Review

Recommendation

The PNL of the underbalanced side should be shared between the float pool and the underbalanced side pool.

JasoonS commented 1 year ago

Details on this are very vague. As far as I can see it is correct.

We divide the valueChange between the pools using the totalEffectiveLiquidityPoolType[poolType] as the denomitator which includes the Float pool.

(unlike how we split the funding fee, which uses actualTotalEffectiveLiquidityForPoolType as the denominator which excludes the funding fee).

Will get rest of team to go through this also.

It is something our multi-epoch simulations would have picked up - since this would have an effect on the pool sizes. Sum of all pool sizes should remain constant (with some minor 1 wei rounding errors).

WooSungD commented 1 year ago

Agree with Jason.

totalEffectiveLiquidityPoolType for the underbalancedSide is increased by effective leverage of float pool on L128 (https://github.com/sherlock-audit/2022-11-float-capital/blob/090c1096aacc0e7dc31bc1d00a82357f9c76fbd4/contracts/market/template/MarketCore.sol#L128).

Thus less than 100% of valueChange is distributed to pools on the underbalancedSide when divided by totalEffectiveLiquidityPoolType on L47.

moose-code commented 1 year ago

Agreed there is no issue as far as I can see, the detail missed is actualTotalEffectiveLiquidityForPoolType accounts for the float liquidity as well as all under balanced liquidity.