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

L01-logic to compute the status in getOrderDetailsString is both redundant and inconsistent

accessdenied

Root + Impact

Description

  • The function getOrderDetailsString returns a user-readable string representing the state of a given order. The status field is intended to reflect whether an order is active, inactive, or expired.

  • The logic for computing the status string is both redundant and inconsistent. The status is initially assigned using a ternary operator, then completely overwritten by an if-else block, making the initial computation unnecessary. This can lead to confusion for maintainers, and could result in misleading or inconsistent outputs if not properly maintained.

// Root cause in the codebase with @> marks to highlight the relevant section
function getOrderDetailsString(uint256 _orderId) public view returns (string memory details) {
Order storage order = orders[_orderId];
if (order.seller == address(0)) revert OrderNotFound(); // Check if order exists
​
string memory tokenSymbol;
if (order.tokenToSell == address(iWETH)) {
tokenSymbol = "wETH";
} else if (order.tokenToSell == address(iWBTC)) {
tokenSymbol = "wBTC";
} else if (order.tokenToSell == address(iWSOL)) {
tokenSymbol = "wSOL";
}
@> assign value to status
string memory status = order.isActive
? (block.timestamp < order.deadlineTimestamp ? "Active" : "Expired (Active but past deadline)")
: "Inactive (Filled/Cancelled)";
​
@> this block will overwrite the previous assignement in any case
if (order.isActive && block.timestamp >= order.deadlineTimestamp) {
status = "Expired (Awaiting Cancellation)";
} else if (!order.isActive) {
status = "Inactive (Filled/Cancelled)";
} else {
status = "Active";
}

Risk

Likelihood:

  • This code is always executed when someone calls getOrderDetailsString, especially via UI or external API display integrations.

  • The redundant logic is unlikely to break functionality but introduces the risk of future bugs if someone updates only one part of the logic.

Impact:

  • Inconsistent or confusing status outputs to users, especially if the ternary and conditional logic are not kept in sync.

  • Wasted gas during execution due to unused string concatenation and unnecessary computation if performed on-chain (unlikely nevertheless).

  • Increase the code size uncessary and therefore the deployment code

Proof of Concept

Risk of future bug example:

  1. A frontend integrates with getOrderDetailsString to show order status.

  2. A developer later modifies only the ternary part to introduce a new label (e.g., "Soon to Expire") but forgets to update the if-else block, causing a mismatch and misleading output.


Recommended Mitigation

  • Remove the useless first assignement

  • Improve the if block
    Starts with the most definitive check — if the order is inactive — reducing nested condition complexity.

if (!order.isActive) {
status = "Inactive (Filled or Cancelled)";
} else if (block.timestamp >= order.deadlineTimestamp) {
status = "Expired (Awaiting Cancellation)";
} else {
status = "Active";
}

Updates

Lead Judging Commences

yeahchibyke Lead Judge about 1 month ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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