auditsea - `orderId` needs to be unique

[sherlock-admin]

auditsea

medium

orderId needs to be unique

Summary

orderId can be duplicated, which will cause issues on off-chain operating service.

Vulnerability Detail

orderId is generated from orderRequest and salt both given by a user, it means there can be multiple orders with same orderId that can cause some problems on off-chain operating service.(Even though there is only one active order with one orderId)

Impact

This will cause some problems on off-chain operating service, especially when orderId is used as a primary key of database used by off-chain operating service. For example:

1. requestCancel would cause confusions to the off-chain operating service. When a user creates an order and tries to cancel it, but let's say requestCancel is called twice by a mistake, off-chain service cancels the order, and after a delay, off-chain service tries to handle 2nd cancellation event. If user creates a same order between two cancellation events, 2nd order will also be cancelled even though both cancellation events were for the first order.
2. When orderId is used as a primary key on off-chain database, user's order history with same fields will not be recorded, causing lose of history.

Code Snippet

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

Tool used

Manual Review

Recommendation

Use randomly on-chain generated salt (not recommended) or implement user nonce that increases whenever orders are created.

[Delvir0]

Escalate

1. If there's an active order with certain orderId, it's not possible to set a new order with the same values and salt since that results in the same orderId. Code will prevent this by if (_orders[orderId].remainingOrder > 0) revert DuplicateOrder();
2. It's possible to create a "duplicate" orderId when remainingOrder = 0, but that does not cause any harm. At most it can cause the off-chain component to ignore the order which can be fixed by simply using an other salt.
3. Mentioned impact (confusion and loss of history) does not permanently break any functionality or cause loss of funds
4. Watson mentions off-chain components which are out of scope

[sherlock-admin]

Escalate

1. If there's an active order with certain orderId, it's not possible to set a new order with the same values and salt since that results in the same orderId. Code will prevent this by if (_orders[orderId].remainingOrder > 0) revert DuplicateOrder();
2. It's possible to create a "duplicate" orderId when remainingOrder = 0, but that does not cause any harm. At most it can cause the off-chain component to ignore the order which can be fixed by simply using an other salt.
3. Mentioned impact (confusion and loss of history) does not permanently break any functionality or cause loss of funds
4. Watson mentions off-chain components which are out of scope

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[ctf-sec]

On the verge of low / medium

[Oot2k]

Yes on the verge of low.

[sherlock-admin]

escalate

This should be invalidated because you cannot create the same order twice https://github.com/sherlock-audit/2023-06-dinari/blob/main/sbt-contracts/src/issuer/OrderProcessor.sol#L252

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.

[hrishibhat]

Result: Low Has Duplicates This is a valid low based on the points raised in the escalations

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• Delvir0: accepted

[ctf-sec]

the fix use (recipient address + recipient nonce) to make sure the order id is unique, looks fine,

or using global nonce works as well