DittoETH - serial-coder's results

Judge has assessed an item in Issue #266 as 3 risk. The relevant finding follows:

[L-02] Incorrectly executing the updateErcDebt() leads to virtually minting the ercDebt more than necessary

Judge has assessed an item in Issue #266 as 3 risk. The relevant finding follows:

Incorrect verification of shorter and shortRecordId in RedemptionFacet::claimRemainingCollateral()

Lines of code

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/RedemptionFacet.sol#L81 https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/RedemptionFacet.sol#L110-L114 https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/RedemptionFacet.sol#L38

Vulnerability details

Impact

The BidOrdersFacet::bidMatchAlgo() allows a shortOrder to be partially matched and leave its ercAmount * price < minAskEth due to the DUST_FACTOR constant (as long as its corresponding shortRecord is maintaining enough ercDebt + shortOrder's ercAmount to keep the position >= the minShortErc threshold).

The redemption process enables redeemers to redeem their ercEscrowed for ethCollateral on target shortRecords. If a shortRecord was partially filled, the RedemptionFacet::proposeRedemption() must guarantee that the corresponding shortOrder maintains the ercAmount >= minShortErc. In other words, if the shortOrder's ercAmount is less than the minShortErc threshold, the shortOrder must be canceled from the order book. Otherwise, the shortRecord position will be less than the minShortErc threshold when the order is matched again. Subsequently, the small shortRecord (short position) will not incentivize liquidators to liquidate it even if it is liquidable, leaving bad debt to the protocol.

I discovered that the proposeRedemption() improperly verifies the proposer (redeemer)'s inputted shortOrderId param, allowing an attacker to specify the shortOrderId param to another shortOrder's id that does not correspond to the target redeeming shortRecord.

The vulnerability can bypass the minShortErc threshold verification process on the shortOrder corresponding to the processing shortRecord, eventually allowing an attacker to leave a small shortRecord position that disincentivizes liquidators from liquidating the position. Furthermore, the small shortRecord also disables the redemption mechanism from redeeming it for ethCollateral.

Proof of Concept

While processing the proposalInput param, the proposeRedemption() will load the shortOrder from storage according to the attacker-inputted shortOrderId param (i.e., p.shortOrderId).

Suppose that the attacker wants to leave their small shortRecord position to disincentivize liquidators from liquidating it (as well as disabling the redemption mechanism from redeeming it for ethCollateral). They can place their partially-filled shortRecord that has the corresponding shortOrder.ercAmount < minShortErc to be redeemed via a Sybil account.

To bypass the minShortErc threshold verification process from canceling their small shortOrder, the attacker must specify the p.shortOrderId param to another shortOrder's id with ercAmount >= minShortErc. The manipulated p.shortOrderId param can bypass the verification process because if the loaded shortOrder.ercAmount >= minShortErc, the proposeRedemption() will not proceed to verify the validity of the shortOrder.

Hence, the attacker can target their shortRecord to be redeemed and leave its corresponding shortOrder with ercAmount < minShortErc to keep alive on the order book. Once their small shortOrder is matched again, it will leave the shortRecord position less than the minShortErc threshold, disincentivizing liquidators from liquidating it.

Furthermore, the small shortRecord (i.e., shortRecord.ercDebt < minShortErc) also disables the redemption mechanism from redeeming it for ethCollateral.

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

        for (uint8 i = 0; i < proposalInput.length; i++) {
            p.shorter = proposalInput[i].shorter;
            p.shortId = proposalInput[i].shortId;
            p.shortOrderId = proposalInput[i].shortOrderId;
            STypes.ShortRecord storage currentSR = s.shortRecords[p.asset][p.shorter][p.shortId];

            ...

@1          STypes.Order storage shortOrder = s.shorts[asset][p.shortOrderId]; //@audit -- The shortOrder is loaded from storage according to the attacker's p.shortOrderId param
@2          if (currentSR.status == SR.PartialFill && shortOrder.ercAmount < minShortErc) { //@audit -- The attacker can leave their target partially-filled shortOrder (corresponding to the redeeming shortRecord) with ercAmount < minShortErc alive by specifying the p.shortOrderId param to another shortOrder's id with ercAmount >= minShortErc
@3              if (shortOrder.shortRecordId != p.shortId || shortOrder.addr != p.shorter) revert Errors.InvalidShortOrder(); //@audit -- The root cause is that the proposeRedemption() verifies the loaded shortOrder against the legitimate shortRecordId and shorter params only after the condition in @2 was met
                LibOrders.cancelShort(asset, p.shortOrderId);
            }

            ...
        }

        ...
    }

• @1 -- The shortOrder is loaded from storage according to the attacker's p.shortOrderId param: https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/RedemptionFacet.sol#L110

• @2 -- The attacker can leave their target partially-filled shortOrder (corresponding to the redeeming shortRecord) with ercAmount < minShortErc alive by specifying the p.shortOrderId param to another shortOrder's id with ercAmount >= minShortErc: https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/RedemptionFacet.sol#L111

• @3 -- The root cause is that the proposeRedemption() verifies the loaded shortOrder against the legitimate shortRecordId and shorter params only after the condition in @2 was met: https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/RedemptionFacet.sol#L112

Tools Used

Manual Review

Recommended Mitigation Steps

To fix the vulnerability, move out the shortOrder verification check and execute it immediately after loading the shortOrder from storage.

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

        for (uint8 i = 0; i < proposalInput.length; i++) {
            p.shorter = proposalInput[i].shorter;
            p.shortId = proposalInput[i].shortId;
            p.shortOrderId = proposalInput[i].shortOrderId;
            STypes.ShortRecord storage currentSR = s.shortRecords[p.asset][p.shorter][p.shortId];

            ...

            STypes.Order storage shortOrder = s.shorts[asset][p.shortOrderId];
+           if (shortOrder.shortRecordId != p.shortId || shortOrder.addr != p.shorter) revert Errors.InvalidShortOrder();
            if (currentSR.status == SR.PartialFill && shortOrder.ercAmount < minShortErc) {
-               if (shortOrder.shortRecordId != p.shortId || shortOrder.addr != p.shorter) revert Errors.InvalidShortOrder();
                LibOrders.cancelShort(asset, p.shortOrderId);
            }

            ...
        }

        ...
    }

Assessed type

Invalid Validation

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

        for (uint8 i = 0; i < proposalInput.length; i++) {
            p.shorter = proposalInput[i].shorter;
            p.shortId = proposalInput[i].shortId;
            p.shortOrderId = proposalInput[i].shortOrderId;
            STypes.ShortRecord storage currentSR = s.shortRecords[p.asset][p.shorter][p.shortId];

            ...

            STypes.Order storage shortOrder = s.shorts[asset][p.shortOrderId]; //@audit -- Step 1
+           if (shortOrder.shortRecordId != p.shortId || shortOrder.addr != p.shorter) revert Errors.InvalidShortOrder(); //@audit -- Process Step 3 before Step 2 to fix the vulnerability
            if (currentSR.status == SR.PartialFill && shortOrder.ercAmount < minShortErc) { //@audit -- Step 2
-               if (shortOrder.shortRecordId != p.shortId || shortOrder.addr != p.shorter) revert Errors.InvalidShortOrder(); //@audit -- Step 3
                LibOrders.cancelShort(asset, p.shortOrderId);
            }

            ...
        }

        ...
    }

Lines of code

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

Vulnerability details

Impact

This report raises an issue regarding the lack of a stale price check for the base oracle (ETH/USD price) in the oracleCircuitBreaker() only, as the lack of a stale price check for the non-ETH/USD asset oracle (multi-asset oracle) was flagged as a known issue.

The LibOracle::oracleCircuitBreaker() does not check the stale price for the base oracle (ETH/USD price). Specifically, I'm talking about the condition: "block.timestamp > 2 hours + baseTimeStamp" (i.e., the 2-hour stale heartbeat as per the docs). Hence, the function cannot verify whether or not the baseChainlinkPrice is stale.

Consequently, the oracleCircuitBreaker() will not revert the transaction as expected if the baseChainlinkPrice is stale. As a result, the protocol's core functions will consume the stale price, harming the protocol's and users' funds.

Proof of Concept

The oracleCircuitBreaker() does not check the stale price for the base oracle (ETH/USD price) (i.e., the condition: "block.timestamp > 2 hours + baseTimeStamp") compared to the baseOracleCircuitBreaker().

Without the stale price check mentioned above, the oracleCircuitBreaker() cannot verify whether the baseChainlinkPrice is stale. For this reason, the oracleCircuitBreaker() will not revert the transaction as expected if the baseChainlinkPrice is stale.

    //@audit -- This report raises an issue regarding the lack of a stale price check for the base oracle (ETH/USD price) in the oracleCircuitBreaker() only, as the lack of a stale price check for the non-ETH/USD asset oracle (multi-asset oracle) was flagged as a known issue
    function oracleCircuitBreaker(
        uint80 roundId,
        uint80 baseRoundId,
        int256 chainlinkPrice,
        int256 baseChainlinkPrice,
        uint256 timeStamp,
        uint256 baseTimeStamp
    ) private view {
@1      bool invalidFetchData = roundId == 0 || timeStamp == 0 || timeStamp > block.timestamp || chainlinkPrice <= 0
@1          || baseRoundId == 0 || baseTimeStamp == 0 || baseTimeStamp > block.timestamp || baseChainlinkPrice <= 0; //@audit -- The oracleCircuitBreaker() lacks checking the stale price for the base oracle (ETH/USD price) (condition: "block.timestamp > 2 hours + baseTimeStamp")

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

    ...

    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
@2          || block.timestamp > 2 hours + timeStamp; //@audit -- Whereas the baseOracleCircuitBreaker() already checks that condition as per the docs
        uint256 chainlinkDiff =
            chainlinkPriceInEth > protocolPrice ? chainlinkPriceInEth - protocolPrice : protocolPrice - chainlinkPriceInEth;
        bool priceDeviation = protocolPrice > 0 && chainlinkDiff.div(protocolPrice) > 0.5 ether;

        ...
    }

• @1 -- The oracleCircuitBreaker() lacks checking the stale price for the base oracle (ETH/USD price) (condition: "block.timestamp > 2 hours + baseTimeStamp"): https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibOracle.sol#L125-L126

• @2 -- Whereas the baseOracleCircuitBreaker() already checks that condition as per the docs: https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibOracle.sol#L77

Tools Used

Manual Review

Recommended Mitigation Steps

Add the stale price check for the base oracle (the condition: block.timestamp > 2 hours + baseTimeStamp) in the oracleCircuitBreaker().

    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;
+           || baseRoundId == 0 || baseTimeStamp == 0 || baseTimeStamp > block.timestamp || baseChainlinkPrice <= 0
+           || block.timestamp > 2 hours + baseTimeStamp; //@audit -- Add the stale price check for the base oracle (ETH/USD price)

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

Assessed type

Oracle

    //@audit -- This report raises an issue regarding the lack of a stale price check for the base oracle (ETH/USD price) in the oracleCircuitBreaker() only, as the lack of a stale price check for the non-ETH/USD asset oracle (multi-asset oracle) was flagged as a known issue
    function oracleCircuitBreaker(
        uint80 roundId,
        uint80 baseRoundId,
        int256 chainlinkPrice,
        int256 baseChainlinkPrice,
        uint256 timeStamp,
        uint256 baseTimeStamp
    ) private view {
@1      bool invalidFetchData = roundId == 0 || timeStamp == 0 || timeStamp > block.timestamp || chainlinkPrice <= 0
@1          || baseRoundId == 0 || baseTimeStamp == 0 || baseTimeStamp > block.timestamp || baseChainlinkPrice <= 0; //@audit -- The oracleCircuitBreaker() lacks checking the stale price for the base oracle (ETH/USD price) (condition: "block.timestamp > 2 hours + baseTimeStamp")

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

    ...

    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
@2          || block.timestamp > 2 hours + timeStamp; //@audit -- Whereas the baseOracleCircuitBreaker() already checks that condition as per the docs
        uint256 chainlinkDiff =
            chainlinkPriceInEth > protocolPrice ? chainlinkPriceInEth - protocolPrice : protocolPrice - chainlinkPriceInEth;
        bool priceDeviation = protocolPrice > 0 && chainlinkDiff.div(protocolPrice) > 0.5 ether;

        ...
    }

[L-01] Incorrect verification of shorter and shortRecordId in RedemptionFacet::claimRemainingCollateral()

Impact

The RedemptionFacet::claimRemainingCollateral() incorrectly verifies the shorter (msg.sender) and shortRecordId (the inputted id) parameters, which can lead to further exploits or unexpected circumstances.

Proof of Concept

As you can see below, the claimRemainingCollateral() verifies the shorter (msg.sender) and shortRecordId (the inputted id) parameters using the && operator.

Therefore, the function caller can easily bypass the verification by inputting one of the two logical expressions to be false. For example, inputting the id parameter == claimProposal.shortId.

    function claimRemainingCollateral(address asset, address redeemer, uint8 claimIndex, uint8 id)
        external
        isNotFrozen(asset)
        nonReentrant
    {
        STypes.AssetUser storage redeemerAssetUser = s.assetUser[asset][redeemer];
        if (redeemerAssetUser.SSTORE2Pointer == address(0)) revert Errors.InvalidRedemption();
        if (redeemerAssetUser.timeToDispute > LibOrders.getOffsetTime()) revert Errors.TimeToDisputeHasNotElapsed();

        // @dev Only need to read up to the position of the SR to be claimed
        MTypes.ProposalData[] memory decodedProposalData =
            LibBytes.readProposalData(redeemerAssetUser.SSTORE2Pointer, claimIndex + 1);
        MTypes.ProposalData memory claimProposal = decodedProposalData[claimIndex];

        //@audit -- Incorrect conditional logic (must use the "||" operator instead of the "&&" operator)
@>      if (claimProposal.shorter != msg.sender && claimProposal.shortId != id) revert Errors.CanOnlyClaimYourShort();

        STypes.Asset storage Asset = s.asset[asset];
        _claimRemainingCollateral({asset: asset, vault: Asset.vault, shorter: msg.sender, shortId: id});
    }

• Incorrect conditional logic (must use the "||" operator instead of the "&&" operator): https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/RedemptionFacet.sol#L361

Tools Used

Manual Review

Recommended Mitigation Steps

Use the operator || instead of the &&.

    function claimRemainingCollateral(address asset, address redeemer, uint8 claimIndex, uint8 id)
        external
        isNotFrozen(asset)
        nonReentrant
    {
        STypes.AssetUser storage redeemerAssetUser = s.assetUser[asset][redeemer];
        if (redeemerAssetUser.SSTORE2Pointer == address(0)) revert Errors.InvalidRedemption();
        if (redeemerAssetUser.timeToDispute > LibOrders.getOffsetTime()) revert Errors.TimeToDisputeHasNotElapsed();

        // @dev Only need to read up to the position of the SR to be claimed
        MTypes.ProposalData[] memory decodedProposalData =
            LibBytes.readProposalData(redeemerAssetUser.SSTORE2Pointer, claimIndex + 1);
        MTypes.ProposalData memory claimProposal = decodedProposalData[claimIndex];

-       if (claimProposal.shorter != msg.sender && claimProposal.shortId != id) revert Errors.CanOnlyClaimYourShort();
+       if (claimProposal.shorter != msg.sender || claimProposal.shortId != id) revert Errors.CanOnlyClaimYourShort();

        STypes.Asset storage Asset = s.asset[asset];
        _claimRemainingCollateral({asset: asset, vault: Asset.vault, shorter: msg.sender, shortId: id});
    }

---

[L-02] Incorrectly executing the updateErcDebt() leads to virtually minting the ercDebt more than necessary

Impact

The ExitShortFacet::exitShort() executes the checkCancelShortOrder() before the updateErcDebt().

Consequently, if the shortRecord is partially filled and has the ercDebt less than the minShortErc threshold, the shortOrder will be canceled and the ercDebt will be virtually minted more than necessary to the shortRecord (to maintain the minShortErc threshold).

Proof of Concept

The exitShort() executes the checkCancelShortOrder() to check whether the shortRecord is partially filled. The checkCancelShortOrder() will cancel the shortOrder if the shortRecord is partially filled.

Then, the exitShort() invokes the updateErcDebt() to update the ercDebt of the shortRecord.

    function exitShort(
        address asset,
        uint8 id,
        uint88 buybackAmount,
        uint80 price,
        uint16[] memory shortHintArray,
        uint16 shortOrderId
    ) external isNotFrozen(asset) nonReentrant onlyValidShortRecord(asset, msg.sender, id) {
        ...

        //@audit -- The checkCancelShortOrder() is executed before the updateErcDebt()
@1      e.shortOrderIsCancelled = LibSRUtil.checkCancelShortOrder({
@1          asset: asset,
@1          initialStatus: short.status,
@1          shortOrderId: shortOrderId,
@1          shortRecordId: id,
@1          shorter: msg.sender
@1      });
@1
@1      short.updateErcDebt(e.asset);

        ...
    }

• @1 -- The checkCancelShortOrder() is executed before the updateErcDebt(): https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/ExitShortFacet.sol#L158-L166

To cancel the shortOrder, the cancelShort() is triggered. The function will check if the position has the ercDebt less than the minShortErc threshold. If that is the case, the function will spend some of the collateral to virtually mint the ercDebt (to be == minShortErc).

However, since the updateErcDebt() is executed after the shortOrder has already been canceled, the cancelShort() will virtually mint the ercDebt more than necessary.

Note: the updateErcDebt() will increase the ercDebt of the shortRecord. Therefore, if we execute the updateErcDebt() before, the cancelShort() will mint the ercDebt lesser to maintain the minShortErc threshold.

    function cancelShort(address asset, uint16 id) internal {
        ...

        if (shortRecord.status == SR.Closed) {
            ...

        } else {
            uint88 minShortErc = uint88(LibAsset.minShortErc(asset));
            if (shortRecord.ercDebt < minShortErc) {
                // @dev prevents leaving behind a partially filled SR is under minShortErc
                // @dev if the corresponding short is cancelled, then the partially filled SR's debt will == minShortErc
@2              uint88 debtDiff = minShortErc - shortRecord.ercDebt;
                {
                    STypes.Vault storage Vault = s.vault[vault];

@2                  uint88 collateralDiff = shortOrder.price.mulU88(debtDiff).mulU88(LibOrders.convertCR(shortOrder.shortOrderCR));

                    //@audit -- If the shortRecord is partially filled, the shortOrder will be canceled,
                    //          and if shortRecord.ercDebt < minShortErc, the collateral will be used to 
                    //          virtually mint ercDebt (to be == minShortErc) more than expected
                    LibShortRecord.fillShortRecord(
                        asset,
                        shorter,
                        shortRecordId,
                        SR.FullyFilled,
@2                      collateralDiff,
@2                      debtDiff,
                        Asset.ercDebtRate,
                        Vault.dethYieldRate
                    );

@2                  Vault.dethCollateral += collateralDiff;
@2                  Asset.dethCollateral += collateralDiff;
@2                  Asset.ercDebt += debtDiff;

                    // @dev update the eth refund amount
@2                  eth -= collateralDiff;
                }
                // @dev virtually mint the increased debt
@2              s.assetUser[asset][shorter].ercEscrowed += debtDiff;
            } else {
                shortRecord.status = SR.FullyFilled;
            }
        }

        ...
    }

• @2 -- If the shortRecord is partially filled, the shortOrder will be canceled, and if shortRecord.ercDebt < minShortErc, the collateral will be used to virtually mint ercDebt (to be == minShortErc) more than expected: https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibOrders.sol#L907-L932

Tools Used

Manual Review

Recommended Mitigation Steps

Execute the updateErcDebt() before the checkCancelShortOrder().

    function exitShort(
        address asset,
        uint8 id,
        uint88 buybackAmount,
        uint80 price,
        uint16[] memory shortHintArray,
        uint16 shortOrderId
    ) external isNotFrozen(asset) nonReentrant onlyValidShortRecord(asset, msg.sender, id) {
        ...

+       short.updateErcDebt(e.asset);

        e.shortOrderIsCancelled = LibSRUtil.checkCancelShortOrder({
            asset: asset,
            initialStatus: short.status,
            shortOrderId: shortOrderId,
            shortRecordId: id,
            shorter: msg.sender
        });

-       short.updateErcDebt(e.asset);

        ...
    }

---

[L-03] The price deviation in LibOracle::baseOracleCircuitBreaker() should not be hardcoded

Impact

The hardcoded value of 50% price deviation (0.5 ether) may be too large when using ETH as a base price reference. Moreover, the fixed % deviation is considered too risky because the protocol's DAO or admin will not be able to update it in production.

Consequently, the check for price deviation in the LibOracle::baseOracleCircuitBreaker() may not be effective enough to filter out the stale price in production, directly affecting the quality of the oracle price that will be consumed by the core functions of the Ditto protocol.

Proof of Concept

The baseOracleCircuitBreaker() verifies the price reported by Chainlink. If the reported price is invalid or its price deviation when compared to the protocol's cached oracle price is more than 50%, the function will fall back to get Uniswap's TWAP price instead.

However, the baseOracleCircuitBreaker() uses a hardcoded value of 50% price deviation (0.5 ether), which may be too large when using ETH as a base price reference. Moreover, the fixed % deviation is considered too risky because the protocol's DAO or admin will not be able to update it in production.

    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;
        uint256 chainlinkDiff =
            chainlinkPriceInEth > protocolPrice ? chainlinkPriceInEth - protocolPrice : protocolPrice - chainlinkPriceInEth;
@>      bool priceDeviation = protocolPrice > 0 && chainlinkDiff.div(protocolPrice) > 0.5 ether;

        // @dev if there is issue with chainlink, get twap price. Verify twap and compare with chainlink
        if (invalidFetchData) {
            ...

@>      } else if (priceDeviation) {
            ...

        } else {
            ...
        }
    }

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

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

Tools Used

Manual Review

Recommended Mitigation Steps

The % price deviation should be a variable updatable by the protocol's DAO or admin in production.

---

[L-04] Inconsistency in calculating the minShortErc in the ShortOrdersFacet::createLimitShort()

Impact

I noticed the inconsistency in calculating the p.minShortErc in the ShortOrdersFacet::createLimitShort() when the cr param < 1 ether and when the param >= 1 ether.

If the cr param < 1 ether, the p.minShortErc has to be modified to be bigger. But, the resulting p.minShortErc will be bigger than expected. Consequently, a shorter must supply the ercAmount more than expected to create a limit shortOrder with the cr < 1 ether.

Proof of Concept

If the cr param < 1 ether, p.minShortErc = LibAsset.minShortErc(asset).mul(1 ether + cr.inv()).

Suppose the cr == 0.5 ether.

p.minShortErc = LibAsset.minShortErc(asset).mul(1 ether + (1/0.5 ether)) = LibAsset.minShortErc(asset).mul(1 ether + 2 ether) = LibAsset.minShortErc(asset).mul(3 ether)

As you can see, the resulting multiplier will be 3 ether instead of 2 ether (compared to the case cr param >= 1 ether).

    function createLimitShort(
        address asset,
        uint80 price,
        uint88 ercAmount,
        MTypes.OrderHint[] memory orderHintArray,
        uint16[] memory shortHintArray,
        uint16 shortOrderCR
    ) external isNotFrozen(asset) onlyValidAsset(asset) nonReentrant {
        ...

        // @dev minShortErc needs to be modified (bigger) when cr < 1
@>      p.minShortErc = cr < 1 ether ? LibAsset.minShortErc(asset).mul(1 ether + cr.inv()) : LibAsset.minShortErc(asset);
        p.eth = price.mul(ercAmount);
        p.minAskEth = LibAsset.minAskEth(asset);
        if (ercAmount < p.minShortErc || p.eth < p.minAskEth) revert Errors.OrderUnderMinimumSize();

        ...
    }

• https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/ShortOrdersFacet.sol#L56

Tools Used

Manual Review

Recommended Mitigation Steps

The formula for calculating the p.minShortErc when the cr param < 1 ether should be LibAsset.minShortErc(asset).mul(cr.inv()).

    function createLimitShort(
        address asset,
        uint80 price,
        uint88 ercAmount,
        MTypes.OrderHint[] memory orderHintArray,
        uint16[] memory shortHintArray,
        uint16 shortOrderCR
    ) external isNotFrozen(asset) onlyValidAsset(asset) nonReentrant {
        ...

        // @dev minShortErc needs to be modified (bigger) when cr < 1
-       p.minShortErc = cr < 1 ether ? LibAsset.minShortErc(asset).mul(1 ether + cr.inv()) : LibAsset.minShortErc(asset);
+       p.minShortErc = cr < 1 ether ? LibAsset.minShortErc(asset).mul(cr.inv()) : LibAsset.minShortErc(asset);
        p.eth = price.mul(ercAmount);
        p.minAskEth = LibAsset.minAskEth(asset);
        if (ercAmount < p.minShortErc || p.eth < p.minAskEth) revert Errors.OrderUnderMinimumSize();

        ...
    }