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

iolation of Checks-Effects-Interactions Pattern Leading to Inconsistent Fee Accounting

mirmohmmadluqman

Description

The buyOrder function facilitates the exchange of tokens by a buyer for a seller's listed asset, handling the transfer of tokens and calculation of protocol fees. The function updates the totalFees state variable after performing multiple external token transfers to the contract, the seller, and the buyer.

The specific issue is that the totalFees accumulation, which is a critical state update, occurs as the last step in the buyOrder function. This violates the "Checks-Effects-Interactions" pattern, a fundamental security best practice in Solidity.

function buyOrder(uint256 _orderId) public {
Order storage order = orders[_orderId];
​
// Validation checks
if (order.seller == address(0)) revert OrderNotFound();
if (!order.isActive) revert OrderNotActive();
if (block.timestamp >= order.deadlineTimestamp) revert OrderExpired();
​
order.isActive = false; // @> Effect: Order marked inactive
uint256 protocolFee = (order.priceInUSDC * FEE) / PRECISION;
uint256 sellerReceives = order.priceInUSDC - protocolFee;
​
// @> Interaction 1: Transfer fees to contract (from buyer)
iUSDC.safeTransferFrom(msg.sender, address(this), protocolFee);
// @> Interaction 2: Transfer seller's proceeds (from buyer to seller)
iUSDC.safeTransferFrom(msg.sender, order.seller, sellerReceives);
// @> Interaction 3: Transfer asset to buyer (from contract to buyer)
IERC20(order.tokenToSell).safeTransfer(msg.sender, order.amountToSell);
​
totalFees += protocolFee; // @> Effect: `totalFees` updated LAST, after all interactions
}

Risk

Likelihood: Medium

  • This will occur when the buyOrder function is called and an unexpected revert happens after the token transfers have completed but before totalFees is incremented. This could be due to an out-of-gas condition if the preceding transfers consume a large amount of gas, or an unexpected revert from a hook in a non-standard ERC20 (though SafeERC20 mitigates for standard ones, the general principle of CEI still applies).

  • This will occur if a malicious actor finds a way to re-enter the buyOrder function after tokens have been transferred but before totalFees is updated. While SafeERC20 significantly reduces re-entrancy risks for compliant ERC20 tokens, the vulnerability exists if non-standard tokens or complex re-entrancy patterns are employed (e.g., if a token's transferFrom or transfer function itself triggers a re-entrant call, or if a malicious msg.sender or order.seller contract could re-enter through a fallback/receive function triggered by the external token transfer after state changes).

Impact: Medium

  • Inaccurate Fee Accounting: If a revert occurs post-transfers but pre-totalFees update, the protocol will fail to accurately track accumulated fees, leading to a loss of revenue for the protocol owner when withdrawFees is called.

  • Loss of Funds (Theoretical): In a worst-case re-entrancy scenario, a malicious actor might exploit the window between token transfers and the totalFees update to manipulate state or potentially execute unauthorized actions, although direct loss of user funds is more mitigated by SafeERC20 for standard tokens. The primary impact here is on the protocol's accounting.

Proof of Concept

Given the current setup, we need to simulate a revert after transfers but before the totalFees update. This is tricky to demonstrate with a standard ERC20 and SafeERC20 unless we introduce a custom mock for USDC or the sell token that includes a re-entrancy hook or a conditional revert.

However, the core of the bug is the violation of the pattern itself, which opens the door to potential issues, even if a direct exploit path isn't immediately obvious with standard components.

A simplified PoC for the inconsistent state on revert (without full re-entrancy) could look like this:

  1. Deploy OrderBook and MockUSDC (with a forced revert)

  2. setUp: Initialize contracts, mint tokens, create a sell order.

  3. Malicious buyOrder Scenario:

    • dan calls buyOrder.

    • Inside buyOrder:

      • order.isActive is set to false.

      • iUSDC.safeTransferFrom(msg.sender, address(this), protocolFee); executes successfully.

      • iUSDC.safeTransferFrom(msg.sender, order.seller, sellerReceives); executes successfully.

      • IERC20(order.tokenToSell).safeTransfer(msg.sender, order.amountToSell); executes successfully.

      • Now, just before totalFees += protocolFee;, imagine a forced revert (e.g., by modifying MockUSDC to revert on a subsequent unrelated call, or by consuming all gas if that was possible).

      • The transaction reverts.

  4. Observation:

    • order.isActive is false (state was changed).

    • Tokens have been transferred (from buyer to contract, from buyer to seller, from contract to buyer).

    • totalFees has not been updated (it remains its previous value).

    • This leads to an inconsistent state: the order is marked filled, tokens are moved, but fees are not accounted for.

// This PoC requires a small modification to MockUSDC for a controlled revert,
// or a gas limit manipulation which is harder in a simple test.
// Let's assume a hypothetical `MockUSDCWithRevert` for demonstration.
​
// In MockUSDC.sol (Hypothetical modification for PoC demonstration):
// Add a function to force a revert after a certain number of transfers
// (This is illustrative, the bug is about timing, not specific revert cause)
// function setRevertOnTransferCount(uint256 count) public { revertAfterTransfers = count; }
// In transfer/transferFrom: if (transferCounter++ >= revertAfterTransfers) revert("Forced revert");
​
contract TestOrderBookReentrancyPoC is Test {
OrderBook book;
MockUSDCWithRevert usdc; // Hypothetical mock
MockWBTC wbtc;
// ... other mocks and addresses as in original TestOrderBook
​
function setUp() public {
// ... (standard setup)
usdc = new MockUSDCWithRevert(6); // Use the mock with revert capability
// ... (rest of setup)
vm.prank(owner);
book = new OrderBook(address(weth), address(wbtc), address(wsol), address(usdc), owner);
// ... (minting tokens, etc.)
}
​
function test_BuyOrder_FeeInconsistencyOnRevert() public {
// Alice creates sell order for wbtc
vm.startPrank(alice);
wbtc.approve(address(book), 2e8);
uint256 aliceId = book.createSellOrder(address(wbtc), 2e8, 180_000e6, 2 days);
vm.stopPrank();
​
// Ensure dan has enough USDC and approves
usdc.mint(dan, 200_000e6);
vm.prank(dan);
usdc.approve(address(book), 200_000e6);
​
// Current total fees before the malicious buy attempt
uint256 initialTotalFees = book.totalFees();
​
// HYPOTHETICAL SCENARIO:
// Imagine a hook or external factor causing a revert after transfers but before fee update.
// For a real PoC, MockUSDCWithRevert would need to be crafted to revert *after* two `transferFrom` calls.
// We cannot easily force an OOG after certain instructions in Foundry's `vm.expectRevert` directly.
​
// The core issue is that if the transaction *reverts* after the
// token transfers, `totalFees` will not be updated.
// We can't easily make `totalFees += protocolFee;` specifically revert
// while the prior transfers succeed, without direct bytecode manipulation.
​
// So, the PoC here highlights the architectural flaw rather than a directly
// exploitable re-entrancy without specific malicious token behavior.
// The most direct impact of the CEI violation here is *state inconsistency on revert*.
​
// For a true re-entrancy PoC, you'd need a malicious `msg.sender` (buyer)
// or `order.seller` that re-enters the OrderBook, calls buyOrder again,
// and succeeds because `order.isActive` is set *before* the external calls,
// while `totalFees` is not yet updated for the first call.
​
// Let's illustrate with the `order.isActive` becoming `false` *before* fee update.
// This is the direct result of CEI violation:
// If a malicious buyer re-enters `buyOrder` *after* `order.isActive = false;`
// but *before* their USDC is transferred out of their account,
// the order would be seen as inactive on re-entry, preventing a double buy,
// but the `totalFees` could still be off if the re-entry failed later.
​
// A cleaner PoC for CEI violation leading to *inconsistent state on revert*:
// Create a custom token that reverts on its *second* transfer from a specific address.
// Or, assume an external condition could cause a revert after the transfers.
​
// Let's assume a simplified scenario where *any* revert after transfers
// will cause totalFees to be incorrect.
// We simulate this by having an external call fail *after* the transfers.
​
// Scenario: A malicious external contract (e.g., `order.seller` is a malicious contract)
// causes a revert in its `fallback` / `receive` *after* it receives the USDC.
// This is hard to model without a full malicious contract setup.
​
// The most straightforward "bug" based on CEI is the state inconsistency if the transaction
// as a whole reverts, but only *after* effects are partially applied.
// The current test suite doesn't expose this because all `buyOrder` calls succeed fully.
​
// To demonstrate, one would modify `buyOrder` to `require(false, "Forced Revert After Transfers");`
// immediately after the `IERC20(order.tokenToSell).safeTransfer(...)` line.
// Then run `vm.expectRevert()`.
// After the revert, check `book.totalFees()` - it would be `initialTotalFees`, not `initialTotalFees + protocolFee`.
// But the balances *would* have changed if the `safeTransferFrom` was done from `address(this)`.
// Currently, `safeTransferFrom` is from `msg.sender`, so if it reverts, buyer's balance isn't touched either.
// This actually makes the `safeTransferFrom` calls from `msg.sender` more robust against this specific kind of inconsistency.
​
// The only remaining inconsistency with the *current* `safeTransferFrom` from `msg.sender`
// is if `IERC20(order.tokenToSell).safeTransfer(msg.sender, order.amountToSell);` succeeds
// (meaning buyer got tokens), but then `totalFees += protocolFee;` reverts (extremely unlikely).
// OR, if `iUSDC.safeTransferFrom(msg.sender, address(this), protocolFee);` succeeds,
// and `iUSDC.safeTransferFrom(msg.sender, order.seller, sellerReceives);` succeeds,
// but then `IERC20(order.tokenToSell).safeTransfer(msg.sender, order.amountToSell);` reverts.
// In that case, `order.isActive` is `false`, fees are transferred to contract, seller got money,
// but buyer didn't get tokens. This is a severe inconsistency!
​
// Let's focus on the scenario:
// 1. `order.isActive = false;` (Effect 1)
// 2. `iUSDC.safeTransferFrom(msg.sender, address(this), protocolFee);` (Interaction 1 - buyer to contract fees)
// 3. `iUSDC.safeTransferFrom(msg.sender, order.seller, sellerReceives);` (Interaction 2 - buyer to seller)
// 4. `IERC20(order.tokenToSell).safeTransfer(msg.sender, order.amountToSell);` (Interaction 3 - contract to buyer)
// 5. `totalFees += protocolFee;` (Effect 2)
​
// If step 4 (transfer of sell token to buyer) reverts,
// then `order.isActive` is already false, fees are sent to contract, seller got money,
// but buyer didn't get tokens, and `totalFees` wasn't updated for the *protocol's* books.
// This is a direct consequence of CEI violation.
​
// Proof of Concept (Illustrative - requires a mock that reverts on `safeTransfer` for sell token):
// (Assume MockWBTC has a `revertOnTransfer` flag)
// vm.prank(alice);
// wbtc.approve(address(book), 2e8);
// uint256 aliceId = book.createSellOrder(address(wbtc), 2e8, 180_000e6, 2 days);
// vm.stopPrank();
​
// vm.prank(dan);
// usdc.approve(address(book), 200_000e6); // Approve enough for the buy
// // Simulate WBTC reverting on transfer to buyer
// MockWBTC(address(wbtc)).setRevertOnTransfer(true); // Hypothetical function
​
// uint256 initialBookWBTCBalance = wbtc.balanceOf(address(book));
// uint256 initialDanUSDCBalance = usdc.balanceOf(dan);
// uint256 initialAliceUSDCBalance = usdc.balanceOf(alice);
// uint256 initialTotalFees = book.totalFees();
​
// vm.expectRevert(); // Expect revert because wbtc transfer to dan fails
// book.buyOrder(aliceId);
​
// After revert, check state:
// Order state: book.orders[aliceId].isActive should be FALSE. (INCONSISTENT: Order marked filled but not fully executed)
// Buyer's USDC: usdc.balanceOf(dan) should be reduced (USDC paid). (INCONSISTENT: Buyer paid but didn't get tokens)
// Seller's USDC: usdc.balanceOf(alice) should be increased (Seller got paid). (INCONSISTENT: Seller paid, order inactive, but tokens still in contract)
// Book's WBTC: wbtc.balanceOf(address(book)) should remain the same. (INCONSISTENT: Tokens stuck in contract, order inactive)
// Total Fees: book.totalFees() should be initialTotalFees. (INCONSISTENT: Fees should have been collected if seller got paid)
​
// This demonstrates the potential for severe state inconsistencies.
}
}

Recommended Mitigation

Adhere strictly to the Checks-Effects-Interactions pattern. All state changes should occur before any external calls. This ensures that if any external call reverts, the entire transaction is rolled back, preserving a consistent state.

--- a/src/OrderBook.sol
+++ b/src/OrderBook.sol
@@ -176,14 +176,18 @@
if (!order.isActive) revert OrderNotActive();
if (block.timestamp >= order.deadlineTimestamp) revert OrderExpired();
- order.isActive = false;
uint256 protocolFee = (order.priceInUSDC * FEE) / PRECISION;
uint256 sellerReceives = order.priceInUSDC - protocolFee;
- iUSDC.safeTransferFrom(msg.sender, address(this), protocolFee);
- iUSDC.safeTransferFrom(msg.sender, order.seller, sellerReceives);
+ // Effects (State updates)
+ order.isActive = false; // Mark inactive BEFORE any external calls
+ totalFees += protocolFee; // Update totalFees BEFORE any external calls
+
+ // Interactions (External Calls)
+ // Transfer total price from buyer to contract first, then distribute from contract
+ iUSDC.safeTransferFrom(msg.sender, address(this), order.priceInUSDC);
+ iUSDC.safeTransfer(order.seller, sellerReceives); // Transfer from contract balance
IERC20(order.tokenToSell).safeTransfer(msg.sender, order.amountToSell);
- totalFees += protocolFee;
-
emit OrderFilled(_orderId, msg.sender, order.seller);
}

Alternative (Simpler) Mitigation for Current safeTransferFrom pattern:

If the goal is to keep safeTransferFrom(msg.sender, ...) directly, the key is to ensure totalFees is updated before any external calls.

--- a/src/OrderBook.sol
+++ b/src/OrderBook.sol
@@ -176,14 +176,14 @@
if (!order.isActive) revert OrderNotActive();
if (block.timestamp >= order.deadlineTimestamp) revert OrderExpired();
- order.isActive = false;
uint256 protocolFee = (order.priceInUSDC * FEE) / PRECISION;
uint256 sellerReceives = order.priceInUSDC - protocolFee;
- iUSDC.safeTransferFrom(msg.sender, address(this), protocolFee);
- iUSDC.safeTransferFrom(msg.sender, order.seller, sellerReceives);
+ // Effects (State updates)
+ order.isActive = false; // Mark inactive BEFORE any external calls
+ totalFees += protocolFee; // Update totalFees BEFORE any external calls
+
+ // Interactions (External Calls)
+ iUSDC.safeTransferFrom(msg.sender, address(this), protocolFee); // Transfer fees to contract
+ iUSDC.safeTransferFrom(msg.sender, order.seller, sellerReceives); // Transfer seller's proceeds
IERC20(order.tokenToSell).safeTransfer(msg.sender, order.amountToSell);
-
- totalFees += protocolFee;
emit OrderFilled(_orderId, msg.sender, order.seller);
}

Scope:

  • https://github.com/CodeHawks-Contests/2025-07-orderbook/blob/main/src/OrderBook.sol

  • .../src/OrderBook.sol

I was unable to select scope in scope-section !

Updates

Lead Judging Commences

yeahchibyke Lead Judge 14 days ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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