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

Reentrancy Vulnerability in Order Amendment

whoami

Root + Impact

Description

  • The amendSellOrder function should follow the checks-effects-interactions pattern to prevent reentrancy attacks during token transfers.

  • The function performs external token transfers before updating the order state, creating a potential reentrancy vulnerability where a malicious token could re-enter the contract.

function amendSellOrder(uint256 _orderId, uint256 _newAmountToSell, uint256 _newPriceInUSDC, uint256 _newDeadlineDuration) public {
// ... validation checks ...
// @> External interaction before state update
if (_newAmountToSell > order.amountToSell) {
uint256 diff = _newAmountToSell - order.amountToSell;
token.safeTransferFrom(msg.sender, address(this), diff);
} else if (_newAmountToSell < order.amountToSell) {
uint256 diff = order.amountToSell - _newAmountToSell;
token.safeTransfer(order.seller, diff);
}
// @> State updates after external interactions
order.amountToSell = _newAmountToSell;
order.priceInUSDC = _newPriceInUSDC;
order.deadlineTimestamp = newDeadlineTimestamp;
}

Risk

Likelihood: Medium

  • Requires malicious ERC20 token to be whitelisted by owner

  • Attack complexity is moderate but feasible

  • Could be exploited if new tokens are added without proper vetting

Impact: High

  • Reentrancy could allow manipulation of order state

  • Potential double-spending or order manipulation attacks

  • Could lead to fund loss or protocol disruption

Proof of Concept

// Malicious token contract
contract MaliciousToken is IERC20 {
OrderBook public orderBook;
function transfer(address to, uint256 amount) external returns (bool) {
// Reenter during amendment
orderBook.amendSellOrder(1, 0, 0, 1 days);
return true;
}
}

Recommended Mitigation

function amendSellOrder(uint256 _orderId, uint256 _newAmountToSell, uint256 _newPriceInUSDC, uint256 _newDeadlineDuration) public {
// ... validation checks ...
+ // Update state before external interactions
+ uint256 oldAmount = order.amountToSell;
+ order.amountToSell = _newAmountToSell;
+ order.priceInUSDC = _newPriceInUSDC;
+ order.deadlineTimestamp = newDeadlineTimestamp;
// Handle token amount changes
- if (_newAmountToSell > order.amountToSell) {
- uint256 diff = _newAmountToSell - order.amountToSell;
+ if (_newAmountToSell > oldAmount) {
+ uint256 diff = _newAmountToSell - oldAmount;
token.safeTransferFrom(msg.sender, address(this), diff);
- } else if (_newAmountToSell < order.amountToSell) {
- uint256 diff = order.amountToSell - _newAmountToSell;
+ } else if (_newAmountToSell < oldAmount) {
+ uint256 diff = oldAmount - _newAmountToSell;
token.safeTransfer(order.seller, diff);
}
- // Update order details
- order.amountToSell = _newAmountToSell;
- order.priceInUSDC = _newPriceInUSDC;
- order.deadlineTimestamp = newDeadlineTimestamp;
}
Updates

Lead Judging Commences

yeahchibyke Lead Judge
about 1 month ago
yeahchibyke Lead Judge about 1 month ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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