Chainlink oracle prices can be stale or incorrect

Summary

There is no check if the data received from Chainlink's price feed is carried over or fresh. The price could be outdated.

Vulnerability Detail

The function to update the collateral price via Chainlink checks that updatedAt is not more than threshold in the past to avoid stale data:

    function updateChainLinkCollateralPrice(uint256 collateralIndex) internal {
        ...

        // fetch latest price
        (
            ,
            // roundId
            int256 answer, // startedAt
            ,
            uint256 updatedAt,

        ) = // answeredInRound
            priceFeed.latestRoundData();

        ...

        // validation
        require(answer > 0, "Invalid price");
        require(
            block.timestamp - updatedAt <
                poolStorage.collateralPriceFeedStalenessThresholds[
                    collateralIndex
                ],
            "Stale data"
        );

        ...
    }

This check is insufficient as data could be carried over from previous rounds and be stale anyway as there is no check for round ID.

See: https://docs.chain.link/data-feeds/historical-data and https://github.com/sherlock-audit/2023-02-blueberry-judging/issues/94 or https://github.com/code-423n4/2022-04-backd-findings/issues/17

Impact

A stale price of the collateral is highly detrimental to the protocol. Users will be able to mint more Ubiquity dollars than they should be allowed to or redeem more collateral when burning their Ubiquity dollars.

Code Snippet

https://github.com/sherlock-audit/2023-12-ubiquity/blob/main/ubiquity-dollar/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol#L523-L562

Tool used

Manual Review

Recommendation

Check that the data from the oracle is not carried over from previous rounds:

        // fetch latest price
        (
            uint80 roundId,
            int256 answer,
            , // startedAt
            uint256 updatedAt,

        ) = // answeredInRound
            priceFeed.latestRoundData();  // @audit does not check for stale prices
        require(updatedAt >= uint256(roundId), "Stale data");

Duplicate of #133

sherlock-admin2 commented 7 months ago

1 comment(s) were left on this issue during the judging contest.

auditsea commented:

The issue describes about not checking min/max value from Chainlink for data staleness, but this is not required