Hubble contest - rfa's results

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

the comment:

            makerNotional = newNotional * makerPos / totalPos //<-------- This line
            if (side remains same)
            reducedOpenNotional = takerOpenNotional * makerPos / takerPos
            pnl = makerNotional - reducedOpenNotional

and the actual code was

 uint totalPosition = abs(makerPosition + takerPosition).toUint256();
        if (abs(takerPosition) > abs(makerPosition)) {  // taker position side remains same
            uint reducedOpenNotional = takerOpenNotional * abs(makerPosition).toUint256() / 
            abs(takerPosition).toUint256(); 
            uint makerNotional = newNotional * abs(makerPosition).toUint256() / totalPosition; //<------- this line
            pnlToBeRealized = _getPnlToBeRealized(takerPosition, makerNotional, reducedOpenNotional);

the line

 uint makerNotional = newNotional * abs(makerPosition).toUint256() / totalPosition;

was intended to executed outside of if() body(Not sure which one is the correct, the comment or the code)

##GAS OPTIMAZATION

#1 Better increment for saving more gas

Using ++i for all the loops in the variable. it is known and common case that implementation by using ++i can cost less gas per iteration than i++

##POC it can bee seen from here : https://github.com/ethereum/solidity/issues/10695

##Occurance

main/contracts/ClearingHouse.sol#L122;
main/contracts/ClearingHouse.sol#L170; 
main/contracts/ClearingHouse.sol#L194;
main/contracts/ClearingHouse.sol#L251;
main/contracts/ClearingHouse.sol#L263;
main/contracts/ClearingHouse.sol#L278;
main/contracts/MarginAccount.sol#L331;
main/contracts/MarginAccount.sol#L373;
main/contracts/MarginAccount.sol#L521;
main/contracts/MarginAccount.sol#L552;

##Mitigation

i++ change to ++i

#2 Using storage instead memory for saving more gas

https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/AMM.sol#L588 https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/VUSD.sol#L58

instead of caching with memory it can be used by caching with storage, just read it directly for saving more gas

change to

Position storage position = positions[trader]; Withdrawal storage withdrawal = withdrawals[i];

#3 using && is more expensive gas

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/MarginAccount.sol#L461 using mutiple require() is cheaper than use && if this was used many times. Because it can save gas execution cost

##Mitigation

require(idx > VUSD_IDX && idx < supportedCollateral.length, "collateral not seizable");

change to

require idx > VUSD_IDX;
require supportedCollateral.length, "collateral not seizable";

#4 Using constructor() instead initialize() or vice versa https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L35-L56 better to execute all the codes in the initialize() using constructor(). The idea is of using initialize is to run all the code inside it once in a lifetime. The current implementation of some of the contract is using both constructor() & initialize()

#5 Directly call msg.sender https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L69 https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L65 by using msg.sender instead of _msgSender() can save gas

6# The vars can set to immutable https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L22-L24 vusd, insuranceFund, & marginAccount set once at initialize(). Use immutable

7# There is no int which using Safecast lib https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L13 In the ClearingHouse.sol there is no int which is using the lib. Remove the line 13

8# Better way to call SafeCastlib function https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L12-L13 By removing lines which declaring that this contract is using SafeCast lib and directly call it on each function, it can save execution gas cost

int256 marginCharge = realizedPnl - SafeCast.toInt256(fee);

The SafeCast.function is only called 3 times at ClearingHouse.sol so i recommend to change it thes way https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L332

9# Calling block.timestamp directly can save gas https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L86 https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L201 instead of using _blockTimestamp() func to get block.timestamp value, using block.timestamp can save gas

10# Caching position.size in isLongPosition bool is not gas efficient https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/AMM.sol#L614 https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/AMM.sol#L141 By putting position.size inside the if() condition, and removing L141 and L614, can save a lot of gas

11# Using != instead > https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L211 https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L253 since uint cant be < 0, by using != operator to check condition can save gas

12# Custom error can save gas Using string to throw an error to user is more gas consuming. From solidity 0.8.4 we can use custom error to save gas. Custom errors are defined using the error statement https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L51 https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L75 https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L84 https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L101 https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L120 https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L154 https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L164 https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L189