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

Lack of Input Validation in `amendSellOrder`

mukulkolpe

Lack of Input Validation in amendSellOrder

Description

  • The amendSellOrder function allows users to modify existing sell orders by changing amount, price, or deadline

  • The function does not validate whether the new values are different from the current values, allowing users to waste gas on no-op amendments

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 (_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);
​
// @> No check if new values are different from current values
// Token amount changes (even if amount is the same)
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);
}
// @> If amounts are equal, no transfer occurs but state is still updated
​
// Update order details (even if values are unchanged)
order.amountToSell = _newAmountToSell;
order.priceInUSDC = _newPriceInUSDC;
order.deadlineTimestamp = newDeadlineTimestamp;
​
emit OrderAmended(_orderId, _newAmountToSell, _newPriceInUSDC, newDeadlineTimestamp);
}

Risk

Likelihood: Low

  • Users would need to intentionally or accidentally submit amendments with identical values

  • No economic incentive exists for users to waste their own gas

  • More likely to occur due to user error or poorly designed front-end interfaces

Impact: Low

  • Users waste gas on unnecessary state writes and events

  • No security implications or fund loss

  • Slight network resource consumption for no-op operations

  • Potential user experience degradation if users repeatedly make ineffective amendments

Recommended Mitigation

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 (_newAmountToSell == 0) revert InvalidAmount();
if (_newPriceInUSDC == 0) revert InvalidPrice();
if (_newDeadlineDuration == 0 || _newDeadlineDuration > MAX_DEADLINE_DURATION) revert InvalidDeadline();
​
+ // Prevent wasteful amendments with identical values
+ uint256 newDeadlineTimestamp = block.timestamp + _newDeadlineDuration;
+ if (_newAmountToSell == order.amountToSell &&
+ _newPriceInUSDC == order.priceInUSDC &&
+ newDeadlineTimestamp == order.deadlineTimestamp) {
+ revert("No changes detected");
+ }
​
- 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);
}
​
+ // Add custom error for better UX
+ error NoChangesDetected();

Alternative Approach (More Flexible):

+ // Allow deadline-only amendments but prevent completely identical amendments
+ if (_newAmountToSell == order.amountToSell &&
+ _newPriceInUSDC == order.priceInUSDC) {
+ // Allow if deadline is meaningfully different (e.g., > 1 hour difference)
+ if (newDeadlineTimestamp <= order.deadlineTimestamp + 3600) {
+ revert("Insufficient changes detected");
+ }
+ }

Benefits of Mitigation:

  1. Reduces gas waste - Prevents users from paying for no-op operations

  2. Improves user experience - Clear feedback when no changes are made

  3. Reduces network load - Fewer unnecessary state updates and events

  4. Maintains flexibility - Still allows legitimate use cases like deadline extensions

Updates

Lead Judging Commences

yeahchibyke Lead Judge 15 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.