Gas Optimizations

[code423n4]

Title: Rearrange state variables
Severity: GAS

You can change the order of the storage variables to decrease memory uses.

In InsuranceFund.sol,rearranging the storage fields can optimize to: 4 slots from: 5 slots.
The new order of types (you choose the actual variables):

    1. uint
    2. IERC20
    3. uint
    4. address
    5. uint8

Title: Prefix increments are cheaper than postfix increments
Severity: GAS

Prefix increments are cheaper than postfix increments.
Further more, using unchecked {++x} is even more gas efficient, and the gas saving accumulates every iteration and can make a real change
There is no risk of overflow caused by increamenting the iteration index in for loops (the ++i in for (uint256 i = 0; i < numIterations; ++i)).
But increments perform overflow checks that are not necessary in this case.
These functions use not using prefix increments (++x) or not using the unchecked keyword:

    change to prefix increment and unchecked: MarginAccount.sol, i, 552
    change to prefix increment and unchecked: ClearingHouse.sol, i, 194
    change to prefix increment and unchecked: ClearingHouse.sol, i, 277
    change to prefix increment and unchecked: MarginAccount.sol, i, 373
    change to prefix increment and unchecked: MarginAccount.sol, i, 521
    change to prefix increment and unchecked: ClearingHouse.sol, i, 122
    change to prefix increment and unchecked: ClearingHouse.sol, i, 251
    change to prefix increment and unchecked: MarginAccount.sol, i, 331
    change to prefix increment and unchecked: ClearingHouse.sol, i, 170
    change to prefix increment and unchecked: ClearingHouse.sol, i, 263
    change to prefix increment and unchecked: ClearingHouse.sol, i, 130

Title: Public functions to external
Severity: GAS

The following functions could be set external to save gas and improve code quality.
External call cost is less expensive than of public functions.

    InsuranceFund.sol, decimals
    MarginAccount.sol, syncDeps
    InsuranceFund.sol, syncDeps
    Oracle.sol, getUnderlyingTwapPrice
    ClearingHouse.sol, isMaker
    MarginAccount.sol, getNormalizedMargin
    ClearingHouse.sol, liquidateTaker
    ClearingHouse.sol, liquidateMaker
    ClearingHouse.sol, isAboveMinAllowableMargin
    VUSD.sol, decimals

Title: Consider inline the following functions to save gas
Severity: GAS

You can inline the following functions instead of writing a specific function to save gas.
(see https://github.com/code-423n4/2021-11-nested-findings/issues/167 for a similar issue.)


    MarginAccount.sol, _min, { return a < b ? a : b; }
    Oracle.sol, _blockTimestamp, { return block.timestamp; }
    AMM.sol, _blockTimestamp, { return block.timestamp; }
    AMM.sol, abs, { return x >= 0 ? x : -x; }
    MarginAccount.sol, _scaleDecimals, { return amount * (10 ** decimals); }
    ClearingHouse.sol, _calculateTradeFee, { return quoteAsset * tradeFee / PRECISION; }
    Oracle.sol, formatPrice, { return (_price / 100).toInt256(); // 6 decimals }
    ClearingHouse.sol, _calculateLiquidationPenalty, { return quoteAsset * liquidationPenalty / PRECISION; }

Title: Unnecessary cast
Severity: Gas

    address ClearingHouse.sol.updatePositions - unnecessary casting address(trader)

Title: Inline one time use functions
Severity: GAS

The following functions are used exactly once. Therefore you can inline them and save gas and improve code clearness.

    ClearingHouse.sol, _calculateTradeFee
    AMM.sol, _getPnlWhileReducingPosition
    AMM.sol, _openReversePosition
    AMM.sol, _updateFundingRate
    ClearingHouse.sol, _calculateLiquidationPenalty
    MarginAccount.sol, _liquidateFlexible
    AMM.sol, _calcTwap
    Oracle.sol, getLatestRoundData
    MarginAccount.sol, _scaleDecimals
    ClearingHouse.sol, _getMarginFraction

Title: Unnecessary equals boolean
Severity: GAS

Boolean variables can be checked within conditionals directly without the use of equality operators to true/false.

    AMM.sol, 121: bool isNewPosition = position.size == 0 ? true : false;

Title: Short the following require messages
Severity: GAS

The following require messages are of length more than 32 and we think are short enough to short
them into exactly 32 characters such that it will be placed in one slot of memory and the require
function will cost less gas.
The list:

    Solidity file: AMM.sol, In line 511, Require message length to shorten: 38, The message: VAMM._short: baseAssetQuantity is >= 0
    Solidity file: MarginAccount.sol, In line 453, Require message length to shorten: 37, The message: Need to repay more to seize that much
    Solidity file: AMM.sol, In line 487, Require message length to shorten: 37, The message: VAMM._long: baseAssetQuantity is <= 0
    Solidity file: ClearingHouse.sol, In line 101, Require message length to shorten: 34, The message: CH: Below Minimum Allowable Margin

Title: Unnecessary index init
Severity: GAS

In for loops you initialize the index to start from 0, but it already initialized to 0 in default and this assignment cost gas.
It is more clear and gas efficient to declare without assigning 0 and will have the same meaning:

    ClearingHouse.sol, 263
    MarginAccount.sol, 331
    ClearingHouse.sol, 130
    MarginAccount.sol, 552
    ClearingHouse.sol, 277
    ClearingHouse.sol, 170
    ClearingHouse.sol, 194
    ClearingHouse.sol, 122
    MarginAccount.sol, 521
    ClearingHouse.sol, 251

Title: Unused state variables
Severity: GAS

Unused state variables are gas consuming at deployment (since they are located in storage) and are
a bad code practice. Removing those variables will decrease deployment gas cost and improve code quality.
This is a full list of all the unused storage variables we found in your code base.

    VUSD.sol, __gap
    Oracle.sol, __gap
    InsuranceFund.sol, __gap
    ClearingHouse.sol, __gap
    MarginAccount.sol, __gap
    AMM.sol, __gap

Title: Unnecessary constructor
Severity: GAS

The following constructors are empty.
(A similar issue code-423n4/2021-11-fei-findings#12)

    ClearingHouse.sol.constructor
    MarginAccount.sol.constructor

Title: Caching array length can save gas
Severity: GAS

Caching the array length is more gas efficient.
This is because access to a local variable in solidity is more efficient than query storage / calldata / memory.
We recommend to change from:

for (uint256 i=0; i<array.length; i++) { ... }

to:

uint len = array.length  
for (uint256 i=0; i<len; i++) { ... }


    ClearingHouse.sol, amms, 263
    ClearingHouse.sol, amms, 122
    MarginAccount.sol, _collaterals, 552
    ClearingHouse.sol, amms, 194
    ClearingHouse.sol, amms, 251
    ClearingHouse.sol, amms, 277
    ClearingHouse.sol, amms, 130
    MarginAccount.sol, assets, 373
    MarginAccount.sol, idxs, 331
    MarginAccount.sol, assets, 521
    ClearingHouse.sol, amms, 170

Title: Storage double reading. Could save SLOAD
Severity: GAS

Reading a storage variable is gas costly (SLOAD). In cases of multiple read of a storage variable in the same scope, caching the first read (i.e saving as a local variable) can save gas and decrease the
overall gas uses. The following is a list of functions and the storage variables that you read twice:

    MarginAccount.sol: PRECISION is read twice in isLiquidatable

Title: Unused imports
Severity: GAS

In the following files there are contract imports that aren't used
Import of unnecessary files costs deployment gas (and is a bad coding practice that is important to ignore)

    InsuranceFund.sol, line 10, import { IRegistry } from "./Interfaces.sol";
    AMM.sol, line 4, import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol";
    Oracle.sol, line 4, import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol";
    ClearingHouse.sol, line 7, import { IAMM, IInsuranceFund, IMarginAccount, IClearingHouse } from "./Interfaces.sol";
    Oracle.sol, line 8, import { AggregatorV3Interface } from "./Interfaces.sol";
    AMM.sol, line 8, import { ERC20Detailed, IOracle, IRegistry, IVAMM, IAMM } from "./Interfaces.sol";

Title: State variables that could be set immutable
Severity: GAS

In the following files there are state variables that could be set immutable to save gas.

    vusd in MarginAccount.sol
    insuranceFund in ClearingHouse.sol
    name in AMM.sol
    marginAccount in ClearingHouse.sol
    vamm in AMM.sol
    vusd in ClearingHouse.sol
    underlyingAsset in AMM.sol

Title: Unnecessary default assignment
Severity: GAS

Unnecessary default assignments, you can just declare and it will save gas and have the same meaning.

    MarginAccount.sol (L#31) : uint constant VUSD_IDX = 0; 

Title: Use calldata instead of memory
Severity: GAS

Use calldata instead of memory for function parameters
In some cases, having function arguments in calldata instead of
memory is more optimal.

    AMM.initialize (_name)

Title: Internal functions to private
Severity: GAS

The following functions could be set private to save gas and improve code quality:

    AMM.sol, _increasePosition
    MarginAccount.sol, _getLiquidationInfo
    Oracle.sol, formatPrice
    ClearingHouse.sol, _disperseLiquidationFee
    AMM.sol, _openReversePosition
    MarginAccount.sol, _transferInVusd
    AMM.sol, _calcTwap        

Title: Use != 0 instead of > 0
Severity: GAS

Using != 0 is slightly cheaper than > 0. (see code-423n4/2021-12-maple-findings#75 for similar issue)

    MarginAccount.sol, 150: change 'amount > 0' to 'amount != 0'
    AMM.sol, 450: change 'baseAssetQuantity > 0' to 'baseAssetQuantity != 0'
    InsuranceFund.sol, 78: change 'pendingObligation > 0' to 'pendingObligation != 0'
    AMM.sol, 122: change 'baseAssetQuantity > 0' to 'baseAssetQuantity != 0'
    Oracle.sol, 153: change '_round > 0' to '_round != 0'
    VUSD.sol, 55: change 'balance > 0' to 'balance != 0'
    MarginAccount.sol, 574: change 'credit > 0' to 'credit != 0'
    AMM.sol, 487: change 'baseAssetQuantity > 0' to 'baseAssetQuantity != 0'
    AMM.sol, 562: change 'takerPosition > 0' to 'takerPosition != 0'
    MarginAccount.sol, 174: change 'balance > 0' to 'balance != 0'
    ClearingHouse.sol, 51: change '_maintenanceMargin > 0' to '_maintenanceMargin != 0'
    AMM.sol, 297: change 'positionSize > 0' to 'positionSize != 0'
    MarginAccount.sol, 175: change 'balance > 0' to 'balance != 0'
    AMM.sol, 573: change 'baseAssetQuantity > 0' to 'baseAssetQuantity != 0'
    ClearingHouse.sol, 211: change 'liquidationFee > 0' to 'liquidationFee != 0'