osmanozdemir1 - Canceled order refunds should be sent to the `requester`, not the `recipient`.

[sherlock-admin]

osmanozdemir1

high

Canceled order refunds should be sent to the requester, not the recipient.

Summary

When an order is requested, payments are made from the requester to the escrow. Payments should return to the requester in case of a cancelation, but they are transferred to the recipient. The requester will lose funds in the event of cancellation, as the protocol does not enforce the requester and the receiver to be the same.

Vulnerability Detail

Buying or selling real-world assets in this protocol starts with requesting an order. A user creates an order request with some information like the asset token and payment token etc. and calls requestOrder() function in the OrderProcessor. When an order is requested, payment tokens are transferred to the issuer contract which acts like an escrow. Tokens stay there until cancellation or filling of the order.

Here is the _requestOrderAccounting function that makes the transfer to the escrow from the requester:

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

// File: BuyOrderIssuer.sol
141.     IERC20(orderRequest.paymentToken).safeTransferFrom(msg.sender, address(this), orderRequest.quantityIn);

As you can see above, the transfer is made from the requester to the escrow.

Now let's examine the cancelation process. The user makes a cancel request first. This function is only callable by the initial order requester. After these checks are performed _cancelOrderAccounting function is called.

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

// File: BuyOrderIssuer.sol
    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
214.--> IERC20(orderRequest.paymentToken).safeTransfer(orderRequest.recipient, refund);
    }

As you can see in line 214, payment tokens are transferred to the orderRequest.recipient, not to the orderState.requester who is the actual payer of the tokens.

The issue is the same in the SellOrderProcessor.sol contract too.
https://github.com/sherlock-audit/2023-06-dinari/blob/main/sbt-contracts/src/issuer/SellOrderProcessor.sol#L150

The recipient gets the tokens whether the order is filled or canceled. There is no problem if the requester and the recipient are the same address but it doesn't have to be. It is not enforced by the protocol. Maybe Bob wanted to buy asset tokens for Alice but then decided not to. Alice still gets Bob's payment tokens.

Impact

The order requester will lose funds in case of a cancelation of the order if the recipient is different.

Code Snippet

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

// File: BuyOrderIssuer.sol
141.     IERC20(orderRequest.paymentToken).safeTransferFrom(msg.sender, address(this), orderRequest.quantityIn);

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

// File: BuyOrderIssuer.sol
    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
214.--> IERC20(orderRequest.paymentToken).safeTransfer(orderRequest.recipient, refund);
    }

Tool used

Manual Review

Recommendation

Refund payments to the requester. Change this:

   IERC20(orderRequest.paymentToken).safeTransfer(orderRequest.recipient, refund);

to this:

   IERC20(orderRequest.paymentToken).safeTransfer(orderState.requester, refund);

Duplicate of #61