Hubble contest - pauliax's results

Lines of code

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

Vulnerability details

Impact

Oracle returns Chainlink latestRoundData without proper validation, e.g.:

  function getUnderlyingPrice(address underlying)
      ...
      (,answer,,,) = AggregatorV3Interface(chainLinkAggregatorMap[underlying]).latestRoundData();
      answer /= 100;
  }

And other functions that call latestRoundData are quite similar. The problem is that there are no checks if the return value indicates stale data. This could lead to stale prices according to the Chainlink documentation: “if answeredInRound < roundId could indicate stale data.” “A timestamp with zero value means the round is not complete and should not be used.”

Recommended Mitigation Steps

Add missing checks for stale data. See example here: https://github.com/cryptexfinance/contracts/blob/master/contracts/oracles/ChainlinkOracle.sol#L58-L65

• The error message should be <= 0 :

  require(_maintenanceMargin > 0, "_maintenanceMargin < 0");

• There are functions that do not follow the Check-Effects-Interaction pattern, e.g. addMarginFor, processWithdrawals. They have external calls in the middle of execution, e.g. inside the loop, so should have extra protection from re-entrancy just in case, unless you 100% trust these external contracts (e.g. tokens), but nevertheless I think you should always act preventively.

• VUSD returns hardcoded 6 decimals:

    function decimals() public pure override returns (uint8) {
        return 6;
    }

While in practice it should be tied with USDC token that has 6 decimals:

    /// @notice vUSD is backed 1:1 with reserveToken (USDC)
    IERC20 public immutable reserveToken;

There is no restriction of setting another reserveToken, so consider calling .decimals() when setting the reserveToken, and then assign the same value to the VUSD decimals.

• There are TODOs left:

  // @todo put checks on slippage
  // sending market orders can fk the trader. @todo put some safe guards around price of liquidations
  // @todo handle case when totalPosition = 0
  @todo consider providing some incentive from insurance fund to execute a liquidation in this scenario.

• I think the condition should not be inclusive here:

  while (i < withdrawals.length && (i - start) <= maxWithdrawalProcesses)

e.g. when maxWithdrawalProcesses = 3, it will actually execute the loop 4 times.

• Consider introducing reasonable upper and lower limits for setMaxWithdrawalProcesses, otherwise an admin can grief by setting it to 0 and thus blocking the withdrawals, unless this may be intended.

• Oracle always assumes that the result will have 8 decimals, thus it divides by a hardcoded value of 100. You should verify that by calling .decimals on Chainlink oracle: https://docs.chain.link/docs/price-feeds-api-reference/#decimals

• Repeated operations should be cached, e.g. amount.toInt256() is calculated twice:

  require(margin[idx][trader] >= amount.toInt256(), "Insufficient balance");
  margin[idx][trader] -= amount.toInt256();

• ++ is a bit cheaper:

 i += 1;

• 'withdrawals' and 'start' state variables accessed multiple times in a loop should be cached before the loop:

 while (i < withdrawals.length && (i - start) <= maxWithdrawalProcesses)