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

Reentrancy Attack

abhishek2416

Root + Impact

The createSellOrder() function makes an external call to a user-supplied ERC20 token (safeTransferFrom). If the token is malicious or non-standard, it can reenter the function during the transfer, which occurs before all state updates are finalized. This opens the door for reentrancy, which could corrupt internal contract state (e.g., _nextOrderId, orders) and lead to multiple unintended orders being created in a single transaction.

The impact includes:

  • Corrupted order book state

  • Unexpected multiple orders

  • Inconsistent accounting

  • Denial-of-service or griefing attacks

Description

  • Under normal behavior, createSellOrder() allows users to list tokens for sale by transferring them to the contract and recording an order in storage. It assumes that the call to safeTransferFrom() will not trigger any unexpected side effects.

  • The issue arises because _tokenToSell is user-controlled. If it is a malicious token contract, its transferFrom() implementation can reenter the contract, triggering createSellOrder() again before the original call finishes. This violates the assumptions around order sequencing and state updates, potentially causing duplicate or corrupted orders.

function createSellOrder(...) public returns (uint256) {
...
uint256 orderId = _nextOrderId++; // @> Vulnerable state change
...
IERC20(_tokenToSell).safeTransferFrom(...) // @> External call to user-controlled token
...
orders[orderId] = Order(...); // @> Could be manipulated or duplicated via reentrancy
}
​

Risk

Likelihood:

  • This will occur whenever a malicious ERC20 token is passed to createSellOrder() and its transferFrom() is designed to reenter the contract.

  • It’s especially likely in unpermissioned systems or on mainnet, where tokens may not conform to standard behavior.

Impact:

  • Allows multiple unintended orders to be created within a single transaction.

  • Can corrupt _nextOrderId tracking and the orders mapping.

  • Could lead to incorrect or overwritten order data, impacting buyers and the protocol’s integrity.

Proof of Concept

In this case, a single call to createSellOrder() causes multiple nested calls to execute — leading to multiple unexpected entries in the order book.

contract MaliciousToken is ERC20 {
address public orderBook;
​
constructor(address _orderBook) ERC20("Malicious", "MAL") {
orderBook = _orderBook;
}
​
function transferFrom(address from, address to, uint256 amount) public override returns (bool) {
_transfer(from, to, amount);
​
// Reentrant call back into the vulnerable function
OrderBook(orderBook).createSellOrder(address(this), 1, 1000e6, 1 days);
​
return true;
}
}
​

Recommended Mitigation

Add the nonReentrant modifier using OpenZeppelin’s ReentrancyGuard to prevent reentrant calls during execution.

nonReentrant ensures that no reentrant call to createSellOrder() can occur while it’s already executing. This prevents state corruption and guarantees the integrity of order creation, even when interacting with non-standard or malicious tokens.

+ import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
​
contract OrderBook is ReentrancyGuard {
...
​
- function createSellOrder(...) public returns (uint256) {
+ function createSellOrder(...) public nonReentrant returns (uint256) {
...
}
​
Updates

Lead Judging Commences

yeahchibyke Lead Judge
about 2 months ago
yeahchibyke Lead Judge about 1 month ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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