hats-finance / Proof-Of-Humanity-V2-0xef0709445d394a22704850c772a28a863bb780b0

Proof of Humanity Protocol v2
2 stars 1 forks source link

USAGE OF ABI ENCODEPACKED and KECCAK256 may lead to hash collision #33

Open hats-bug-reporter[bot] opened 3 weeks ago

hats-bug-reporter[bot] commented 3 weeks ago

Github username: @giorgiodalla Twitter username: 0xAuditism Submission hash (on-chain): 0xbef0ce7be7040f401c937c1675d9770b6585ecfb13cc3d8ab89bbc049cfecdd2 Severity: low

Description: Description\

The usage of keccak256 hshing and abo.encodepacked may lead to hash collision. It is best to use abi.encode instead

Attack Scenario\ ecery hash has to be unique, collision is critical

Attachments

  1. Proof of Concept (PoC) File https://github.com/hats-finance/Proof-Of-Humanity-V2-0xef0709445d394a22704850c772a28a863bb780b0/blob/1632b61bf2c3104916b9c501a0af65d0502dfc4c/contracts/CrossChainProofOfHumanity.sol#L268-L270
        bytes32 tHash = keccak256(
      @>      abi.encodePacked(humanityId, block.timestamp, address(this), bridgeGateways[_bridgeGateway].foreignProxy)
        );
  1. Revised Code File (Optional)

https://github.com/hats-finance/Proof-Of-Humanity-V2-0xef0709445d394a22704850c772a28a863bb780b0/blob/1632b61bf2c3104916b9c501a0af65d0502dfc4c/contracts/CrossChainProofOfHumanity.sol#L268-L270

        bytes32 tHash = keccak256(
            abi.encode(humanityId, block.timestamp, address(this), bridgeGateways[_bridgeGateway].foreignProxy)
        );
clesaege commented 3 weeks ago

encodepacked can only create a collision if the inputs are of variable length. In our case, all inputs are of fixed length, thus using encodepacked is appropriate.

If you believe you found a collision with elements of fixed lengths, please provide an example of such collision.