Description (normal behavior):
The contract uses custom errors for gas efficiency and clear revert reasons.
Description (issue):
In emergencyWithdrawERC20, it uses a string revert:
if (_tokenAddress == address(iWETH) || ...)
revert("Cannot withdraw core order book tokens via emergency function");
This is inconsistent and slightly more gas costly.
Risk
Likelihood:
Low.
Reason 1: Occurs when the owner tries to withdraw core tokens.
Reason 2: Design inconsistency only — no direct user exploit.
Impact:
Gas inefficiency.
Reduced audit clarity and code consistency.
- remove this code
+ add this code
+ error CannotWithdrawCoreToken();
- revert("Cannot withdraw core order book tokens via emergency function");
+ revert CannotWithdrawCoreToken();
Explanation:
This mitigation is about explicitly preventing reentrancy attacks on the buyOrder function.
⛓️ What is reentrancy?
Reentrancy is when an external contract is able to call back into your contract before the first function call finishes.
It can allow attackers to exploit inconsistent or unfinished internal states, drain funds, or bypass checks.
Famous example
The DAO hack on Ethereum in 2016 was a reentrancy attack, which allowed an attacker to withdraw funds repeatedly before balances were updated.
⚔️ Why is buyOrder a potential target?
In buyOrder, you do:
1️⃣ Mark the order as inactive.
2️⃣ Transfer USDC from buyer to seller.
3️⃣ Transfer the sold token to the buyer.
While you correctly mark order.isActive = false first (which protects against some simple reentrancy), if any future change adds a vulnerable external call before that, or if tokens with malicious callbacks are used, it could become vulnerable.
🛡️ What does nonReentrant do?
When you add nonReentrant, you use OpenZeppelin’s ReentrancyGuard, which works like this:
1️⃣ When a nonReentrant function is called, a status flag inside the contract is set to "entered."
2️⃣ If a reentrant call happens (a nested call into the same function or another nonReentrant function), the contract immediately reverts.
3️⃣ Once the outer function finishes, the flag resets to "not entered."
🔧 How does this protect you?
It forces your function to complete before it can be called again from within itself or another vulnerable path.
🟢 The actual change
- function buyOrder(uint256 _orderId) public {
+ function buyOrder(uint256 _orderId) public nonReentrant {
You simply add nonReentrant as a modifier to this function.
No further code changes needed inside the function itself.
✍️ Why is this mitigation important (even if you already set flags)?
✅ Future-proofing: Prevents future changes from introducing reentrancy bugs accidentally.
✅ Cleaner audits: Makes it very obvious to auditors and other devs that reentrancy is handled explicitly.
✅ Defense in depth: Even if your logic is safe today, new features (like supporting callbacks or hooks) might break assumptions.
⚡ Summary
Without mitigation With mitigation
Only relies on manual state checks Enforced at the EVM level via OpenZeppelin guard
Reentrancy bugs possible if code changes later Strong reentrancy lock automatically prevents nested calls
Higher audit burden Clear and safe design, easier to audit
✅ In one sentence
👉 Adding nonReentrant ensures buyOrder cannot be re-entered during execution, eliminating an entire class of potential attacks automatically.
💬 Let me know if you'd also like a visual diagram showing how a reentrancy exploit would work with and without this guard!