Summary
DittoEth has a mechanism to handle the possibility that orderId has hit limit, it is used to deter attackers, which is good.
This mechanism is the function in OrdersFacets.sol#cancelOrderFarFromOracle(), and the only check for this function is that orderId has to be bigger than 65000.
So if an attacker creates a lot of orders or during big usage of the protocol lot orders are created and orderId becomes bigger than 65000 any order with orderId > 65000 at the TAIL can be cancelled (attackers and normal users orders), the problem is this mechanisms does not include reimbursements of money to place these orders.
If an attacker creates 65000 orders and then a normal user create the 65001 , attacker can immediately cancel it's order and normal user money is losen.
Vulnerability Details
Here is the code of OrdersFacet.sol#cancelOrderFarFromOracle() :
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) {
if (
orderType == O.LimitBid
&& s.bids[asset][lastOrderId].nextId == Constants.TAIL
) {
s.bids.cancelManyOrders(asset, lastOrderId, numOrdersToCancel);
} else if (
orderType == O.LimitAsk
&& s.asks[asset][lastOrderId].nextId == Constants.TAIL
) {
s.asks.cancelManyOrders(asset, lastOrderId, numOrdersToCancel);
} else if (
orderType == O.LimitShort
&& s.shorts[asset][lastOrderId].nextId == Constants.TAIL
) {
s.shorts.cancelManyOrders(asset, lastOrderId, numOrdersToCancel);
} else {
revert Errors.NotLastOrder();
}
} else {
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();
}
}
}
and here is the code of libOrders.sol#cancelOrder() :
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);
}
function _reuseOrderIds(
mapping(address => mapping(uint16 => STypes.Order)) storage orders,
address asset,
uint16 id,
uint16 prevHEAD,
O cancelledOrMatched
) private {
orders[asset][id].orderType = cancelledOrMatched;
orders[asset][Constants.HEAD].prevId = id;
if (prevHEAD != Constants.HEAD) {
orders[asset][id].prevId = prevHEAD;
} else {
orders[asset][id].prevId = Constants.HEAD;
}
}
As you can see nowhere the order owner of the canceled order is refunded but orders[asset][id].orderType is set to O.Cancelled which prevent any user from getting back it's money.
The process is different when a user intentionally cancel it's own order , here is an example of an Ask cancellation :
function cancelAsk(address asset, uint16 id)
external
onlyValidAsset(asset)
nonReentrant
{
STypes.Order storage ask = s.asks[asset][id];
if (msg.sender != ask.addr) revert Errors.NotOwner();
O orderType = ask.orderType;
if (orderType == O.Cancelled || orderType == O.Matched) {
revert Errors.NotActiveOrder();
}
s.assetUser[asset][msg.sender].ercEscrowed += ask.ercAmount;
s.asks.cancelOrder(asset, id);
}
As you can see in this case user is refunded
Impact
As there is no possibility to see if an orderId > 65000 correspond to an attacker or a normal user, OrdersFacets.sol#cancelOrderFarFromOracle() can be either used to deter an attacker but also to punish a normal user , the result is the same : cancelled order owner lose it's money.
Tools Used
VSCode
Recommendations
Refund cancelled order owner the same way an order owner is refunded when he/she intentionally cancel it's own order in the cancelOrderFarFromOracle() function.
I think the protocol doesn't need this kind of function as user threshold for an order is 240 but if you want to deter attacker, lower this threshold or increase the minEthbid/ask/short threshold.
