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

Risk of Hash Collision via abi.encodePacked() with Mixed Dynamic Types in OrderBook::getOrderDetailsString

ryzen_xp

Root + Impact

Description

  • In ./src/OrderBook.sol, the OrderBook::getOrderDetailsString(uint256 _orderId) function uses abi.encodePacked() to concatenate various dynamic and static values (strings, addresses, numbers) into a single details string, which is returned for off-chain or UI usage.

  • However, using abi.encodePacked() with multiple dynamic types (e.g., string, bytes) or mixed with static types can lead to hash collisions if the resulting string is ever hashed (e.g., with keccak256) for uniqueness, authentication, or digital signatures. This behavior introduces a silent vulnerability if developers assume the string is safe to hash.

// @audit: abi.encodePacked() can cause hash collisions with mixed dynamic args.
// @> details = string(
// @> abi.encodePacked(
// @> "Order ID: ",
// @> order.id.toString(),
// @> "\n",
// @> "Seller: ",
// @> Strings.toHexString(uint160(order.seller), 20),
// @> ...

Reference Files

File Function Lines Note
./src/OrderBook.sol getOrderDetailsString L221ā€“L272 Vulnerable use of abi.encodePacked() with mixed dynamic types

Risk

Likelihood

  • This issue occurs when abi.encodePacked() is used with a combination of dynamic types (string, bytes) and static types in the same call.

  • It becomes exploitable when the resulting data is passed to a hash function such as keccak256, especially for signing, commitment, or uniqueness guarantees.

Impact

  • Hash collisions may occur, causing different input values to produce the same hash output.

  • If the details string is used in off-chain signing, commitment schemes, or message verification, an attacker could spoof valid messages or break integrity checks.

PoC (Proof of Concept)

šŸŸ” Status: Present but Could Be More Instructive

The PoC shows a basic hash collision:

keccak256(abi.encodePacked("AAA", "BBB")); // keccak256("AAABBB")
keccak256(abi.encodePacked("AA", "ABBB")); // keccak256("AAABBB") ā€” collision!

While correct, it lacks context. This matters if the resulting string is hashed (e.g., keccak256(bytes(details))) for signatures or commit-reveal logic. An attacker could craft different inputs that produce the same hash, bypassing integrity checks or forging valid-looking data.

ā€‹
***
ā€‹
## Mitigation
ā€‹
Avoid `abi.encodePacked()` for combining dynamic types. Use `abi.encode()` or `bytes.concat()` for safer behavior.
ā€‹
```diff
- details = string(
- abi.encodePacked(
- "Order ID: ",
- order.id.toString(),
- ...
- )
- );
ā€‹
+ bytes memory encoded = abi.encode(
+ "Order ID: ", order.id.toString(), "\n",
+ "Seller: ", Strings.toHexString(uint160(order.seller), 20), "\n",
+ "Selling: ", order.amountToSell.toString(), " ", tokenSymbol, "\n",
+ "Asking Price: ", order.priceInUSDC.toString(), " USDC\n",
+ "Deadline Timestamp: ", order.deadlineTimestamp.toString(), "\n",
+ "Status: ", status
+ );
+ details = string(encoded);

šŸ›‘ Note: abi.encode() produces non-UTF-8 padded bytes. If this string is intended purely for human-readable output, consider using bytes.concat() instead for proper formatting:

details = string(bytes.concat(
bytes("Order ID: "), bytes(order.id.toString()), bytes("\n"),
...
));
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.