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

`OrderBook::getOrderDetailsString` contains redundant and verbose status logic

prongsdev125

OrderBook::getOrderDetailsString contains redundant and verbose status logic

Description

  • The getOrderDetailsString function contains redundant status determination logic that makes the code verbose and harder to maintain.

  • The function first correctly calculates the status using a ternary operator, then immediately overwrites it with redundant if-else logic that produces the same results.

function getOrderDetailsString(uint256 _orderId) public view returns (string memory details) {
// ... other code ...
string memory status = order.isActive
? (block.timestamp < order.deadlineTimestamp ? "Active" : "Expired (Active but past deadline)")
: "Inactive (Filled/Cancelled)";
@> // @audit no need this part
@> if (order.isActive && block.timestamp >= order.deadlineTimestamp) {
@> status = "Expired (Awaiting Cancellation)";
@> } else if (!order.isActive) {
@> status = "Inactive (Filled/Cancelled)";
@> } else {
@> status = "Active";
@> }
// ... rest of function ...
}

Risk

Likelihood: Code quality issue present in every call to the function

Impact:

  • Reduced code readability and maintainability

  • Potential for bugs if logic is modified in one place but not the other

  • Increased gas consumption due to unnecessary operations

  • Makes the codebase harder to understand and audit

Recommended Mitigation

Remove the redundant if-else chain as the ternary operator already handles all cases correctly:

function getOrderDetailsString(uint256 _orderId) public view returns (string memory details) {
// ... other code ...
string memory status = order.isActive
? (block.timestamp < order.deadlineTimestamp ? "Active" : "Expired (Active but past deadline)")
: "Inactive (Filled/Cancelled)";
- // @audit no need this part
- if (order.isActive && block.timestamp >= order.deadlineTimestamp) {
- status = "Expired (Awaiting Cancellation)";
- } else if (!order.isActive) {
- status = "Inactive (Filled/Cancelled)";
- } else {
- status = "Active";
- }
// ... rest of function ...
}

This simplification maintains the same functionality while improving code clarity and reducing gas costs.

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.