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 Factory contract can results in hash collisions, bypassing the signedOnly modifier.
As the solidity docs describe, two or more dynamic types are passed to abi.encodePacked. Moreover, these dynamic values are user-specified function arguments in external functions, meaning anyone can directly specify the value of these arguments when calling the function. The signedOnly modifier is supposed to protect functions to permit only function arguments that have been properly signed to be passed to the function logic, but because a collision can be created, the modifier can be bypassed for certain select inputs that result in the same encodePacked value.
Impact
The signedOnly modifier (line 537) is not effective because the modifier can be bypassed by different function arguments that result in the same signature when the values are encodePacked together. This can result in the submission of values that were not actually signed.
Instead of writing functions to accept several arguments that are hashed inside the function, consider rewriting the function to take the hashed value as a function argument directly so that the hashing process happens off-chain. This approach would solve the issue and save gas.
keccak123
high
abi.encodePacked
Allows Hash CollisionSummary
From the solidity documentation: https://docs.soliditylang.org/en/v0.8.17/abi-spec.html?highlight=collisions#non-standard-packed-mode
This issue exists in the Factory contract can results in hash collisions, bypassing the
signedOnly
modifier.Vulnerability Detail
The issue is in these lines of code: https://github.com/sherlock-audit/2022-10-nftport/blob/main/evm-minting-master/contracts/Factory.sol#L171 https://github.com/sherlock-audit/2022-10-nftport/blob/main/evm-minting-master/contracts/Factory.sol#L195 https://github.com/sherlock-audit/2022-10-nftport/blob/main/evm-minting-master/contracts/Factory.sol#L222
As the solidity docs describe, two or more dynamic types are passed to
abi.encodePacked
. Moreover, these dynamic values are user-specified function arguments in external functions, meaning anyone can directly specify the value of these arguments when calling the function. ThesignedOnly
modifier is supposed to protect functions to permit only function arguments that have been properly signed to be passed to the function logic, but because a collision can be created, the modifier can be bypassed for certain select inputs that result in the sameencodePacked
value.Impact
The
signedOnly
modifier (line 537) is not effective because the modifier can be bypassed by different function arguments that result in the same signature when the values areencodePacked
together. This can result in the submission of values that were not actually signed.Code Snippet
All instances of
abi.encodePacked
in the contract pass multiple dynamic type arguments https://github.com/sherlock-audit/2022-10-nftport/blob/main/evm-minting-master/contracts/Factory.sol#L171 https://github.com/sherlock-audit/2022-10-nftport/blob/main/evm-minting-master/contracts/Factory.sol#L195 https://github.com/sherlock-audit/2022-10-nftport/blob/main/evm-minting-master/contracts/Factory.sol#L222Tool used
Manual Review
Recommendation
Instead of writing functions to accept several arguments that are hashed inside the function, consider rewriting the function to take the hashed value as a function argument directly so that the hashing process happens off-chain. This approach would solve the issue and save gas.