Update hashProposal visibility to view

[0xDiscotech]

Fixes

Update hashProposal() function visibility to view instead of pure to allow an override using block.chainid and/or address(this) in order to guarantee more uniqueness on the same proposal but on different networks.

Can be very useful when the proposal is used as a kind of proof to validate the vote, and the same proposal is created on different networks. This allows the developer to mitigate a replay attack over the same proposalId in a clean way by overriding the function to read from the environment or the contract's state.

PR Checklist

• Tests
• Documentation
• Changeset entry (run npx changeset add)

[changeset-bot]

🦋 Changeset detected

Latest commit: 667d07d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[ernestognw]

Hey, thanks for the contribution! Some thoughts

Changing to view allows to extend the hash and include block.chainid or addres(this), but also enables developers to read from storage, which concerns me more.

As opposed to pure functions, a view function depends on the state, which means that the proposalId may unexpectedly change if a state variable is added to the formula. In that case, the proposal may end up stuck (i.e. not deriving the same proposalId in queue or execute)

Though the concern of replayability does make sense in the context of signatures, I would add a nonce to the signature instead.

[Amxx]

Thank you @0xDiscotech for this proposal.

I'm not sure what the consequences of changing the proposalId computation would be on UI such as Tally ... but given it is virtual, I don't see why we would allow some changes but not that change.

So its good to me

[Amxx]

@0xDiscotech, we discussed this PR internally, and we have some concern with the consequences of using pure vs view.

Using pure would allow change that make the fonction change value independently of the input parameter (that is the whole point of pure vs view). This means reading state variable, reading the time, reading anything that is not constant.

If this was to happen, then the proposalId may not be consistently computed through the lifecycle of a proposal, and that would be bad. Potentially, it could be used to hide some attack vector that allows a priviledge user to prevent a proposal from being executed. This could be happen by mistake, or intentionally by a malicious dev.

On the other hand, we don't really understand the usecase. If two governors, on the same chain, or on two different chains, are planning to execute the exact same logic, what is the issue using the same identifier for both proposals ? The same proposalId means the same input was proposed. The execution outcome may differ depending on the governor instance doing it, but the input remains the same.

Whe voting on it, the voting scheme (that is based on EIP-721) does already prevent cross-instance / cross-chain replay.

With all that in mind, we see a not very likelly but still very clear risk ... and we don't really see an upside to that.

[0xDiscotech]

Thanks for you response @Amxx @ernestognw!

You have a good point there, but I will explain the reason behind my proposal:
The idea of adding it to view came from a project where our team created a Governor extension interacting with Worldcoin's World ID protocol. In this project, we used the proposalId as a key part of the input to verify that a user is a unique human. Since Worldcoin preserves user information and only verifies if the user is human, it always returns the same nullifierHash for the input action. It makes sense to use the proposalId as input and store that nullifierHash to avoid double-spending by the same user. However, you must ensure that the proposalId is unique to prevent replay attacks.

In our solution, we had to concatenate the description with a kind of 'signature' using the chain ID and the Governor's address. This breaks the functionality of the hashProposal() function because it won't return the exact same proposalId after calling propose(). You can see our implementation here: GovernorWorldID.sol. The solution was somewhat 'hacky' because we couldn't add that data inside the hashProposal() function, which would have been the cleanest way.

This approach might be very specific to the mentioned use case, but it's not uncommon for future Governors to use similar protocols to get proof-of-personhood while preserving user privacy.

In summary, my point is that developers can still modify the logic behind proposalId generation. However, restricting this to pure functions makes things more difficult and complex to handle. The benefit of allowing more flexibility in these cases is that it ensures guaranteed uniqueness not only across different chains but also across different DAOs. Currently, the same proposalId can be generated in two different DAOs with the same proposal as input.

[0xDiscotech]

Thanks for you response @Amxx @ernestognw!

The idea of adding it to view came from a project where our team created a Governor extension interacting with Worldcoin's World ID protocol. In this project, we used the proposalId as a key part of the input to verify that a user is a unique human. Since Worldcoin preserves user information and only verifies if the user is human, it always returns the same nullifierHash for the input action. It makes sense to use the proposalId as input and store that nullifierHash to avoid double-spending by the same user. However, you must ensure that the proposalId is unique to prevent replay attacks.

In our solution, we had to concatenate the description with a kind of 'signature' using the chain ID and the Governor's address. This breaks the functionality of the hashProposal() function because it won't return the exact same proposalId after calling propose(). You can see our implementation here: GovernorWorldID.sol. The solution was somewhat 'hacky' because we couldn't add that data inside the hashProposal() function, which would have been the cleanest way.

This approach might be very specific to the mentioned use case, but it's not uncommon for future Governors to use similar protocols to get proof-of-personhood while preserving user privacy.

In summary, my point is that developers can still modify the logic behind proposalId generation. However, restricting this to pure functions makes things more difficult and complex to handle. The benefit of allowing more flexibility in these cases is that it ensures guaranteed uniqueness not only across different chains but also across different DAOs. Currently, the same proposalId can be generated in two different DAOs with the same proposal as input.

Also we could use the address(this) and block.chainid as the default implementation of hashProposal(), but I don't know if it something you are interested in. Maybe I can open a new PR with that change (only in case you agree on moving forward with the pure to view modification).

[Amxx]

Also we could use the address(this) and block.chainid as the default implementation of hashProposal(), but I don't know if it something you are interested in.

This is something that was discussed in the past, and was rejected. We were concerned that changing the hash computation could break tools such as Tally, and we did not see a clear advantage (as discussed above). It would also cause issues if a governor contract is upgraded while proposals are active.

It is not inconceivable that we make the function view, so that overrides can use address(this) ... but for sure we are not going to change the default function.

[0xDiscotech]

Also we could use the address(this) and block.chainid as the default implementation of hashProposal(), but I don't know if it something you are interested in.

This is something that was discussed in the past, and was rejected. We were concerned that changing the hash computation could break tools such as Tally, and we did not see a clear advantage (as discussed above). It would also cause issues if a governor contract is upgraded while proposals are active.

It is not inconceivable that we make the function view, so that overrides can use address(this) ... but for sure we are not going to change the default function.

Awesome, makes sense. Good to know 👍

[ernestognw]

The idea of adding it to view came from a project where our team created a Governor extension interacting with Worldcoin's World ID protocol. In this project, we used the proposalId as a key part of the input to verify that a user is a unique human

This is an interesting insight. I guess the problem here is that we never intended proposalId to be something "non-replayable" outside of the context of this contract. The security assumptions here indicate that there shouldn't be an issue, though using the proposalId in the way you describe definitely requires replayability protection.

From your code I see you added some information to the returned proposalId. That's fine in my opinion since it's not altering the internal assumptions the Governor does about the proposalId.

I would be fine making the function view, it's just that most of the lifecycle of the governor assumes that the proposal id function is a pure function (in the sense that it doesn't have any side effect), and allowing to remove such guarantee sounds like a big compromise imo

[0xDiscotech]

That's fine in my opinion since it's not altering the internal assumptions the Governor does about the proposalId.
Yes, even though that approach works, the off-chain interaction is affected since hashProposal() won't return the same proposalId as _propose().

My point of view is that by setting it to view, you provide more flexibility to cover scenarios requiring more uniqueness. This allows developers to implement it straightforwardly without breaking any off-chain interaction.

That said, you make a good point. While I maintain my perspective, I respect and understand any decision you make 🫡

[ernestognw]

Hi @0xDiscotech,

We've been discussing this change since #5290 came up as a requirement for some other DAOs too. Those changes may likely end up being even more aggressive than this visibility change so we might get this merged before.

[0xDiscotech]

Hi @0xDiscotech,

We've been discussing this change since #5290 came up as a requirement for some other DAOs too. Those changes may likely end up being even more aggressive than this visibility change so we might get this merged before.

Hi @ernestognw,
Thanks for letting me know, this sounds good!
Do you need some change from side on the code?

[ernestognw]

Hi @ernestognw, Thanks for letting me know, this sounds good! Do you need some change from side on the code?

All good, just an update :D