DittoETH

Summary

OrdersFacet.sol:cancelOrderFarFromOracle() does not check if the order to be cancelled is already cancelled or matched, making it possible to cancel an order multiple times and causing wrong assignations in the orders linked list.

Vulnerability Details

cancelOrderFarFromOracle function can be called by anyone to cancel the last order of a side (bid, ask, or short). This function checks that the order to be cancelled is the last in the linked list by asserting that nextId == Constants.TAIL. However, this check is not enough, as nextId is not reset when an order is cancelled. This means that when the last order is cancelled, it will still have nextId == Constants.TAIL.

Let's see this with an example. Suppose we have the following linked list of orders:

If we cancel the last order, we will have:

Although nextId for cancelled orders is not relevant, it still points to the value it had before being cancelled. So, in this case, the order with id 65101 will have nextId == TAIL. This means that if we call cancelOrderFarFromOracle with lastOrderId = 65101, the function will not revert, as nextId == TAIL. This will cause several issues in the linked list:

As the prevId of the cancelled order is set to HEAD.prevId, in this case the cancelled order prevId will point to itself, and all the other cancelled orders are unlinked from the linked list.

Now, let's see what happens if we create a new order:

The new order is inserted in the linked list reusing the id of the last cancelled order. However, as prevId of the cancelled order points to itself, HEAD.prevId will also point to 65101. This means that new orders will reuse the ids of active orders.

Impact

This vulnerability can lead to multiple issues once the orderId of an asset reaches 65000, which can also be reached artificially by a bad actor.

• Active orders can be overwritten by new orders.

• Cancelled orders can be left out of the linked list, not being available for future use, which in turn can lead to not having available ids for new orders.

Given that the linked list can have more than one order pointing to the same id, the protocol is effectively broken as the predicted behavior of the linked list is not met anymore.

Proof of Concept

Add the following code snippet into test/CancelOrder.t.sol and run forge test --mt testCancelOrderAlreadyCancelled.

Tools Used

Manual inspection.

Recommendations

An alternative fix would be performing the check in the cancelOrder function of LibOrders.sol: