peanuts - Double accounting of remainingOrder when filling order which may result in underflow

[sherlock-admin]

peanuts

high

Double accounting of remainingOrder when filling order which may result in underflow

Summary

orderState.remainingOrder deducts fillAmount twice when filling order, resulting in accounting error

Vulnerability Detail

When fillOrder() is called, remainingOrder is calculated by subtracting fillAmount from orderState.remainingOrder. If remainingOrder is not equal to 0, _orders[orderId].remainingOrder is updated to theremainingOrderamount. Then,_fillOrderAccounting` is called.

        // Update order state
>      uint256 remainingOrder = orderState.remainingOrder - fillAmount;
        // If order is completely filled then clear order state
        if (remainingOrder == 0) {
            // Notify order fulfilled
            emit OrderFulfilled(orderId, orderRequest.recipient);
            // Clear order state
            delete _orders[orderId];
            _numOpenOrders--;
        } else {
            // Otherwise update order state
 >         _orders[orderId].remainingOrder = remainingOrder;
            _orders[orderId].received = orderState.received + receivedAmount;
        }
      _fillOrderAccounting(orderRequest, orderId, orderState, fillAmount, receivedAmount);

When _fillOrderAccounting() is called, it calls _fillBuyOrder() and the function subtracts remainingOrder from fillAmount again.

    function _fillBuyOrder(
        OrderRequest calldata orderRequest,
        bytes32 orderId,
        OrderState memory orderState,
        uint256 fillAmount,
        uint256 receivedAmount
    ) internal virtual {
        FeeState memory feeState = _feeState[orderId];
>      uint256 remainingOrder = orderState.remainingOrder - fillAmount;
        // If order is done, close order and transfer fees

For example, let's say for a buy market order, order.paymentTokenQuantity is 1000 so remainingOrder is 1000.

        // Initialize order state
        uint256 orderAmount = order.sell ? order.assetTokenQuantity : order.paymentTokenQuantity;
        _orders[orderId] = OrderState({requester: msg.sender, remainingOrder: orderAmount, received: 0});

fillAmount is 700. When the operator calls fillOrder(), 1000 is subtracted from 700 twice, which results in underflow and revert. By right, orderAmount should be 300 ( how much is left to fill), and should not be subtracted again in _fillBuyOrder.

Impact

remainingOrder is subtracted by fillAmount twice, which might make the calculation underflow.

Code Snippet

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

Tool used

Manual Review

Recommendation

Recommend only subtracting remainingOrder once and not subtracting it again in _fillBuyOrder.

[jaketimothy]

It is to protect the developer from issues such as this that orderState is passed as memory into _fillOrderAccounting and it's subcalls. mapping(bytes32 => OrderState) private _orders; is private in OrderProcessor so that the state is only updated once.

There may be opportunities to optimize in-memory operations here, but https://github.com/sherlock-audit/2023-06-dinari/blob/4851cb7ebc86a7bc26b8d0d399a7dd7f9520f393/sbt-contracts/src/issuer/OrderProcessor.sol#L290 and https://github.com/sherlock-audit/2023-06-dinari/blob/4851cb7ebc86a7bc26b8d0d399a7dd7f9520f393/sbt-contracts/src/issuer/BuyOrderIssuer.sol#L169 will always produce the same value as implemented.

[ctf-sec]

Will look into this one!

[ctf-sec]

The finding is invalid, agree with sponsor's comment cc judge @Oot2k

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

        // Notify order filled
        emit OrderFill(orderId, orderRequest.recipient, fillAmount, receivedAmount);

        // Update order state
        uint256 remainingOrder = orderState.remainingOrder - fillAmount;
        // If order is completely filled then clear order state
        if (remainingOrder == 0) {
            // Notify order fulfilled
            emit OrderFulfilled(orderId, orderRequest.recipient);
            // Clear order state
            delete _orders[orderId];
            _numOpenOrders--;
        } else {
            // Otherwise update order state
            _orders[orderId].remainingOrder = remainingOrder;
            _orders[orderId].received = orderState.received + receivedAmount;
        }

        // Move tokens
        _fillOrderAccounting(orderRequest, orderId, orderState, fillAmount, receivedAmount);
    }

So we first cache the orderState

then we subtract to update the remainingOrder in storage

then we passed the cached OrderState to the function _fillOrderAccounting

    function _fillBuyOrder(
        OrderRequest calldata orderRequest,
        bytes32 orderId,
        OrderState memory orderState,
        uint256 fillAmount,
        uint256 receivedAmount
    ) internal virtual {
        FeeState memory feeState = _feeState[orderId];
        uint256 remainingOrder = orderState.remainingOrder - fillAmount;

the orderRequest is the cached order state, so we need to run the code to get the remainingOrder amount

uint256 remainingOrder = orderState.remainingOrder - fillAmount;

there is no double counting.

[Oot2k]

Agree with @ctf-sec