diff --git a/.changeset/popular-geese-tan.md b/.changeset/popular-geese-tan.md new file mode 100644 index 000000000..397627518 --- /dev/null +++ b/.changeset/popular-geese-tan.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`GovernorNoncesKeyed`: Extension of `Governor` that adds support for keyed nonces when voting by sig. diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 828dbc395..5a7b16170 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -538,16 +538,9 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 address voter, bytes memory signature ) public virtual returns (uint256) { - bool valid = SignatureChecker.isValidSignatureNow( - voter, - _hashTypedDataV4(keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support, voter, _useNonce(voter)))), - signature - ); - - if (!valid) { + if (!_validateVoteSig(proposalId, support, voter, signature)) { revert GovernorInvalidSignature(voter); } - return _castVote(proposalId, voter, support, ""); } @@ -560,31 +553,56 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 bytes memory params, bytes memory signature ) public virtual returns (uint256) { - bool valid = SignatureChecker.isValidSignatureNow( - voter, - _hashTypedDataV4( - keccak256( - abi.encode( - EXTENDED_BALLOT_TYPEHASH, - proposalId, - support, - voter, - _useNonce(voter), - keccak256(bytes(reason)), - keccak256(params) - ) - ) - ), - signature - ); - - if (!valid) { + if (!_validateExtendedVoteSig(proposalId, support, voter, reason, params, signature)) { revert GovernorInvalidSignature(voter); } - return _castVote(proposalId, voter, support, reason, params); } + /// @dev Validate the `signature` used in {castVoteBySig} function. + function _validateVoteSig( + uint256 proposalId, + uint8 support, + address voter, + bytes memory signature + ) internal virtual returns (bool) { + return + SignatureChecker.isValidSignatureNow( + voter, + _hashTypedDataV4(keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support, voter, _useNonce(voter)))), + signature + ); + } + + /// @dev Validate the `signature` used in {castVoteWithReasonAndParamsBySig} function. + function _validateExtendedVoteSig( + uint256 proposalId, + uint8 support, + address voter, + string memory reason, + bytes memory params, + bytes memory signature + ) internal virtual returns (bool) { + return + SignatureChecker.isValidSignatureNow( + voter, + _hashTypedDataV4( + keccak256( + abi.encode( + EXTENDED_BALLOT_TYPEHASH, + proposalId, + support, + voter, + _useNonce(voter), + keccak256(bytes(reason)), + keccak256(params) + ) + ) + ), + signature + ); + } + /** * @dev Internal vote casting mechanism: Check that the vote is pending, that it has not been cast yet, retrieve * voting weight using {IGovernor-getVotes} and call the {_countVote} internal function. Uses the _defaultParams(). diff --git a/contracts/governance/README.adoc b/contracts/governance/README.adoc index 0a9c2c4e3..3131a0be0 100644 --- a/contracts/governance/README.adoc +++ b/contracts/governance/README.adoc @@ -54,6 +54,8 @@ Other extensions can customize the behavior or interface in multiple ways. * {GovernorSuperQuorum}: Extension of {Governor} with a super quorum. Proposals that meet the super quorum (and have a majority of for votes) advance to the `Succeeded` state before the proposal deadline. +* {GovernorNoncesKeyed}: An extension of {Governor} with support for keyed nonces in addition to traditional nonces when voting by signature. + In addition to modules and extensions, the core contract requires a few virtual functions to be implemented to your particular specifications: * <>: Delay (in ERC-6372 clock) since the proposal is submitted until voting power is fixed and voting starts. This can be used to enforce a delay after a proposal is published for users to buy tokens, or delegate their votes. @@ -100,6 +102,8 @@ NOTE: Functions of the `Governor` contract do not include access control. If you {{GovernorSuperQuorum}} +{{GovernorNoncesKeyed}} + == Utils {{Votes}} diff --git a/contracts/governance/extensions/GovernorNoncesKeyed.sol b/contracts/governance/extensions/GovernorNoncesKeyed.sol new file mode 100644 index 000000000..e99a47dba --- /dev/null +++ b/contracts/governance/extensions/GovernorNoncesKeyed.sol @@ -0,0 +1,90 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.24; + +import {Governor} from "../Governor.sol"; +import {Nonces} from "../../utils/Nonces.sol"; +import {NoncesKeyed} from "../../utils/NoncesKeyed.sol"; +import {SignatureChecker} from "../../utils/cryptography/SignatureChecker.sol"; + +/** + * @dev An extension of {Governor} that extends existing nonce management to use {NoncesKeyed}, where the key is the first 192 bits of the `proposalId`. + * This is useful for voting by signature while maintaining separate sequences of nonces for each proposal. + * + * NOTE: Traditional (un-keyed) nonces are still supported and can continue to be used as if this extension was not present. + */ +abstract contract GovernorNoncesKeyed is Governor, NoncesKeyed { + function _useCheckedNonce(address owner, uint256 nonce) internal virtual override(Nonces, NoncesKeyed) { + super._useCheckedNonce(owner, nonce); + } + + /** + * @dev Check the signature against keyed nonce and falls back to the traditional nonce. + * + * NOTE: This function won't call `super._validateVoteSig` if the keyed nonce is valid. + * Side effects may be skipped depending on the linearization of the function. + */ + function _validateVoteSig( + uint256 proposalId, + uint8 support, + address voter, + bytes memory signature + ) internal virtual override returns (bool) { + if ( + SignatureChecker.isValidSignatureNow( + voter, + _hashTypedDataV4( + keccak256( + abi.encode(BALLOT_TYPEHASH, proposalId, support, voter, nonces(voter, uint192(proposalId))) + ) + ), + signature + ) + ) { + _useNonce(voter, uint192(proposalId)); + return true; + } else { + return super._validateVoteSig(proposalId, support, voter, signature); + } + } + + /** + * @dev Check the signature against keyed nonce and falls back to the traditional nonce. + * + * NOTE: This function won't call `super._validateExtendedVoteSig` if the keyed nonce is valid. + * Side effects may be skipped depending on the linearization of the function. + */ + function _validateExtendedVoteSig( + uint256 proposalId, + uint8 support, + address voter, + string memory reason, + bytes memory params, + bytes memory signature + ) internal virtual override returns (bool) { + if ( + SignatureChecker.isValidSignatureNow( + voter, + _hashTypedDataV4( + keccak256( + abi.encode( + EXTENDED_BALLOT_TYPEHASH, + proposalId, + support, + voter, + nonces(voter, uint192(proposalId)), + keccak256(bytes(reason)), + keccak256(params) + ) + ) + ), + signature + ) + ) { + _useNonce(voter, uint192(proposalId)); + return true; + } else { + return super._validateExtendedVoteSig(proposalId, support, voter, reason, params, signature); + } + } +} diff --git a/contracts/mocks/governance/GovernorNoncesKeyedMock.sol b/contracts/mocks/governance/GovernorNoncesKeyedMock.sol new file mode 100644 index 000000000..91021cc31 --- /dev/null +++ b/contracts/mocks/governance/GovernorNoncesKeyedMock.sol @@ -0,0 +1,45 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.24; + +import {Governor, Nonces} from "../../governance/Governor.sol"; +import {GovernorSettings} from "../../governance/extensions/GovernorSettings.sol"; +import {GovernorCountingSimple} from "../../governance/extensions/GovernorCountingSimple.sol"; +import {GovernorVotesQuorumFraction} from "../../governance/extensions/GovernorVotesQuorumFraction.sol"; +import {GovernorProposalGuardian} from "../../governance/extensions/GovernorProposalGuardian.sol"; +import {GovernorNoncesKeyed} from "../../governance/extensions/GovernorNoncesKeyed.sol"; + +abstract contract GovernorNoncesKeyedMock is + GovernorSettings, + GovernorVotesQuorumFraction, + GovernorCountingSimple, + GovernorNoncesKeyed +{ + function proposalThreshold() public view override(Governor, GovernorSettings) returns (uint256) { + return super.proposalThreshold(); + } + + function _validateVoteSig( + uint256 proposalId, + uint8 support, + address voter, + bytes memory signature + ) internal virtual override(Governor, GovernorNoncesKeyed) returns (bool) { + return super._validateVoteSig(proposalId, support, voter, signature); + } + + function _validateExtendedVoteSig( + uint256 proposalId, + uint8 support, + address voter, + string memory reason, + bytes memory params, + bytes memory signature + ) internal virtual override(Governor, GovernorNoncesKeyed) returns (bool) { + return super._validateExtendedVoteSig(proposalId, support, voter, reason, params, signature); + } + + function _useCheckedNonce(address owner, uint256 nonce) internal virtual override(Nonces, GovernorNoncesKeyed) { + super._useCheckedNonce(owner, nonce); + } +} diff --git a/test/governance/Governor.test.js b/test/governance/Governor.test.js index 0e4283c3b..e66dd2571 100644 --- a/test/governance/Governor.test.js +++ b/test/governance/Governor.test.js @@ -198,31 +198,35 @@ describe('Governor', function () { }); describe('vote with signature', function () { - it('votes with an EOA signature', async function () { + it('votes with an EOA signature on two proposals', async function () { await this.token.connect(this.voter1).delegate(this.userEOA); - const nonce = await this.mock.nonces(this.userEOA); + for (let i = 0; i < 2; i++) { + const nonce = await this.mock.nonces(this.userEOA); - // Run proposal - await this.helper.propose(); - await this.helper.waitForSnapshot(); - await expect( - this.helper.vote({ - support: VoteType.For, - voter: this.userEOA.address, - nonce, - signature: signBallot(this.userEOA), - }), - ) - .to.emit(this.mock, 'VoteCast') - .withArgs(this.userEOA, this.proposal.id, VoteType.For, ethers.parseEther('10'), ''); + // Run proposal + await this.helper.propose(); + await this.helper.waitForSnapshot(); + await expect( + this.helper.vote({ + support: VoteType.For, + voter: this.userEOA.address, + nonce, + signature: signBallot(this.userEOA), + }), + ) + .to.emit(this.mock, 'VoteCast') + .withArgs(this.userEOA, this.proposal.id, VoteType.For, ethers.parseEther('10'), ''); - await this.helper.waitForDeadline(); - await this.helper.execute(); + // After + expect(await this.mock.hasVoted(this.proposal.id, this.userEOA)).to.be.true; + expect(await this.mock.nonces(this.userEOA)).to.equal(nonce + 1n); - // After - expect(await this.mock.hasVoted(this.proposal.id, this.userEOA)).to.be.true; - expect(await this.mock.nonces(this.userEOA)).to.equal(nonce + 1n); + // Update proposal to allow for re-propose + this.helper.description += ' - updated'; + } + + await expect(this.mock.nonces(this.userEOA)).to.eventually.equal(2n); }); it('votes with a valid EIP-1271 signature', async function () { diff --git a/test/governance/extensions/GovernorNoncesKeyed.test.js b/test/governance/extensions/GovernorNoncesKeyed.test.js new file mode 100644 index 000000000..e2cbd8ff0 --- /dev/null +++ b/test/governance/extensions/GovernorNoncesKeyed.test.js @@ -0,0 +1,244 @@ +const { ethers } = require('hardhat'); +const { expect } = require('chai'); +const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers'); + +const { GovernorHelper } = require('../../helpers/governance'); +const { getDomain, Ballot, ExtendedBallot } = require('../../helpers/eip712'); +const { VoteType } = require('../../helpers/enums'); +const { shouldBehaveLikeNoncesKeyed } = require('../../utils/Nonces.behavior'); + +const name = 'OZ-Governor'; +const version = '1'; +const tokenName = 'MockToken'; +const tokenSymbol = 'MTKN'; +const tokenSupply = ethers.parseEther('100'); +const votingDelay = 4n; +const votingPeriod = 16n; +const value = ethers.parseEther('1'); + +const signBallot = account => (contract, message) => + getDomain(contract).then(domain => account.signTypedData(domain, { Ballot }, message)); +const signExtendedBallot = account => (contract, message) => + getDomain(contract).then(domain => account.signTypedData(domain, { ExtendedBallot }, message)); + +describe('GovernorNoncesKeyed', function () { + const fixture = async () => { + const [owner, proposer, voter1, voter2, voter3, voter4, userEOA] = await ethers.getSigners(); + const receiver = await ethers.deployContract('CallReceiverMock'); + + const token = await ethers.deployContract('$ERC20Votes', [tokenName, tokenSymbol, tokenName, version]); + const mock = await ethers.deployContract('$GovernorNoncesKeyedMock', [ + name, // name + votingDelay, // initialVotingDelay + votingPeriod, // initialVotingPeriod + 0n, // initialProposalThreshold + token, // tokenAddress + 10n, // quorumNumeratorValue + ]); + + await owner.sendTransaction({ to: mock, value }); + await token.$_mint(owner, tokenSupply); + + const helper = new GovernorHelper(mock, 'blocknumber'); + await helper.connect(owner).delegate({ token: token, to: voter1, value: ethers.parseEther('10') }); + await helper.connect(owner).delegate({ token: token, to: voter2, value: ethers.parseEther('7') }); + await helper.connect(owner).delegate({ token: token, to: voter3, value: ethers.parseEther('5') }); + await helper.connect(owner).delegate({ token: token, to: voter4, value: ethers.parseEther('2') }); + + return { + owner, + proposer, + voter1, + voter2, + voter3, + voter4, + userEOA, + receiver, + token, + mock, + helper, + }; + }; + + beforeEach(async function () { + Object.assign(this, await loadFixture(fixture)); + + // default proposal + this.proposal = this.helper.setProposal( + [ + { + target: this.receiver.target, + value, + data: this.receiver.interface.encodeFunctionData('mockFunction'), + }, + ], + '', + ); + }); + + it('deployment check', async function () { + await expect(this.mock.name()).to.eventually.equal(name); + await expect(this.mock.token()).to.eventually.equal(this.token); + await expect(this.mock.votingDelay()).to.eventually.equal(votingDelay); + await expect(this.mock.votingPeriod()).to.eventually.equal(votingPeriod); + }); + + describe('vote with signature', function () { + for (const nonceType of ['default', 'keyed']) { + describe(`with ${nonceType} nonce`, function () { + beforeEach(async function () { + await this.helper.propose(); + + const maskedProposalId = BigInt(this.helper.id) & (2n ** 192n - 1n); + + this.getNonce = async address => { + return await (nonceType === 'default' + ? this.mock.nonces(address) + : this.mock['nonces(address,uint192)'](address, maskedProposalId)); + }; + }); + + it('votes with an EOA signature', async function () { + await this.token.connect(this.voter1).delegate(this.userEOA); + + const nonce = await this.getNonce(this.userEOA); + + await this.helper.waitForSnapshot(); + await expect( + this.helper.vote({ + support: VoteType.For, + voter: this.userEOA.address, + nonce, + signature: signBallot(this.userEOA), + }), + ) + .to.emit(this.mock, 'VoteCast') + .withArgs(this.userEOA, this.proposal.id, VoteType.For, ethers.parseEther('10'), ''); + + await this.helper.waitForDeadline(); + await this.helper.execute(); + + // After + expect(await this.mock.hasVoted(this.proposal.id, this.userEOA)).to.be.true; + expect(await this.getNonce(this.userEOA)).to.equal(nonce + 1n); + }); + + it('votes with an EOA signature with reason', async function () { + await this.token.connect(this.voter1).delegate(this.userEOA); + + const nonce = await this.getNonce(this.userEOA); + + await this.helper.waitForSnapshot(); + await expect( + this.helper.vote({ + support: VoteType.For, + voter: this.userEOA.address, + nonce, + reason: 'This is an example reason', + signature: signExtendedBallot(this.userEOA), + }), + ) + .to.emit(this.mock, 'VoteCast') + .withArgs( + this.userEOA, + this.proposal.id, + VoteType.For, + ethers.parseEther('10'), + 'This is an example reason', + ); + + await this.helper.waitForDeadline(); + await this.helper.execute(); + + // After + expect(await this.mock.hasVoted(this.proposal.id, this.userEOA)).to.be.true; + expect(await this.getNonce(this.userEOA)).to.equal(nonce + 1n); + }); + + it('votes with a valid EIP-1271 signature', async function () { + const wallet = await ethers.deployContract('ERC1271WalletMock', [this.userEOA]); + + await this.token.connect(this.voter1).delegate(wallet); + + const nonce = await this.getNonce(wallet.target); + + await this.helper.waitForSnapshot(); + await expect( + this.helper.vote({ + support: VoteType.For, + voter: wallet.target, + nonce, + signature: signBallot(this.userEOA), + }), + ) + .to.emit(this.mock, 'VoteCast') + .withArgs(wallet, this.proposal.id, VoteType.For, ethers.parseEther('10'), ''); + await this.helper.waitForDeadline(); + await this.helper.execute(); + + // After + expect(await this.mock.hasVoted(this.proposal.id, wallet)).to.be.true; + expect(await this.getNonce(wallet)).to.equal(nonce + 1n); + }); + + afterEach('no other votes are cast', async function () { + expect(await this.mock.hasVoted(this.proposal.id, this.owner)).to.be.false; + expect(await this.mock.hasVoted(this.proposal.id, this.voter1)).to.be.false; + expect(await this.mock.hasVoted(this.proposal.id, this.voter2)).to.be.false; + }); + }); + } + }); + + describe('on vote by signature', function () { + beforeEach(async function () { + await this.token.connect(this.voter1).delegate(this.userEOA); + + // Run proposal + await this.helper.propose(); + await this.helper.waitForSnapshot(); + }); + + it('if signature does not match signer', async function () { + const nonce = await this.mock.nonces(this.userEOA); + + function tamper(str, index, mask) { + const arrayStr = ethers.getBytes(str); + arrayStr[index] ^= mask; + return ethers.hexlify(arrayStr); + } + + const voteParams = { + support: VoteType.For, + voter: this.userEOA.address, + nonce, + signature: (...args) => signBallot(this.userEOA)(...args).then(sig => tamper(sig, 42, 0xff)), + }; + + await expect(this.helper.vote(voteParams)) + .to.be.revertedWithCustomError(this.mock, 'GovernorInvalidSignature') + .withArgs(voteParams.voter); + }); + + for (const nonceType of ['default', 'keyed']) { + it(`if vote nonce is incorrect with ${nonceType} nonce`, async function () { + const nonce = await (nonceType === 'default' + ? this.mock.nonces(this.userEOA) + : this.mock['nonces(address,uint192)'](this.userEOA, BigInt(this.helper.id) & (2n ** 192n - 1n))); + + const voteParams = { + support: VoteType.For, + voter: this.userEOA.address, + nonce: nonce + 1n, + signature: signBallot(this.userEOA), + }; + + await expect(this.helper.vote(voteParams)) + .to.be.revertedWithCustomError(this.mock, 'GovernorInvalidSignature') + .withArgs(voteParams.voter); + }); + } + }); + + shouldBehaveLikeNoncesKeyed(); +});