Hubble contest - Omik's results

LOW : 1. Title : Missing limit on how many AMMs can be added

Impact : The governance can add an amm, by calling whitelistAmm function, however there is no limit on how many amm that the contract can be held, if the governance keep adding amm, then the clearing house will brick with out of gas, since all other user is interacting with the clearing house and the main functionality of this contract is updatePoition and this function is being called by removeLiquidity, addLiquidity, openPosition, closePosition function

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

Title : Missing check on duplicate amm

Impact : There is missing check on axisting amm, and amm that will going to be added from whitelistAmm function, since there is no check whether the same amm is already being added or not, a multiple amm might be added without error.

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

Title : Grieve in processWtihdrawals

Impact : an attacker could grieve other user by burning many small amount VUSD, to inflate the withdrawals length until 99, and if the victim want to burn their VUSD to USDC, the victim will be placed in the 100, and when the victim want to take the USDC, by calling processWithdrawal, the victim will pay extra fee, that's because the victim must process the withdrawal that the attacker make 99 times, before the victim accept their USDC.

POC :

1. An attacker mint 100 wei VUSD, with USDC
2. Burn 1 wei VUSD
3. Repeat step 2 99 times
4. The victim use burn function to get USDC back, however the victim must pay extra gas, just to send 1 wei that was belong to the attacker.

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

GAS : 1. Title : Its cheaper using delete

Impact : In the removeLiquidity function if the diff value is 0, its cheaper to use delete instead setting the pos, vAsset, vUSD, dToken to zero, this can save +- 3 gas

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

Mitigation :

	delete __maker.pos;
	delete __maker.vAsset;
	delete __maker.vUSD;
	delete __maker.dToken;

Title : its cheaper to save length value to a local variable

Impact : when using a loop, its cheaper to save the length value of an array to a variable, so the loop didn't have to read the storage length in each loop, but comparing with the length that was already save in the local variable. For an array that has a length 10 it can save +- 1800 gas.

POC : https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L122 https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L130 https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L170 https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L194

Title : its cheaper to use block.timestamp directly

Impact : Its cheaper to use block.timestamp directly instead calling it through a _blocktimestamp() function.

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