Description

The UniswapTwapPriceOracleV2 is a modified version of the Compound UniswapTwapPriceOracleV2 contract. The Compound contract used Open Zeppelin's SafeMathUpgradeable to check for arithmetic overflow and underflow issues.

Ionic Protocol removed SafeMathUpgradeable and modified the Compound contracts to compile with solidity versions >=0.8.0 which by default includes checked arithmetic to revert on overflows and underflows.

The UniswapTwapPriceOracleV2 imports UniswapTwapPriceOracleV2Root which has also been modified to replace the SafeMathUpgradeable functionality with solidity 0.8.0+ default checked arithmetic. However, in UniswapTwapPriceOracleV2Root there are portions of code related to price accumulation (currentPx0Cumu, currentPx1Cumu) and time weighted average price (price0TWAP, price1TWAP) where arithmetic overflow is desirable. For further reading see Dapp's audit report↗ of Uniswap v2.

This issue was duplicated with a parallel, internal review of the code conducted by Ionic Protocol.

Impact

When calling getUnderlyingPrice, an overflow in either currentPx0Cumu or currentPx1Cumu would lead to an unexpected transaction reversion, rendering the oracle useless.

Recommendations

Review all contracts in the codebase which were updated to compile with solidity 0.8.0+ and place unchecked blocks around code where overflow is desireable. This will allow values to wrap on overflows and underflows as expected in versions of solidity prior to 0.8.0.

Below is an example of corrected code for currentPx0Cumu in UniswapTwapPriceOracleV2Root:

function currentPx0Cumu(address pair) internal view returns (uint256 px0Cumu) {
  uint32 currTime = uint32(block.timestamp);
  px0Cumu = IUniswapV2Pair(pair).price0CumulativeLast();
  (uint256 reserve0, uint256 reserve1, uint32 lastTime) = IUniswapV2Pair(pair).getReserves();
  if (lastTime != block.timestamp) {
-    uint32 timeElapsed = currTime - lastTime; // overflow is desired
-    px0Cumu += uint256((reserve1 << 112) / reserve0) * timeElapsed; 
+    unchecked {
+      uint32 timeElapsed = currTime - lastTime; // overflow is desired
+      px0Cumu += uint256((reserve1 << 112) / reserve0) * timeElapsed;
+    }
  }
}

Remediation

The issue has been fixed by Ionic Protocol in commit a562fda↗.