Submission Details
Severity:
[M-1] Missing Expiry Check in cancelSellOrder() Leads to Indefinite Token Locking
Root + Impact
Description
-
Describe the normal behavior in one or more sentences
-
Explain the specific issue or problem in one or more sentences
## Summary
The OrderBook contract has inconsistent expiry logic that can lead to permanent
token locking. While expired orders cannot be bought (correctly blocked), they
remain in an "active" state and can only be cancelled by the seller. If a seller
forgets to cancel an expired order or loses access to their wallet, their tokens
remain locked in the contract indefinitely.
## Vulnerability Details
The contract implements expiry checks inconsistently across different functions:
1. --buyOrder() - Correctly prevents buying expired orders with `OrderExpired()` revert
2. --amendSellOrder() - Correctly prevents amending expired orders with `OrderExpired()` revert
3. --cancelSellOrder()** - **MISSING expiry check** - allows cancellation of expired orders
The `cancelSellOrder()` function only checks if the order is active (`isActive == true`) but doesn't
verify if the order has expired. This means:
- Expired orders remain in an "active" state (`isActive = true`)
- Expired orders cannot be bought (correctly blocked by `buyOrder()`)
- Expired orders cannot be amended (correctly blocked by `amendSellOrder()`)
- **Only the original seller can cancel expired orders**
- No mechanism exists for automatic cancellation or public cancellation
- Tokens can be locked forever if seller is unavailable
## Impact
- Permanent token locking: Seller's tokens can be locked indefinitely
- Poor user experience: Users must remember to cancel expired orders
- Lost funds: If seller loses wallet access, tokens are unrecoverable
- Contract bloat: Accumulation of expired but uncancelled orders
- Inconsistent state: Orders remain "active" even after expiry
## Affected Functions
- `cancelSellOrder()` - Lines 176-192 (missing expiry check)
- `getOrderDetailsString()` - Lines 220-250 (shows confusing status for expired orders)
Risk
Likelihood:
-
Reason 1 // Describe WHEN this will occur (avoid using "if" statements)
-
Reason 2
Impact:
-
Impact 1
-
Impact 2
Proof of Concept
# Proof of Concept
1. User creates order with 1-day deadline
2. Order expires after 1 day
3. Order remains "active" but cannot be bought
4. Only seller can cancel the expired order
5. If seller forgets or loses wallet access, tokens are locked forever
## The Problem
The `cancelSellOrder()` function doesn't check if an order has expired:
```solidity
// Lines 176-192: cancelSellOrder()
function cancelSellOrder(uint256 _orderId) public {
// ... validation checks ...
if (!order.isActive) revert OrderAlreadyInactive(); // Only checks if active, not if expired
// No expiry check here!
order.isActive = false;
IERC20(order.tokenToSell).safeTransfer(order.seller, order.amountToSell);
}
```
## What Happens
- Expired orders remain `isActive = true`
- Cannot be bought** (correctly blocked by `buyOrder()`)
- Cannot be amended** (correctly blocked by `amendSellOrder()`)
- Can only be cancelled by seller** (the problem)
- If seller unavailable → tokens locked forever
Recommended Mitigation
# Mitigation
## What's Wrong
The `cancelSellOrder()` function doesn't check if an order has expired, creating inconsistent expiry logic. While expired orders cannot be bought or amended, they remain active and can only be cancelled by the seller, potentially leading to permanent token locking.
**Vulnerable Code:**
// Lines 176-192: cancelSellOrder()
function cancelSellOrder(uint256 _orderId) 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(); // Only checks if active, not if expired
// No expiry validation here!
order.isActive = false;
IERC20(order.tokenToSell).safeTransfer(order.seller, order.amountToSell);
}
--The Problem:
Expired orders remain `isActive = true` and can only be cancelled by the seller.
If the seller forgets or loses wallet access, tokens are locked forever.
## How to Fix
### Solution 1: Add Expiry Check (Recommended)
function cancelSellOrder(uint256 _orderId) 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(); // Add expiry check
order.isActive = false;
IERC20(order.tokenToSell).safeTransfer(order.seller, order.amountToSell);
}
### Solution 2: Automatically cleanup orders
// In buyOrder() - automatically clean up expired orders
function buyOrder(uint256 _orderId) public {
Order storage order = orders[_orderId];
// Auto-cleanup expired orders
if (block.timestamp >= order.deadlineTimestamp && order.isActive) {
order.isActive = false;
IERC20(order.tokenToSell).safeTransfer(order.seller, order.amountToSell);
emit OrderCancelled(_orderId, order.seller);
revert OrderExpired();
}
// ... rest of function
}
Updates
Lead Judging Commences
yeahchibyke 8 days ago
Submission Judgement Published Assigned finding tags:
Expired orders can cause backlog
By design only `seller` can call `cancelSellOrder()` on their `order`. But when an `order` expires, and the `seller` doesn't have access to the protocol, the expired `order `should be be able to be cancelled by an `admin`.
Rewards breakdown
nSLOC
217This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.