Offchain oracle price failure is handled incorrectly

Summary

Offchain oracle price failure is handled incorrectly.

Vulnerability Detail

The OracleModule is used to get a price of collateral. To get a price the contract calls _getPrice(maxAge) and then tries to fetch an onchain chainlink oracle and offchain pyth oracle.

function _getPrice(uint32 maxAge) internal view returns (uint256 price, uint256 timestamp) {
    (uint256 onchainPrice, uint256 onchainTime) = _getOnchainPrice(); // will revert if invalid
    (uint256 offchainPrice, uint256 offchainTime, bool offchainInvalid) = _getOffchainPrice();
    ...
}

The oracle price is chosen based on the latest timestamp, or if offchain oracle price is invalid it fallbacks to an onchain chainlink oracle.

if (offchainInvalid == false) {
    // return the freshest price
    if (offchainTime >= onchainTime) {
        price = offchainPrice;
        timestamp = offchainTime;
        offchain = true;
    } else {
        price = onchainPrice;
        timestamp = onchainTime;
    }
} else {
    price = onchainPrice;
    timestamp = onchainTime;
}

The main idea is if the offchainInvalid == true which indicates that the offchain oracle price is invalid the contract should revert to the onchain oracle.

The offchainInvalid will be true in one of three cases.

function _getOffchainPrice() internal view returns (uint256 price, uint256 timestamp, bool invalid) {
    IPyth oracle = offchainOracle.oracleContract;
    if (address(oracle) == address(0)) revert FlatcoinErrors.ZeroAddress("oracle");

    try oracle.getPriceNoOlderThan(offchainOracle.priceId, offchainOracle.maxAge) returns (   <-
        PythStructs.Price memory priceData
    ) {
        timestamp = priceData.publishTime;

        // Check that Pyth price and confidence is a positive value
        // Check that the exponential param is negative (eg -8 for 8 decimals)
        if (priceData.price > 0 && priceData.conf > 0 && priceData.expo < 0) {                <-
            price = ((priceData.price).toUint256()) * (10 ** (18 + priceData.expo).toUint256()); // convert oracle expo/decimals eg 8 -> 18

            // Check that Pyth price confidence meets minimum
            if (priceData.price / int64(priceData.conf) < int32(offchainOracle.minConfidenceRatio)) { <-
                invalid = true; // price confidence is too low
            }
        } else {
            invalid = true;
        }
    } catch {
        invalid = true; // couldn't fetch the price with the asked input param
    }
}

1.) There is an error when calling the oracle.getPriceNoOlderThan
2.) price <= 0 or conf <= 0 or expo >= 0
3.) the price/confidence < minConfidenceRatio

In any of these cases, the price should rely on the onchain oracle as a fallback oracle.

Let's look at the code. Suppose we are fetching ETH/USD price. The onchainPrice = 2000e18 and the offchainPrice = 0e18 and offchainInvalid = true due to a failure in the _getOffchainPrice()

function _getPrice(uint32 maxAge) internal view returns (uint256 price, uint256 timestamp) {
    (uint256 onchainPrice, uint256 onchainTime) = _getOnchainPrice(); // will revert if invalid
    (uint256 offchainPrice, uint256 offchainTime, bool offchainInvalid) = _getOffchainPrice();
    bool offchain;

    uint256 priceDiff = (int256(onchainPrice) - int256(offchainPrice)).abs();

    uint256 diffPercent = (priceDiff * 1e18) / onchainPrice;
    if (diffPercent > maxDiffPercent) revert FlatcoinErrors.PriceMismatch(diffPercent);

    if (offchainInvalid == false) {
        // return the freshest price
        if (offchainTime >= onchainTime) {
            price = offchainPrice;
            timestamp = offchainTime;
            offchain = true;
        } else {
            price = onchainPrice;
            timestamp = onchainTime;
        }
    } else {
        price = onchainPrice;
        timestamp = onchainTime;
    }
    ...
}

We then calculate the

priceDiff = 2000e18 - 0 = 2000e18
diffPercent = 2000e18 * 1e18 / 2000e18 = 1e18

The diffPercent > maxDiffPercent and the function reverts. The function should fallback to the onchain oracle in case of the invalid price data from the pyth oracle but it does not do so. The maxDiffPercent should be set to 1% or so.

Impact

If offchain oracle fails the system is DOSsed in the meantime instead of reverting to the onchain oracle.

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/OracleModule.sol#L111-L128

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/OracleModule.sol#L163-L187

Tool used

Manual Review

Recommendation

Only check the priceDiff if offchainInvalid = false. This will now correctly fallback to the onchain chainlink oracle in case of a pyth oracle failure. In cases where both prices are fetched correctly, it checks the price difference and selects the one with the latest timestamp.

-   uint256 priceDiff = (int256(onchainPrice) - int256(offchainPrice)).abs();

-   uint256 diffPercent = (priceDiff * 1e18) / onchainPrice;
-   if (diffPercent > maxDiffPercent) revert FlatcoinErrors.PriceMismatch(diffPercent);

    if (offchainInvalid == false) {
+       uint256 priceDiff = (int256(onchainPrice) - int256(offchainPrice)).abs();

+       uint256 diffPercent = (priceDiff * 1e18) / onchainPrice;
+       if (diffPercent > maxDiffPercent) revert FlatcoinErrors.PriceMismatch(diffPercent);

        // return the freshest price
        if (offchainTime >= onchainTime) {
            price = offchainPrice;
            timestamp = offchainTime;
            offchain = true;
        } else {
            price = onchainPrice;
            timestamp = onchainTime;
        }
    } else {
        price = onchainPrice;
        timestamp = onchainTime;
    }

Duplicate of #177

sherlock-admin commented 8 months ago

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

takarez commented:

valid: duplicate of issue 106 due to the fact that if the returned offChainInvalid is true the diffPercent should be skipped and not checked; medium(2)