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

[hats-bug-reporter[bot]]

Github username: @ShaheenRehman Twitter username: 0x_Shaheen Submission hash (on-chain): 0x606297ef1e266827c51394aa84a4a1bea3b2ebd3af56fff76f8e7e57304cf716 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").

This issue exists in the helpers contract's processAndSortSignatures function:

               .....
               bytes memory concatenatedSignatures;
               .....
                bytes memory signature = abi.encodePacked(r, s, v);
                if (signer == currentOwner) {
                    ///@audit hash collision! both dynamic types
                    concatenatedSignatures =
                        abi.encodePacked(concatenatedSignatures, signature);
                    break;
                }
               .....

Notice, both concatenatedSignatures & signature are bytes, dynamic types

1. Proof of Concept (PoC) File https://docs.soliditylang.org/en/v0.8.17/abi-spec.html?highlight=collisions#non-standard-packed-mode

2. Revised Code File (Optional) Use abi.encode or use bytes32 inputs

[0xRizwan]

Issue is correct and should be low severity.

[rotcivegaf]

I think this is invalid:

• concatenatedSignatures = ""
• bytes memory signature = abi.encodePacked(r, s, v); where (uint8 v, bytes32 r, bytes32 s)

So abi.encodePacked("", v, r, s); there are no dynamic types so not Hash Collision

[eugenioclrc]

No real poc, no link to the code, no context at all. Totally invalid and should be considered as spam.

[ShaheenRehman]

No real poc, no link to the code, no context at all. Totally invalid and should be considered as spam.

This is submitted as a low finding. Do you expect me to create a poc for that?

More sounds like "I didn't submitted it first, thats why its invalid" ahhh cry xD

[ShaheenRehman]

I think this is invalid:

• concatenatedSignatures = ""
• bytes memory signature = abi.encodePacked(r, s, v); where (uint8 v, bytes32 r, bytes32 s)

So abi.encodePacked("", v, r, s); there are no dynamic types so not Hash Collision

The instance of hash collision i mentioned is not the one you are talking ser. Hash collision is happening in the encoding of concatenatedSignatures not signatures. concatenatedSignatures both inputs are dynamic type inputs i.e bytes

[rotcivegaf]

Make a valid PoC

[ShaheenRehman]

Make a valid PoC

PoC are created for High severity and complex bugs. I'm not wasting my time for creating a PoC for a low and pretty straight forward bug.

The sponsor already have accepted this issue as valid.

You clearly didn't understood the bug first and mentioned a wrong code instance in your escalation. Pls respect everyone's time. Thanks!

[hex69493e6f]

@ShaheenRehman, Can you explain this bug in more detail? Because I don't think it's even an informational bug.

[eugenioclrc]

I think @chaoticpunk and @ShaheenRehman are the same person lol

[AlexAurthur]

No real poc, no link to the code, no context at all. Totally invalid and should be considered as spam.

@eugenioclrc , u should check this invalid issues #52 , #72 marked as valid, with no proper poc nor logic but just manipulation of facts and leveraging the lead auditor role

[0xad1onchain]

Hash collision can only happen if hashes are being used as hashes. This is simple signature encoding being used to build a concatenated signature that too after sorting. How and where can this signature collide with another one? Just because a scenario is theoretically possible according to a general purpose documentation does not mean it is applicable at every possible instance of its usage. Kindly provide the least level of POC or atleast describe a scenario where it would actually be applicable as a bug

[eugenioclrc]

You should be reported to Hats team and be banned @chaoticpunk