moneyversed - Risk of Unfair Order Execution Price in `_validateOrdersAndDetermineFillPrice` Function

[sherlock-admin]

moneyversed

medium

Risk of Unfair Order Execution Price in _validateOrdersAndDetermineFillPrice Function

Summary

While the system seems to have appropriate checks in place for order matching, a potential risk exists around the order execution pricing logic, specifically when the orders are placed in the same block. In such cases, the system currently selects the sell order price as the fill price. This may not be fair for the buyer, who may end up paying more than necessary.

Vulnerability Detail

In the _validateOrdersAndDetermineFillPrice function, when two orders are placed in the same block, the fill price is set as the price of the sell order (orders[1]). This is potentially unfair because it could lead to a scenario where the buyer is paying the maximum possible price, even when the sell order was potentially willing to sell at a lower price.

Impact

This could affect the trust of participants in the exchange, particularly those who frequently make buy orders. The unfair pricing might result in monetary losses for these participants, leading to a negative perception of the platform.

Code Snippet

        if (blockPlaced0 < blockPlaced1) {
            mode0 = OrderExecutionMode.Maker;
            fillPrice = orders[0].price;
        } else if (blockPlaced0 > blockPlaced1) {
            mode1 = OrderExecutionMode.Maker;
            fillPrice = orders[1].price;
        } else { // both orders are placed in the same block, not possible to determine what came first in solidity
            // executing both orders as taker order
            mode0 = OrderExecutionMode.SameBlock;
            mode1 = OrderExecutionMode.SameBlock;
            // Bulls (Longs) are our friends. We give them a favorable price in this corner case
            fillPrice = orders[1].price;
        }

https://github.com/sherlock-audit/2023-04-hubble-exchange/blob/main/hubble-protocol/contracts/orderbooks/OrderBook.sol#L288-L300

Tool used

Manual Review

Recommendation

A more fair approach might be to calculate the average of the buy order and sell order prices, which might better reflect the market conditions at the moment both orders were made. In case of any restrictions for using the average price, a price setting rule that better approximates a fair market price should be used.

Proof Of Concept

To reproduce this vulnerability:

1. Deploy the smart contract on a local testnet or Ethereum mainnet fork.
2. Place two orders: a buy order and a sell order within the same block with different prices.
3. Once the orders are matched and executed, check the execution price.
4. You will notice that the execution price is exactly the same as the sell order, regardless of the buy order's price.

Note: This is a potential risk, meaning that it depends on the intentions of the traders when they place their orders. Traders who are willing to trade at the execution price would not consider this a problem, while others might see it as an unfair pricing practice.

[0xshinobii]

In the case of same block trade, shorts are selling at the price they quoted and longs are buying at price ≤ long order price. As it is a limit order system, the user get to decide the price of the order and they are always getting a better or the same price than what they quoted. There is no scenario where a user is getting unfair/worst price.

[ctf-sec]

Although the issue has sponsor dispute tag, I am more incline with the watson and leave this as a medium issue

Because

When a validator is selected as the block producer, the buildBlock function fetches active markets and open orders from the indexer, evaluates open positions for potential liquidations, runs the matching engine, and then relays these operations as local transactions before continuing the normal transaction bundling process.

the block producer may determine the order matching and manipulate who is order[0] and who is order[1] to extract value

{ // both orders are placed in the same block, not possible to determine what came first in solidity
            // executing both orders as taker order
            mode0 = OrderExecutionMode.SameBlock;
            mode1 = OrderExecutionMode.SameBlock;
            // Bulls (Longs) are our friends. We give them a favorable price in this corner case
            fillPrice = orders[1].price;
        }

Want to quote the first second in the report:

https://github.com/sherlock-audit/2023-04-hubble-exchange-judging/issues/183

Validators are given the exclusive privilege to match.

[0xshinobii]

Yes correct validator can extract MEV from this by opening longs at better price everytime. Changed status to confirmed. FYI: Fill price determination happens through precompile now and long order is executed as taker and short as maker (maker fee is less than taker fee). So long will get better price but pay more fee and short order will execute at order price but pay less fee

[0xshinobii]

We'll be launching with a trusted set of validators. So, we'll fix this in later releases when we allow anyone to become a validator.