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
4 months ago
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.