Delvir0 - Incorrect function flow when using DirectBuyIssuer to fill order.

[sherlock-admin]

Delvir0

high

Incorrect function flow when using DirectBuyIssuer to fill order.

Summary

When using DirectBuyIssuer.sol to take an escrow and fill an order, BuyOrderIssuer._fillOrderAccounting is called instead of DirectBuyIssuer._fillOrderAccounting. This skips all the accounting regarding escrow.

Vulnerability Detail

The contract architecture of DirectBuyIssuer.sol is al follows: OrderProcessor	BuyOrderIssuer

DirectBuyIssuer

The flow of filling an order = takeEscrow() -> OrderProcessor.fillOrder(), which calls BuyOrderIssuer_fillOrderAccounting and BuyOrderIssuer_fillBuyOrder.

This skips the following code which does the accounting when an escrow is involved: https://github.com/sherlock-audit/2023-06-dinari/blob/4851cb7ebc86a7bc26b8d0d399a7dd7f9520f393/sbt-contracts/src/issuer/DirectBuyIssuer.sol#L122-L123

The test cases in DirectBuyIssuerTest also do not check for the escrow values, meaning it has not been tested weather the correct function is being called.

Impact

Escrow of an account is not updated meaning multiple escrows can be taken while exceeding the users true escrow value.

POC: I've recreated the structure to check which function is being called.

abstract contract orderProcessor {
   uint public number = 0;

   function go() public {
     fill();
   }
   function fill() public virtual {}
}

contract BuyProcessor is orderProcessor {
   function fill() public virtual override{
   number = 5;
   }
}

contract DirectBuy is BuyProcessor {
   function fill() public virtual override  {
   number = 10;
   }
}

In this case, number = 5 while it should be 10

Code Snippet

provided

Tool used

Manual Review

Recommendation

Check if an escrow is taken for the order and explicitly call DirectBuy._fillOrderAccounting

Duplicate of #56