Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Governor signature nonces #4378

Merged

Conversation

ernestognw
Copy link
Member

Fixes LIB-895

This PR adds nonce and voter arguments to governor vote casting signatures with the following objectives:

  • The nonce is added so that signatures can be invalidated.
  • The voter address guarantees that a signature can't be forged for any random address.

PR Checklist

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

@ernestognw ernestognw requested review from frangio and Amxx June 22, 2023 01:48
@changeset-bot
Copy link

changeset-bot bot commented Jun 22, 2023

🦋 Changeset detected

Latest commit: 2ae0a13

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 Major

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

@frangio
Copy link
Contributor

frangio commented Jun 22, 2023

To remove the initcode size warning that is causing the upgradeable tests to fail, we have to update the hardhat-ignore-warnings package to the latest version and add in the config:

    'contracts-exposed/**/*': {
      'code-size': 'off',
+     'initcode-size': 'off',
    },

@socket-security
Copy link

socket-security bot commented Jun 22, 2023

Updated dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
hardhat-ignore-warnings 0.2.8...0.2.9 None +0/-0 28.6 kB frangio

@ernestognw
Copy link
Member Author

@SocketSecurity ignore hardhat-ignore-warnings@0.2.9

@ernestognw
Copy link
Member Author

CI is failing because the storage layout became incompatible by the addition of _nonces, but that's expected

Copy link
Collaborator

@Amxx Amxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very small comments about formating.

What I'm worried about is not the code, its the "breaking the voteBySig" without params.
The params part I don't really worry about, but the the without params might be breaking tally. Do we have info on that ?

contracts/governance/Governor.sol Outdated Show resolved Hide resolved
contracts/governance/Governor.sol Outdated Show resolved Hide resolved
test/governance/Governor.test.js Outdated Show resolved Hide resolved
test/governance/extensions/GovernorWithParams.test.js Outdated Show resolved Hide resolved
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
@ernestognw
Copy link
Member Author

Very small comments about formating.

What I'm worried about is not the code, its the "breaking the voteBySig" without params. The params part I don't really worry about, but the the without params might be breaking tally. Do we have info on that ?

We asked to Tally yesterday and they don't seem to have any major concerns 👍🏻

@ernestognw ernestognw requested a review from Amxx June 23, 2023 05:03
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments and side notes.

@@ -519,17 +523,23 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive
function castVoteBySig(
uint256 proposalId,
uint8 support,
address voter,
Copy link
Contributor

@frangio frangio Jun 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is changing the ERC-165 ids, i.e. removing the previously supported interfaces (correctly) and adding new ones. However, we've discussed before that ERC-165 is not a good fit for this contract so we should really think whether we want to continue using it and particularly adding new interfaces.

No need to change anything on this PR, but it is an unsolved question we need to address.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is relevant to #4360

.changeset/sixty-numbers-reply.md Outdated Show resolved Hide resolved
test/governance/Governor.test.js Outdated Show resolved Hide resolved
Comment on lines 554 to 556
uint8 v,
bytes32 r,
bytes32 s
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that we are changing the signature of the castVoteBySig functions, what do you think about changing these arguments to bytes signature and using the opportunity to add EIP-1271 compatibility?

I don't think we should do this here so it's not a blocker for this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also aligns really well with the addition of the voter argument, which is needed for EIP-1271.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to what we discussed, I'll add the signature changes and 1271 compatibility in a follow up PR

Amxx
Amxx previously approved these changes Jun 28, 2023
Copy link
Collaborator

@Amxx Amxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM,

+1 to frangio idea of changing the signature type and enabling ERC1271 in a followup PR.

This was referenced Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants