OrderBook

Summary

The contract's token transfer functions, such as those in cancelSellOrder and buyOrder, do not validate whether the amount being transferred is non-zero before execution. While the ERC20 standard requires tokens to handle zero-value transfers, some widely used tokens do not comply and will revert the transaction. If such a non-compliant token is ever whitelisted, any operation that results in a zero-value transfer attempt will unexpectedly fail. This represents a lack of defensive programming against known token integration issues, creating a potential Denial of Service (DoS) vector.

Finding Description

Several functions within the OrderBook contract perform token transfers without first ensuring the amount is greater than zero. A key example is in cancelSellOrder:

If an order with amountToSell = 0 were to exist in the contract state, calling cancelSellOrder on it would trigger safeTransfer(..., 0). With a non-compliant token that reverts on zero-value transfers, this call would fail.

A similar pattern exists in buyOrder where sellerReceives could theoretically be zero:

While the current business logic in createSellOrder prevents the creation of zero-amount orders, relying solely on upstream logic is fragile. Best practices dictate that low-level interactions (like a token transfer) should be self-defensive. A future code change could inadvertently allow a zero-amount state, at which point this latent issue would become an active bug.

Impact

The impact is a potential Denial of Service (DoS) for certain operations, conditional on a non-compliant token being whitelisted.

• A call to cancelSellOrder for a (malformed) zero-amount order would be permanently unrevertable, trapping the order state.

• A buyOrder call that results in a zero share for the seller would fail, preventing the order from being filled.
  This degrades the protocol's robustness and its compatibility with the broader token ecosystem.

Likelihood

Low. This vulnerability requires the owner to whitelist a non-compliant ERC20 token and for a zero-value state to occur. However, given the existence of such tokens, this represents a tangible integration risk.

Proof of Concept

A practical PoC is difficult to construct without modifying the contract's logic, as the current validation in createSellOrder effectively prevents the necessary preconditions (a zero-amount order) from being met. Attempts to bypass this using storage manipulation (vm.store) proved complex and unreliable due to the packed nature of the Order struct.

However, a conceptual PoC remains valid:

1. Assumption: A non-compliant RevertingToken that reverts on transfer(to, 0) is whitelisted.

2. Assumption: A code path (perhaps through a future, unforeseen update) allows an Order with amountToSell = 0 to be created.

3. Action: A user calls cancelSellOrder on this specific order.

4. Result: The contract attempts safeTransfer(seller, 0). The RevertingToken reverts, causing the entire cancelSellOrder transaction to fail. The order is now permanently stuck and cannot be cancelled.

The inability to easily test this highlights the issue: the contract's safety relies on perfect upstream logic, rather than defensive, self-contained function calls.

Recommended Mitigation

Implement explicit non-zero checks before any token transfer to make the contract more robust and resilient to integration issues.

This approach follows the principle of defensive programming and ensures the contract will not fail due to known non-compliant token behaviors.