DittoETH
Ditto
DeFiFoundryOracle
55,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

Attacker can trap legitimate traders' asset or collateral by cancelling their order

flacko

Summary

Once an order book grows close to it's maximum capacity (65000+ order ids minted, or asset.orderId >= 65000), it enables the arbitrary cancelling of new orders that have just been added to the order book.

Vulnerability Details

Let's take a look at the mechanism the protocol provides for deterring attackers, ignoring the functionality that only the DAO has access to and focusing on the functionality that any one can call:

// contracts/facets/OrdersFacet.sol
​
function cancelOrderFarFromOracle(
address asset,
O orderType,
uint16 lastOrderId,
uint16 numOrdersToCancel
) external onlyValidAsset(asset) nonReentrant {
if (s.asset[asset].orderId < 65000) {
revert Errors.OrderIdCountTooLow();
}
​
if (numOrdersToCancel > 1000) {
revert Errors.CannotCancelMoreThan1000Orders();
}
​
if (msg.sender == LibDiamond.diamondStorage().contractOwner) {
// ... allow DAO to cancel orders in bulk
} else {
//@dev if address is not DAO, you can only cancel last order of a side
if (
orderType == O.LimitBid
&& s.bids[asset][lastOrderId].nextId == Constants.TAIL
) {
s.bids.cancelOrder(asset, lastOrderId);
} else if (
orderType == O.LimitAsk
&& s.asks[asset][lastOrderId].nextId == Constants.TAIL
) {
s.asks.cancelOrder(asset, lastOrderId);
} else if (
orderType == O.LimitShort
&& s.shorts[asset][lastOrderId].nextId == Constants.TAIL
) {
s.shorts.cancelOrder(asset, lastOrderId);
} else {
revert Errors.NotLastOrder();
}
}

First, s.asset[asset].orderId functions as a counter for the next new order's id. When a new order is added to an order book, the LibOrders#addOrder() function is called:

// Note: comments omitted for a more concise presentation
function addOrder(
mapping(address => mapping(uint16 => STypes.Order)) storage orders,
address asset,
STypes.Order memory incomingOrder,
uint16 hintId
) private {
AppStorage storage s = appStorage();
uint16 prevId = findPrevOfIncomingId(orders, asset, incomingOrder, hintId);
uint16 nextId = orders[asset][prevId].nextId;
incomingOrder.prevId = prevId;
incomingOrder.nextId = nextId;
uint16 id = incomingOrder.id;
uint16 canceledID = orders[asset][Constants.HEAD].prevId;
if (canceledID != Constants.HEAD) {
incomingOrder.prevOrderType = orders[asset][canceledID].orderType;
uint16 prevCanceledID = orders[asset][canceledID].prevId;
if (prevCanceledID != Constants.HEAD) {
orders[asset][Constants.HEAD].prevId = prevCanceledID;
} else {
orders[asset][Constants.HEAD].prevId = Constants.HEAD;
}
id = incomingOrder.id = canceledID;
} else {
s.asset[asset].orderId += 1; // @audit If there are no cancelled orders, issue a new order ID
}
orders[asset][id] = incomingOrder;
if (nextId != Constants.TAIL) {
orders[asset][nextId].prevId = incomingOrder.id;
}
​
orders[asset][prevId].nextId = incomingOrder.id;
}

If there are no cancelled orders whose order id can be reused, a new order id is issued. Given enough time and protocol popularity, this order id can surely grow up to the 65000 mark. Also the fact that both bids, shorts and asks rely on the very same counter makes this easier.

Back to the cancelOrderFarFromOracle() method:
Had that asset.orderId counter reached the value of 65000 once, it becomes a child's play to cancel legitimate trader's orders right after they are placed (they need to be the last one in the linked list).

Because of how cancel order works, it simply links the ids of adjacent orders of the one being cancelled:

function cancelOrder(
mapping(address => mapping(uint16 => STypes.Order)) storage orders,
address asset,
uint16 id
) internal {
uint16 prevHEAD = orders[asset][Constants.HEAD].prevId;
orders[asset][orders[asset][id].nextId].prevId = orders[asset][id].prevId;
orders[asset][orders[asset][id].prevId].nextId = orders[asset][id].nextId;
emit Events.CancelOrder(asset, id, orders[asset][id].orderType);
_reuseOrderIds(orders, asset, id, prevHEAD, O.Cancelled);
}

And moves it left of HEAD in the orders linked list, and changes it's status to O.Cancelled.

function _reuseOrderIds(
mapping(address => mapping(uint16 => STypes.Order)) storage orders,
address asset,
uint16 id,
uint16 prevHEAD,
O cancelledOrMatched
) private {
orders[asset][id].orderType = cancelledOrMatched; // @audit We mark the order as cancelled right here
orders[asset][Constants.HEAD].prevId = id;
if (prevHEAD != Constants.HEAD) {
orders[asset][id].prevId = prevHEAD;
} else {
orders[asset][id].prevId = Constants.HEAD;
}
}

This becomes an issue when legitimate traders want to add a LIMIT order (be it a bid, an ask or god forbid a short order).
In the case of a limit bid order, the ETH needed for executing the exchange is taken from the trader:

// contracts/libraries/LibOrders.sol#addBid()
106: uint88 eth = order.ercAmount.mulU88(order.price);
107: s.vaultUser[vault][order.addr].ethEscrowed -= eth;

In the case of a limit ask order, the ERC being sold is taken from the trader in the same fashion:

// contracts/libraries/LibOrders.sol#addAsk()
129: s.vaultUser[vault][order.addr].ercEscrowed -= order.ercAmount;

And in the case of a limit short order, the collateral for the position is deducted from the trader's ETH escrow balance:

// contracts/libraries/LibOrders.sol#addAsk()
158: uint88 eth = order.ercAmount.mulU88(order.price).mulU88(
159: LibOrders.convertCR(order.initialMargin)
160: );
161: s.vaultUser[vault][order.addr].ethEscrowed -= eth;

Once an order is added to the order book, an attacker can cancel it and thus trap any amount of asset or ETH that were posted by a trader, rendering them unrecoverable by the trader.

Impact

Normally, when an order is cancelled using the functions that the OrdersFacet provides, the posted asset or ETH is credited back to the trader who owns the order before marking their order as closed. In cancelOrderFarFromOracle() however, as a form of punishment (I assume), the protocol does not refund the posted funds back to the order owner.

The trader can no longer use the order cancel functionality that the OrdersFacet provides because any of the three functions (cancelBid, cancelAsk, cancelShort) require the order that's being cancelled to not be Cancelled.

Tools Used

Manual review

Recommendations

Allow only DAO to cancel orders or refactor the part of the functionality that allows arbitrary users to cancel the last orders so that posted funds are returned to the order owner but the caller receives a fee that covers the transaction cost so they have an incentive to do that part of the housekeeping for the orderbook.

Updates

Lead Judging Commences

0xnevi Lead Judge almost 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-436

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.