ni8mare - Possible DoS attack that results in user `requestOrder` to be cancelled.

[sherlock-admin]

ni8mare

medium

Possible DoS attack that results in user requestOrder to be cancelled.

Summary

Possible DOS attack that results in user requestOrder being cancelled.

Vulnerability Detail

Consider this scenario (Assume for a buy order): When users submit their request Order, they input 2 parameters - orderRequest and salt. These values are publicly known to everyone, once they are part of the mempool. A malicious user could use these known values and submits the order on the user's behalf (frontrunning the user's transactions), generating the same orderId.

As the orderId is calculated using the getOrderIdFromRequest which just calculates the hash based on publicly known params, it is easy for an attacker to get the same orderId.

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

This malicious user takes the value of the requester in the OrderState struct but the quantityIn would be transferred from the attacker's account (check _requestOrderAccounting from the BuyOrderIssuer contract), not the recipient or the victim inthis case.

function _requestOrderAccounting(OrderRequest calldata orderRequest, bytes32 orderId)
    internal
    virtual
    override
    returns (Order memory order)
{
    // Determine fees
    (uint256 flatFee, uint256 percentageFee) = getFeesForOrder(orderRequest.paymentToken, orderRequest.quantityIn);
    uint256 totalFees = flatFee + percentageFee;
    // Fees must not exceed order input value
    if (totalFees >= orderRequest.quantityIn) revert OrderTooSmall();

    // Initialize fee state for order
    _feeState[orderId] = FeeState({remainingPercentageFees: percentageFee, feesEarned: flatFee});

    // Construct order
    order = Order({
        recipient: orderRequest.recipient,
        assetToken: orderRequest.assetToken,
        paymentToken: orderRequest.paymentToken,
        // Buy order
        sell: false,
        // Market order
        orderType: OrderType.MARKET,
        assetTokenQuantity: 0,
        // Hold fees back from order amount
        paymentTokenQuantity: orderRequest.quantityIn - totalFees,
        price: orderRequest.price,
        // Good until cancelled
        tif: TIF.GTC,
        // Emit fees held back from order amount
        fee: totalFees
    });

    // Escrow payment for purchase
    IERC20(orderRequest.paymentToken).safeTransferFrom(msg.sender, address(this), orderRequest.quantityIn);
    //@audit - here msg.sender is the requester or the attacker in this case.
}

Also, the victim users request order gets cancelled because it does not satisfy this condition for the same orderId - if (_orders[orderId].remainingOrder > 0) revert DuplicateOrder(); as seen in the requestOrder function.

The operator would call the cancelOrder function, cancelling the order. But, one would think that the attack fails because, during cancellation of the order, the refund goes to the order.recipient(victim) and not the requester(attacker) of the order (check the _cancelOrderAccounting function in BuyOrderIssuer contract).

function _cancelOrderAccounting(OrderRequest calldata orderRequest, bytes32 orderId, OrderState memory orderState)
    internal
    virtual
    override
{
    FeeState memory feeState = _feeState[orderId];
    // If no fills, then full refund
    // This addition is required to check for any fills
    uint256 refund = orderState.remainingOrder + feeState.remainingPercentageFees;
    // If any fills, then orderState.remainingOrder would not be large enough to satisfy this condition
    // feesEarned is always needed to recover flat fee
    if (refund + feeState.feesEarned == orderRequest.quantityIn) {
        _closeOrder(orderId, orderRequest.paymentToken, 0);
        // Refund full payment
        refund = orderRequest.quantityIn;
    } else {
        // Otherwise close order and transfer fees
        _closeOrder(orderId, orderRequest.paymentToken, feeState.feesEarned);
    }

    // Return escrow
    IERC20(orderRequest.paymentToken).safeTransfer(orderRequest.recipient, refund);
    //@audit - paymentToken is transferred to recipient(victim) not requestor(attacker) on cancellation
}

But, this attack would be considered in those scenarios where the order quantityIn submitted by the victim is small and hence makes it viable for an attacker to lose their funds, to deny the user from submitting a buy request. The attacker may repeat this attack as long as they think it's viable for them. Also, the project aims to launch in different jurisdictions, where there could be securities worth cents. So, even if the quantityIn (paymentToken) amount is quite small, the intended amount of the underlying security to be bought might not be.

Also, consider the point of view of an attacker who has submitted a big buy order. They may not want other buy orders before their order gets submitted, as it would result in the attacker minting lower-than-expected underlying asset tokens. So, they would use this attack and try to cancel the buy orders of other users. This denies the other users from participating.

Impact

User buy orders can get cancelled. Preventing them from using the intended functionality. A possible DoS attack.

Code Snippet

https://github.com/dinaricrypto/sbt-contracts/blob/6d36760def25449c3f35f6ed38128a7eaf352903/src/issuer/OrderProcessor.sol#L205

Tool used

Manual Review

Recommendation

This happens because the orderRequest and salt parameters are used to generate the orderId which are known once the transaction sits in the mempool, so a malicious user can use these to generate the same orderId and produce the attack as mentioned above. A possible recommendation would be to generate a different id by encoding the msg.sender while calculating the hash.

function getUniqueOrderIdFromOrderRequest(OrderRequest memory orderRequest, bytes32 salt) public pure returns (bytes32) {
  return keccak256(
      abi.encode(
          ORDERREQUEST_TYPE_HASH,
          salt,
          orderRequest.recipient,
          orderRequest.assetToken,
          orderRequest.paymentToken,
          orderRequest.quantityIn,
          msg.sender
      )
  );
}