Hubble contest - cmichel's results

Lines of code

https://github.com/code-423n4/2022-02-hubble/blob/8c157f519bc32e552f8cc832ecc75dc381faa91e/contracts/VUSD.sol#L50-L57

Vulnerability details

Impact

The VUSD.processWithdrawals function only performs maxWithdrawalProcesses (actually maxWithdrawalProcesses + 1) iterations per call. Withdrawals can be freely spammed by a griefer calling burn(amount) with a zero amount. All future withdrawals are blocked until someone calls processWithdrawals to iterate through all the spam withdrawals.

Recommended Mitigation Steps

Add a minimum withdrawal amount in withdraw to increase the cost of the attack and make it infeasible.

Lines of code

https://github.com/code-423n4/2022-02-hubble/blob/8c157f519bc32e552f8cc832ecc75dc381faa91e/contracts/InsuranceFund.sol#L44-L54

Vulnerability details

Impact

The InsuranceFund.deposit function mints initial shares equal to the deposited amount. The deposit / withdraw functions also use the VUSD contract balance for the shares computation. (balance() = vusd.balanceOf(address(this)))

It's possible to increase the share price to very high amounts and price out smaller depositors.

POC

• deposit(_amount = 1): Deposit the smallest unit of VUSD as the first depositor. Mint 1 share and set the total supply and VUSD balance to 1.
• Perform a direct transfer of 1000.0 VUSD to the InsuranceFund. The balance() is now 1000e6 + 1
• Doing any deposits of less than 1000.0 VUSD will mint zero shares: shares = _amount * _totalSupply / _pool = 1000e6 * 1 / (1000e6 + 1) = 0.
• The attacker can call withdraw(1) to burn their single share and receive the entire pool balance, making a profit. (balance() * _shares / totalSupply() = balance())

I give this a high severity as the same concept can be used to always steal the initial insurance fund deposit by frontrunning it and doing the above-mentioned steps, just sending the frontrunned deposit amount to the contract instead of the fixed 1000.0. They can then even repeat the steps to always frontrun and steal any deposits.

Recommended Mitigation Steps

The way UniswapV2 prevents this is by requiring a minimum deposit amount and sending 1000 initial shares to the zero address to make this attack more expensive. The same mitigation can be done here.

Lines of code

https://github.com/code-423n4/2022-02-hubble/blob/8c157f519bc32e552f8cc832ecc75dc381faa91e/contracts/ClearingHouse.sol#L122

Vulnerability details

Impact

The ClearingHouse.updatePositions function iterates over all amms. There is no limit to the number of AMMs that can be added in ClearingHouse.whitelistAmm.

The transactions can fail if the arrays get too big and the transaction would consume more gas than the block limit. This will then result in a denial of service for the desired functionality and break core functionality. It also leads to a higher gas cost.

A similar issue was recently marked as medium severity.

Recommended Mitigation Steps

It would be best to set a sanity maximum number of AMMs/ that can be added.

Lines of code

https://github.com/code-423n4/2022-02-hubble/blob/8c157f519bc32e552f8cc832ecc75dc381faa91e/contracts/Oracle.sol#L34

Vulnerability details

Impact

The Oracle.getUnderlyingPrice function divides the chainlink price by 100. It probably assumes that the answer for the underlying is in 8 decimals but then wants to reduce it for 6 decimals to match USDC.

However, arbitrary underlying tokens are used and the chainlink oracles can have different decimals.

Recommended Mitigation Steps

While most USD price feeds use 8 decimals, it's better to take the on-chain reported decimals into account by doing AggregatorV3Interface(chainLinkAggregatorMap[underlying]).decimals(), see Chainlink docs. The price should then be scaled down to 6 decimals.