Inadequate Handling of Market and Limit Orders in the _createBid Function: Analysis and Mitigation

[c4-bot-7]

Lines of code

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

Vulnerability details

Impact

The _createBid function in the provided code does not distinguish between market orders and limit orders properly. This oversight can lead to significant issues when users attempt to

1. exit a short using create force or
2. when executing regular trades with either a limit or market order.

The current implementation does not handle the different execution behaviours of the market and limits orders correctly, potentially causing unexpected behaviour and financial loss for users(e.g i want to buy eth at 4000 limit but eventually ended up getting it at 4000 plus or more or i want to buy at market order, enabled makret order yet i was listed as a limit order). This report is Based on the standard used by all big crypto trading platforms (Binance,kucoin,OKX,including forex and other trading platforms) used for their execution, they do not take in price as an input for market orders, market orders should be executed based on the present market price/ask price regardless of how much it is.

Proof of Concept

The inaccurate code snippet:

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();

    STypes.Asset storage Asset = s.asset[asset];
    if (s.vaultUser[Asset.vault][sender].ethEscrowed < eth) revert Errors.InsufficientETHEscrowed();

    STypes.Order memory incomingBid;
    incomingBid.addr = sender;
    incomingBid.price = price;
    incomingBid.ercAmount = ercAmount;
    incomingBid.id = Asset.orderIdCounter;
    incomingBid.orderType = isMarketOrder ? O.MarketBid : O.LimitBid;
    incomingBid.creationTime = LibOrders.getOffsetTime();

    MTypes.BidMatchAlgo memory b;
    b.askId = s.asks[asset][C.HEAD].nextId;
    // @dev setting initial shortId to match "backwards" (See _shortDirectionHandler() below)
    b.shortHintId = b.shortId = Asset.startingShortId;

    STypes.Order memory lowestSell = _getLowestSell(asset, b);
    if (incomingBid.price >= lowestSell.price && (lowestSell.orderType == O.LimitAsk || lowestSell.orderType == O.LimitShort)) {
                     // @dev if match and match price is gt .5% to saved oracle in either direction, update startingShortId
        LibOrders.updateOracleAndStartingShortViaThreshold(asset, LibOracle.getPrice(asset), incomingBid, shortHintArray);
        b.shortHintId = b.shortId = Asset.startingShortId;
        b.oraclePrice = LibOracle.getPrice(asset);
        return bidMatchAlgo(asset, incomingBid, orderHintArray, b);
    } else {
        // @dev no match, add to market if limit order
        LibOrders.addBid(asset, incomingBid, orderHintArray);
        return (0, ercAmount);
    }
}

The main difference between a market order and a limit order is that market orders trigger the immediate purchase or sale of a stock at its current market value, whereas limit orders allow you to delay transactions until the stock meets a specified price.

Based on the formal code the difference between the two are not spelt, and they can act like one another.

Differentiating between market orders and limit orders is crucial in a trading system to ensure proper execution and avoid potential issues. Here are some issues that can arise if a market order is not differentiated from a limit order:

1. Execution Price:

   • Market Order: Executes immediately at the current market price.
   • Limit Order: Executes at a specific price or better. If the market moves away from the limit price, the order may not be executed.

   If a market order is not properly differentiated from a limit order, the wrong execution price could be used, leading to incorrect trade execution.

2. Order Book Management:

   • Market Order: Removes liquidity from the order book immediately.
   • Limit Order: Adds liquidity to the order book until it is filled.

   Treating all orders as the same type can lead to incorrect management of the order book, affecting the market's liquidity and depth.

3. Risk Management:

   • Market Order: Carries immediate execution risk.
   • Limit Order: Carries potential market risk if the order is not filled due to price movement.

   Without proper differentiation, risk management systems (take profit/stop limits) may not function correctly, leading to increased risk exposure.

4. Order Matching and Priority:

   • Market Order: Generally has higher priority and is executed before other order types.
   • Limit Order: Executed based on the specified price and time priority.

   Not differentiating between market and limit orders can disrupt the order matching process and priority handling.

5. User Experience and Expectations: Traders expect different behaviors and outcomes from market and limit orders. Failing to differentiate between the two can confuse traders and lead to dissatisfaction with the trading platform.

6. Regulatory Compliance: Financial markets and trading platforms are subject to regulations that may require specific handling and disclosure of market and limit orders. Failing to differentiate between the two could result in non-compliance with regulatory requirements.

Thus, confirming the function createbid to the exact function handle of this order is necessary, this handling will ensure that the right price mechanism that will be used to differentiate the order types are available for the function, so a market order won't act like a limit order and vice-versa.

Note that if the appropriate handling is done, then the later condition

if (incomingBid.price >= lowestSell.price && (lowestSell.orderType == O.LimitAsk || lowestSell.orderType == O.LimitShort))

will be met appropriately to send the order to the bidMatchAlgo for market orders

and the else will create a limit when desired for limit orders because the later conditions work based on price and not on market order type.

Tools Used

Manual code analysis.

Recommended Mitigation Steps

To handle the difference between market and limit orders properly,

the function should

1. first fetch the Oracle price.

2. If the incoming order is a market order, set the price to the Oracle price.

3. If it is a limit order, check if the price is greater than the orcale price.

4. If it is, set the price to oraclePrice - 1.(this should be ok since price is in uint80 (positive)). Here's the revised code:

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) {
    uint80 oraclePrice = LibOracle.getPrice(asset);

    if (isMarketOrder==O.MarketBid) {
        if (price < oraclePrice) {
           price = oraclePrice;
        }
    } else {
        if (price > oraclePrice) {
            price = oraclePrice - 1;
        }
    }

    uint256 eth = ercAmount.mul(price);
    if (eth < LibAsset.minBidEth(asset)) revert Errors.OrderUnderMinimumSize();

      STypes.Order memory incoming bid;
    incomingBid.addr = sender;
    incomingBid.ercAmount = ercAmount;
    incomingBid.id = Asset.orderIdCounter;
    incomingBid.orderType = isMarketOrder ? O.MarketBid : O.LimitBid;
    incomingBid.creation time = LibOrders.getOffsetTime();

    MTypes.BidMatchAlgo memory b;
    b.askId = s.asks[asset][C.HEAD].nextId;
    b.shortHintId = b.shortId = Asset.startingShortId;

    STypes.Order memory lowestSell = _getLowestSell(asset, b);
    if (incomingBid.price >= lowestSell.price && (lowestSell.orderType == O.LimitAsk || lowestSell.orderType == O.LimitShort)) {
        LibOrders.updateOracleAndStartingShortViaThreshold(asset, oraclePrice, incomingBid, shortHintArray);
        b.shortHintId = b.shortId = Asset.startingShortId;
        b.oraclePrice = oraclePrice;
        return bidMatchAlgo(asset, incomingBid, orderHintArray, b);
    } else {
        LibOrders.addBid(asset, incomingBid, order in array);
        return (0, ercAmount);
    }
}

This revision ensures that the function handles market and limit orders correctly, preventing unexpected behaviour and potential financial loss for users.

Assessed type

Error

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[raymondfam]

Unstructured elaboration.

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-judge]

hansfriese marked the issue as unsatisfactory: Insufficient proof