NoBey/openzeppelin-contracts
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Enable ERC-1271 signature checks in Governor castVoteBySig (#4418)

Browse Source 
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Co-authored-by: Francisco <fg@frang.io>
This commit is contained in:
Ernesto García
2023-07-05 07:11:29 -06:00
committed by GitHub
parent 90163661df
commit 63bfab1a0c
8 changed files with 215 additions and 139 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
.changeset/popular-deers-raise.md Normal file
View File

@ -0,0 +1,5 @@
---
'openzeppelin-solidity': major
---
`Governor`: Add support for casting votes with ERC-1271 signatures by using a `bytes memory signature` instead of `r`, `s` and `v` arguments in the `castVoteBySig` and `castVoteWithReasonAndParamsBySig` functions.

36
contracts/governance/Governor.sol
View File

@ -5,8 +5,8 @@ pragma solidity ^0.8.19;
import {IERC721Receiver} from "../token/ERC721/IERC721Receiver.sol";
import {IERC1155Receiver} from "../token/ERC1155/IERC1155Receiver.sol";
import {ECDSA} from "../utils/cryptography/ECDSA.sol";
import {EIP712} from "../utils/cryptography/EIP712.sol";
import {SignatureChecker} from "../utils/cryptography/SignatureChecker.sol";
import {IERC165, ERC165} from "../utils/introspection/ERC165.sol";
import {SafeCast} from "../utils/math/SafeCast.sol";
import {DoubleEndedQueue} from "../utils/structs/DoubleEndedQueue.sol";
@ -519,22 +519,19 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72
uint256 proposalId,
uint8 support,
address voter,
uint8 v,
bytes32 r,
bytes32 s
bytes memory signature
) public virtual override returns (uint256) {
address signer = ECDSA.recover(
bool valid = SignatureChecker.isValidSignatureNow(
voter,
_hashTypedDataV4(keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support, voter, _useNonce(voter)))),
v,
r,
s
signature
);
if (voter != signer) {
revert GovernorInvalidSigner(signer, voter);
if (!valid) {
revert GovernorInvalidSignature(voter);
}
return _castVote(proposalId, signer, support, "");
return _castVote(proposalId, voter, support, "");
}
/**
@ -546,11 +543,10 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72
address voter,
string calldata reason,
bytes memory params,
uint8 v,
bytes32 r,
bytes32 s
bytes memory signature
) public virtual override returns (uint256) {
address signer = ECDSA.recover(
bool valid = SignatureChecker.isValidSignatureNow(
voter,
_hashTypedDataV4(
keccak256(
abi.encode(
@ -564,16 +560,14 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72
)
)
),
v,
r,
s
signature
);
if (voter != signer) {
revert GovernorInvalidSigner(signer, voter);
if (!valid) {
revert GovernorInvalidSignature(voter);
}
return _castVote(proposalId, signer, support, reason, params);
return _castVote(proposalId, voter, support, reason, params);
}
/**

18
contracts/governance/IGovernor.sol
View File

@ -81,9 +81,10 @@ abstract contract IGovernor is IERC165, IERC6372 {
error GovernorInvalidVoteType();
/**
* @dev The `voter` doesn't match with the recovered `signer`.
* @dev The provided signature is not valid for the expected `voter`.
* If the `voter` is a contract, the signature is not valid using {IERC1271-isValidSignature}.
*/
error GovernorInvalidSigner(address signer, address voter);
error GovernorInvalidSignature(address voter);
/**
* @dev Emitted when a proposal is created.
@ -353,7 +354,7 @@ abstract contract IGovernor is IERC165, IERC6372 {
) public virtual returns (uint256 balance);
/**
* @dev Cast a vote using the user's cryptographic signature.
* @dev Cast a vote using the voter's signature, including ERC-1271 signature support.
*
* Emits a {VoteCast} event.
*/
@ -361,13 +362,12 @@ abstract contract IGovernor is IERC165, IERC6372 {
uint256 proposalId,
uint8 support,
address voter,
uint8 v,
bytes32 r,
bytes32 s
bytes memory signature
) public virtual returns (uint256 balance);
/**
* @dev Cast a vote with a reason and additional encoded parameters using the user's cryptographic signature.
* @dev Cast a vote with a reason and additional encoded parameters using the voter's signature,
* including ERC-1271 signature support.
*
* Emits a {VoteCast} or {VoteCastWithParams} event depending on the length of params.
*/
@ -377,8 +377,6 @@ abstract contract IGovernor is IERC165, IERC6372 {
address voter,
string calldata reason,
bytes memory params,
uint8 v,
bytes32 r,
bytes32 s
bytes memory signature
) public virtual returns (uint256 balance);
}

2
contracts/utils/cryptography/SignatureChecker.sol
View File

@ -9,7 +9,7 @@ import {IERC1271} from "../../interfaces/IERC1271.sol";
/**
* @dev Signature verification helper that can be used instead of `ECDSA.recover` to seamlessly support both ECDSA
* signatures from externally owned accounts (EOAs) as well as ERC1271 signatures from smart contract wallets like
* Argent and Gnosis Safe.
* Argent and Safe Wallet (previously Gnosis Safe).
*
* _Available since v4.1._
*/

179
test/governance/Governor.test.js
View File

@ -2,7 +2,6 @@ const { constants, expectEvent, expectRevert } = require('@openzeppelin/test-hel
const { expect } = require('chai');
const ethSigUtil = require('eth-sig-util');
const Wallet = require('ethereumjs-wallet').default;
const { fromRpcSig, toRpcSig } = require('ethereumjs-util');
const Enums = require('../helpers/enums');
const { getDomain, domainType } = require('../helpers/eip712');
@ -18,6 +17,7 @@ const Governor = artifacts.require('$GovernorMock');
const CallReceiver = artifacts.require('CallReceiverMock');
const ERC721 = artifacts.require('$ERC721');
const ERC1155 = artifacts.require('$ERC1155');
const ERC1271WalletMock = artifacts.require('ERC1271WalletMock');
const TOKENS = [
{ Token: artifacts.require('$ERC20Votes'), mode: 'blocknumber' },
@ -166,55 +166,6 @@ contract('Governor', function (accounts) {
expect(await web3.eth.getBalance(this.receiver.address)).to.be.bignumber.equal(value);
});
it('votes with signature', async function () {
const voterBySig = Wallet.generate();
const voterBySigAddress = web3.utils.toChecksumAddress(voterBySig.getAddressString());
const signature = (contract, message) =>
getDomain(contract)
.then(domain => ({
primaryType: 'Ballot',
types: {
EIP712Domain: domainType(domain),
Ballot: [
{ name: 'proposalId', type: 'uint256' },
{ name: 'support', type: 'uint8' },
{ name: 'voter', type: 'address' },
{ name: 'nonce', type: 'uint256' },
],
},
domain,
message,
}))
.then(data => ethSigUtil.signTypedMessage(voterBySig.getPrivateKey(), { data }))
.then(fromRpcSig);
await this.token.delegate(voterBySigAddress, { from: voter1 });
const nonce = await this.mock.nonces(voterBySigAddress);
// Run proposal
await this.helper.propose();
await this.helper.waitForSnapshot();
expectEvent(
await this.helper.vote({ support: Enums.VoteType.For, voter: voterBySigAddress, nonce, signature }),
'VoteCast',
{
voter: voterBySigAddress,
support: Enums.VoteType.For,
},
);
await this.helper.waitForDeadline();
await this.helper.execute();
// After
expect(await this.mock.hasVoted(this.proposal.id, owner)).to.be.equal(false);
expect(await this.mock.hasVoted(this.proposal.id, voter1)).to.be.equal(false);
expect(await this.mock.hasVoted(this.proposal.id, voter2)).to.be.equal(false);
expect(await this.mock.hasVoted(this.proposal.id, voterBySigAddress)).to.be.equal(true);
expect(await this.mock.nonces(voterBySigAddress)).to.be.bignumber.equal(nonce.addn(1));
});
it('send ethers', async function () {
const empty = web3.utils.toChecksumAddress(web3.utils.randomHex(20));
@ -244,6 +195,106 @@ contract('Governor', function (accounts) {
expect(await web3.eth.getBalance(empty)).to.be.bignumber.equal(value);
});
describe('vote with signature', function () {
const sign = privateKey => async (contract, message) => {
const domain = await getDomain(contract);
return ethSigUtil.signTypedMessage(privateKey, {
data: {
primaryType: 'Ballot',
types: {
EIP712Domain: domainType(domain),
Ballot: [
{ name: 'proposalId', type: 'uint256' },
{ name: 'support', type: 'uint8' },
{ name: 'voter', type: 'address' },
{ name: 'nonce', type: 'uint256' },
],
},
domain,
message,
},
});
};
afterEach('no other votes are cast for proposalId', async function () {
expect(await this.mock.hasVoted(this.proposal.id, owner)).to.be.equal(false);
expect(await this.mock.hasVoted(this.proposal.id, voter1)).to.be.equal(false);
expect(await this.mock.hasVoted(this.proposal.id, voter2)).to.be.equal(false);
});
it('votes with an EOA signature', async function () {
const voterBySig = Wallet.generate();
const voterBySigAddress = web3.utils.toChecksumAddress(voterBySig.getAddressString());
await this.token.delegate(voterBySigAddress, { from: voter1 });
const nonce = await this.mock.nonces(voterBySigAddress);
// Run proposal
await this.helper.propose();
await this.helper.waitForSnapshot();
expectEvent(
await this.helper.vote({
support: Enums.VoteType.For,
voter: voterBySigAddress,
nonce,
signature: sign(voterBySig.getPrivateKey()),
}),
'VoteCast',
{
voter: voterBySigAddress,
support: Enums.VoteType.For,
},
);
await this.helper.waitForDeadline();
await this.helper.execute();
// After
expect(await this.mock.hasVoted(this.proposal.id, voterBySigAddress)).to.be.equal(true);
expect(await this.mock.nonces(voterBySigAddress)).to.be.bignumber.equal(nonce.addn(1));
});
it('votes with a valid EIP-1271 signature', async function () {
const ERC1271WalletOwner = Wallet.generate();
ERC1271WalletOwner.address = web3.utils.toChecksumAddress(ERC1271WalletOwner.getAddressString());
const wallet = await ERC1271WalletMock.new(ERC1271WalletOwner.address);
await this.token.delegate(wallet.address, { from: voter1 });
const nonce = await this.mock.nonces(wallet.address);
// Run proposal
await this.helper.propose();
await this.helper.waitForSnapshot();
expectEvent(
await this.helper.vote({
support: Enums.VoteType.For,
voter: wallet.address,
nonce,
signature: sign(ERC1271WalletOwner.getPrivateKey()),
}),
'VoteCast',
{
voter: wallet.address,
support: Enums.VoteType.For,
},
);
await this.helper.waitForDeadline();
await this.helper.execute();
// After
expect(await this.mock.hasVoted(this.proposal.id, wallet.address)).to.be.equal(true);
expect(await this.mock.nonces(wallet.address)).to.be.bignumber.equal(nonce.addn(1));
});
afterEach('no other votes are cast', async function () {
expect(await this.mock.hasVoted(this.proposal.id, owner)).to.be.equal(false);
expect(await this.mock.hasVoted(this.proposal.id, voter1)).to.be.equal(false);
expect(await this.mock.hasVoted(this.proposal.id, voter2)).to.be.equal(false);
});
});
describe('should revert', function () {
describe('on propose', function () {
it('if proposal already exists', async function () {
@ -328,9 +379,9 @@ contract('Governor', function (accounts) {
}));
this.signature = (contract, message) =>
this.data(contract, message)
.then(data => ethSigUtil.signTypedMessage(this.voterBySig.getPrivateKey(), { data }))
.then(fromRpcSig);
this.data(contract, message).then(data =>
ethSigUtil.signTypedMessage(this.voterBySig.getPrivateKey(), { data }),
);
await this.token.delegate(this.voterBySig.address, { from: voter1 });
@ -348,19 +399,13 @@ contract('Governor', function (accounts) {
nonce,
signature: async (...params) => {
const sig = await this.signature(...params);
sig.s[12] ^= 0xff;
return sig;
const tamperedSig = web3.utils.hexToBytes(sig);
tamperedSig[42] ^= 0xff;
return web3.utils.bytesToHex(tamperedSig);
},
};
const { r, s, v } = await this.helper.sign(voteParams);
const message = this.helper.forgeMessage(voteParams);
const data = await this.data(this.mock, message);
await expectRevertCustomError(this.helper.vote(voteParams), 'GovernorInvalidSigner', [
ethSigUtil.recoverTypedSignature({ sig: toRpcSig(v, r, s), data }),
voteParams.voter,
]);
await expectRevertCustomError(this.helper.vote(voteParams), 'GovernorInvalidSignature', [voteParams.voter]);
});
it('if vote nonce is incorrect', async function () {
@ -373,15 +418,11 @@ contract('Governor', function (accounts) {
signature: this.signature,
};
const { r, s, v } = await this.helper.sign(voteParams);
const message = this.helper.forgeMessage(voteParams);
const data = await this.data(this.mock, { ...message, nonce });
await expectRevertCustomError(
this.helper.vote(voteParams),
// The signature check implies the nonce can't be tampered without changing the signer
'GovernorInvalidSigner',
[ethSigUtil.recoverTypedSignature({ sig: toRpcSig(v, r, s), data }), voteParams.voter],
'GovernorInvalidSignature',
[voteParams.voter],
);
});
});

100
test/governance/extensions/GovernorWithParams.test.js
View File

@ -2,7 +2,6 @@ const { expectEvent } = require('@openzeppelin/test-helpers');
const { expect } = require('chai');
const ethSigUtil = require('eth-sig-util');
const Wallet = require('ethereumjs-wallet').default;
const { fromRpcSig, toRpcSig } = require('ethereumjs-util');
const Enums = require('../../helpers/enums');
const { getDomain, domainType } = require('../../helpers/eip712');
@ -11,6 +10,7 @@ const { expectRevertCustomError } = require('../../helpers/customError');
const Governor = artifacts.require('$GovernorWithParamsMock');
const CallReceiver = artifacts.require('CallReceiverMock');
const ERC1271WalletMock = artifacts.require('ERC1271WalletMock');
const rawParams = {
uintParam: web3.utils.toBN('42'),
@ -143,30 +143,27 @@ contract('GovernorWithParams', function (accounts) {
message,
}));
this.signature = (contract, message) =>
this.data(contract, message)
.then(data => ethSigUtil.signTypedMessage(this.voterBySig.getPrivateKey(), { data }))
.then(fromRpcSig);
await this.token.delegate(this.voterBySig.address, { from: voter2 });
// Run proposal
await this.helper.propose();
await this.helper.waitForSnapshot();
this.sign = privateKey => async (contract, message) =>
ethSigUtil.signTypedMessage(privateKey, { data: await this.data(contract, message) });
});
it('is properly supported', async function () {
it('suports EOA signatures', async function () {
await this.token.delegate(this.voterBySig.address, { from: voter2 });
const weight = web3.utils.toBN(web3.utils.toWei('7')).sub(rawParams.uintParam);
const nonce = await this.mock.nonces(this.voterBySig.address);
// Run proposal
await this.helper.propose();
await this.helper.waitForSnapshot();
const tx = await this.helper.vote({
support: Enums.VoteType.For,
voter: this.voterBySig.address,
nonce,
reason: 'no particular reason',
params: encodedParams,
signature: this.signature,
signature: this.sign(this.voterBySig.getPrivateKey()),
});
expectEvent(tx, 'CountParams', { ...rawParams });
@ -184,53 +181,94 @@ contract('GovernorWithParams', function (accounts) {
expect(await this.mock.nonces(this.voterBySig.address)).to.be.bignumber.equal(nonce.addn(1));
});
it('supports EIP-1271 signature signatures', async function () {
const ERC1271WalletOwner = Wallet.generate();
ERC1271WalletOwner.address = web3.utils.toChecksumAddress(ERC1271WalletOwner.getAddressString());
const wallet = await ERC1271WalletMock.new(ERC1271WalletOwner.address);
await this.token.delegate(wallet.address, { from: voter2 });
const weight = web3.utils.toBN(web3.utils.toWei('7')).sub(rawParams.uintParam);
const nonce = await this.mock.nonces(wallet.address);
// Run proposal
await this.helper.propose();
await this.helper.waitForSnapshot();
const tx = await this.helper.vote({
support: Enums.VoteType.For,
voter: wallet.address,
nonce,
reason: 'no particular reason',
params: encodedParams,
signature: this.sign(ERC1271WalletOwner.getPrivateKey()),
});
expectEvent(tx, 'CountParams', { ...rawParams });
expectEvent(tx, 'VoteCastWithParams', {
voter: wallet.address,
proposalId: this.proposal.id,
support: Enums.VoteType.For,
weight,
reason: 'no particular reason',
params: encodedParams,
});
const votes = await this.mock.proposalVotes(this.proposal.id);
expect(votes.forVotes).to.be.bignumber.equal(weight);
expect(await this.mock.nonces(wallet.address)).to.be.bignumber.equal(nonce.addn(1));
});
it('reverts if signature does not match signer', async function () {
await this.token.delegate(this.voterBySig.address, { from: voter2 });
const nonce = await this.mock.nonces(this.voterBySig.address);
const signature = this.sign(this.voterBySig.getPrivateKey());
// Run proposal
await this.helper.propose();
await this.helper.waitForSnapshot();
const voteParams = {
support: Enums.VoteType.For,
voter: this.voterBySig.address,
nonce,
signature: async (...params) => {
const sig = await this.signature(...params);
sig.s[12] ^= 0xff;
return sig;
const sig = await signature(...params);
const tamperedSig = web3.utils.hexToBytes(sig);
tamperedSig[42] ^= 0xff;
return web3.utils.bytesToHex(tamperedSig);
},
reason: 'no particular reason',
params: encodedParams,
};
const { r, s, v } = await this.helper.sign(voteParams);
const message = this.helper.forgeMessage(voteParams);
const data = await this.data(this.mock, message);
await expectRevertCustomError(this.helper.vote(voteParams), 'GovernorInvalidSigner', [
ethSigUtil.recoverTypedSignature({ sig: toRpcSig(v, r, s), data }),
voteParams.voter,
]);
await expectRevertCustomError(this.helper.vote(voteParams), 'GovernorInvalidSignature', [voteParams.voter]);
});
it('reverts if vote nonce is incorrect', async function () {
await this.token.delegate(this.voterBySig.address, { from: voter2 });
const nonce = await this.mock.nonces(this.voterBySig.address);
// Run proposal
await this.helper.propose();
await this.helper.waitForSnapshot();
const voteParams = {
support: Enums.VoteType.For,
voter: this.voterBySig.address,
nonce: nonce.addn(1),
signature: this.signature,
signature: this.sign(this.voterBySig.getPrivateKey()),
reason: 'no particular reason',
params: encodedParams,
};
const { r, s, v } = await this.helper.sign(voteParams);
const message = this.helper.forgeMessage(voteParams);
const data = await this.data(this.mock, { ...message, nonce });
await expectRevertCustomError(
this.helper.vote(voteParams),
// The signature check implies the nonce can't be tampered without changing the signer
'GovernorInvalidSigner',
[ethSigUtil.recoverTypedSignature({ sig: toRpcSig(v, r, s), data }), voteParams.voter],
'GovernorInvalidSignature',
[voteParams.voter],
);
});
});

8
test/helpers/governance.js
View File

@ -91,16 +91,16 @@ class GovernorHelper {
return vote.signature
? // if signature, and either params or reason →
vote.params || vote.reason
? this.sign(vote).then(({ v, r, s }) =>
? this.sign(vote).then(signature =>
this.governor.castVoteWithReasonAndParamsBySig(
...concatOpts(
[proposal.id, vote.support, vote.voter, vote.reason || '', vote.params || '', v, r, s],
[proposal.id, vote.support, vote.voter, vote.reason || '', vote.params || '', signature],
opts,
),
),
)
: this.sign(vote).then(({ v, r, s }) =>
this.governor.castVoteBySig(...concatOpts([proposal.id, vote.support, vote.voter, v, r, s], opts)),
: this.sign(vote).then(signature =>
this.governor.castVoteBySig(...concatOpts([proposal.id, vote.support, vote.voter, signature], opts)),
)
: vote.params
? // otherwise if params

6
test/utils/introspection/SupportsInterface.behavior.js
View File

@ -67,7 +67,7 @@ const INTERFACES = {
'execute(address[],uint256[],bytes[],bytes32)',
'castVote(uint256,uint8)',
'castVoteWithReason(uint256,uint8,string)',
'castVoteBySig(uint256,uint8,address,uint8,bytes32,bytes32)',
'castVoteBySig(uint256,uint8,address,bytes)',
],
GovernorWithParams: [
'name()',
@ -88,8 +88,8 @@ const INTERFACES = {
'castVote(uint256,uint8)',
'castVoteWithReason(uint256,uint8,string)',
'castVoteWithReasonAndParams(uint256,uint8,string,bytes)',
'castVoteBySig(uint256,uint8,address,uint8,bytes32,bytes32)',
'castVoteWithReasonAndParamsBySig(uint256,uint8,address,string,bytes,uint8,bytes32,bytes32)',
'castVoteBySig(uint256,uint8,address,bytes)',
'castVoteWithReasonAndParamsBySig(uint256,uint8,address,string,bytes,bytes)',
],
GovernorCancel: ['proposalProposer(uint256)', 'cancel(address[],uint256[],bytes[],bytes32)'],
GovernorTimelock: ['timelock()', 'proposalEta(uint256)', 'queue(address[],uint256[],bytes[],bytes32)'],