c4-bot-5 commented 3 months ago

Lines of code

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

Vulnerability details

Proof of Concept

Take a look at https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibOracle.sol#L19-L67

    function getOraclePrice(address asset) internal view returns (uint256) {
        AppStorage storage s = appStorage();
        AggregatorV3Interface baseOracle = AggregatorV3Interface(s.baseOracle);
        uint256 protocolPrice = getPrice(asset);

        AggregatorV3Interface oracle = AggregatorV3Interface(s.asset[asset].oracle);
        if (address(oracle) == address(0)) revert Errors.InvalidAsset();

        try baseOracle.latestRoundData() returns (uint80 baseRoundID, int256 basePrice, uint256, uint256 baseTimeStamp, uint80) {
            if (oracle == baseOracle) {
                // @dev multiply base oracle by 10**10 to give it 18 decimals of precision
                uint256 basePriceInEth = basePrice > 0 ? uint256(basePrice * C.BASE_ORACLE_DECIMALS).inv() : 0;
                basePriceInEth = baseOracleCircuitBreaker(protocolPrice, baseRoundID, basePrice, baseTimeStamp, basePriceInEth);
                return basePriceInEth;
            } else {
                // prettier-ignore
                (
                    uint80 roundID,
                    int256 price,
                    /*uint256 startedAt*/
                    ,
                    uint256 timeStamp,
                    /*uint80 answeredInRound*/
                ) = oracle.latestRoundData();
                uint256 priceInEth = uint256(price).div(uint256(basePrice));
                oracleCircuitBreaker(roundID, baseRoundID, price, basePrice, timeStamp, baseTimeStamp);
                return priceInEth;
            }
        } catch {
            if (oracle == baseOracle) {
                //@audit
                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;
            }
        }
    }

This function is used to get the price from the oracle, it now includes a try/catch logic in order to ensure a more robust integration, issue here is that there are two deviaitional implementation of the price returned depending on Chainlink's latestRoundData()availaability, if the call in the try block fails for whatever reason the execution goes to the catch block, now in the catch block, issue however is not pertaining to this but more fo how the checks applied to oracleCircuitBreaker & baseOracleCircuitBreaker are different, here are snippets from both implementation:

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

    function oracleCircuitBreaker(
        uint80 roundId,
        uint80 baseRoundId,
        int256 chainlinkPrice,
        int256 baseChainlinkPrice,
        uint256 timeStamp,
        uint256 baseTimeStamp
    ) private view {
        bool invalidFetchData = roundId == 0 || timeStamp == 0 || timeStamp > block.timestamp || chainlinkPrice <= 0
            || baseRoundId == 0 || baseTimeStamp == 0 || baseTimeStamp > block.timestamp || baseChainlinkPrice <= 0;

        if (invalidFetchData) revert Errors.InvalidPrice();
    }

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

    function baseOracleCircuitBreaker(
        uint256 protocolPrice,
        uint80 roundId,
        int256 chainlinkPrice,
        uint256 timeStamp,
        uint256 chainlinkPriceInEth
    ) private view returns (uint256 _protocolPrice) {
        bool invalidFetchData = roundId == 0 || timeStamp == 0 || timeStamp > block.timestamp || chainlinkPrice <= 0
            || block.timestamp > 2 hours + timeStamp;
            //..snip
            }

Evidently there is a deviation in the checks applied, the baseOracleCircuitBreaker() has a correct stale check integrated with it, i..e block.timestamp > 2 hours + timeStamp which ensures that the price being ingested is not older than the accepted 2 hours value, however this is not present in the oracleCircuitBreaker check and as such would mean that protocol would probably ingest heavily outdated data, whenever oracle != baseOracle

Impact

Inaccurate prices would be used , completely breaking the logic of all the pricing functionalities attached to the protocol

Recommended Mitigation Steps

Consider having the correct checks in both helper fucntions.

Assessed type

Oracle

c4-pre-sort commented 3 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 3 months ago

raymondfam marked the issue as duplicate of #4