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

Inconsistent Allowance Check in `amendSellOrder` Allows Modification of Orders for Disabled Tokens

hosam

Finding Title

Inconsistent Allowance Check in amendSellOrder Allows Modification of Orders for Disabled Tokens

Summary

There is a logic inconsistency between how new orders and existing orders are handled. The createSellOrder function correctly checks if a token is allowed for trading via the allowedSellToken mapping. However, the amendSellOrder function completely omits this check. This allows a user to successfully amend an order (e.g., change its price or amount) even after the owner has disabled the token for trading, leading to inconsistent and unexpected behavior.

Finding Description

The protocol's control flow for managing token allowances is incomplete.

  1. Correct Behavior in createSellOrder:

    // src/OrderBook.sol:113
    if (!allowedSellToken[_tokenToSell]) revert InvalidToken();

  2. Missing Check in amendSellOrder: The amendSellOrder function checks for many conditions (e.g., ownership, order status, deadline) but fails to check if the token is still allowed.

    // src/OrderBook.sol:138
    function amendSellOrder(...) public {
    // ...
    // NO CHECK for allowedSellToken[order.tokenToSell]
    // ...
    }

Scenario: A user creates an order for token XYZ. The owner then disables XYZ. The user can still call amendSellOrder on their XYZ order, while new XYZ orders are blocked.

Impact

This inconsistency weakens the administrative control provided by setAllowedSellToken and creates confusing behavior.

  • Inconsistent State Management: The protocol behaves differently for creating versus amending orders. If a token is disabled, all state-changing operations related to it should logically be disabled.

  • Bypassed Controls: Users can continue to modify orders for tokens that the owner has explicitly tried to freeze from new activity.

Likelihood

High. This inconsistent behavior will occur every time an order is amended for a token that has been subsequently disabled.

Proof of Concept

The following test proves that amendSellOrder can be successfully called on an order whose token has been disabled.

Test File: test/PoC/InconsistentAllowance.t.sol

// SPDX-License-Identifier: SEE LICENSE IN LICENSE
pragma solidity ^0.8.0;
​
import {Test, console2} from "forge-std/Test.sol";
import {OrderBook} from "../../src/OrderBook.sol";
import {MockUSDC} from "../mocks/MockUSDC.sol";
import {MockWETH} from "../mocks/MockWETH.sol";
​
contract InconsistentAllowanceTest is Test {
OrderBook book;
MockWETH weth;
MockUSDC usdc;
MockWETH customToken;
​
address owner = makeAddr("owner");
address seller1 = makeAddr("seller1");
​
function setUp() public {
weth = new MockWETH(18);
usdc = new MockUSDC(6);
customToken = new MockWETH(18); // Using MockWETH as a generic custom token
vm.prank(owner);
book = new OrderBook(address(weth), address(weth), address(weth), address(usdc), owner);
​
customToken.mint(seller1, 10e18);
}
​
/// @notice This PoC proves that an order can be successfully amended even after
/// its corresponding token has been disabled by the owner.
function test_PoC_AmendSucceedsForDisabledToken() public {
// --- Setup: Enable token and create an order ---
vm.prank(owner);
book.setAllowedSellToken(address(customToken), true);
vm.stopPrank();
vm.startPrank(seller1);
customToken.approve(address(book), 3e18);
uint256 orderId = book.createSellOrder(address(customToken), 3e18, 1000e6, 1 days);
vm.stopPrank();
// --- Action: Owner disables the token ---
vm.prank(owner);
book.setAllowedSellToken(address(customToken), false);
vm.stopPrank();
​
console2.log("Token has been disabled by the owner.");
assertFalse(book.allowedSellToken(address(customToken)));
​
// --- Vulnerability: Seller can still amend the order ---
vm.startPrank(seller1);
book.amendSellOrder(orderId, 2e18, 900e6, 1 days); // Attempt to amend
vm.stopPrank();
​
// --- Assertion ---
OrderBook.Order memory orderAfter = book.getOrder(orderId);
assertEq(orderAfter.amountToSell, 2e18, "Amount should have been updated");
assertEq(orderAfter.priceInUSDC, 900e6, "Price should have been updated");
​
console2.log("VULNERABILITY CONFIRMED: Order was successfully amended even though its token is disabled.");
}
}

Successful Test Output:

[PASS] test_PoC_AmendSucceedsForDisabledToken()
Logs:
Token has been disabled by the owner.
VULNERABILITY CONFIRMED: Order was successfully amended even though its token is disabled.

Recommended Mitigation

Add the missing allowedSellToken check to the amendSellOrder function to ensure consistent behavior across the protocol.

// src/OrderBook.sol
function amendSellOrder(
//...
) public {
Order storage order = orders[_orderId];
​
// ... (existing checks) ...
if (block.timestamp >= order.deadlineTimestamp) revert OrderExpired();
​
+ // NEW: Ensure token is still allowed for trading before amending
+ if (!allowedSellToken[order.tokenToSell]) revert InvalidToken();
​
if (_newAmountToSell == 0) revert InvalidAmount();
// ...
}
Updates

Lead Judging Commences

yeahchibyke Lead Judge
12 days ago
yeahchibyke Lead Judge 12 days ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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