[HIGH] OrderProcessor#requestedOrder and #fillOrder - Malicious user could monitor the events OrderRequested and Orderfulfilled to perform sandwich attacks causing losses for the users

Summary

A malicious user can monitor the events OrderRequested and OrderFulfilled to perform a successful sandwich-attack which can lead to financial losses for other protocol users.

Vulnerability Detail

Front-runners can monitor the OrderRequested and OrderFulfilled events to detect significant order transactions. They can then preemptively submit a transaction with higher gas fees to manipulate the price before the user's order is processed (by monitoring the OrderRequested event) and again afterward to take advantage of the price discrepancy for a profit (by monitoring the OrderFulfilled event).

Steps

Monitor the Arbitrum blockchain after the eventOrderRequested.
When a significant order transaction is spotted (pending in the mempool), the malicious user will submit an order by calling requestOrder with higher gas fees and front-running the significant order (if the significant order is a buy order, the malicious transaction will also be a buy order and vice-versa).
When the significant order is settled (OrderFulfilled), the malicious user can call the requestOrder again and take advantage of the price slippage.

Impact

A successful sandwich attack can lead to financial losses for the affected users due to unfavorable price slippage. It can also reduce the overall efficiency and fairness of the protocol, leading to a loss of user trust and potential damage to the platform's reputation.

Code Snippet

https://github.com/sherlock-audit/2023-06-dinari/blob/main/sbt-contracts/src/issuer/OrderProcessor.sol#L244

https://github.com/sherlock-audit/2023-06-dinari/blob/main/sbt-contracts/src/issuer/OrderProcessor.sol#L272

    function fillOrder(OrderRequest calldata orderRequest, bytes32 salt, uint256 fillAmount, uint256 receivedAmount)
        external
        nonReentrant
        onlyRole(OPERATOR_ROLE)
    {
        ...
        // Notify order filled
        emit OrderFill(orderId, orderRequest.recipient, fillAmount, receivedAmount);
        ...
        // If order is completely filled then clear order state
        if (remainingOrder == 0) {
            // Notify order fulfilled
            emit OrderFulfilled(orderId, orderRequest.recipient);
            ...
        } else {
            ...
        }
        ...
    }
    function requestOrder(OrderRequest calldata orderRequest, bytes32 salt) public nonReentrant whenOrdersNotPaused {
        ...
        // Send order to bridge
        emit OrderRequested(orderId, order.recipient, order, salt);
        ...
    }

Tool used

Manual Review

Recommendation

The recommendations are the following:

Commit-Reveal Schemes: These schemes involve two steps: first, the user sends a transaction with a hashed version of their action ("commit"), and then after some time sends the actual data ("reveal"). This can make it harder for front-runners to profit because they do not know what to front-run until the reveal phase. The downside is this process takes longer and requires at least two transactions, which can be costly and complex for the user.
Batch Transactions: Transactions can be grouped together and executed all at once at a certain time, to prevent front-running. This way, no one can get a transaction confirmed ahead of another, since they all execute simultaneously.
Time-Weighted Average Price (TWAP): Some automated market makers like Uniswap v3, use the time-weighted average price. This prevents price manipulation and front-running by referencing a price that's averaged over a certain time period.

sherlock-audit / 2023-06-dinari-judging

vangrim - [HIGH] OrderProcessor#requestedOrder and #fillOrder - Malicious user could monitor the events OrderRequested and Orderfulfilled to perform sandwich attacks causing losses for the users #119