keccak123 - abi.encodePacked Allows Hash Collision

[sherlock-admin]

keccak123

high

abi.encodePacked Allows Hash Collision

Summary

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 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. 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.

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#L222

Tool 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.

[Evert0x]

Downgrading to medium severity, fails to show an exploit pattern.

[hyperspacebunny]

Fixed in https://github.com/nftport/evm-minting-sherlock-fixes/pull/18

[rayn731]

Fixed, and it follows EIP-712 standard for hashing and signing data.