Stale price can be used in getValueFromChainlinkFeed function

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-04-noya/blob/cc3854f634a72bd4a8b597021887088ca2d6d29f/contracts/helpers/valueOracle/oracles/ChainlinkOracleConnector.sol#L56-L62
https://github.com/code-423n4/2024-04-noya/blob/cc3854f634a72bd4a8b597021887088ca2d6d29f/contracts/helpers/valueOracle/oracles/ChainlinkOracleConnector.sol#L115-L135

Vulnerability details

Impact

According to the following updateChainlinkPriceAgeThreshold function, the minimum possible chainlinkPriceAgeThreshold would be 61 seconds. However, there are Chainlink oracles that have heartbeat that is less than 61 seconds; these oracles are essential for providing prices for the ERC20 tokens that should be supported by this protocol in which these tokens have the in scope token behaviors and exist on the intended chains described in https://code4rena.com/audits/2024-04-noya. For example, the USDC / USD Chainlink oracle on Polygon has a heartbeat of 27 seconds according to the popup of the Trigger parameters section's information icon in https://data.chain.link/feeds/polygon/mainnet/usdc-usd.

https://github.com/code-423n4/2024-04-noya/blob/cc3854f634a72bd4a8b597021887088ca2d6d29f/contracts/helpers/valueOracle/oracles/ChainlinkOracleConnector.sol#L56-L62

    function updateChainlinkPriceAgeThreshold(uint256 _chainlinkPriceAgeThreshold) external onlyMaintainer {
        if (_chainlinkPriceAgeThreshold <= 1 hours || _chainlinkPriceAgeThreshold >= 10 days) {
            revert NoyaChainlinkOracle_INVALID_INPUT();
        }
        chainlinkPriceAgeThreshold = _chainlinkPriceAgeThreshold;
        ...
    }

For Chainlink oracles that have heartbeat that is less than 61 seconds,block.timestamp - updatedAt > chainlinkPriceAgeThreshold in the following getValueFromChainlinkFeed function can be false when the updatedAt actually corresponds to a stale price. For instance, when the updatedAt returned by the USDC / USD Chainlink oracle on Polygon is block.timestamp - 27 * 2, a newer price should be reported at block.timestamp - 27 but that did not happen so the corresponding price reported at block.timestamp - 27 * 2 is already stale; yet, because block.timestamp - updatedAt > chainlinkPriceAgeThreshold is false for such updatedAt, the getValueFromChainlinkFeed function does not revert with the NoyaChainlinkOracle_DATA_OUT_OF_DATE error. As a result, the stale price is used in the getValueFromChainlinkFeed function.

Moreover, the chainlinkPriceAgeThreshold being set to only one value can fail to accommodate all Chainlink oracles that are intended to be supported by the ChainlinkOracleConnector contract when these oracles have different heartbeats. In this case, the chainlinkPriceAgeThreshold that ensures that one oracle's price is not stale can fail to ensure that the other oracle's price is not stale if the latter oracle's heartbeat is smaller than the former's.

https://github.com/code-423n4/2024-04-noya/blob/cc3854f634a72bd4a8b597021887088ca2d6d29f/contracts/helpers/valueOracle/oracles/ChainlinkOracleConnector.sol#L115-L135

    function getValueFromChainlinkFeed(
        AggregatorV3Interface source,
        uint256 amountIn,
        uint256 sourceTokenUnit,
        bool isInverse
    ) public view returns (uint256) {
        int256 price;
        uint256 updatedAt;
        (, price,, updatedAt,) = source.latestRoundData();
        uint256 uintprice = uint256(price);
        if (block.timestamp - updatedAt > chainlinkPriceAgeThreshold) {
            revert NoyaChainlinkOracle_DATA_OUT_OF_DATE();
        }
        if (price <= 0) {
            revert NoyaChainlinkOracle_PRICE_ORACLE_UNAVAILABLE(address(source), address(0), address(0));
        }
        if (isInverse) {
            return (amountIn * sourceTokenUnit) / uintprice;
        }
        return (amountIn * uintprice) / (sourceTokenUnit);
    }

Proof of Concept

Please add the following test in testFoundry\testOracle.sol. This test will pass to demonstrate the described scenario.

    function test_stalePriceCanBeUsed() public {
        vm.startPrank(owner);

        // cannot set price age threshold to 1 hour
        vm.expectRevert();
        chainlinkOracle.updateChainlinkPriceAgeThreshold(1 hours);

        // minimum price age threshold is 61 seconds
        chainlinkOracle.updateChainlinkPriceAgeThreshold(1 hours + 1);

        // For USDC / USD Chainlink oracle on Polygon, heartbeat is 27 seconds.
        // If updatedAt returned by such oracle is block.timestamp - 27 * 2, corresponding price is stale already.
        uint256 updatedAt = block.timestamp - 27 * 2;

        // Yet, block.timestamp - updatedAt > chainlinkPriceAgeThreshold is false for such updatedAt.
        // In this case, ChainlinkOracleConnector.getValueFromChainlinkFeed function does not revert with NoyaChainlinkOracle_DATA_OUT_OF_DATE error,
        //   which causes such stale price to be used.
        assertFalse(block.timestamp - updatedAt > chainlinkOracle.chainlinkPriceAgeThreshold());

        vm.stopPrank();
    }

Tools Used

Manual Review

Recommended Mitigation Steps

chainlinkPriceAgeThreshold can be updated to be a mapping by Chainlink oracle in which each oracle has one chainlinkPriceAgeThreshold value. Then, the updateChainlinkPriceAgeThreshold function can be updated to set the chainlinkPriceAgeThreshold value for the corresponding oracle in which such value can be set to be less than 61 seconds. At last, the getValueFromChainlinkFeed function can be updated to use the corresponding oracle's chainlinkPriceAgeThreshold value for checking whether the reported price is stale or not.

Assessed type

Oracle

[c4-pre-sort]

DadeKuma marked the issue as duplicate of #917

[c4-pre-sort]

DadeKuma marked the issue as not a duplicate

[c4-pre-sort]

DadeKuma marked the issue as primary issue

[c4-pre-sort]

DadeKuma marked the issue as sufficient quality report

[HadiEsna]

needs to be fixed. the recommended mitigation step is accepted.

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[c4-judge]

gzeon-c4 marked the issue as selected for report

[pkqs90]

Hey @gzeon-c4, this issue is identical to #917, they should be counted as duplicates.

[HadiEsna]

fix in commit c8ccd96c4aea5161f052cd17dd8263425d2560a0

[Bauchibred]

Hi @pksq90, I believe this is not a duplicate of 917, this report and its duplicates talk about how the lower bound of the stale check is not safe for some feeds a they are going to always ingest stale prices since their heartbeats are shorter than the lower bound. Whereas 917 and its duplicates hints why using a fixed chainlinkPriceAgeThreshold is bad practise and would lead to frequent DOS for some feeds asides other issues, see my duplicate to 917 (#59) and my duplicate to this (#234), for more info.

[pkqs90]

Hi @pksq90, I believe this is not a duplicate of 917, this report and its duplicates talk about how the lower bound of the stale check is not safe for some feeds a they are going to always ingest stale prices since their heartbeats are shorter than the lower bound. Whereas 917 and its duplicates hints why using a fixed chainlinkPriceAgeThreshold is bad practise and would lead to frequent DOS for some feeds asides other issues, see my duplicate to 917 (#59) and my duplicate to this (#234), for more info.

Thank you, I think you are right. They are indeed two different issues.

[gzeon-c4]

Thanks @Bauchibred