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): 0x7499c3f4d678efb2a682d61428adf422dd2e2bd4e134870c9fa904bfb3cb3991 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").

Attack Scenario\ Describe how the vulnerability can be exploited.

Attachments

1. Proof of Concept (PoC) File

   function _getDeploymentData() internal view returns (bytes memory) {
       // Address of the atomWalletBeacon contract
       address beaconAddress = walletConfig.atomWalletBeacon;
   
       // BeaconProxy creation code
       bytes memory code = type(BeaconProxy).creationCode;
   
       // encode the init function of the AtomWallet contract with the entryPoint and atomWarden as constructor arguments
       bytes memory initData = abi.encodeWithSelector(
           AtomWallet.init.selector, IEntryPoint(walletConfig.entryPoint), walletConfig.atomWarden, address(this)
       );
   
       // encode constructor arguments of the BeaconProxy contract (address beacon, bytes memory data)
       bytes memory encodedArgs = abi.encode(beaconAddress, initData);
   
       // concatenate the BeaconProxy creation code with the ABI-encoded constructor arguments
       return abi.encodePacked(code, encodedArgs);
   }

   return abi.encodePacked(code, encodedArgs); both inputs are dynamic type inputs i.e bytes

Recommendation

Use abi.encode or use bytes32 inputs

[Jelev123]

Its similar issue in Palmera contest is accepted as valid low https://github.com/hats-finance/Palmera-0x5fee7541ddcd51ba9f4af606f87b2c42eea655be/issues/13

[mihailo-maksa]

• Using abi.encode is not feasible here as it throws an EVM level error. The entire _getDeploymentData returned value, although using dynamic types, is essentially of constant length. Users cannot influence it in any way. Additionally, atomWarden is designed to practically never change, further mitigating the risk.
• Therefore, we consider this issue invalid as there is no practical vulnerability that can be exploited.