OrderBook

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

H -2 Reentrancy via Malicious Token in createSellOrder

Root + Impact

Reentrancy Attack via Malicious ERC-20 Token During Sell Order Creation

Description

The createSellOrder() function allows users to create sell orders by transferring ERC-20 tokens into the contract. However, the function performs an external call to IERC20(_tokenToSell).safeTransferFrom() before updating its internal state (orders[orderId] assignment). This introduces a reentrancy vulnerability.

A malicious ERC-20 token can override its transferFrom() function to include a callback to createSellOrder() or another contract function that relies on the internal state. Since the orders[orderId] storage is not written before the external call, reentrant calls can result in:

  • Order ID reuse, since _nextOrderId++ increments but the mapping isn't populated yet.

  • Double bookings, where multiple orders share the same ID or overwrite each other.

  • Denial of Service (DoS) by locking up the order creation logic through repeated reentrancy loops.

This violates the Checks-Effects-Interactions pattern, which is a best practice in Solidity to prevent such exploits.

Risk

Likelihood:

  • Can be triggered by anyone using a malicious ERC-20 token.

  • Requires minimal setup by attacker.

Impact:

  • Leads to broken order logic, overwritten data, inconsistent internal state.

  • May cause denial-of-service or fund loss through corrupted order mapping.

Proof of Concept

Malicious Token Contract (simplified):

contract MaliciousToken is IERC20 {
address public orderBook;
constructor(address _orderBook) {
orderBook = _orderBook;
}
function transferFrom(address, address, uint256) external override returns (bool) {
// Reentrant call back to create another order
IOrderBook(orderBook).createSellOrder(address(this), 1, 100, 1000);
return true;
}
// Add dummy implementations for required ERC-20 functions
function totalSupply() public view override returns (uint256) {}
function balanceOf(address) public view override returns (uint256) {}
function allowance(address, address) public view override returns (uint256) {}
function approve(address, uint256) public override returns (bool) {}
function transfer(address, uint256) public override returns (bool) {}
}

This allows the attacker to recursively call createSellOrder() while _nextOrderId is incremented but orders[orderId] is not yet written — leading to corrupted order data.

Recommended Mitigation

Apply the Checks-Effects-Interactions pattern:

  • Always update internal state before any external call (especially safeTransferFrom or call()).

🔧 Refactored Secure Code:

function createSellOrder(
address _tokenToSell,
uint256 _amountToSell,
uint256 _priceInUSDC,
uint256 _deadlineDuration
) public returns (uint256) {
if (!allowedSellToken[_tokenToSell]) revert InvalidToken();
if (_amountToSell == 0) revert InvalidAmount();
if (_priceInUSDC == 0) revert InvalidPrice();
if (_deadlineDuration == 0 || _deadlineDuration > MAX_DEADLINE_DURATION) revert InvalidDeadline();
uint256 deadlineTimestamp = block.timestamp + _deadlineDuration;
uint256 orderId = _nextOrderId++;
// ✅ First: Store the order BEFORE calling external transferFrom
orders[orderId] = Order({
id: orderId,
seller: msg.sender,
tokenToSell: _tokenToSell,
amountToSell: _amountToSell,
priceInUSDC: _priceInUSDC,
deadlineTimestamp: deadlineTimestamp,
isActive: true
});
// ✅ Then: Perform external interaction
IERC20(_tokenToSell).safeTransferFrom(msg.sender, address(this), _amountToSell);
emit OrderCreated(orderId, msg.sender, _tokenToSell, _amountToSell, _priceInUSDC, deadlineTimestamp);
return orderId;
}
Updates

Lead Judging Commences

yeahchibyke Lead Judge
8 days ago
yeahchibyke Lead Judge 8 days ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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