Hubble contest - Meta0xNull's results

Lines of code

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

Vulnerability details

Impact

1. Attacker call function withdraw() with lowest amount eg. 1 (0.000001 VUSD)
2. The withdrawal request will go into waiting list in variable "withdrawals"
3. Attacker can repeat Step 1 & 2 to make Making Multiple Small Requests to withdraw() until pending withdrawal list go above maxWithdrawalProcesses
4. Now Normal User who call withdraw() and processWithdrawals() will not receive their Withdrawal because their waiting number is above start + maxWithdrawalProcesses

Proof of Concept

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

Tools Used

Manual Review

Recommended Mitigation Steps

In function withdraw(), require minimum withdrawal amount to avoid near zero cost DDOS. require(amount >= 10000000, 'Min Amount >= 10 VUSD');

1. Use uint256 Instead of uint

Impact

Use uint256 because it is consistent with other uint data types, which also specify their size, and also because making the size of the data explicit reminds the developer and the reader how much data they've got to play with, which may help prevent or detect bugs.

It's also better to be explicit when constructing method signature ID's. For example if doing bytes4(keccak('transfer(address, uint)')), you'll get a different method sig ID than bytes4(keccak('transfer(address, uint256)')) and smart contracts will only understand the latter when comparing method sig IDs.

Proof of Concept

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

2. TODOs List May Leak Important Info & Errors

Impact

Open TODOs can hint at programming or architectural errors that still need to be fixed.

Proof of Concept

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/AMM.sol#L142 https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/AMM.sol#L555 https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L172 https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/MarginAccount.sol#L277

Recommended Mitigation Steps

Fix TODOs List and Remove it.

1. Remove Unuse Variable __gap to Save Gas

Impact

uint256[50] private __gap;

Variable __gap was not used in VUSD.sol and some other contracts. They will increase the size of deployment with no real benefit

Proof of Concept

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

Recommended Mitigation Steps

Remove Line 30 in VUSD.sol.

2. Variable "name" - Use bytes32 Rather Than String

Impact

string public name;

If data can fit into 32 bytes, then you should use bytes32 datatype rather than bytes or strings as it is much cheaper in solidity. Basically, Any fixed size variable in solidity is cheaper than variable size. That will save gas on the contract.

Proof of Concept

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

Recommended Mitigation Steps

bytes32 public name;

3. Long Revert Strings are Waste of Gas

Impact

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition has been met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Proof of Concept

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/AMM.sol#L487 https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/AMM.sol#L511 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/MarginAccount.sol#L174 https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/MarginAccount.sol#L354 https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/MarginAccount.sol#L453 https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/VUSD.sol#L55

Recommended Mitigation Steps

Shorten the revert strings to fit in 32 bytes.

Or consider using Custom Errors (solc >=0.8.4).

4. Variable Default Value is 0 and thus Initialized to 0 is Waste of Gas

Impact

uint constant VUSD_IDX = 0;

The local variable need not be initialized to 0 because the default value is 0. Avoiding this anti-pattern can save a few opcodes and therefore a tiny bit of gas.

Proof of Concept

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

Recommended Mitigation Steps

Remove explicit 0 initialization.

uint constant VUSD_IDX;

5. Constructor Does Not Check for Zero Addresses

Impact

A wrong user input or wallets defaulting to the zero addresses for a missing input can lead to the contract needing to redeploy or wasted gas.

Proof of Concept

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/Registry.sol#L12-L23

Recommended Mitigation Steps

Requires Addresses is not zero.

require(_oracle != address(0), "Address Can't Be Zero") require(_clearingHouse != address(0), "Address Can't Be Zero") require(_insuranceFund != address(0), "Address Can't Be Zero") require(_marginAccount != address(0), "Address Can't Be Zero") require(_vusd != address(0), "Address Can't Be Zero")