Risk of Financial Loss Due to Chainlink Price Feed Fallback Vulnerability

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibOracle.sol#L50-L66

Vulnerability details

Impact

The project employs a recommended safer approach for querying Chainlink price feeds, utilizing a defensive approach with Solidity’s try/catch structure. This ensures that if the call to the price feed fails, the caller contract remains in control and can handle any errors safely and explicitly. However, there are critical missing checks for received asset prices that could lead to one of the following two issues:

• Use of stale data
• Incorrect calculation for the asset's price.

In both cases, the impact could result in users and the protocol losing funds, potentially harming the protocol's reputation. Therefore, the severity should be determined by considering the following criteria:

• Impact: High - Could result in unfair losses for users and the protocol.
• Likelihood: Low - Only occurs in specific rare conditions.

Severity: MEDIUM

Proof of Concept

While a safer method is implemented for querying Chainlink price feeds, the fallback logic in case of errors is vulnerable. Specifically, if the asset being queried for price is a non-USD asset, meaning it has an asset-specific oracle different from the baseOracle, then the else statement of the catch block could lead to the following scenarios:

1. Stale data: The price may be stale, as there is no check for it like the one in baseOracleCircuitBreaker(), i.e., block.timestamp > 2 hours + timeStamp;. This could result in users' trades being executed with incorrect asset prices, leading to loss of funds for users or the protocol.

2. Incorrect price: In rare cases, the price returned from the oracle could be a negative number. The current check does not handle this case, as it only checks if price == 0, which is not sufficient. If the price is a negative number, a direct typecast to uint256 could yield unexpected results, resulting in an incorrect price for the asset.

To verify the second issue, the following function could be used in Remix:

 function testNegativeTypeCast(int256 a) external pure returns(uint256) {
        return uint256(a);
    }

The output is as follows:

to  T.testNegativeTypeCast(int256) 0xddaAd340b0f1Ef65169Ae5E41A8b10776a75482d
...
decoded input   {
    "int256 a": "-5"
}
decoded output  {
    "0": "uint256: 115792089237316195423570985008687907853269984665640564039457584007913129639931"
}

Vulnerable code in the context:

function getOraclePrice(address asset) internal view returns (uint256) {
        ...

        try baseOracle.latestRoundData() returns (uint80 baseRoundID, int256 basePrice, uint256, uint256 baseTimeStamp, uint80) {
            ...
        } catch {
            if (oracle == baseOracle) {
                return twapCircuitBreaker();
            } else {
                // prettier-ignore
                (
                    uint80 roundID,
                    int256 price,
                    /*uint256 startedAt*/
                    ,
                    uint256 timeStamp,
                    /*uint80 answeredInRound*/
                ) = oracle.latestRoundData();
                if (roundID == 0 || price == 0 || timeStamp > block.timestamp) revert Errors.InvalidPrice();

                uint256 twapInv = twapCircuitBreaker();
                uint256 priceInEth = uint256(price * C.BASE_ORACLE_DECIMALS).mul(twapInv);
                return priceInEth;
            }
        }
}

Tools Used

Manual Review, Remix

Recommended Mitigation Steps

To implement robust failover logic, add the following checks to the else statement of the catch block:

- if (roundID == 0 || price == 0 || timeStamp > block.timestamp) revert Errors.InvalidPrice();
+ if (roundID == 0 || price <= 0 || timeStamp > block.timestamp || block.timestamp > 2 hours + timeStamp) { revert Errors.InvalidPrice(); }

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #4

[raymondfam]

See #8.

[c4-judge]

hansfriese marked the issue as unsatisfactory: Out of scope

[hansfriese]

Nice finding!

193 and #155 of the same warden fully cover the primary issue. (#164).

I will close #155 and give full credit to this one.

[c4-judge]

hansfriese marked the issue as not a duplicate

[c4-judge]

hansfriese removed the grade

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese marked the issue as duplicate of #164