hats-finance / Safe-0x2909fdefd24a1ced675cb1444918fa766d76bdac

A collection of modules that can be used with the Safe contract
GNU Lesser General Public License v3.0
0 stars 1 forks source link

`abi.encodePacked()` should not be used with dynamic types when passing the result to a hash function such as `keccak256()` #6

Open hats-bug-reporter[bot] opened 1 week ago

hats-bug-reporter[bot] commented 1 week ago

Github username: -- Twitter username: -- Submission hash (on-chain): 0xa6a3284659a5587ce0cf8e25c0a192477e4f6047474707d2241935f16754f2c3 Severity: low

Description: Description\ 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". 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

Instances (3):

File: SafeWebAuthnSignerFactory.sol

38:                 type(SafeWebAuthnSignerProxy).creationCode,

45:         signer = address(uint160(uint256(keccak256(abi.encodePacked(hex"ff", address(this), bytes32(0), codeHash)))));

73:             abi.encodeWithSignature("isValidSignature(bytes32,bytes)", message, signature),
nlordell commented 1 week ago

This submission is invalid.

The use of abi.encodePacked here is correct - this is how the CREATE2 address derivation is specified. Note that none of the parameters to abi.encodePacked are dynamic types: