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

## [M-#] Disabled Allowed Tokens Can Still Be Amended (Missing Validation + Invalid Order Modifications)

mcqueengodice

[M-#] Disabled Allowed Tokens Can Still Be Amended (Missing Validation + Invalid Order Modifications)

Description:

The OrderBook::amendSellOrder function does not check for OrderBook::allowedSellToken[order.tokenToSell], allowing sellers to amend orders for tokens disabled via OrderBook::setAllowedSellToken(_token, false) possibly because the admin has considered them compromised. While createSellOrder enforces the allowedSellToken check, amendSellOrder does not, permitting modifications to orders for disabled tokens.

Impact:

Sellers can amend orders for disallowed tokens, violating token restrictions and potentially leading to invalid order states. This could allow manipulation of orders that should no longer be active, undermining the order book's integrity and risking unexpected behavior whereby a malicious seller can increase the amounts of tokens on sell and since OrderBook::buyOrder function does not check for it as well, it'll be bought by a victim; a buyer buys the huge amount of compromised token unbeknownst to him in subsequent trades.

Proof of Concept:

  1. Deploy the OrderBook contract with valid addresses for wETH, wBTC, wSOL, and USDC.

  2. Alice creates a sell order for wETH via createSellOrder(address(iWETH), 1e18, 2500e6, 1 days), transferring 1 wETH to the contract.

  3. Owner disables wETH by calling setAllowedSellToken(address(iWETH), false).

  4. Alice amends the wETH order via amendSellOrder(orderId, 0.5e18, 1250e6, 1 days), which succeeds, updating the order despite wETH being disabled.

Add this to test/TestOrderBook.t.sol

PoC Test Code:
function test_amendDisabledToken() public {
// Bob creates a sell order for wETH
vm.startPrank(bob);
weth.approve(address(book), 1e18);
uint256 orderId = book.createSellOrder(address(weth), 1e18, 2500e6, 1 days);
vm.stopPrank();
assertEq(weth.balanceOf(address(book)), 1e18, "Contract should hold 1 wETH");
​
// Owner disables wETH
vm.prank(owner);
book.setAllowedSellToken(address(weth), false);
assertFalse(book.allowedSellToken(address(weth)), "wETH should be disabled");
​
// Bob amends wETH order
vm.startPrank(bob);
book.amendSellOrder(orderId, 0.5e18, 1250e6, 1 days);
vm.stopPrank();
​
// Verify order was amended
OrderBook.Order memory order = book.getOrder(orderId);
assertEq(order.amountToSell, 0.5e18, "Order amount should be updated to 0.5 wETH");
assertEq(order.priceInUSDC, 1250e6, "Order price should be updated to 1250 USDC");
assertEq(weth.balanceOf(address(book)), 0.5e18, "Contract should hold 0.5 wETH");
assertEq(weth.balanceOf(bob), 1.5e18, "Bob should receive 0.5 wETH back + an extra 1 wETH he has before");
}

Recommended Mitigation:

Add a check in OrderBook::amendSellOrder to ensure the token is still allowed in OrderBook::allowedSellToken.

Recommended solution:
function amendSellOrder(
uint256 _orderId,
uint256 _newAmountToSell,
uint256 _newPriceInUSDC,
uint256 _newDeadlineDuration
) public {
Order storage order = orders[_orderId];
​
// Validation checks
if (order.seller == address(0)) revert OrderNotFound();
if (order.seller != msg.sender) revert NotOrderSeller();
if (!order.isActive) revert OrderAlreadyInactive();
if (block.timestamp >= order.deadlineTimestamp) revert OrderExpired();
+ if (!allowedSellToken[order.tokenToSell]) revert InvalidToken();
if (_newAmountToSell == 0) revert InvalidAmount();
if (_newPriceInUSDC == 0) revert InvalidPrice();
if (_newDeadlineDuration == 0 || _newDeadlineDuration > MAX_DEADLINE_DURATION) revert InvalidDeadline();
​
uint256 newDeadlineTimestamp = block.timestamp + _newDeadlineDuration;
IERC20 token = IERC20(order.tokenToSell);
​
// Handle token amount changes
if (_newAmountToSell > order.amountToSell) {
uint256 diff = _newAmountToSell - order.amountToSell;
token.safeTransferFrom(msg.sender, address(this), diff);
} else if (_newAmountToSell < order.amountToSell) {
uint256 diff = order.amountToSell - _newAmountToSell;
token.safeTransfer(order.seller, diff);
}
​
// Update order details
order.amountToSell = _newAmountToSell;
order.priceInUSDC = _newPriceInUSDC;
order.deadlineTimestamp = newDeadlineTimestamp;
​
emit OrderAmended(_orderId, _newAmountToSell, _newPriceInUSDC, newDeadlineTimestamp);
}
Updates

Lead Judging Commences

yeahchibyke Lead Judge
about 1 month ago
yeahchibyke Lead Judge about 1 month ago
Submission Judgement Published
Invalidated
Reason: Design choice
Assigned finding tags:

Current orders with delisted tokens can still be amended

Any current order with a delisted token can be amended.

Support

FAQs

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