Healthy positions can be liquidated as margin calculations ignore decimals of the components

[code423n4]

Lines of code

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

Vulnerability details

Impact

It is assumed that Oracle reported price have the same decimals as fee variables (which have fixed decimals of 6) and VAMM positions (which have base asset decimals).

In fact, Oracle reported price have decimals for individual tokens of either 8 (for USD based feeds, which is the main case for Hubble) or 18 (ETH based feeds).

As a result, margin calculations are incorrect, having parts being added together with different decimals. This can lead to ignoring either market value of collateral (when base asset have more decimals than Oracle price feed, say ETH position with 18 decimals and ETH / USD price feed with 8 decimals) or unrealized P&L and funding fees (when it is otherwise, say Oracle feed of USDC / USD have 8 decimals, but USDC have only 6).

As the notional position will have base asset decimals, this will generally lead to margin ratio understatement and to the liquidations by any malicious actors, who noticed that, of the healthy positions of the Hubble traders.

Also, isLiquidatable miscalculates incentivePerDollar variable by ignoring the spot part, making liquidations even more desirable.

Proof of Concept

Oracle.getUnderlyingPrice just passes on the latest Oracle answer, not checking it anyhow:

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/Oracle.sol#L24-L35

MarginAccount.weightedAndSpotCollateral returns getUnderlyingPrice sized results (collateral decimals and weight decimals of 6 are both removed):

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/MarginAccount.sol#L524-L528

Chainlink oracles for non-index assets have either 8 or 18 decimals (USD or ETH based pairs correspondingly):

https://docs.chain.link/docs/ethereum-addresses/

There two key occurrences in the core logic where getUnderlyingPrice sized results (say 8 decimals for USD based Oracles) are treated as if they have decimals of 6 (as VUSD and fee PRECISION have) or decimals of the base asset (say 18 for ETH itself; while ETH / USD Chainlink feed still has 8 decimals being USD based).

Namely:

1. isLiquidatable comparing spot (8 decimals) with liquidationIncentive (6 decimals) in order to add the minimum of the two to incentivePerDollar (= PRECISION, 6 decimals):

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

2. getSpotCollateralValue and getNormalizedMargin use weightedAndSpotCollateral results and so are Oracle.getUnderlyingPrice sized:

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/MarginAccount.sol#L505-L511

While getSpotCollateralValue is only compared to 0 (apart from Leaderboard), getNormalizedMargin is added linearly to unrealizedPnl and getTotalFunding:

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

getTotalNotionalPositionAndUnrealizedPnl's unrealizedPnl have AMM position decimals:

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

getTotalFunding have AMM base asset decimals (AMM's positions[].size constructed from baseAssetQuantity):

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

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

Recommended Mitigation Steps

Consider adding decimals variables and scaling Oracle.getUnderlyingPrice results to match decimals of 6 when combining it with the system fee variables and to match base asset decimals of AMM returned position results when dealing with P&L and funding figures, so that equal precision of the components be ensured in the formulas.

[atvanguard]

This report is inaccurate. We only use chainlink USD pairings, which are guaranteed to have 8 decimals of precision and oracle.getUnderlyingPrice scales down the price to 6 decimals which is then consumed everywhere.

[JasoonS]

Need to have a think on this and might circle back.

It is possible that you wish to use other oracles with the system besides chainlink and/or 8 decimals.

However you'd need to change that line to answer /= 1000000000000 or whatever is necessary.

I'll put this as Low since it is more a restriction on your flexibility for oracle feeds.

[moose-code]

Agree with Jason, its very restrictive, and magic numbers dividing by a certain decimal are certainly not ideal, but will class this as low based on a low probability of this being a security issue.

[JeeberC4]

Grouping with warden's QA Report #30