OrderBook

The `amendSellOrder()` function violates the Checks-Effects-Interactions (CEI) design pattern by calling token.

`safeTransfer(....)` before updating state variables.

This could open the door to reentrancy attacks or unpredictable behaviors in malicious or poorly written ERC20 tokens.

​

​

You're calling `token.safeTransfer()` before updating `order.amountToSell`.

If the token contract is malicious (e.g.,a custom ERC-20 with a fallback),

the attacker can re-enter and exploit the old state before it's updated.

​

1. Reentrancy: Malicious ERC20 token could re-enter before order is updated.

While not critical here, it's still bad practice.

2. Inconsistent State: If transfer fails (e.g.,due malicious logic), the order stays unchanged, but partial logic may have run.

3. Predictability: Poor state management makes behavior harder to reason about and harder to audit or integrate.

I can give theorical POC:

​

```javascript

contract MaliciousToken is IERC20 {

function transfer(address recipient, uint256 amount) external override returns (bool) {

// Try to re-enter amendSellOrder or call back into the contract

OrderBook(orderBookAddress).cancelSellOrder(orderId);

return true;

}

}

```

​

Even if this doesn't cause reentrancy today, it introduces fragility into the system.

​

In a `OrderBook::amendSellOrder`

​

```diff

} else if (_newAmountToSell < order.amountToSell) {

// Decreasing amount: Transfer excess tokens back to seller

uint256 diff = order.amountToSell - _newAmountToSell;

}

​

// Update order details

order.amountToSell = _newAmountToSell;

order.priceInUSDC = _newPriceInUSDC;

order.deadlineTimestamp = newDeadlineTimestamp;

```

Benefits of CEI Compliance :

​

=> Prevents reentrancy risks from malicious token contracts

​

=> Maintains predictable and consistent state, even on failure

​

=> Easier to reason about during formal verification and auditing

​

=> Improves integration safety with other contracts

​