Description: The tokenURI function uses abi.encodePacked() with multiple dynamic string parameters. When abi.encodePacked() is used with dynamic types, it can lead to hash collisions because the function performs a simple concatenation without length checks.
function tokenURI(uint256 tokenId) public view virtual override returns (string memory) {
...
return string(
abi.encodePacked(
_baseURI(),
Base64.encode(
bytes(
abi.encodePacked(
'{"name":"',
profileName,
'", ',
'"description":"A soulbound dating profile NFT.", ',
'"attributes": [{"trait_type": "Age", "value": ',
Strings.toString(profileAge),
"}], ",
'"image":"',
imageURI,
'"}'
)
)
)
)
);
}
Impact:
Potential hash collisions when encoding different profile data
Could lead to incorrect or duplicate token URIs
May affect systems that rely on unique token URI hashes
Proof of Concept:
Consider these two different profile data sets that would produce the same packed encoding:
profileName = "Bob"
imageURI = " Crypto.jpg"
profileName = "Bo"
imageURI = "b Crypto.jpg"
Proof of Code: Add following tests to testSoulboundProfileNFT.t.sol
function test_TokenURIHashCollision() public {
string memory name1 = "Bob";
string memory image1 = " Crypto.jpg";
uint8 age1 = 25;
string memory name2 = "Bo";
string memory image2 = "b Crypto.jpg";
uint8 age2 = 25;
vm.prank(user);
soulboundNFT.mintProfile(name1, age1, image1);
vm.prank(user2);
soulboundNFT.mintProfile(name2, age2, image2);
string memory uri1 = soulboundNFT.tokenURI(1);
string memory uri2 = soulboundNFT.tokenURI(2);
bytes memory packed1 = abi.encodePacked(name1, image1);
bytes memory packed2 = abi.encodePacked(name2, image2);
assertEq(keccak256(packed1), keccak256(packed2), "Collision detected: different data produces same hash");
assertTrue(
keccak256(bytes(uri1)) != keccak256(bytes(uri2)), "TokenURIs should be different for different profiles"
);
}
function test_SafeEncoding() public pure {
string memory name1 = "Bob";
string memory image1 = " Crypto.jpg";
string memory name2 = "Bo";
string memory image2 = "b Crypto.jpg";
bytes memory encoded1 = abi.encode(name1, image1);
bytes memory encoded2 = abi.encode(name2, image2);
assertTrue(keccak256(encoded1) != keccak256(encoded2), "abi.encode properly handles different inputs");
bytes memory packedSafe1 = abi.encodePacked(name1, "|", image1);
bytes memory packedSafe2 = abi.encodePacked(name2, "|", image2);
assertTrue(
keccak256(packedSafe1) != keccak256(packedSafe2),
"encodePacked with delimiters properly handles different inputs"
);
}
Recommended Mitigation: Use abi.encode() instead of abi.encodePacked() which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). Unless there is a compelling reason, abi.encode should be preferred. If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.
If all arguments are strings and or bytes, bytes.concat() should be used instead.
function tokenURI(uint256 tokenId) public view virtual override returns (string memory) {
if (ownerOf(tokenId) == address(0)) {
revert ERC721Metadata__URI_QueryFor_NonExistentToken();
}
string memory profileName = _profiles[tokenId].name;
uint256 profileAge = _profiles[tokenId].age;
string memory imageURI = _profiles[tokenId].profileImage;
return string(
- abi.encodePacked(
+ abi.encode
_baseURI(),
Base64.encode(
bytes(
- abi.encodePacked(
+ abi.encode(
'{"name":"',
profileName,
'", ',
'"description":"A soulbound dating profile NFT.", ',
'"attributes": [{"trait_type": "Age", "value": ',
Strings.toString(profileAge),
"}], ",
'"image":"',
imageURI,
'"}'
)
)
)
)
);
}
Alternatively, if gas optimization is a concern, ensure proper delimiters are used between dynamic string components to prevent collisions.
