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

[L-03] Unidentical repetition of status checks in getOrderDetailsString() causes one result to be shadowed

soarinskysagar

Root + Impact

Description

  • The getOrderDetailsString() function checks for the order status

  • However, the logi for status checking has been placed twice with different outputs, shadowing one of the logics and wasting gas.

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";
}

As we can see, status is getting checked twice and for expired orders we have different strings - "Expired (Active but past deadline)" and "Expired (Awaiting Cancellation)". This can lead to irregularities on frontends that depend on this function

Risk

Likelihood:

  • Whenever getOrderDetailsString() is called

Impact:

  • unexpected output where output from first logic is expected

Proof of Concept

Place the following function into TestOrderbook.t.sol and run with forge test --mt test_wrongTokenStatus -vv:

function test_wrongTokenStatus() public {
weth.mint(alice, 10e18);
​
vm.startPrank(alice);
weth.approve(address(book), 10e18);
uint256 aliceId = book.createSellOrder(address(weth), 8e18, 1e18, 3 days);
​
vm.warp(block.timestamp + 4 days);
​
string memory res = book.getOrderDetailsString(aliceId);
​
console2.log(res);
}

We get this output:

[⠊] Compiling...
[⠒] Compiling 2 files with Solc 0.8.26
[⠑] Solc 0.8.26 finished in 537.32ms
Compiler run successful!
​
Ran 1 test for test/TestOrderBook.t.sol:TestOrderBook
[PASS] test_wrongTokenStatus() (gas: 313139)
Logs:
Order ID: 1
Seller: 0xaf6db259343d020e372f4ab69cad536aaf79d0ac
Selling: 8000000000000000000 wETH
Asking Price: 1000000000000000000 USDC
Deadline Timestamp: 259201
Status: Expired (Awaiting Cancellation)
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.42ms (354.09µs CPU time)
​
Ran 1 test suite in 6.12ms (2.42ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

For frontend expecting "Expired (Active but past deadline)", this output can be confusing.

Recommended Mitigation

Remove one of the logics and ensure only one consistent pattern is followed

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";
- }
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.