Inconsistent orderbook state, potential loss of funds, incorrect token balances.

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibOrders.sol#L853-L866

Vulnerability details

Description

cancelBid, cancelAsk, and cancelShort functions are responsible for canceling orders from their respective ordersbooks. They take the asset and id of the order as inputs and directly remove the order from the orderbook without checking if it is currently being matched.

cancelBid

    // Helper Functions for cancelling orders
    function cancelBid(address asset, uint16 id) internal {
        AppStorage storage s = appStorage();
        STypes.Order storage bid = s.bids[asset][id];

        O orderType = bid.orderType;
        if (orderType == O.Cancelled || orderType == O.Matched) revert Errors.NotActiveOrder();

        uint256 vault = s.asset[asset].vault;
        uint88 eth = bid.ercAmount.mulU88(bid.price);
        s.vaultUser[vault][bid.addr].ethEscrowed += eth;

        cancelOrder(s.bids, asset, id);
    }

This retrieves the bid order, checks if it's not already canceled or matched, updates the user's escrowed balance, and then calls the internal cancelOrder function to remove the order from the orderbook.

Vulnerability Details

When an order is in the process of being matched by the bidMatchAlgo or sellMatchAlgo functions, and simultaneously, the cancelBid, cancelAsk, or cancelShort function is called for the same order.

The issue lies in the fact that the cancel functions do not check whether the order is currently being matched. They directly remove the order from the orderbook without any locks or checks.

Due to the lack of checks and locks, if the cancel function is called while the order is being matched, it can lead to inconsistencies in the orderbook state. The order may be partially filled and then cancelled, resulting in an incorrect remaining amount or a discrepancy between the user's balances and the order status.

For example, consider the following scenario:

1. A bid order is being matched in the bidMatchAlgo function.
2. While the matching is in progress, the cancelBid function is called for the same bid order.
3. The cancelBid function removes the order from the orderbook, even though it is still being matched.

This can lead to a situation where the order is partially filled by the matching process, but then gets cancelled, leaving the remaining amount in an inconsistent state.

Impact

• The orderbook may contain orders with incorrect remaining amounts or orders that should have been fully filled but are marked as cancelled.
• Users may have incorrect balances if their orders are partially filled and then cancelled, leading to discrepancies between their actual and expected balances.

Proof of Concept

Scenario:

1. User A creates a bid order for 100 tokens at a price of 1 ETH each.
2. User B creates an ask order for 50 tokens at a price of 1 ETH each.
3. The bidMatchAlgo function starts processing User A's bid order and matches it with User B's ask order.
4. While the matching process is in progress, User A calls the cancelBid function to cancel their bid order.

Steps:

1. User A creates a bid order

   function createBid(address asset, uint80 price, uint88 ercAmount) external {
      // ... other code ...
      uint16 bidId = 1;
      s.bids[asset][bidId] = STypes.Order({
          addr: msg.sender,
          price: 1 ether,
          ercAmount: 100,
          id: bidId,
          orderType: O.LimitBid,
          creationTime: block.timestamp
      });
      // ... other code ...
   }

2. User B creates an ask order

   function createAsk(address asset, uint80 price, uint88 ercAmount) external {
      // ... other code ...
      uint16 askId = 1;
      s.asks[asset][askId] = STypes.Order({
          addr: msg.sender,
          price: 1 ether,
          ercAmount: 50,
          id: askId,
          orderType: O.LimitAsk,
          creationTime: block.timestamp
      });
      // ... other code ...
   }

3. The bidMatchAlgo function starts matching User A's bid order with User B's ask order

   function bidMatchAlgo(address asset, STypes.Order memory incomingBid) external {
      // ... other code ...
      STypes.Order memory lowestSell = s.asks[asset][1];
      if (incomingBid.price >= lowestSell.price) {
          // Start the matching process
          matchHighestBid(incomingBid, lowestSell, asset);
          // ... other code ...
      }
      // ... other code ...
   }

4. While the matching process is in progress, User A calls the cancelBid function

   function cancelBid(address asset, uint16 id) external {
      STypes.Order storage bid = s.bids[asset][id];
      // ... other code ...
      cancelOrder(s.bids, asset, id);
   }

We can see

• The cancelBid function removes User A's bid order from the orderbook, even though it is currently being matched with User B's ask order.
• The matching process continues, and User B's ask order gets partially filled with the available amount from User A's bid order.
• However, since User A's bid order is cancelled during the matching, the remaining amount of their order is not properly accounted for.
• This results in an inconsistent state where User B's ask order is partially filled, but User A's bid order is cancelled, leaving the remaining amount in limbo.
• The orderbook state becomes inconsistent, with the potential for incorrect token balances and unfilled orders.

Additional information to consider

• If the remaining amount of User A's bid order is not properly credited back to their account, they may lose the funds associated with the unfilled portion of their order.
• The inconsistency in the orderbook state can lead to incorrect token balances for users, as the partially filled orders may not be accurately reflected in their accounts.
• Malicious users could potentially exploit this vulnerability by strategically canceling orders during the matching process to gain an unfair advantage over other users.

Recommended Mitigation Steps

Add a 'filling' status to orders:

• Introduce a new status flag for orders that are currently being matched.
• Set this flag when an order enters the matching process and unset it when the matching is complete.
• Modify the cancel functions to check this flag and prevent cancellation of orders that are mid-match.

function cancelBid(address asset, uint16 id) internal {
    AppStorage storage s = appStorage();
    STypes.Order storage bid = s.bids[asset][id];

+   if (bid.status == O.Filling) revert Errors.OrderIsBeingMatched();

    // Rest of the cancellation logic...
}

• Add a reentrancy guard to the bidMatchAlgo and sellMatchAlgo functions to prevent any external calls (including to the cancel functions) while a match is in progress.

• In the matching functions, update the order statuses to 'matched' before making any external calls or interactions.

  • This ensures that the order state is always consistent, even if an external call reverts or tries to cancel the order.

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]

Hypothetical example devoid of a concrete flow of interacting with the coded contracts.

[c4-judge]

hansfriese marked the issue as unsatisfactory: Insufficient proof