DittoETH - falconhoof's results

Lines of code

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

Vulnerability details

Summary

Inconsistent price checks in getOraclePrice() will lead to wrong prices being used with detrimental effects to stability of protocol .

Vulnerability Details

In getOraclePrice(), the returned price value for "other" oracles (e.g. JPY, XAU) is checked inconsistently.

Firstly, it is mentioned in the Known Issues regarding heartbeats:

" 2 hours staleness means it can be somewhat out of date "

However, both the try block and the catch block are missing even any heartbeat check as it is stated in the docs that there should be here. The try block does price checks in OracleCircuitBreaker() as below:

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

Secondly, there is a missing check in the catch block that the timestamp is not equal to 0, which is present in oracleCircuitBreaker() i.e. timeStamp == 0.

Thirdly, there is an inconsistent use of relational operators, where in the try block there is a check for negative prices but in the catch block the is just a check that the value is not equal to 0.

In oracleCircuitBreaker() we see the check chainlinkPrice <= 0 reverts if there is a negative price returned which, given that Chainlink's prices are int values, is relevant. However in the catch block, as below, we see the check price == 0.

if (roundID == 0 || price == 0 || timeStamp > block.timestamp) revert Errors.InvalidPrice();

Impact

Regarding the heartbeat and timestamp issues, these are esstential missing checks which protect the protocol from using stale prices.

Regarding the inconsistent application of relational operators, this means that if there is a failure in the try block because a negative price reverted there, that same incorrect price will be accepted and used in the catch block.

Both of these issues will cause huge issues to the correct functioning of the protocol via incorrect prices being fetched.

Tools Used

Manual Review Foundry Testing

Recommendations

Given that different feeds have different heartbeats, create an Oracle data struct whereby their specific infomation can be stored and fetched to be used in the oracleCircuitBreaker() and the catch block.

Update the price check and add the timestamp check to the catch block in getOraclePrice() as:

    } catch {

        // SOME CODE

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

        // SOME CODE
    }

Assessed type

Oracle

Lines of code

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibShortRecord.sol#L27-L28 https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/RedemptionFacet.sol#L56-L211

Vulnerability details

Summary

Redemption proposals should be processed using an up to date price rather than a cached price which may be stale

Vulnerability Details

In the docs it is stated here:

shortRecords that are invalid to be proposed: if it's greater than the max allowable CR

proposeRedemption() uses a cached price p.oraclePrice to calculate the collateral Ratio of Short Records being proposed. The cached price may be using stale data so the CRs in the list of proposals may now have different CRs which violate the C.MAX_REDEMPTION_CR check and therefore should never have been included in the slate.

    function proposeRedemption(
        address asset,
        MTypes.ProposalInput[] calldata proposalInput,
        uint88 redemptionAmount,
        uint88 maxRedemptionFee
    ) external isNotFrozen(asset) nonReentrant {

        // SOME CODE

        p.oraclePrice = LibOracle.getPrice(p.asset);

        // SOME CODE

        p.currentCR = currentSR.getCollateralRatio(p.oraclePrice);
        if (p.previousCR > p.currentCR || p.currentCR >= C.MAX_REDEMPTION_CR) continue;

        // SOME CODE
    }

Impact

Invalid Short Records are added to the slate. Paying off their debt increases their CR beyond the max allowed level which can affect stability of the protocol.

Tools Used

Manual Review Foundry Testing

Recommendations

Use getSavedOrSpotOraclePrice() which will use getPrice() if it's not stale to save gas or else return the spot price.

    function proposeRedemption(
        address asset,
        MTypes.ProposalInput[] calldata proposalInput,
        uint88 redemptionAmount,
        uint88 maxRedemptionFee
    ) external isNotFrozen(asset) nonReentrant {

        // SOME CODE

-       p.oraclePrice = LibOracle.getPrice(p.asset);
+       p.oraclePrice = LibOracle.getSavedOrSpotOraclePrice(p.asset);

        // SOME CODE

        p.currentCR = currentSR.getCollateralRatio(p.oraclePrice);
        if (p.previousCR > p.currentCR || p.currentCR >= C.MAX_REDEMPTION_CR) continue;

        // SOME CODE
    }

Assessed type

Oracle