ClearingHouse margin calculations will break up if an AMM returning non-6 decimals positions be white listed

[code423n4]

Lines of code

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L332

Vulnerability details

Impact

It is assumed that VAMM returned positions have exactly 6 decimals for all AMMs white listed in ClearingHouse.

In the same time an array of different AMMs/VAMMs is supported, and there are no guarantees/checks of the precision of the position values they return.

If an VAMM that have different precision is whitelisted, for example having 18 decimals for position figures, then margin requirements checks become invalid.

This will lead to various malfunctions, say perfectly valid positions will be liquidated by any attacker noticing that the calculations are skewed.

Proof of Concept

ClearingHouse's _calcMarginFraction is the function that is used for margin requirements checks:

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L163-L167

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L188-L189

_calcMarginFraction calls getNotionalPositionAndMargin:

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L319-L320

getNotionalPositionAndMargin calls getTotalNotionalPositionAndUnrealizedPnl:

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L291

getTotalNotionalPositionAndUnrealizedPnl sums up AMM's getNotionalPositionAndUnrealizedPnl results:

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L269-L282

AMM's getNotionalPositionAndUnrealizedPnl returns vamm.get_notional result:

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/AMM.sol#L395-L410

The above calls are linear decimals wise (i.e. do subtractions/additions kind of operations, preserving the decimals).

Then, _getMarginFraction mixes up these notionalPosition and margin, obtained from AMM without rescaling, as if they are PRECISION scaled:

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L332

PRECISION is hard coded to be 1e6:

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L15

For other VAMM operations base precision is set to 1e18:

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/AMM.sol#L17

For example, VAMM returned supply is assumed to have 18 decimals:

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/AMM.sol#L523

Comment says that exchangeExactOut returned quantity will have 6 decimals precision:

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/AMM.sol#L495

As the system imply that VAMMs can vary it is neither guaranteed, nor checked in any way (briefly checked dydx api code, it looks like there are no explicit guarantees either).

If any of VAMM referenced via white listed AMMs return VAMM.get_notional with decimals different from 6, the _calcMarginFraction result will become grossly incorrect.

Recommended Mitigation Steps

If AMM contract is desired to deal with various VAMMs consider removing decimals related hard coding, adding decimals variables and scaling VAMM returned results accordingly, so that position and margin values' decimals of 6, implied by ClearingHouse logic, be ensured.

[atvanguard]

Protocols developers will ensure that correct decimals are used everywhere. It's not possible to assert this in code at all places.

[JasoonS]

Set to medium as unlikely that this would be done. However, checks and notices in the documentation of this code would be very important to prevent new devs from making these mistakes.