QA Report

[code423n4]

QA Report

Table of Contents:

• QA Report
  • Foreword
  • Summary
  • File: AMM.sol
    • Imports
      • Useless import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol"
    • function addLiquidity()
      • A Magical number should be explained: 1e30
    • function lastPrice()
      • A Magical number should be explained: 1e12
    • function _updateFundingRate()
      • A Magical number should be explained: 1e6
  • File: ClearingHouse.sol
    • General
      • Unbounded iteration over all amms
    • function _liquidateTaker()
      • Use same revert string on L189 as L166 for consistency: "CH: Above Maintenance Margin"
  • File: MarginAccount.sol
    • function weightedAndSpotCollateral()
      • A Magical number should be explained: 6
  • File: Oracle.sol
    • Imports
      • Useless import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol"
  • File: Registry.sol
    • constructor
      • Missing address(0) checks

Foreword

• @audit tags

The code is annotated at multiple places with //@audit comments to pinpoint the issues. Please, pay attention to them for more details.

Summary

• All initialize() or ìnit() functions are front-runnable in the whole solution. I suggest adding some access control to them:

contracts/AMM.sol:
  93:     function initialize(

contracts/ClearingHouse.sol:
  35:     function initialize(

contracts/InsuranceFund.sol:
  34:     function initialize(address _governance) external {

contracts/MarginAccount.sol:
  121:     function initialize(

contracts/Oracle.sol:
  20:     function initialize(address _governance) external initializer {

contracts/VUSD.sol:
  38:         super.initialize("Hubble USD", "hUSD"); // has initializer modifier

• There are 4 Open TODOs to not forget before deployment:

contracts/AMM.sol:
  142:         // sending market orders can fk the trader. @todo put some safe guards around price of liquidations
  555:             // @todo handle case when totalPosition = 0

contracts/ClearingHouse.sol:
  172:             // @todo put checks on slippage

contracts/MarginAccount.sol:
  277:             @todo consider providing some incentive from insurance fund to execute a liquidation in this scenario.

• While Ownable is never used, it's imported twice.
• Several places use magic numbers. I suggest either adding comments or storing the values in well-named constants for maintainability and readability.
• All @param comments concerning timestamp are missing on the events in MarginAccount.sol
• For the future of the solution and as a good practice, consider calculating the balance before and after a transfer:

*From:*
File: InsuranceFund.sol
51:         vusd.safeTransferFrom(msg.sender, address(this), _amount);

*To:*
51:         uint256 balanceBefore = vusd.balanceOf(address(this));
52:         vusd.safeTransferFrom(msg.sender, address(this), _amount);
53:         uint256 receivedAmount = vusd.balanceOf(address(this)) - balanceBefore;

File: AMM.sol

Imports

Useless import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol"

Ownable capabilities are not used in this contract.

function addLiquidity()

A Magical number should be explained: 1e30

Please, either add a comment or store the value in a constant for maintainability and readability. Here, 1e30 isn't easily guessable:

190:             quoteAsset = baseAssetQuantity * vamm.price_scale() / 1e30; //@audit magical number should be explained

function lastPrice()

A Magical number should be explained: 1e12

Please, either add a comment or store the value in a constant for maintainability and readability. Here, 1e12 isn't easily guessable:

470:         return vamm.last_prices() / 1e12; //@audit magical number should be explained

function _updateFundingRate()

A Magical number should be explained: 1e6

Here, we can guess that it's the number of decimals. However, hardcoding this value in the code is bad practice. Consider storing it in a constant.

710:         fundingRate = _premiumFraction * 1e6 / _underlyingPrice; //@audit magical number should be explained

File: ClearingHouse.sol

General

Unbounded iteration over all amms

There are many for-loops iterating over the dynamic array IAMM[] amms:

ClearingHouse.sol:122:        for (uint i = 0; i < amms.length; i++) {
ClearingHouse.sol:130:        for (uint i = 0; i < amms.length; i++) {
ClearingHouse.sol:170:        for (uint i = 0; i < amms.length; i++) {
ClearingHouse.sol:194:        for (uint i = 0; i < amms.length; i++) { // liquidate all positions
ClearingHouse.sol:251:        for (uint i = 0; i < amms.length; i++) {
ClearingHouse.sol:263:        for (uint i = 0; i < amms.length; i++) {
ClearingHouse.sol:277:        for (uint i = 0; i < amms.length; i++) {

I suggest being very careful as the execution may exceed the block gas limit, consume all the gas provided, and fail.
A removal function is missing here.
You can consider introducing max limits on items in the arrays or make sure that elements can be removed from dynamic arrays in case it becomes too large.

function _liquidateTaker()

Use same revert string on L189 as L166 for consistency: "CH: Above Maintenance Margin"

I suggest using the same string on L189 as L166 for consistency:

166:             "CH: Above Maintenance Margin"
...
189:         require(_calcMarginFraction(trader, false /* check funding payments again */) < maintenanceMargin, "Above Maintenance Margin");

File: MarginAccount.sol

function weightedAndSpotCollateral()

A Magical number should be explained: 6

Here, we can guess that it's the number of decimals. However, hardcoding this value in the code is bad practice. Consider storing it in a constant.

File: MarginAccount.sol
528:             weighted += (numerator * _collateral.weight.toInt256() / int(10 ** (denomDecimals + 6)));

File: Oracle.sol

Imports

Useless import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol"

Ownable capabilities are not used in this contract.

File: Registry.sol

constructor

Missing address(0) checks

Immutable addresses should be checked for address(0) to avoid needed to redeploy the contract.
It's done here:

File: VUSD.sol
32:     constructor(address _reserveToken) {
33:         require(_reserveToken != address(0), "vUSD: null _reserveToken");
34:         reserveToken = IERC20(_reserveToken);
35:     }

Therefore, I suggest doing the same for the addresses in Registry.sol:

12:     constructor(
13:         address _oracle,
14:         address _clearingHouse,
15:         address _insuranceFund,
16:         address _marginAccount,
17:         address _vusd
18:     ) {
19: 
20:         oracle = _oracle; //@audit missing address(0) check
21:         clearingHouse = _clearingHouse; //@audit missing address(0) check
22:         insuranceFund = _insuranceFund; //@audit missing address(0) check
23:         marginAccount = _marginAccount; //@audit missing address(0) check
24:         vusd = _vusd; //@audit missing address(0) check
25:     }

[atvanguard]

1 issue duplicate of #97, others acknowledged severity = 0.

[moose-code]

"Several places use magic numbers. I suggest either adding comments or storing the values in well-named constants for maintainability and readability." This is a very good take. Helps make it much easier to audit. Something off by 1e6 could be catastrophic