The shortOrder verification bug on the RedemptionFacet::proposeRedemption() allows an attacker to leave a small shortOrder on the order book, leading to the protocol's bad debt

[c4-bot-9]

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

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

Inadequate/unstructured proof to support the intended code refactoring.

[c4-judge]

hansfriese marked the issue as unsatisfactory:
Insufficient proof

[serial-coder]

Hi @hansfriese,

Appeal

Vulnerability Details

Please have a second look at the following from the Proof of Concept section:

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.

To demystify the vulnerability again, please follow the links along:

1. The shortOrder is loaded from storage according to the attacker's p.shortOrderId param.

2. The attacker can leave their target partially-filled shortOrder (corresponding to the redeeming shortRecord) with ercAmount < minShortErc alive on the order book by specifying the p.shortOrderId param to another shortOrder's id with ercAmount >= minShortErc (to bypass the if statement).

3. The root cause is that the proposeRedemption() will verify the loaded shortOrder against the shortRecordId and shorter params only after the condition in Step 2 was met.

4. Since the attacker already bypassed the condition check in Step 2, Step 3 would not be executed.

Hence, if the attacker points the p.shortOrderId param to another (fake) shortOrder's id with ercAmount >= minShortErc, they can bypass the validity check of the loaded (fake) shortOrder, preventing their targeted small shortOrder (the real shortOrder corresponding to the redeeming shortRecord) from being canceled.

Subsequently, the attacker can leave their target small shortOrder on the order book and when it is matched, its short position (shortRecord) will be less than the minShortErc threshold that will disincentivize liquidators from liquidating the position even if it is liquidable, leading to the protocol's bad debt. Moreover, the small shortRecord (i.e., shortRecord.ercDebt < minShortErc) will also disable the redemption mechanism from redeeming it for ethCollateral.

Summary Of Impacts

1. An attacker can leave small shortOrders on the order book. As a result, large Bid orders can run out of gas if the attacker places many small shortOrders on the order book.

2. Liquidators are disincentivized from liquidating small shortRecords because of their small amounts.

3. Small shortRecords disable the redemption mechanism from redeeming them for ethCollateral even if they have poor collateralization.

Solution

The snippet below presents how to fix the vulnerability by verifying the validity of the loaded shortOrder (Step 1) against the shortRecordId and shorter params (Step 3) before processing the loaded shortOrder (Step 2).

This way, an attacker cannot manipulate the p.shortOrderId param to another (fake) shortOrder's id.

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

            ...
        }

        ...
    }

[ditto-eth]

This is valid, good find

[c4-sponsor]

ditto-eth (sponsor) confirmed

[hansfriese]

Nice finding.
Medium is appropriate as an attacker can bypass the minShortErc validation.

[c4-judge]

hansfriese removed the grade

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese marked the issue as selected for report