code-423n4 / 2022-04-phuture-findings

0 stars 0 forks source link

`UniswapV2PriceOracle#refreshedAssetPerBaseInUQ()` will revert when pair cumulative prices underflow #63

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

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

function refreshedAssetPerBaseInUQ(address _asset) external override returns (uint) {
    (uint price0Cumulative, uint price1Cumulative, uint32 blockTimestamp) = address(pair).currentCumulativePrices();
    uint32 timeElapsed = blockTimestamp - blockTimestampLast;

    if (timeElapsed >= MIN_UPDATE_INTERVAL) {
        price0Average = (price0Cumulative - price0CumulativeLast) / timeElapsed;
        price1Average = (price1Cumulative - price1CumulativeLast) / timeElapsed;

        price0CumulativeLast = price0Cumulative;
        price1CumulativeLast = price1Cumulative;
        blockTimestampLast = blockTimestamp;
    }

    return lastAssetPerBaseInUQ(_asset);
}

According to Uniswap official docs:

The UniswapV2Pair cumulative price variables are designed to eventually overflow, i.e. price0CumulativeLast and price1CumulativeLast and blockTimestampLast will overflow through 0.

Ref: https://docs.uniswap.org/protocol/V2/guides/smart-contract-integration/building-an-oracle

It means that blockTimestampLast can be lower than blockTimestamp, and the new price0Cumulative can be less than price0CumulativeLast, and so on...

In that case, the current implementation of refreshedAssetPerBaseInUQ() will revert due to underflow.

As a result, price0Average and price1Average will not be updated for a long time, any other contracts or functions that rely on this price will be affected.

Recommendation

Change to:

if (timeElapsed >= MIN_UPDATE_INTERVAL) {
    unchecked {
        price0Average = (price0Cumulative - price0CumulativeLast) / timeElapsed;
        price1Average = (price1Cumulative - price1CumulativeLast) / timeElapsed;
    }

    price0CumulativeLast = price0Cumulative;
    price1CumulativeLast = price1Cumulative;
    blockTimestampLast = blockTimestamp;
}
olivermehr commented 2 years ago

Duplicate issue of #62