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

[hats-bug-reporter]

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
hats-finance/Palmera-0x5fee7541ddcd51ba9f4af606f87b2c42eea655be#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.