Lines of code

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

Vulnerability details

Impact

It's possible for a short order to remain partially filled, typically when the bid order amount is less than the short order amount. The issue arises when these ShortRecords remain partially filled without the presence of an associated short, rendering them unable to be liquidated or redeemed if necessary.

The problem occurs when the amount of the bid order is slightly less than the amount of the short order. The order fills in LibOrders#L582, and then, if there are no more bid orders at that price, it adds a sell order if the remaining amount is greater than minAskEth in LibOrders#L594. However, if the remaining amount is less than minAskEth, no short is created (LibOrders#L594), leaving the ShortRecord in PartialFill status without an associated short.

File: LibOrders.sol
556:     function sellMatchAlgo(
557:         address asset,
558:         STypes.Order memory incomingAsk,
559:         MTypes.OrderHint[] memory orderHintArray,
560:         uint256 minAskEth
561:     ) internal {
...
...
582:                 matchHighestBid(incomingAsk, highestBid, asset, matchTotal);
583:                 if (incomingAsk.ercAmount > highestBid.ercAmount) {
584:                     incomingAsk.ercAmount -= highestBid.ercAmount;
585:                     highestBid.ercAmount = 0;
586:                     matchOrder(s.bids, asset, highestBid.id);
587: 
588:                     // loop
589:                     startingId = highestBid.nextId;
590:                     if (startingId == C.TAIL) {
591:                         matchIncomingSell(asset, incomingAsk, matchTotal);
592: 
593:                         if (incomingAsk.ercAmount.mul(incomingAsk.price) >= minAskEth) {
594:                             addSellOrder(incomingAsk, asset, orderHintArray);
595:                         }
596:                         s.bids[asset][C.HEAD].nextId = C.TAIL;
597:                         return;
598:                     }
...
...

As a result, during liquidation, when calling LibSRUtil.checkCancelShortOrder in PrimaryLiquidationFacet#L59, the transaction will revert (LibSRUtil#L57) because there is no short associated with the user's ShortRecord.

File: PrimaryLiquidationFacet.sol
47:     function liquidate(address asset, address shorter, uint8 id, uint16[] memory shortHintArray, uint16 shortOrderId)
48:         external
49:         isNotFrozen(asset)
50:         nonReentrant
51:         onlyValidShortRecord(asset, shorter, id)
52:         returns (uint88, uint88)
53:     {
...
...
59:         LibSRUtil.checkCancelShortOrder({
60:             asset: asset,
61:             initialStatus: s.shortRecords[asset][shorter][id].status,
62:             shortOrderId: shortOrderId,
63:             shortRecordId: id,
64:             shorter: shorter
65:         });
...
...

File: LibSRUtil.sol
49:     function checkCancelShortOrder(address asset, SR initialStatus, uint16 shortOrderId, uint8 shortRecordId, address shorter)
50:         internal
51:         returns (bool isCancelled)
52:     {
53:         AppStorage storage s = appStorage();
54:         if (initialStatus == SR.PartialFill) {
55:             STypes.Order storage shortOrder = s.shorts[asset][shortOrderId];
56:             STypes.ShortRecord storage shortRecord = s.shortRecords[asset][shorter][shortRecordId];
57:             if (shortOrder.shortRecordId != shortRecordId || shortOrder.addr != shorter) revert Errors.InvalidShortOrder();
58: 
59:             if (shorter == msg.sender) {
60:                 // If call comes from exitShort() or combineShorts() then always cancel
61:                 LibOrders.cancelShort(asset, shortOrderId);
62:                 assert(shortRecord.status != SR.PartialFill);
63:                 return true;
64:             } else if (shortRecord.ercDebt < LibAsset.minShortErc(asset)) {
65:                 // If call comes from liquidate() and SR ercDebt under minShortErc
66:                 LibOrders.cancelShort(asset, shortOrderId);
67:                 return true;
68:             }
69:         }
70:     }

The same issue occurs in RedemptionFacet#L112, where the transaction will revert because the ShortRecord being redeemed is in PartialFill status and has no associated short.

File: RedemptionFacet.sol
056:     function proposeRedemption(
057:         address asset,
058:         MTypes.ProposalInput[] calldata proposalInput,
059:         uint88 redemptionAmount,
060:         uint88 maxRedemptionFee
061:     ) external isNotFrozen(asset) nonReentrant {
...
...
108:             // @dev Cancel attached shortOrder if below minShortErc, regardless of ercDebt in SR
109:             // @dev All verified SR have ercDebt >= minShortErc so CR does not change in cancelShort()
110:             STypes.Order storage shortOrder = s.shorts[asset][p.shortOrderId];
111:             if (currentSR.status == SR.PartialFill && shortOrder.ercAmount < minShortErc) {
112:                 if (shortOrder.shortRecordId != p.shortId || shortOrder.addr != p.shorter) revert Errors.InvalidShortOrder();
113:                 LibOrders.cancelShort(asset, p.shortOrderId);
114:             }

A malicious user could exploit this to place orders that remain PartialFill without any associated short, making them unliquidatable.

Proof of Concept

The following test demonstrates how a match between a receiver bid and a sender short results in the sender's ShortRecord being left in PartialFill status without an associated short (shortId=0). When attempting to liquidate this ShortRecord, it will not be possible due to the absence of an associated short.

    // File: test/Redemption.t.sol:RedemptionTest
    function test_liquidationAndRedemptionAreRevertedWhenPartialFill() public {
        //
        // 1. Match order between `receiver` and `sender`. The bidder amount is a little less than the shorter amount.
        fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT + 11, receiver);
        fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT + 33, sender);
        //
        // 2. The sender's shortRecord is `partialFill`. The problem is that there are no `sender's shorts` because the left
        // amount was less than `minAskEth`
        STypes.ShortRecord memory shortRecordFilled = getShortRecord(sender, C.SHORT_STARTING_ID);
        assertTrue(shortRecordFilled.status == SR.PartialFill);
        assertEq(shortRecordFilled.ercDebt, DEFAULT_AMOUNT + 11); // ercDebt filled by receiver bidder
        STypes.Order[] memory shortsUnfilled = getShorts(); // there are no shorts in the system
        assertEq(shortsUnfilled.length, 0);
        //
        // 3. Price drops and the shorter can be liquidated but the liquidation will be reverted by `InvalidShortOrder`
        _setETH(1000 ether);
        uint88 amount = shortRecordFilled.ercDebt + shortRecordFilled.ercDebt.mulU88(diamond.getAssetStruct(asset).ercDebtRate - shortRecordFilled.ercDebtRate);
        fundLimitAskOpt(DEFAULT_PRICE, amount, receiver);
        vm.expectRevert(Errors.InvalidShortOrder.selector);
        vm.prank(extra);
        diamond.liquidate(asset, sender, C.SHORT_STARTING_ID, shortHintArrayStorage, 0);
        //
        // 4. Also the Redemption will be reverted by `InvalidShortOrder`
        address redeemer = address(1212);
        depositUsd(redeemer, DEF_REDEMPTION_AMOUNT);
        depositEth(redeemer, DEFAULT_AMOUNT);
        MTypes.ProposalInput[] memory proposalInputs = new MTypes.ProposalInput[](1);
        proposalInputs[0] = MTypes.ProposalInput({shorter: sender, shortId: C.SHORT_STARTING_ID, shortOrderId: 0});
        vm.expectRevert(Errors.InvalidShortOrder.selector);
        vm.prank(redeemer);
        diamond.proposeRedemption(asset, proposalInputs, DEFAULT_AMOUNT * 2, MAX_REDEMPTION_FEE);
    }

Tools used

Manual review

Recommended Mitigation Steps

When a match occurs in LibOrders::sellMatchAlgo and the short order is left in PartialFill status, it's necessary to change it to FullyFilled status if the remaining amount is not sufficient to add a sell order for the user using the LibOrders::addSellOrder function either in LibOrders#L594 or LibOrders#L621.

There's no need to leave a ShortRecord in PartialFill status if NO sellOrder will be added to the order book for the user. Technically, the order should be marked as FullyFilled since the remaining amount did not meet the minAskEth requirement. The same in BidOrdersFacet::bidMatchAlgo.

Assessed type

Context

code-423n4 / 2024-03-dittoeth-findings

Liquidation or Redemption of `ShortRecords` in `PartialFill` status without associated `Short` will be reverted #168