Description

The function buyOrder() performs external calls (safeTransferFrom and safeTransfer) before updating state variables, which is dangerous. Although ERC20 tokens are generally safe, a malicious token (non-compliant ERC20) could re-enter the contract and exploit logic or steal funds.

These external token calls could reenter if the token is malicious or not ERC20-compliant, especially before order.isActive = false is set.

Impact:

• A malicious ERC20 token could exploit the lack of ReentrancyGuard by reentering into the contract during a token transfer (safeTransferFrom or safeTransfer) and invoking functions like buyOrder() or cancelSellOrder() again.

Proof of Concept

1. Deploy a malicious ERC20 token with a transfer or transferFrom hook that calls back into the OrderBook contract (e.g., calls buyOrder() again).

2. Create a sell order using this malicious token.

3. Call buyOrder() on the order.

4. During the execution of safeTransfer, the malicious token reenters and calls buyOrder() again before order.isActive is set to false.

5. Now both calls think the order is still active — the order can be filled twice.

Recommended Mitigation

Add the ReentrancyGuard modifier from OpenZeppelin and apply nonReentrant to:

• buyOrder()

• cancelSellOrder()

• amendSellOrder()