OrderBook
First Flight #43
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Impact: high
Likelihood: medium
Invalid

Unsafe external token transfers before state updates (Reentrancy Risk)

whoami

Root + Impact

Description

  • The contract transfers seller tokens with safeTransferFrom before recording the order in state. For example, in createSellOrder:

IERC20(_tokenToSell).safeTransferFrom(msg.sender, address(this), _amountToSell);
@> orders[orderId] = Order({ ... isActive: true });
​

  • This violates the Checks-Effects-Interactions pattern. If a malicious token implements callbacks (e.g. ERC777 hooks), it could re-enter the contract during the transfer and manipulate state before the order is recorded. SafeERC20 only reverts on failed transfers; it does not prevent reentrancy

Risk

Likelihood: Moderate

  • Core tokens (WETH, WBTC, WSOL) are standard ERC20s (no callbacks), but the owner can add arbitrary tokens. If a malicious token is added, reentrancy becomes feasible.

  • Without a nonReentrant guard or reordering, any new token with callbacks could exploit these functions.

Impact: High

  • An attacker could, for example, trigger cancelSellOrder or alter orders mid-execution, potentially freeing tokens or disrupting accounting. This could lead to theft of user tokens or corruption of order state.

Proof of Concept

Here, when createSellOrder calls safeTransferFrom, the malicious token’s transferFrom re-enters and cancels order 1 before the contract records it, leaving inconsistent state.

// Example malicious token with a reentrancy callback during transferFrom
contract AttackerToken is IERC20 {
OrderBook book;
bool reentered = false;
constructor(address _book) { book = OrderBook(_book); }
function transferFrom(address, address, uint256) external override returns (bool) {
if (!reentered) {
reentered = true;
// Re-enter the OrderBook to cancel the order before it's stored
book.cancelSellOrder(1);
}
return true;
}
/* ...implement other ERC20 methods as no-ops... */
}
​

Recommended Mitigation

+ import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
+ contract OrderBook is Ownable, ReentrancyGuard {
...
- IERC20(_tokenToSell).safeTransferFrom(msg.sender, address(this), _amountToSell);
- orders[orderId] = Order({ id: orderId, seller: msg.sender, ... isActive: true });
+ // State update before external call to follow Checks-Effects-Interactions
+ orders[orderId] = Order({ id: orderId, seller: msg.sender, ... isActive: true });
+ IERC20(_tokenToSell).safeTransferFrom(msg.sender, address(this), _amountToSell);
​

Additionally, add a reentrancy guard to critical functions:

+ function createSellOrder(...) public nonReentrant returns (uint256) { ... }
+ function amendSellOrder(...) public nonReentrant { ... }
+ function buyOrder(...) public nonReentrant { ... }
​

These changes ensure state is updated before transfers and prevent reentrant calls, eliminating the exploit vector.

Updates

Lead Judging Commences

yeahchibyke Lead Judge
about 1 month ago
yeahchibyke Lead Judge about 1 month ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.