Insufficient check on dusty bid creation can DOS market system

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/BidOrdersFacet.sol#L77-L117

Vulnerability details

Summary

User can get around checks to create many miniscule bids and which DOS's the Order Book's matching system.

Vulnerability Details

The protocol has a number of measures to try and reduce gas intensive matching between bids and asks. One such measure is a check in _createBid() to prevent the addition of dust bids onto the Order Book and avoid excessive looping in the matching algo.

As can be seen in the code snippet below, the user provided params uint80 price and uint88 ercAmount are multiplied and then compared against LibAsset.minBidEth(asset). If the product of price and ercAmount is below minimum bid, the function reverts. However, user can get around this check to add dust amounts by setting a combination of the maximum possible price with the lowest possible ercAmount.

    function _createBid(
        address sender,
        address asset,
        uint80 price,
        uint88 ercAmount,
        bool isMarketOrder,
        MTypes.OrderHint[] memory orderHintArray,
        uint16[] memory shortHintArray
    ) private returns (uint88 ethFilled, uint88 ercAmountLeft) {
>>>     uint256 eth = ercAmount.mul(price);
>>>     if (eth < LibAsset.minBidEth(asset)) revert Errors.OrderUnderMinimumSize();

        // MORE  CODE
    }

It is somewhat restricted by the uint types of the input params so the largest price will be 1e24 and therefore lowest ercAmount will be 1e11 (equivalent to $0.35 today).

POC

The test function below shows how the check can be gotten around to add 100 dust bids to the order book. Add the function below to BidOrders.t.sol and run.

    function test_FillOrderBookWithSmallBids() public {
        uint80 highPrice = 1e24;
        uint88 lowAmount = 100000000000; // 1e11 which is $0.00035 @ $3500 / ETH
        uint256 numOrders = 100;

        // deal Eth to user & deposit into protocol
        depositEth(receiver, 200 ether);
        assertEq(getBids().length, 0);
        for (uint256 i = 0; i < numOrders; i++) {
            fundLimitBidOpt(highPrice, lowAmount, receiver);
        }
        console.log("getBids().length: ", getBids().length);
        assertEq(getBids().length, numOrders);
        assertEq(getBids()[0].ercAmount, lowAmount);

    }

Impact

A malicious user can add many hundreds of Dust bids to the Order Book at extremely high price. This can completely DOS the Order Book and hence the Market System because any Asks that get created would need to loop through all of these high priced dust Bids before getting to any genuine bids; expending the Block Gas Limit before matching occurs.

The protocol does have a way to counter spam orders see here but it is restrictive and not intended to do a clean up of this size.

Tools Used

Manual Review Foundry Testing

Recommendations

Add a new minimum ercAmount check to _createBid()

Assessed type

DoS

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

Asset.minBidEth is 0.1 ETH currently but may change before launch. Additionally, the returned value of ercAmount.mul(price) uint256. The alleging statement "the largest price will be 1e24 and therefore lowest ercAmount will be 1e11 (equivalent to $0.35 today)" doesn't make sense.

[c4-judge]

hansfriese marked the issue as unsatisfactory: Insufficient proof

[Qormatic]

Hi @hansfriese,

Appeal

Given it's capacity to render completely useless the core functionality of the protocol, i.e. the market system, the issue outlined in the report is of a critical severity. Furthermore, given it's cheapness and simplicity to execute, I would assess the probability of it's being exploited as high. I therefore ask you to reconsider it's inclusion in the report so that it can be communicated to the sponsor; please see my justification below.

Justification

I don't understand why this report was labelled Insufficient proof; given that the issue identified is the ability of bidders to create 'dusty' Bids.

Here it is explained how to circumvent the check, which is supposed to prevent the creation of bids with dusty ercAmounts:

As can be seen in the code snippet below, the user provided params uint80 price and uint88 ercAmount are multiplied and then compared against LibAsset.minBidEth(asset). If the product of price and ercAmount is below minimum bid, the function reverts. However, user can get around this check to add dust amounts by setting a combination of the maximum possible price with the lowest possible ercAmount.

In the next part it notes that the minimum ercAmount a bid can be created with will be 1e11 (protocol setting for minBidEth is 1e17):

It is somewhat restricted by the uint types of the input params so the largest price will be 1e24 and therefore lowest ercAmount will be 1e11

Lookout's comment below shows they have misunderstood in the description that minBidEth can be circumvented and bids can be created with 1e11:

Asset.minBidEth is 0.1 ETH currently but may change before launch.

I'm not sure why the lookout also comments that largest price as 1e24 and lowest ercAmount will be 1e11 make "no sense":

The alleging statement "the largest price will be 1e24 and therefore lowest ercAmount will be 1e11 (equivalent to $0.35 today)" doesn't make sense.

Simply they are values demonstrating a max price * min ercAmount combination which can be used to pass the check; which accounts for price's uint80 restriction on it's uppermost allowed value and by consequence means the lowest ercAmount bidder will be able to put up will be 1e11. Note: there is a typo and $0.35 should actually be $0.00035 as it is correctly noted in the POC

I also added a POC to demonstrate that the check could be gotten around with those values noted above. Therefore I believe that I adequately proved the issue of dusty bids being created and provided in the Impact Section the attack path as a consequence:

A malicious user can add many hundreds of Dust bids to the Order Book at extremely high price. This can completely DOS the Order Book and hence the Market System because any Asks that get created would need to loop through all of these high priced dust Bids before getting to any genuine bids; expending the Block Gas Limit before matching occurs.

I believe the description of the attack path is sufficient given the signposting of this issue throughout the code and documentation. You can expand the section below to see references from the docs which describe the need to prevent dusty bids due to the gas intensive matching process:

References to Dust in Docs

From `Attack ideas` in the audit `Readme.md`: > Dust amounts: want to prevent small orders on the orderbook to prevent skyrocketing gas costs for large orders that match with multiple limit orders On the purpose of the reported check from `Orderbook` page in docs: > Note: Orders have a minimum ETH amount (price \* amount): if (eth < minBidEth (or minAskEth)) revert Errors.OrderUnderMinimumSize(). This prevents small orders from clogging up the Orderbook. See also the page in docs about Orderbook Dust [here](https://dittoeth.com/technical/misc#orderbook-dust).

Expand section below for a more detailed explanation about the Orderbook and the issue of dusty bids:

Impact

In the Orderbook, the highest price `Bids` are placed at the top of the Bid list, while the lowest price `Asks` are placed at the top of the Ask list. When a new `Bid` or `Ask` order is created, `Ditto` immediately tries to match it to a suitable order which is already on the Orderbook. For an `Ask` this means looping through the list of `Bids` starting from the highest price and working down through all the bids with a higher or equal price to itself; continuing to match until all the `Ask's` requested `ercAmount` has been fulfilled by `Bids`. The problem with this is that if there are many 100s of high priced `dust Bids` at the top of the `Bid` list, any new `Ask` will need to loop through all of those high priced bids, only fulfilling dust amounts. This essentially means a complete DOS of the matching algo as all `Asks` would exhaust the block gas limit from looping long before they are fulfilled. Even a very small `Ask`, say with `ercAmount = $1` would require `~2857 matches / loops` to get fulfilled by dust `Bid` orders.

[hansfriese]

I agree ercAmount might be small with the highest price. But the user should lock eth amount to create a bid. It means he should lock 0.1 ETH for each order which is not dust for him.

That's why I still believe it's invalid.