0xpinky - Lack of clarity to classify the order that are requested for order and that are requested for cancel.

[sherlock-admin]

0xpinky

high

Lack of clarity to classify the order that are requested for order and that are requested for cancel.

Summary

when we look at the order handling mechanism, for both requesting the order and requesting for cancel, single struct OrderRequest is used. At the end of each request, relevant event is emitted. I think, the idea behind this event emitting is to capture this by the operator and execute the relevant functionality in the Order Process contract.

But, when we look at the flow, there are no firm control over the order request which are for placing or cancelling at the block chain level. This will make the system clumsy when there are large number of orders are requested. behind the scene, if there is issue with event handling, then the order which are requested for placing could get cancelled and vice versa can happen for cancel request.

Note: events are not stored in the blockchain.

Vulnerability Detail

    function requestOrder(OrderRequest calldata orderRequest, bytes32 salt) public nonReentrant whenOrdersNotPaused {
        // Reject spam orders
        if (orderRequest.quantityIn == 0) revert ZeroValue();
        // 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();

        // Get order from request and move tokens
        Order memory order = _requestOrderAccounting(orderRequest, orderId);

        // Send order to bridge
        emit OrderRequested(orderId, order.recipient, order, salt);

    function requestCancel(OrderRequest calldata orderRequest, bytes32 salt) external {
        bytes32 orderId = getOrderIdFromOrderRequest(orderRequest, salt);
        address requester = _orders[orderId].requester;
        // Order must exist
        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);
    }

In above both functions, events OrderRequested, CancelRequested are emitted. But they are not stored either in the order or in the orderstate struct.

when we look at the fill-order and cancel order functions, OrderRequest data is given as input with relevant bytes, after that the OrderState data is used to do basic validation such as requester, remainingOrder

    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 == address(0)) revert OrderNotFound();
        // Fill cannot exceed remaining order
        if (fillAmount > orderState.remainingOrder) revert AmountTooLarge();

    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();

This way, there are no check which order actually wanted to place and which one wanted to be cancelled.

Impact

The order which are wanted to be placed can get canceled, and the vice versa situation can happen for order cancel request.

Code Snippet

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

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

Tool used

Manual Review

Recommendation

We suggest to introduce new flag called bool Cancel in the OrderState struct. set this true when the order is wanted to cancel and handle the codes for fill and cancel.

    struct OrderState {
        // Account that requested the order
        address requester;
        // Amount of order token remaining to be used
        uint256 remainingOrder;
        // Total amount of received token due to fills
        uint256 received;
        //bool flag to notify the cancel or not
        bool Cancel; ------------------------------------->>> New flag.
    }

[0xpinky]

Escalate for 10 USDC

First, I am not sure why this issue is excluded. There are no clarification for this.

This is valid issue.

The contract relies on the facts that the user will raise a request and operator will process it.

User can raise the request either to process or cancel the order.

But, when we see the implementation, there is no clarity on which order need to be processed and which one need to be cancelled by operator.

The operator can pick order either to process. Since there are no clarity, orders that need to be processed can be cancelled. and order that need to be cancelled can be processed.

User will be affected, contract fails its intended functionality.

[ctf-sec]

@jaketimothy

https://github.com/dinaricrypto/sbt-contracts/issues/113

the fix stores the state to make sure a order cannot be cancel multiple times

[0xpinky]

@ctf-sec why this issue is excluded ?

[ctf-sec]

@ctf-sec why this issue is excluded ?

A few of the issue says the user can cancel order multiple times, but it is up to the operator to only execute one cancel request

and we considered the operator as trusted so how does operator make sure the cancel order request only processed once is not in scope

In this particular report

When create order,

the method requestOrder is called

when cancel order, the method cancelOrder is called

and each method has different 4 bytes function selector

https://solidity-by-example.org/function-selector/

so the 4 bytes function selector combined with the emitted event should be sufficient to distinguish if the user want to cancel order or create / request order

again, how does the operator parse the function selector and the emitted event is not in scope

https://dev.to/mistersingh179/lets-dive-a-little-in-to-function-selectors-in-solidity-2e11

[0xpinky]

I don't agree on your answer due to following reason.

1. As you said, it can be distinguished using function selection, then why do the fixes needed ?
2. relying just 4 bytes values and event will not be sufficient, its just 4 bytes of data which can be easily crafter by using any arbitrary data.
3. there is another issue reported where absent of valid event emitting is missing which lead to rendering the cancelling request. https://github.com/sherlock-audit/2023-06-dinari-judging/issues/139 when incorrect event is emitted, it would not be possible to continue any of the on-chain actions properly.
4. as a whole the current implementation is not sufficient to finish the intended functionality.
5. the issue is not related to malicious operator, its about missing of core feature which halt the upcoming on-chain activities.