Phuture Finance contest - WatchPug's results

Lines of code

https://github.com/code-423n4/2022-04-phuture/blob/594459d0865fb6603ba388b53f3f01648f5bb6fb/contracts/UniswapV2PriceOracle.sol#L62

Vulnerability details

https://github.com/code-423n4/2022-04-phuture/blob/594459d0865fb6603ba388b53f3f01648f5bb6fb/contracts/UniswapV2PriceOracle.sol#L62

(uint price0Cumulative, uint price1Cumulative, uint32 blockTimestamp) = address(pair).currentCumulativePrices();

Because the Solidity version used by the current implementation of UniswapV2OracleLibrary.sol is >=0.8.7, and there are some breaking changes in Solidity v0.8.0:

Arithmetic operations revert on underflow and overflow.

Ref: https://docs.soliditylang.org/en/v0.8.13/080-breaking-changes.html#silent-changes-of-the-semantics

While in UniswapV2OracleLibrary.sol, subtraction overflow is desired at blockTimestamp - blockTimestampLast in currentCumulativePrices():

https://github.com/Uniswap/v2-periphery/blob/master/contracts/libraries/UniswapV2OracleLibrary.sol#L25-L33

if (blockTimestampLast != blockTimestamp) {
    // subtraction overflow is desired
    uint32 timeElapsed = blockTimestamp - blockTimestampLast;
    // addition overflow is desired
    // counterfactual
    price0Cumulative += uint(FixedPoint.fraction(reserve1, reserve0)._x) * timeElapsed;
    // counterfactual
    price1Cumulative += uint(FixedPoint.fraction(reserve0, reserve1)._x) * timeElapsed;
}

In another word, Uniswap/v2-periphery/contracts/libraries/UniswapV2OracleLibrary only works at solidity < 0.8.0.

As a result, when price0Cumulative or price1Cumulative is big enough, currentCumulativePrices will revert due to overflow.

Impact

Since the overflow is desired in the original version, and it's broken because of using Solidity version >0.8. The UniswapV2PriceOracle contract will break when the desired overflow happens, and further breaks other parts of the system that relies on UniswapV2PriceOracle.

Recommendation

Note: this recommended fix requires a fork of the library contract provided by Uniswap.

Change to:

if (blockTimestampLast != blockTimestamp) {
    unchecked {
        // subtraction overflow is desired
        uint32 timeElapsed = blockTimestamp - blockTimestampLast;
        // addition overflow is desired
        // counterfactual
        price0Cumulative += uint(FixedPoint.fraction(reserve1, reserve0)._x) * timeElapsed;
        // counterfactual
        price1Cumulative += uint(FixedPoint.fraction(reserve0, reserve1)._x) * timeElapsed;
    }
}

Lines of code

https://github.com/code-423n4/2022-04-phuture/blob/594459d0865fb6603ba388b53f3f01648f5bb6fb/contracts/ChainlinkPriceOracle.sol#L83

Vulnerability details

https://github.com/code-423n4/2022-04-phuture/blob/594459d0865fb6603ba388b53f3f01648f5bb6fb/contracts/ChainlinkPriceOracle.sol#L83

(, int basePrice, , , ) = baseAggregator.latestRoundData();

On ChainlinkPriceOracle.sol, we are using latestRoundData, but there is no check if the return value indicates stale data. This could lead to stale prices according to the Chainlink documentation:

• https://docs.chain.link/docs/historical-price-data/#historical-rounds
• https://docs.chain.link/docs/faq/#how-can-i-check-if-the-answer-to-a-round-is-being-carried-over-from-a-previous-round

Recommendation

Consider adding missing checks for stale data.

For example:

(uint80 roundID, int256 basePrice, , uint256 timestamp, uint80 answeredInRound) = baseAggregator.latestRoundData();
require(basePrice > 0, "Chainlink price <= 0"); 
require(answeredInRound >= roundID, "Stale price");
require(timestamp != 0, "Round not complete");