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

Missing Reentrancy Protection in amendSellOrder Allows State Manipulation with Malicious Tokens

ishwar

Reentrancy Protection Missing (External Calls Before State Updates)

Description

  • Normal behavior: When a contract interacts with external addresses (e.g., ERC20 transfers), it should update critical state variables before making those calls to prevent reentrancy attacks, or use a reentrancy guard modifier.

  • Specific issue: In functions such as amendSellOrder, the contract makes external calls (e.g., safeTransfer, safeTransferFrom) before updating the order's state variables. If a malicious token is allowed (accidentally or via governance), it could attempt a reentrant call during its transfer logic, potentially leading to state inconsistencies or exploitation.

function amendSellOrder(
uint256 _orderId,
uint256 _newAmountToSell,
uint256 _newPriceInUSDC,
uint256 _newDeadlineDuration
) public {
Order storage order = orders[_orderId];
// ...validation...
if (_newAmountToSell > order.amountToSell) {
uint256 diff = _newAmountToSell - order.amountToSell;
token.safeTransferFrom(msg.sender, address(this), diff); // @> External call before state update
} else if (_newAmountToSell < order.amountToSell) {
uint256 diff = order.amountToSell - _newAmountToSell;
token.safeTransfer(order.seller, diff); // @> External call before state update
}
// ...state update happens after...
order.amountToSell = _newAmountToSell;
// ...
}

Risk

Likelihood:

  • Reason 1: If a non-standard or malicious ERC20 token is allowed, it can execute arbitrary code within transfer or transferFrom functions.

  • Reason 2: This is a recognized class of vulnerability, and many security researchers and attackers scan for such patterns in DeFi protocols.

Impact:

  • Impact 1: Reentrancy could allow state manipulation, causing order amounts, ownership, or funds to become inconsistent or exploited.

  • Impact 2: Even if only standard tokens are allowed, this is a bad precedent and may be overlooked in future upgrades or with governance changes.

Proof of Concept

// Attacker creates a malicious ERC20 token where transfer() calls back into amendSellOrder, or another state-changing function
// If amendSellOrder is called, state is not updated before the token makes a callback, possibly leading to double withdrawal or corruption

Explanation: This PoC highlights the risk of external calls before state update, which can be exploited if a malicious token is whitelisted.

Recommended Mitigation

- token.safeTransferFrom(msg.sender, address(this), diff);
- token.safeTransfer(order.seller, diff);
+ // First, update state variables (order.amountToSell, etc.)
+ // Then, perform external calls
+ // Also, consider adding nonReentrant modifier to all functions that interact with external contracts

Explanation: Always update state before external calls, and strongly consider a reentrancy guard for any function that interacts with external contracts.

Updates

Lead Judging Commences

yeahchibyke Lead Judge
10 months ago
yeahchibyke Lead Judge 10 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.

Give us feedback!