Pieces Protocol
First Flight #32
Beginner FriendlyFoundrySolidityNFT
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Invalid

Insecure Use of abi.encodePacked() with Dynamic Types

sauronsol

Description

The TokenDivider contract uses abi.encodePacked() with dynamic strings to generate the names and symbols of the fractioned ERC20 tokens. This approach can potentially lead to hash collisions, although in the current implementation, the risk is mitigated by the structure of the generated names.

Code Location:

string(abi.encodePacked(ERC721(nftAddress).name(), "Fraccion"))
string(abi.encodePacked("F", ERC721(nftAddress).symbol()))

Impact

Severity: Medium

The use of abi.encodePacked() with dynamic strings can theoretically lead to hash collisions. In the current context, this could allow an attacker to create NFTs with specially designed names to generate ERC20 tokens with identical names, which could create confusion for users.

For example:

  • NFT1 with name="A" and symbol="BC"

  • NFT2 with name="" and symbol="ABC"

Could theoretically generate tokens with similar names after concatenation.

Proof of Concept

A test was created to demonstrate the potential issue:

function testHashCollision() public {
// Create two NFTs with names that could create a collision
MockNFTCollision nft1 = new MockNFTCollision("A", "BC");
MockNFTCollision nft2 = new MockNFTCollision("", "ABC");
// The two ERC20 tokens generated could have names that could be confused
// NFT1: "AFraccion" and "FBC"
// NFT2: "Fraccion" and "FABC"
}

Tools Used

  • Manual code analysis

  • Foundry tests

  • Aderyn (static analysis tool)

Recommendations

  1. Replace abi.encodePacked() with abi.encode() to avoid potential collisions:

string(abi.encode(ERC721(nftAddress).name(), "Fraccion"))
string(abi.encode("F", ERC721(nftAddress).symbol()))

  1. Alternatively, add a unique separator between the concatenated strings:

string(abi.encodePacked(ERC721(nftAddress).name(), "::", "Fraccion"))
string(abi.encodePacked("F", "::", ERC721(nftAddress).symbol()))

  1. Or use bytes.concat() which is more suitable for string concatenation:

string(bytes.concat(bytes(ERC721(nftAddress).name()), bytes("Fraccion")))
string(bytes.concat(bytes("F"), bytes(ERC721(nftAddress).symbol())))
Updates

Lead Judging Commences

fishy Lead Judge 6 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Hash collision

Appeal created

fishy Lead Judge 6 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Hash collision

fishy Lead Judge 6 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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