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

Redundant Status Logic + Code Complexity

blee

Description

  • Status determination logic should be clean and straightforward for better maintainability.

  • The getOrderDetailsString function contains redundant conditional logic that makes the code unnecessarily complex.

function getOrderDetailsString(uint256 _orderId) public view returns (string memory details) {
Order storage order = orders[_orderId];
string memory status = order.isActive
? (block.timestamp < order.deadlineTimestamp ? "Active" : "Expired (Active but past deadline)")
: "Inactive (Filled/Cancelled)";
if (order.isActive && block.timestamp >= order.deadlineTimestamp) {
status = "Expired (Awaiting Cancellation)";
} else if (!order.isActive) {
status = "Inactive (Filled/Cancelled)";
} else {
status = "Active";
}
}

Risk

Likelihood:

  • Every time the function is called

  • When maintaining or modifying the codebase

  • When other developers try to understand the logic

Impact:

  • Increased code complexity and reduced readability

  • Higher chance of introducing bugs during modifications

  • Wasted gas on redundant condition checks

Proof of Concept

Code Redundancy Analysis: This demonstrates the unnecessary complexity in status determination.

// The ternary operator already handles all cases, but then redundant if-else follows:
// Case 1: order.isActive = true, block.timestamp < deadline → "Active"
// Case 2: order.isActive = true, block.timestamp >= deadline → "Expired (Active but past deadline)"
// Case 3: order.isActive = false → "Inactive (Filled/Cancelled)"
​
// The subsequent if-else block repeats the same logic unnecessarily

Complexity issues:

  • Redundant conditions: Same logic is checked twice using different patterns

  • Inconsistent status strings: Ternary and if-else blocks return different strings for same conditions

  • Maintenance burden: Changes require updating multiple locations

  • Gas inefficiency: Extra conditional checks consume unnecessary gas

  • Readability: Mixed conditional patterns make code harder to understand

Recommended Mitigation

function getOrderDetailsString(uint256 _orderId) public view returns (string memory details) {
Order storage order = orders[_orderId];
if (order.seller == address(0)) revert OrderNotFound();
​
string memory tokenSymbol;
// ... token symbol logic ...
​
- string memory status = order.isActive
- ? (block.timestamp < order.deadlineTimestamp ? "Active" : "Expired (Active but past deadline)")
- : "Inactive (Filled/Cancelled)";
- if (order.isActive && block.timestamp >= order.deadlineTimestamp) {
- status = "Expired (Awaiting Cancellation)";
- } else if (!order.isActive) {
- status = "Inactive (Filled/Cancelled)";
- } else {
- status = "Active";
- }
​
+ string memory status;
+ if (!order.isActive) {
+ status = "Inactive (Filled/Cancelled)";
+ } else if (block.timestamp >= order.deadlineTimestamp) {
+ status = "Expired (Awaiting Cancellation)";
+ } else {
+ status = "Active";
+ }
​
// ... rest of the function
}

Why this works:

  • Single source of truth: Status is determined in one place using consistent logic

  • Clear precedence: Conditions are checked in logical order (inactive → expired → active)

  • Maintainable: Changes only need to be made in one location

  • Readable: Standard if-else pattern is more familiar to developers

  • Efficient: Eliminates redundant condition checks

Updates

Lead Judging Commences

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