`abi.encodePacked` Allows Hash Collision when dynamic types are the input

[hats-bug-reporter[bot]]

Github username: @Jelev123 Twitter username: zhulien_zhelev Submission hash (on-chain): 0x03be03e3e5fa64cc9ac173281ac369e4852695880eae791eb138aa730ff1a336 Severity: low

Description: Description\ From the solidity documentation: https://docs.soliditylang.org/en/v0.8.17/abi-spec.html?highlight=collisions#non-standard-packed-mode > If you use keccak256(abi.encodePacked(a, b)) and both a and b are dynamic types, it is easy to craft collisions in the hash value by moving parts of a into b and vice-versa. More specifically, abi.encodePacked("a", "bc") == abi.encodePacked("ab", "c").

1. Proof of Concept

https://github.com/hats-finance/illuminex-0x0bb4aa1f58719707405c231fcdf0b405714799cf/blob/3ad7c2aedf991493aab45d3e0847b7e07f5c0d07/packages/contracts/contracts/illuminex/xengine/chains/btc/wallet/VaultBitcoinWallet.sol#L461

_recoveryData and _vaultScriptHash are dynamic types

Recommendation

Use abi.encode or use bytes32 inputs

[Jelev123]

Dublicate of wich? @rotcivegaf

[Jelev123]

@rotcivegaf ??

[rotcivegaf]

It's only calculate an id, if you don't provide a real PoC I can't validate The abi.encodePacked spend less gas than abi.encode, so finally it's a good practice use abi.encodePacked

[Jelev123]

Use abi.encode() instead 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".

@rotcivegaf

[party-for-illuminati]

Use abi.encode() instead 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".

@rotcivegaf

This value is used just as entropy value for Sapphire.randomBytes, so effectively there is no difference whatever we use abi.encode or abi.encodePacked

[Jelev123]

The Sapphire.randomBytes it use _entropy which is bytes32. The issue that i subscribe its used bytes (dynamic types) @party-for-illuminati

[party-for-illuminati]

The Sapphire.randomBytes it use _entropy which is bytes32. The issue that i subscribe its used bytes (dynamic types) @party-for-illuminati

It makes no sense to me, can you explain please what is the issue?

[Jelev123]

The Sapphire.randomBytes it use _entropy which is bytes32. The issue that i subscribe its used bytes (dynamic types) @party-for-illuminati

It makes no sense to me, can you explain please what is the issue?

When using keccak256(abi.encodePacked(...)) with dynamic types (such as bytes), there is a risk of hash collisions. This is because abi.encodePacked concatenates the arguments without adding length information, making it possible for different combinations of inputs to produce the same hash. While the theoretical risk of hash collisions exists, I understand the practical implications might be minimal depending on the specific context and nature of the inputs. However, as a best practice and to ensure robustness, I believe the suggested changes would be beneficial.

[party-for-illuminati]

The Sapphire.randomBytes it use _entropy which is bytes32. The issue that i subscribe its used bytes (dynamic types) @party-for-illuminati

It makes no sense to me, can you explain please what is the issue?

When using keccak256(abi.encodePacked(...)) with dynamic types (such as bytes), there is a risk of hash collisions. This is because abi.encodePacked concatenates the arguments without adding length information, making it possible for different combinations of inputs to produce the same hash. While the theoretical risk of hash collisions exists, I understand the practical implications might be minimal depending on the specific context and nature of the inputs. However, as a best practice and to ensure robustness, I believe the suggested changes would be beneficial.

[Jelev123]

wow, is my first submit compiled by artificial intelligence? Тhe issue is valid, I can show you at least 10 issues that are in Solodit, also the same problem was accepted in the Palmera contest