The order lifecycle outlined in DirectBuyIssuer will revert in most cases for filled orders
Summary
In most cases, checks whether the filled amount is less than or equal to the amount taken from escrow will revert given the outlined order lifecycle for the DirectBuyIssuer contract, resulting in attempts to fill orders failing.
Vulnerability Detail
The order lifecycle outlined in the natspec comments detail the following flow:
/// 1. User requests an order (requestOrder)
/// 2. Operator takes escrowed payment (takeEscrow)
/// 3. [Optional] Operator partially fills the order (fillOrder)
/// 4. [Optional] User requests cancellation (requestCancel)
/// 5. Operator returns unused payment to contract (returnEscrow)
Effectively, the operator will first take a reasonable amount of funds from the escrow for an order. Due to changes in market price, a different amount of these funds might be required to purchase a given amount of assetToken. In practice this will often mean that the amount taken from escrow is not the same as the fillAmount used to purchase some receivedAmount of assetToken. Then, based on this lifecycle, the excess funds taken from escrow will be returned, after the operator fulfills the order.
Based on the order lifecycle, the operator will then fill the order by calling fillOrder, which calls _fillOrderAccounting. In _fillOrderAccounting there is the following check:
The issue with this statement is that effectively if fillAmount is less than the amount taken from escrow, this will revert. However, this will happen in most cases due to changes in market price of the asset from when the operator took funds from escrow, and when they actually purchase the asset. For example, let's assume orderState.remainingOrder is 1000. The operator takes 500 from escrow to buy 5 tokens at a price of 100 per token. The price of each token then increases to 110, meaning that 500 can now only purchase 4 tokens at a total price of 440. The fillAmount here will be 440. In the check we get the following statement: 440 > 1000 - 500 or 440 > 500, which will revert.
Impact
Due to the order lifecycle, most calls to fulfill orders in the DirectBuyIssuer contract will revert, resulting in the inability to fulfill orders.
The order lifecycle should be updated so that the excess funds taken from escrow are first returned prior to the operator attempting to fulfill the order.
toshii
medium
The order lifecycle outlined in DirectBuyIssuer will revert in most cases for filled orders
Summary
In most cases, checks whether the filled amount is less than or equal to the amount taken from escrow will revert given the outlined order lifecycle for the DirectBuyIssuer contract, resulting in attempts to fill orders failing.
Vulnerability Detail
The order lifecycle outlined in the natspec comments detail the following flow:
Effectively, the operator will first take a reasonable amount of funds from the escrow for an order. Due to changes in market price, a different amount of these funds might be required to purchase a given amount of
assetToken. In practice this will often mean that the amount taken from escrow is not the same as thefillAmountused to purchase somereceivedAmountofassetToken. Then, based on this lifecycle, the excess funds taken from escrow will be returned, after the operator fulfills the order.Based on the order lifecycle, the operator will then fill the order by calling
fillOrder, which calls_fillOrderAccounting. In_fillOrderAccountingthere is the following check:The issue with this statement is that effectively if
fillAmountis less than the amount taken from escrow, this will revert. However, this will happen in most cases due to changes in market price of the asset from when the operator took funds from escrow, and when they actually purchase the asset. For example, let's assumeorderState.remainingOrderis 1000. The operator takes 500 from escrow to buy 5 tokens at a price of 100 per token. The price of each token then increases to 110, meaning that 500 can now only purchase 4 tokens at a total price of 440. ThefillAmounthere will be 440. In the check we get the following statement:440 > 1000 - 500or440 > 500, which will revert.Impact
Due to the order lifecycle, most calls to fulfill orders in the DirectBuyIssuer contract will revert, resulting in the inability to fulfill orders.
Code Snippet
Referenced lines of code: https://github.com/sherlock-audit/2023-06-dinari/blob/main/sbt-contracts/src/issuer/DirectBuyIssuer.sol#L114-L127
Tool used
Manual Review
Recommendation
The order lifecycle should be updated so that the excess funds taken from escrow are first returned prior to the operator attempting to fulfill the order.