Hubble contest - hyh's results

Lines of code

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

Vulnerability details

Impact

A malicious user can make lots of withdrawal requests to fill up the queue, making VUSD withdrawals unreachable for all other users

Proof of Concept

There is no control of the size or number of the withdrawal requests, and VUSD will burn even 1 wei amount and put it to the queue, and will allow to repeat it unlimited number of times:

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

This way it is possible to do the DOS type of attack, filling up the withdrawal queue with zero/tiny requests.

processWithdrawals will process the saved requests up to maxWithdrawalProcesses per run:

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

As withdraw function is somewhat simpler than processWithdrawals, the cost of attack will be even comparable to the cost of the subsequent cleaning up, making it an attractive path for anyone who desires to disturb VUSD operations

Recommended Mitigation Steps

Consider adding a limit for the number of simultaneous withdraw requests per user and a minimal withdraw value in each of them.

Now:

function withdraw(uint amount) external {
		burn(amount);
		withdrawals.push(Withdrawal(msg.sender, amount));
}
...
function processWithdrawals() external {
...
		reserveToken.safeTransfer(withdrawal.usr, withdrawal.amount);

To be:

function withdraw(uint amount) external {
		require(amount >= minWithdrawalAmount, "min amount isn't met");
		uint _withdrawCounter = withdrawCounter[msg.sender] + 1;
		require(_withdrawCounter < maxSimulWithdraws, "too many at once");
		withdrawCounter[msg.sender] = _withdrawCounter;
		// do the withdraw
		burn(amount);
		withdrawals.push(Withdrawal(msg.sender, amount));
}
...
function processWithdrawals() external {
...
		reserveToken.safeTransfer(withdrawal.usr, withdrawal.amount); // withdrawal.usr can be saved to memory
		withdrawCounter[withdrawal.usr] -= 1;

Lines of code

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

Vulnerability details

Impact

If the price feed is manipulated in any way or there is any malfunction based volatility on the market, a malicious user can use this to liquidate a healthy position.

An attacker can setup a monitoring of the used Oracle feed and act on observing a price outbreak (for example, zero price, which is usually a subject to filtration), liquidating the trader position which is perfectly healthy otherwise, obtaining the collateral with a substantial discount at the expense of the trader.

The same is for a flash crash kind of scenario, i.e. a price outbreak of any nature will allow for non-market liquidation by an attacker, who has the incentives to setup such a monitoring and act on such an outbreak, knowing that it will not be smoothed or filtered out, allowing a liquidation at a non-market price that happen to be printed in the Oracle feed

Proof of Concept

Oracle.getUnderlyingPrice just passes on the latest Oracle answer, not checking it anyhow:

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

It is then used in liquidation triggers providing isLiquidatable and _getLiquidationInfo functions:

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

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

Recommended Mitigation Steps

Add a non-zero Oracle price check, possibly add an additional Oracle feed information usage to control that the price is fresh. Please consult the Chainlink for that as OCR introduction might have changed the state of the art approach (i.e. whether and how to use latestRoundData returned data):

https://docs.chain.link/docs/off-chain-reporting/

Regarding any price spikes it is straightforward to construct a mitigation mechanics for such cases, so the system will be affected by sustainable price movements only.

As price outrages provide a substantial attack surface for the project it's worth adding some complexity to the implementation.

One of the approaches is to track both current and TWAP prices, and condition all state changing actions, including liquidations, on the current price being within a threshold of the TWAP one. If the liquidation margin level is conservative enough and TWAP window is small enough this is safe for the overall stability of the system, while providing substantial mitigation mechanics by allowing state changes on the locally calm market only.

Another approach is to introduce time delay between liquidation request and actual liquidation. Again, conservative enough margin level plus small enough delay keeps the system safe, while requiring that market conditions allow for liquidation both at request time and at execution time provides ample filtration against price feed outbreaks

Lines of code

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

Vulnerability details

Impact

It is assumed that VAMM returned positions have exactly 6 decimals for all AMMs white listed in ClearingHouse.

In the same time an array of different AMMs/VAMMs is supported, and there are no guarantees/checks of the precision of the position values they return.

If an VAMM that have different precision is whitelisted, for example having 18 decimals for position figures, then margin requirements checks become invalid.

This will lead to various malfunctions, say perfectly valid positions will be liquidated by any attacker noticing that the calculations are skewed.

Proof of Concept

ClearingHouse's _calcMarginFraction is the function that is used for margin requirements checks:

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

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

_calcMarginFraction calls getNotionalPositionAndMargin:

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

getNotionalPositionAndMargin calls getTotalNotionalPositionAndUnrealizedPnl:

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

getTotalNotionalPositionAndUnrealizedPnl sums up AMM's getNotionalPositionAndUnrealizedPnl results:

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

AMM's getNotionalPositionAndUnrealizedPnl returns vamm.get_notional result:

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

The above calls are linear decimals wise (i.e. do subtractions/additions kind of operations, preserving the decimals).

Then, _getMarginFraction mixes up these notionalPosition and margin, obtained from AMM without rescaling, as if they are PRECISION scaled:

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

PRECISION is hard coded to be 1e6:

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

For other VAMM operations base precision is set to 1e18:

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

For example, VAMM returned supply is assumed to have 18 decimals:

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

Comment says that exchangeExactOut returned quantity will have 6 decimals precision:

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

As the system imply that VAMMs can vary it is neither guaranteed, nor checked in any way (briefly checked dydx api code, it looks like there are no explicit guarantees either).

If any of VAMM referenced via white listed AMMs return VAMM.get_notional with decimals different from 6, the _calcMarginFraction result will become grossly incorrect.

Recommended Mitigation Steps

If AMM contract is desired to deal with various VAMMs consider removing decimals related hard coding, adding decimals variables and scaling VAMM returned results accordingly, so that position and margin values' decimals of 6, implied by ClearingHouse logic, be ensured.

Lines of code

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

Vulnerability details

Impact

VUSD withdraw queue will be blocked and user funds frozen simply by requesting a zero value withdraw, if the reserve token doesn't support zero value transfers.

Putting it medium only on an assumption that reserve will be USDC and the probability is low, but VUSD do allow any reserve token and the impact here is both funds freeze and stopping of the operations

Proof of Concept

It is possible to burn zero amount in OZ implementation:

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/token/ERC20/ERC20Upgradeable.sol#L285-L300

So, withdraw will burn zero amount and put it to the queue:

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

USDC does support zero value transfers, but not all the tokens do:

https://github.com/d-xo/weird-erc20#revert-on-zero-value-transfers

Currently VUSD can use any reserve token:

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

Withdraw queue position can be modified in the processWithdrawals function only.

But it will fail every time on the zero amount entry, as there is no way to skip it (and mint VUSD back, for example), so anything else after this zero entry will not be processed:

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

This way the withdrawal functionality and the corresponding user funds will be frozen within VUSD contract, which will become inoperable

Recommended Mitigation Steps

Consider adding a zero amount check, as it doesn’t cost much, while zero transfer doesn't make sense anyway.

Now:

reserveToken.safeTransfer(withdrawal.usr, withdrawal.amount);
reserve -= withdrawal.amount;

To be:

if (withdrawal.amount > 0) {
	reserveToken.safeTransfer(withdrawal.usr, withdrawal.amount);
	reserve -= withdrawal.amount;
}

Lines of code

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

Vulnerability details

Impact

The assets sent to InsuranceFund by MarginAccount's settleBadDebt will remain frozen there as there are no means implemented to recover them even manually.

Proof of Concept

MarginAccount's settleBadDebt is used to clear an underfunded account.

It's pushing VUSD debt to the insuranceFund and then transferring all the account's collateral there as well:

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

However, insuranceFund have no ability to manipulate these collateral tokens:

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/InsuranceFund.sol#L39-L85

I.e. deposit/withdrawal, record/settle bad debt is the only functionality implemented.

Even manual retrieval by governance of the collateral tokens sent to the InsuranceFund doesn't seem to be possible.

Recommended Mitigation Steps

Consider adding a manual retrieval function as the first and the simplest step.

Then, as relying on fully manual and off-the-contract process isn't advised, consider an introduction of an asset liquidation function, so the InsuranceFund be able to sell chosen assets (specifying minimum prices to receive from the market as a function argument) to obtain the reserve asset of the VUSD token and then mint VUSD.

Lines of code

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/InsuranceFund.sol#L34-L35

Vulnerability details

Impact

InsuranceFund's initialize doesn't seem to be able to run in the setup provided without initializer modifier, so the contract's ERC20 cannot be initialized, VUSD, marginAccount and governace cannot be set, the contract is inoperable

Proof of Concept

InsuranceFund's initialize calls ERC20Upgradeable's __ERC20_init, stating that it has initializer modifier:

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/InsuranceFund.sol#L34-L35

While actually __ERC20_init only has onlyInitializing modifier:

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/token/ERC20/ERC20Upgradeable.sol#L55-L62

That checks that initialization is taking place:

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/proxy/utils/Initializable.sol#L72-L75

But it hasn't started and this way InsuranceFund's initialize have to fail all the times

Recommended Mitigation Steps

Consider adding initializer modifier (inherited from ERC20Upgradeable) to InsuranceFund's initialize:

Now:

function initialize(address _governance) external {
		__ERC20_init("Hubble-Insurance-Fund", "HIF"); // has initializer modifier
		_setGovernace(_governance);
}

To be:

function initialize(address _governance) external initializer {
		__ERC20_init("Hubble-Insurance-Fund", "HIF"); // has initializer modifier
		_setGovernace(_governance);
}