Protocol could run into incurring losses due to incorrect collateral pricing if when Chainlink Aggregator is reaching minAnswer/maxAnswer

Summary

According to Chainlink doc:

https://docs.chain.link/data-feeds/selecting-data-feeds#periods-of-high-network-congestion

"Users are strongly advised to set up monitoring and alerts in the event of unexpected market failures. Black swan events, hacks, coordinated attacks, or extreme market conditions may trigger unanticipated outcomes such as liquidity pools becoming unbalanced, unexpected re-weighting of indices, abnormal behavior by centralized or decentralized exchanges, or the de-pegging of synthetic assets, stablecoins, and currencies from their intended exchange rates.

Circuit breakers can be created using Chainlink Automation. Circuit breakers are safety measures that monitor data feeds for unexpected scenarios such as stale prices, drastic price changes, or prices approaching a predetermined min/max threshold. If an unexpected scenario occurs, the circuit breaker can send an onchain transaction to pause or halt contract functionality."

In another words, Chainlink aggregators have a built-in circuit breaker to prevent the price of an asset from deviating outside a predefined price range. This circuit breaker may cause the oracle to persistently return, e.g. the minPrice instead of the actual asset price in the event of a significant price drop, as witnessed during the LUNA crash.

Vulnerability Detail

When mintDollar() is called, updateChainLinkCollateralPrice() will first be invoked prior to calling getDollarInCollateral() to get amount of collateral for minting Dollars. As evidenced from the function below, collateralNeeded is determined by the changing denominator, poolStorage.collateralPrices[collateralIndex]:

    function getDollarInCollateral(
        uint256 collateralIndex,
        uint256 dollarAmount
    ) internal view returns (uint256) {
        UbiquityPoolStorage storage poolStorage = ubiquityPoolStorage();
        return
            dollarAmount
                .mul(UBIQUITY_POOL_PRICE_PRECISION)
                .div(10 ** poolStorage.missingDecimals[collateralIndex])
                .div(poolStorage.collateralPrices[collateralIndex]);
    }

Consequently, the incident would return collateralNeeded lower than expected to mint Dollars.

Impact

Users would capitalize on this incidental loophole and mint Dollars cheaply using a collateral already diminished in value. If an asset's price falls below the minPrice, the protocol continues to value the token at the minPrice rather than its real value. For example, if DAI's minPrice is 0.95 USD and its price falls to $0.80, the aggregator continues to report 0.95 USD, rendering the related function calls to entail a value that is higher than the actual value.

Note: The reverse scenario is also true when the aggregator reaches its maxPrice where it can be tapped on by users to redeem Dollars for higher amount of collateralNeeded.

Code Snippet

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

Tool used

Manual Review

Recommendation

updateChainLinkCollateralPrice() should cross-check the returned answer against the minPrice/maxPrice and revert if the answer is outside of these bounds:

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

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

This ensures that a false price will not be returned if the underlying asset's value hits the minPrice/maxPrice.

Duplicate of #144

sherlock-admin2 commented 6 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