Afriauditor high OracleModule::_getPrice returns only Stale price

Summary

OracleModule::_getPrice reverts when timestamp is within the required age, creating a DoS bug for all other modules depending on it to fetch price.

Vulnerability Detail

The _getPrice function fetches the latest asset price by comparing on-chain Chainlink and off-chain Pyth oracle prices and selects the freshest and valid price, however the last check is does is checks if the selected price's timestamp is within the acceptable age range specified by the maxAge parameter. Contrary to the intended logic, the code implementation actually reverts if the sum of the timestamp and maxAge is less than the current block timestamp, causing the code to revert if it's within age and execute when timestamp is old.

        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;
        }

        // Check that the timestamp is within the required age
>>>>>        if (maxAge < type(uint32).max && timestamp + maxAge < block.timestamp) {
            revert FlatcoinErrors.PriceStale(
                offchain ? FlatcoinErrors.PriceSource.OffChain : FlatcoinErrors.PriceSource.OnChain
            );
        }
    }

Impact

This issue poses a risk of disrupting the protocol's functionality, resulting in a denial-of-service scenario. OracleModule::_getPrice, a crucial function relied upon by all other modules for oracle price retrieval. The function will consistently reverts when the timestamp falls within the expected limit, and execute when the timestamp is above the limit (indicating staleness) making protocol to use only stale prices

Code Snippet

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

Tool used

Manual Review

PoC


// PASTE THIS IN THE ORACLE.T.SOL FILE

  function test_bug_in_getprice() public {

        uint256 wethPrice = 2500e8;
        vm.warp(1000);
        setWethPrice(wethPrice);
        uint256 timestamp_value_from_getPrice = block.timestamp;

        skip(50 seconds);// Above AgeMax

        uint32 maxxAge = uint32 ( 10 seconds);

        vm.expectRevert(
            abi.encodeWithSelector(FlatcoinErrors.PriceStale.selector, FlatcoinErrors.PriceSource.OffChain)
        );
        (uint256 price,uint256 timestamp) = oracleModProxy.getPrice(maxxAge);

        console.log(block.timestamp);// returns 1050
        console.log(timestamp); //retuns 0 because foudry testing enviroment cant fectch us offchain data.
        //This function reverts because timestamp was labelled "0"(this show function reverts when timestamp + maxage< block.timestamp)
    // This is so because in the _getPrice function the returns value `uint256 timestamp` from the oracle which is not deployed on foundry local testing environment
    // thus real value of timestamp = timestamp_value_from_getPrice

        timestamp += timestamp_value_from_getPrice;

      assertGt(block.timestamp, timestamp + maxxAge);
       // this shows that after agemax 'timestamp' + `agemax` is not less than block.timestamp which will allow the function to execute

  }

Recommendation

 if (maxAge < type(uint32).max && timestamp + maxAge > block.timestamp) {
            revert FlatcoinErrors.PriceStale(
                offchain ? FlatcoinErrors.PriceSource.OffChain : FlatcoinErrors.PriceSource.OnChain
            );

sherlock-admin commented 5 months ago

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

takarez commented:

valid: the condition should be > instead of <; medium(3)

nevillehuang commented 5 months ago

Invalid, unsure what the watson is pointing to, given check is correct. When sum of timestamp + maxAge of price data is less than current timestamp, it means price is stale. This is synonymous to timestamp < block.timestamp - maxAge

sherlock-audit / 2023-12-flatmoney-judging

Afriauditor - Afriauditor high OracleModule::_getPrice returns only Stale price #197