DittoETH

Summary

The OrdersFacet::cancelOrderFarFromOracle() does not return the unfilled ETH/ERC to owners of the canceled orders after canceling them.

As a result, the owners of the canceled orders will be unable to retrieve their unfilled ETH/ERC; they will be locked forever.

Vulnerability Details

When users place the Bid, Ask, or Short orders on the market, their escrowed ETH/ERC will be deposited into the orders through the addBid(), addAsk(), or addShort(), respectively. The addBid() will take the user's escrowed ETH into the created Bid order. The addAsk() will take the escrowed ERC into the created Ask order. Whereas the addShort() will take the escrowed ETH into the generated Short order.

When the order is matched, the deposited ETH/ERC can be partially filled or fully filled. If the order was partially filled or has never been matched, there would be some unfilled ETH/ERC locked in the order.

The OrdersFacet::cancelOrderFarFromOracle() is a public function to handle when the system's orderId has hit the limit (to deter attackers). The function can cancel Bid/Ask/Short orders that are placed far from the oracle price up to 1000 orders if the function is called by the DAO account. If the function is called by a non-DAO account, only the last order will be canceled.

However, I noticed that the cancelOrderFarFromOracle() does not return the unfilled ETH/ERC to the owners of the canceled orders after canceling them.

During the process of canceling an order, the cancelOrderFarFromOracle() will execute the LibOrders::cancelManyOrders() (in L143for Bid orders, L148 for Ask orders, or L153 for Short orders) or the LibOrders::cancelOrder() (in L163 for single Bid order, L168 for single Ask order, or L173 for single Short order).

Eventually, the cancelOrder() will take control of handling the reuse of the canceled order. Specifically, the cancelOrder() will set the canceled order's orderType property to O.Cancelled. Consequently, the canceled order will become inactive and will be re-used (replaced) by the upcoming order.

Since the orderType property will become O.Cancelled, the owner of the canceled order will be unable to retrieve their unfilled ETH/ERC via the cancelBid(), cancelAsk(), cancelShort(), or any other functions. In other words, the owner will lose their ETH/ERC forever.

• s.bids.cancelManyOrders(): https://github.com/Cyfrin/2023-09-ditto/blob/a93b4276420a092913f43169a353a6198d3c21b9/contracts/facets/OrdersFacet.sol#L143

• s.asks.cancelManyOrders(): https://github.com/Cyfrin/2023-09-ditto/blob/a93b4276420a092913f43169a353a6198d3c21b9/contracts/facets/OrdersFacet.sol#L148

• s.shorts.cancelManyOrders(): https://github.com/Cyfrin/2023-09-ditto/blob/a93b4276420a092913f43169a353a6198d3c21b9/contracts/facets/OrdersFacet.sol#L153

• s.bids.cancelOrder(): https://github.com/Cyfrin/2023-09-ditto/blob/a93b4276420a092913f43169a353a6198d3c21b9/contracts/facets/OrdersFacet.sol#L163

• s.asks.cancelOrder(): https://github.com/Cyfrin/2023-09-ditto/blob/a93b4276420a092913f43169a353a6198d3c21b9/contracts/facets/OrdersFacet.sol#L168

• s.shorts.cancelOrder(): https://github.com/Cyfrin/2023-09-ditto/blob/a93b4276420a092913f43169a353a6198d3c21b9/contracts/facets/OrdersFacet.sol#L173

Impact

The owners of the canceled orders will be unable to retrieve their unfilled ETH/ERC; they will be locked forever.

Tools Used

Manual Review

Recommendations

Return the unfilled ETH/ERC to the canceled orders' owners in the cancelOrderFarFromOracle(). The following presents the code snippets for returning the unfilled ETH/ERC to their owner.

• Return the unfilled ETH to the canceled Bid order's owner

• Return the unfilled ERC to the canceled Ask order's owner

• Return the unfilled ETH to the canceled Short order's owner