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

Potential reentrancy on amendSellOrder

neomartis

Potential reentrancy on amendSellOrder

Description

  • amendSellOrder() is doing state updates AFTER calling safeTransferFrom and safeTransfer functions. While the name "safe" of the function name might suggest that the transfer is safe, it is not. It still does not guard against potential reentrancy issues. Although reentrancy is not possible in the case of "normal" ERC20 tokens (like the core tokens WETH, WBTC, WSOL and USDC), it is possible for ERC777 tokens, which is an extension of ERC20.

function amendSellOrder(
...
​
// Handle token amount changes
if (_newAmountToSell > order.amountToSell) {
// Increasing amount: Transfer additional tokens from seller
uint256 diff = _newAmountToSell - order.amountToSell;
// @audit-issue - EXTERNAL CALLS BEFORE STATE UPDATES
@> token.safeTransferFrom(msg.sender, address(this), diff);
} else if (_newAmountToSell < order.amountToSell) {
// Decreasing amount: Transfer excess tokens back to seller
uint256 diff = order.amountToSell - _newAmountToSell;
// @audit-issue - EXTERNAL CALLS BEFORE STATE UPDATES
@> token.safeTransfer(order.seller, diff);
}
​
// Update order details
order.amountToSell = _newAmountToSell;
order.priceInUSDC = _newPriceInUSDC;
order.deadlineTimestamp = newDeadlineTimestamp;
​
emit OrderAmended(_orderId, _newAmountToSell, _newPriceInUSDC, newDeadlineTimestamp);
}
​
​

Risk

Likelihood: Low

  • While standard ERC20 tokens (WETH, WBTC, WSOL, USDC) are safe, the contract allows custom tokens via setAllowedSellToken()

  • ERC777 tokens and other tokens with hooks can trigger reentrancy

  • The vulnerability exists in a core function that users will frequently call

Impact: High

  • Potential for double-spending of tokens

  • Order state manipulation during reentrancy

  • Complete drainage of contract tokens for affected tokens

  • Compromise of the trust of the orderbook

Proof of Concept

An ERC777 token is deployed that makes external calls to the token recipient, which exposes an attack vector that any user can exploit. When an ERC777 token transfers tokens, it calls the recipient's tokensReceived hook function. If the recipient is a smart contract, this creates a reentrancy opportunity.

Attack Flow:

  1. Attacker creates a malicious smart contract that implements IERC777Recipient

  2. Attacker creates a sell order using the malicious contract as the seller

  3. Attacker calls amendSellOrder() to increase the order amount

  4. During safeTransferFrom(), the ERC777 token calls the malicious contract's tokensReceived hook

  5. The hook re-enters amendSellOrder() before state is updated

  6. Attacker can manipulate order state or drain funds

contract MaliciousSeller is IERC777Recipient {
OrderBook public orderBook;
IERC777 public maliciousToken;
constructor(address _orderBook, address _token) {
orderBook = OrderBook(_orderBook);
maliciousToken = IERC777(_token);
}
function createOrder() external {
// Create a sell order with this contract as seller
}
function attack() external {
// This will trigger the reentrancy
orderBook.amendSellOrder(1, 2000e18, 2000e6, 1 days);
}
// ERC777 hook - this gets called during token transfer
function tokensReceived(
address operator,
address from,
address to,
uint256 amount,
bytes calldata userData,
bytes calldata operatorData
) external override {
// Re-enter the contract before state is updated
if (msg.sender == address(maliciousToken) && to == address(this)) {
// -> REENTER
// Example: Try to amend the same order again
orderBook.amendSellOrder(1, 3000e18, 3000e6, 1 days);
}
}
}

Recommended Mitigation

1. Follow CEI Pattern

function amendSellOrder(
+ // Update order details
+ order.amountToSell = _newAmountToSell;
+ order.priceInUSDC = _newPriceInUSDC;
+ order.deadlineTimestamp = newDeadlineTimestamp;
​
// Handle token amount changes
if (_newAmountToSell > order.amountToSell) {
// Increasing amount: Transfer additional tokens from seller
uint256 diff = _newAmountToSell - order.amountToSell;
token.safeTransferFrom(msg.sender, address(this), diff);
} else if (_newAmountToSell < order.amountToSell) {
// Decreasing amount: Transfer excess tokens back to seller
uint256 diff = order.amountToSell - _newAmountToSell;
token.safeTransfer(order.seller, diff);
}
​
- // Update order details
- order.amountToSell = _newAmountToSell;
- order.priceInUSDC = _newPriceInUSDC;
- order.deadlineTimestamp = newDeadlineTimestamp;
}
​

2. Add ReentrancyGuard (Defense in Depth)

import {ReentrancyGuard} from "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
​
contract OrderBook is Ownable, ReentrancyGuard {
function amendSellOrder(...) public nonReentrant {
// ... implementation
}
}

3. Token Validation

Consider implementing stricter token validation to prevent tokens with hooks from being added to the allowed list.

Updates

Lead Judging Commences

yeahchibyke Lead Judge 4 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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