From 0619b5cb0c3ce4f3df282767af70fc22713b149f Mon Sep 17 00:00:00 2001 From: ernestognw Date: Wed, 21 Jun 2023 19:41:48 -0600 Subject: [PATCH 01/17] Add Governor signature nonces --- contracts/governance/Governor.sol | 37 +++-- contracts/governance/IGovernor.sol | 7 + test/governance/Governor.test.js | 86 +++++++++++- .../extensions/GovernorWithParams.test.js | 131 ++++++++++++------ test/helpers/governance.js | 11 +- .../SupportsInterface.behavior.js | 6 +- 6 files changed, 218 insertions(+), 60 deletions(-) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index a8654ac35bb..47927936dde 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -12,6 +12,7 @@ import "../utils/math/SafeCast.sol"; import "../utils/structs/DoubleEndedQueue.sol"; import "../utils/Address.sol"; import "../utils/Context.sol"; +import "../utils/Nonces.sol"; import "./IGovernor.sol"; /** @@ -25,12 +26,15 @@ import "./IGovernor.sol"; * * _Available since v4.3._ */ -abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receiver, IERC1155Receiver { +abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC721Receiver, IERC1155Receiver { using DoubleEndedQueue for DoubleEndedQueue.Bytes32Deque; - bytes32 public constant BALLOT_TYPEHASH = keccak256("Ballot(uint256 proposalId,uint8 support)"); + bytes32 public constant BALLOT_TYPEHASH = + keccak256("Ballot(uint256 proposalId,uint8 support,address voter,uint256 nonce)"); bytes32 public constant EXTENDED_BALLOT_TYPEHASH = - keccak256("ExtendedBallot(uint256 proposalId,uint8 support,string reason,bytes params)"); + keccak256( + "ExtendedBallot(uint256 proposalId,uint8 support,address voter,uint256 nonce,string reason,bytes params)" + ); // solhint-disable var-name-mixedcase struct ProposalCore { @@ -519,17 +523,25 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive function castVoteBySig( uint256 proposalId, uint8 support, + address voter, uint8 v, bytes32 r, bytes32 s ) public virtual override returns (uint256) { - address voter = ECDSA.recover( - _hashTypedDataV4(keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support))), + uint256 currentNonce = _useNonce(voter); + + address signer = ECDSA.recover( + _hashTypedDataV4(keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support, voter, currentNonce))), v, r, s ); - return _castVote(proposalId, voter, support, ""); + + if (voter != signer) { + revert GovernorInvalidSigner(signer, voter); + } + + return _castVote(proposalId, signer, support, ""); } /** @@ -538,19 +550,24 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive function castVoteWithReasonAndParamsBySig( uint256 proposalId, uint8 support, + address voter, string calldata reason, bytes memory params, uint8 v, bytes32 r, bytes32 s ) public virtual override returns (uint256) { - address voter = ECDSA.recover( + uint256 currentNonce = _useNonce(voter); + + address signer = ECDSA.recover( _hashTypedDataV4( keccak256( abi.encode( EXTENDED_BALLOT_TYPEHASH, proposalId, support, + voter, + currentNonce, keccak256(bytes(reason)), keccak256(params) ) @@ -561,7 +578,11 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive s ); - return _castVote(proposalId, voter, support, reason, params); + if (voter != signer) { + revert GovernorInvalidSigner(signer, voter); + } + + return _castVote(proposalId, signer, support, reason, params); } /** diff --git a/contracts/governance/IGovernor.sol b/contracts/governance/IGovernor.sol index 5b51f088ae2..99d66ae36e8 100644 --- a/contracts/governance/IGovernor.sol +++ b/contracts/governance/IGovernor.sol @@ -80,6 +80,11 @@ abstract contract IGovernor is IERC165, IERC6372 { */ error GovernorInvalidVoteType(); + /** + * @dev The `voter` doesn't match with the recovered `signer`. + */ + error GovernorInvalidSigner(address signer, address voter); + /** * @dev Emitted when a proposal is created. */ @@ -348,6 +353,7 @@ abstract contract IGovernor is IERC165, IERC6372 { function castVoteBySig( uint256 proposalId, uint8 support, + address voter, uint8 v, bytes32 r, bytes32 s @@ -361,6 +367,7 @@ abstract contract IGovernor is IERC165, IERC6372 { function castVoteWithReasonAndParamsBySig( uint256 proposalId, uint8 support, + address voter, string calldata reason, bytes memory params, uint8 v, diff --git a/test/governance/Governor.test.js b/test/governance/Governor.test.js index 909c386862d..fb39a84a1ee 100644 --- a/test/governance/Governor.test.js +++ b/test/governance/Governor.test.js @@ -166,7 +166,7 @@ contract('Governor', function (accounts) { expect(await web3.eth.getBalance(this.receiver.address)).to.be.bignumber.equal(value); }); - it('vote with signature', async function () { + it('votes with signature', async function () { const voterBySig = Wallet.generate(); const voterBySigAddress = web3.utils.toChecksumAddress(voterBySig.getAddressString()); @@ -179,6 +179,8 @@ contract('Governor', function (accounts) { Ballot: [ { name: 'proposalId', type: 'uint256' }, { name: 'support', type: 'uint8' }, + { name: 'voter', type: 'address' }, + { name: 'nonce', type: 'uint256' }, ], }, domain, @@ -189,13 +191,19 @@ contract('Governor', function (accounts) { 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, signature }), 'VoteCast', { - voter: voterBySigAddress, - support: Enums.VoteType.For, - }); + expectEvent( + await this.helper.vote({ support: Enums.VoteType.For, voter: voterBySigAddress, nonce: nonce, signature }), + 'VoteCast', + { + voter: voterBySigAddress, + support: Enums.VoteType.For, + }, + ); await this.helper.waitForDeadline(); await this.helper.execute(); @@ -204,6 +212,7 @@ contract('Governor', function (accounts) { 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 () { @@ -297,6 +306,73 @@ contract('Governor', function (accounts) { }); }); + describe('on vote by signature', function () { + beforeEach(async function () { + this.voterBySig = Wallet.generate(); + this.voterBySig.address = web3.utils.toChecksumAddress(this.voterBySig.getAddressString()); + + this.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(this.voterBySig.getPrivateKey(), { data })) + .then(fromRpcSig); + + await this.token.delegate(this.voterBySig.address, { from: voter1 }); + + // 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.voterBySig.address); + + expectRevertCustomError( + this.helper.vote({ + support: Enums.VoteType.For, + voter: this.voterBySig.address, + nonce, + signature: (...params) => { + const sig = this.signature(...params); + sig[69] ^= 0xff; + return sig; + }, + }), + 'GovernorInvalidSigner', + [], + ); + }); + + it('if vote nonce is incorrect', async function () { + const nonce = await this.mock.nonces(this.voterBySig.address); + + expectRevertCustomError( + this.helper.vote({ + support: Enums.VoteType.For, + voter: this.voterBySig.address, + nonce: nonce.addn(1), + signature: this.signature, + }), + // The signature check implies the nonce can't be tampered without changing the signer + 'GovernorInvalidSigner', + [], + ); + }); + }); + describe('on execute', function () { it('if proposal does not exist', async function () { await expectRevertCustomError(this.helper.execute(), 'GovernorNonexistentProposal', [this.proposal.id]); diff --git a/test/governance/extensions/GovernorWithParams.test.js b/test/governance/extensions/GovernorWithParams.test.js index 00936c505bc..949f2c968dc 100644 --- a/test/governance/extensions/GovernorWithParams.test.js +++ b/test/governance/extensions/GovernorWithParams.test.js @@ -7,6 +7,7 @@ const { fromRpcSig } = require('ethereumjs-util'); const Enums = require('../../helpers/enums'); const { getDomain, domainType } = require('../../helpers/eip712'); const { GovernorHelper } = require('../../helpers/governance'); +const { expectRevertCustomError } = require('../../helpers/customError'); const Governor = artifacts.require('$GovernorWithParamsMock'); const CallReceiver = artifacts.require('CallReceiverMock'); @@ -119,56 +120,102 @@ contract('GovernorWithParams', function (accounts) { expect(votes.forVotes).to.be.bignumber.equal(weight); }); - it('Voting with params by signature is properly supported', async function () { - const voterBySig = Wallet.generate(); - const voterBySigAddress = web3.utils.toChecksumAddress(voterBySig.getAddressString()); - - const signature = (contract, message) => - getDomain(contract) - .then(domain => ({ - primaryType: 'ExtendedBallot', - types: { - EIP712Domain: domainType(domain), - ExtendedBallot: [ - { name: 'proposalId', type: 'uint256' }, - { name: 'support', type: 'uint8' }, - { name: 'reason', type: 'string' }, - { name: 'params', type: 'bytes' }, - ], - }, - domain, - message, - })) - .then(data => ethSigUtil.signTypedMessage(voterBySig.getPrivateKey(), { data })) - .then(fromRpcSig); + describe('voting by signature', function () { + beforeEach(async function () { + this.voterBySig = Wallet.generate(); + this.voterBySig.address = web3.utils.toChecksumAddress(this.voterBySig.getAddressString()); + + this.signature = (contract, message) => + getDomain(contract) + .then(domain => ({ + primaryType: 'ExtendedBallot', + types: { + EIP712Domain: domainType(domain), + ExtendedBallot: [ + { name: 'proposalId', type: 'uint256' }, + { name: 'support', type: 'uint8' }, + { name: 'voter', type: 'address' }, + { name: 'nonce', type: 'uint256' }, + { name: 'reason', type: 'string' }, + { name: 'params', type: 'bytes' }, + ], + }, + domain, + 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(); + }); - await this.token.delegate(voterBySigAddress, { from: voter2 }); + it('is properly supported', async function () { + const weight = web3.utils.toBN(web3.utils.toWei('7')).sub(rawParams.uintParam); - // Run proposal - await this.helper.propose(); - await this.helper.waitForSnapshot(); + const nonce = await this.mock.nonces(this.voterBySig.address); - const weight = web3.utils.toBN(web3.utils.toWei('7')).sub(rawParams.uintParam); + const tx = await this.helper.vote({ + support: Enums.VoteType.For, + voter: this.voterBySig.address, + nonce: nonce, + reason: 'no particular reason', + params: encodedParams, + signature: this.signature, + }); - const tx = await this.helper.vote({ - support: Enums.VoteType.For, - reason: 'no particular reason', - params: encodedParams, - signature, + expectEvent(tx, 'CountParams', { ...rawParams }); + expectEvent(tx, 'VoteCastWithParams', { + voter: this.voterBySig.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(this.voterBySig.address)).to.be.bignumber.equal(nonce.addn(1)); }); - expectEvent(tx, 'CountParams', { ...rawParams }); - expectEvent(tx, 'VoteCastWithParams', { - voter: voterBySigAddress, - proposalId: this.proposal.id, - support: Enums.VoteType.For, - weight, - reason: 'no particular reason', - params: encodedParams, + it('reverts if signature does not match signer', async function () { + const nonce = await this.mock.nonces(this.voterBySig.address); + + expectRevertCustomError( + this.helper.vote({ + support: Enums.VoteType.For, + voter: this.voterBySig.address, + nonce, + signature: (...params) => { + const sig = this.signature(...params); + sig[69] ^= 0xff; + return sig; + }, + }), + 'GovernorInvalidSigner', + [], + ); }); - const votes = await this.mock.proposalVotes(this.proposal.id); - expect(votes.forVotes).to.be.bignumber.equal(weight); + it('reverts if vote nonce is incorrect', async function () { + const nonce = await this.mock.nonces(this.voterBySig.address); + + expectRevertCustomError( + this.helper.vote({ + support: Enums.VoteType.For, + voter: this.voterBySig.address, + nonce: nonce.addn(1), + signature: this.signature, + }), + // The signature check implies the nonce can't be tampered without changing the signer + 'GovernorInvalidSigner', + [], + ); + }); }); }); } diff --git a/test/helpers/governance.js b/test/helpers/governance.js index 665c21605a3..333904dd8c1 100644 --- a/test/helpers/governance.js +++ b/test/helpers/governance.js @@ -95,21 +95,28 @@ class GovernorHelper { .signature(this.governor, { proposalId: proposal.id, support: vote.support, + voter: vote.voter, + nonce: vote.nonce, reason: vote.reason || '', params: vote.params || '', }) .then(({ v, r, s }) => this.governor.castVoteWithReasonAndParamsBySig( - ...concatOpts([proposal.id, vote.support, vote.reason || '', vote.params || '', v, r, s], opts), + ...concatOpts( + [proposal.id, vote.support, vote.voter, vote.reason || '', vote.params || '', v, r, s], + opts, + ), ), ) : vote .signature(this.governor, { proposalId: proposal.id, support: vote.support, + voter: vote.voter, + nonce: vote.nonce, }) .then(({ v, r, s }) => - this.governor.castVoteBySig(...concatOpts([proposal.id, vote.support, v, r, s], opts)), + this.governor.castVoteBySig(...concatOpts([proposal.id, vote.support, vote.voter, v, r, s], opts)), ) : vote.params ? // otherwise if params diff --git a/test/utils/introspection/SupportsInterface.behavior.js b/test/utils/introspection/SupportsInterface.behavior.js index 201a55f4766..71be4313f03 100644 --- a/test/utils/introspection/SupportsInterface.behavior.js +++ b/test/utils/introspection/SupportsInterface.behavior.js @@ -66,7 +66,7 @@ const INTERFACES = { 'execute(address[],uint256[],bytes[],bytes32)', 'castVote(uint256,uint8)', 'castVoteWithReason(uint256,uint8,string)', - 'castVoteBySig(uint256,uint8,uint8,bytes32,bytes32)', + 'castVoteBySig(uint256,uint8,address,uint8,bytes32,bytes32)', ], GovernorWithParams: [ 'name()', @@ -87,8 +87,8 @@ const INTERFACES = { 'castVote(uint256,uint8)', 'castVoteWithReason(uint256,uint8,string)', 'castVoteWithReasonAndParams(uint256,uint8,string,bytes)', - 'castVoteBySig(uint256,uint8,uint8,bytes32,bytes32)', - 'castVoteWithReasonAndParamsBySig(uint256,uint8,string,bytes,uint8,bytes32,bytes32)', + 'castVoteBySig(uint256,uint8,address,uint8,bytes32,bytes32)', + 'castVoteWithReasonAndParamsBySig(uint256,uint8,address,string,bytes,uint8,bytes32,bytes32)', ], GovernorCancel: ['proposalProposer(uint256)', 'cancel(address[],uint256[],bytes[],bytes32)'], GovernorTimelock: ['timelock()', 'proposalEta(uint256)', 'queue(address[],uint256[],bytes[],bytes32)'], From 5ecb405ba6e76f90034bf543b4b04bf3025d604a Mon Sep 17 00:00:00 2001 From: ernestognw Date: Wed, 21 Jun 2023 19:47:49 -0600 Subject: [PATCH 02/17] Add changeset --- .changeset/sixty-numbers-reply.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/sixty-numbers-reply.md diff --git a/.changeset/sixty-numbers-reply.md b/.changeset/sixty-numbers-reply.md new file mode 100644 index 00000000000..285ee196109 --- /dev/null +++ b/.changeset/sixty-numbers-reply.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': major +--- + +`Governor`: Add a `voter` parameter to the `castVoteBySig` and `castVoteWithReasonAndParamsBySig` functions to avoid forging signatures for random addresses and allow the use of an internal nonce for invalidating signatures. From 9e9e238092815b4b6e5aa9a9e8ed36f6b8a94631 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Thu, 22 Jun 2023 13:11:41 -0600 Subject: [PATCH 03/17] Ignore initcode-size warnings --- hardhat.config.js | 1 + package-lock.json | 12 ++++++------ 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/hardhat.config.js b/hardhat.config.js index 6cb8b91441f..b101d264861 100644 --- a/hardhat.config.js +++ b/hardhat.config.js @@ -78,6 +78,7 @@ module.exports = { warnings: { 'contracts-exposed/**/*': { 'code-size': 'off', + 'initcode-size': 'off' }, '*': { 'code-size': withOptimizations, diff --git a/package-lock.json b/package-lock.json index d4cb52694fe..c2a59f98207 100644 --- a/package-lock.json +++ b/package-lock.json @@ -7504,9 +7504,9 @@ } }, "node_modules/hardhat-ignore-warnings": { - "version": "0.2.8", - "resolved": "https://registry.npmjs.org/hardhat-ignore-warnings/-/hardhat-ignore-warnings-0.2.8.tgz", - "integrity": "sha512-vPX94rJyTzYsCOzGIYdOcJgn3iQI6qa+CI9ZZfgDhdXJpda8ljpOT7bdUKAYC4LyoP0Z5fWTmupXoPaQrty0gw==", + "version": "0.2.9", + "resolved": "https://registry.npmjs.org/hardhat-ignore-warnings/-/hardhat-ignore-warnings-0.2.9.tgz", + "integrity": "sha512-q1oj6/ixiAx+lgIyGLBajVCSC7qUtAoK7LS9Nr8UVHYo8Iuh5naBiVGo4RDJ6wxbDGYBkeSukUGZrMqzC2DWwA==", "dev": true, "dependencies": { "minimatch": "^5.1.0", @@ -21441,9 +21441,9 @@ } }, "hardhat-ignore-warnings": { - "version": "0.2.8", - "resolved": "https://registry.npmjs.org/hardhat-ignore-warnings/-/hardhat-ignore-warnings-0.2.8.tgz", - "integrity": "sha512-vPX94rJyTzYsCOzGIYdOcJgn3iQI6qa+CI9ZZfgDhdXJpda8ljpOT7bdUKAYC4LyoP0Z5fWTmupXoPaQrty0gw==", + "version": "0.2.9", + "resolved": "https://registry.npmjs.org/hardhat-ignore-warnings/-/hardhat-ignore-warnings-0.2.9.tgz", + "integrity": "sha512-q1oj6/ixiAx+lgIyGLBajVCSC7qUtAoK7LS9Nr8UVHYo8Iuh5naBiVGo4RDJ6wxbDGYBkeSukUGZrMqzC2DWwA==", "dev": true, "requires": { "minimatch": "^5.1.0", From b84ea6eedf65e896556c2676deccae42c62d4e8b Mon Sep 17 00:00:00 2001 From: ernestognw Date: Thu, 22 Jun 2023 13:15:14 -0600 Subject: [PATCH 04/17] Fix lint --- hardhat.config.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hardhat.config.js b/hardhat.config.js index b101d264861..8ee5d05e5c6 100644 --- a/hardhat.config.js +++ b/hardhat.config.js @@ -78,7 +78,7 @@ module.exports = { warnings: { 'contracts-exposed/**/*': { 'code-size': 'off', - 'initcode-size': 'off' + 'initcode-size': 'off', }, '*': { 'code-size': withOptimizations, From 2781b5f87c67bff519bb3d34666be8f54ddade86 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Thu, 22 Jun 2023 22:56:57 -0600 Subject: [PATCH 05/17] Apply suggestions Co-authored-by: Hadrien Croubois --- contracts/governance/Governor.sol | 8 ++------ test/governance/Governor.test.js | 2 +- test/governance/extensions/GovernorWithParams.test.js | 2 +- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 47927936dde..8aaa2bffbb9 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -528,10 +528,8 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 bytes32 r, bytes32 s ) public virtual override returns (uint256) { - uint256 currentNonce = _useNonce(voter); - address signer = ECDSA.recover( - _hashTypedDataV4(keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support, voter, currentNonce))), + _hashTypedDataV4(keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support, voter, _useNonce(voter)))), v, r, s @@ -557,8 +555,6 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 bytes32 r, bytes32 s ) public virtual override returns (uint256) { - uint256 currentNonce = _useNonce(voter); - address signer = ECDSA.recover( _hashTypedDataV4( keccak256( @@ -567,7 +563,7 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 proposalId, support, voter, - currentNonce, + _useNonce(voter), keccak256(bytes(reason)), keccak256(params) ) diff --git a/test/governance/Governor.test.js b/test/governance/Governor.test.js index fb39a84a1ee..2c1cbf50a98 100644 --- a/test/governance/Governor.test.js +++ b/test/governance/Governor.test.js @@ -197,7 +197,7 @@ contract('Governor', function (accounts) { await this.helper.propose(); await this.helper.waitForSnapshot(); expectEvent( - await this.helper.vote({ support: Enums.VoteType.For, voter: voterBySigAddress, nonce: nonce, signature }), + await this.helper.vote({ support: Enums.VoteType.For, voter: voterBySigAddress, nonce, signature }), 'VoteCast', { voter: voterBySigAddress, diff --git a/test/governance/extensions/GovernorWithParams.test.js b/test/governance/extensions/GovernorWithParams.test.js index 949f2c968dc..afc0d9558ba 100644 --- a/test/governance/extensions/GovernorWithParams.test.js +++ b/test/governance/extensions/GovernorWithParams.test.js @@ -161,7 +161,7 @@ contract('GovernorWithParams', function (accounts) { const tx = await this.helper.vote({ support: Enums.VoteType.For, voter: this.voterBySig.address, - nonce: nonce, + nonce, reason: 'no particular reason', params: encodedParams, signature: this.signature, From ba44fa17e88989023967ad5df0faf0612d67b291 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Fri, 23 Jun 2023 10:12:12 -0600 Subject: [PATCH 06/17] Improve testing coverage --- test/governance/extensions/GovernorWithParams.test.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/governance/extensions/GovernorWithParams.test.js b/test/governance/extensions/GovernorWithParams.test.js index afc0d9558ba..1a0d242acda 100644 --- a/test/governance/extensions/GovernorWithParams.test.js +++ b/test/governance/extensions/GovernorWithParams.test.js @@ -195,6 +195,8 @@ contract('GovernorWithParams', function (accounts) { sig[69] ^= 0xff; return sig; }, + reason: 'no particular reason', + params: encodedParams, }), 'GovernorInvalidSigner', [], @@ -210,6 +212,8 @@ contract('GovernorWithParams', function (accounts) { voter: this.voterBySig.address, nonce: nonce.addn(1), signature: this.signature, + reason: 'no particular reason', + params: encodedParams, }), // The signature check implies the nonce can't be tampered without changing the signer 'GovernorInvalidSigner', From 3f6e520674a7520834f28548663cdd2200879b48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Wed, 28 Jun 2023 20:18:15 -0600 Subject: [PATCH 07/17] Update .changeset/sixty-numbers-reply.md Co-authored-by: Francisco --- .changeset/sixty-numbers-reply.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/sixty-numbers-reply.md b/.changeset/sixty-numbers-reply.md index 285ee196109..4e6faa83723 100644 --- a/.changeset/sixty-numbers-reply.md +++ b/.changeset/sixty-numbers-reply.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': major --- -`Governor`: Add a `voter` parameter to the `castVoteBySig` and `castVoteWithReasonAndParamsBySig` functions to avoid forging signatures for random addresses and allow the use of an internal nonce for invalidating signatures. +`Governor`: Add `voter` and `nonce` parameters in signed ballots, to avoid forging signatures for random addresses, prevent signature replay, and allow invalidating signatures. Add `voter` as a new parameter in the `castVoteBySig` and `castVoteWithReasonAndParamsBySig` functions. From 79c5982464951122b86c09bad339e2c471be9455 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 22 Jun 2023 18:41:56 +0200 Subject: [PATCH 08/17] Do not emit Approval event when calling transferFrom (#4370) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García Co-authored-by: Francisco --- .changeset/heavy-drinks-fail.md | 5 + contracts/mocks/token/ERC20ApprovalMock.sol | 10 + contracts/token/ERC20/ERC20.sol | 27 +- test/token/ERC20/ERC20.behavior.js | 26 +- test/token/ERC20/ERC20.test.js | 544 +++++++++--------- .../ERC20/extensions/ERC20Wrapper.test.js | 20 +- 6 files changed, 341 insertions(+), 291 deletions(-) create mode 100644 .changeset/heavy-drinks-fail.md create mode 100644 contracts/mocks/token/ERC20ApprovalMock.sol diff --git a/.changeset/heavy-drinks-fail.md b/.changeset/heavy-drinks-fail.md new file mode 100644 index 00000000000..bbe93ca90da --- /dev/null +++ b/.changeset/heavy-drinks-fail.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': major +--- + +`ERC20`: Remove `Approval` event previously emitted in `transferFrom` to indicate that part of the allowance was consumed. With this change, allowances are no longer reconstructible from events. See the code for guidelines on how to re-enable this event if needed. diff --git a/contracts/mocks/token/ERC20ApprovalMock.sol b/contracts/mocks/token/ERC20ApprovalMock.sol new file mode 100644 index 00000000000..da8f51e1ecc --- /dev/null +++ b/contracts/mocks/token/ERC20ApprovalMock.sol @@ -0,0 +1,10 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.19; + +import "../../token/ERC20/ERC20.sol"; + +abstract contract ERC20ApprovalMock is ERC20 { + function _approve(address owner, address spender, uint256 amount, bool) internal virtual override { + super._approve(owner, spender, amount, true); + } +} diff --git a/contracts/token/ERC20/ERC20.sol b/contracts/token/ERC20/ERC20.sol index aacfe79b752..5ddebf22b25 100644 --- a/contracts/token/ERC20/ERC20.sol +++ b/contracts/token/ERC20/ERC20.sol @@ -311,6 +311,27 @@ abstract contract ERC20 is Context, IERC20, IERC20Metadata, IERC20Errors { * - `spender` cannot be the zero address. */ function _approve(address owner, address spender, uint256 amount) internal virtual { + _approve(owner, spender, amount, true); + } + + /** + * @dev Alternative version of {_approve} with an optional flag that can enable or disable the Approval event. + * + * By default (when calling {_approve}) the flag is set to true. On the other hand, approval changes made by + * `_spendAllowance` during the `transferFrom` operation set the flag to false. This saves gas by not emitting any + * `Approval` event during `transferFrom` operations. + * + * Anyone who wishes to continue emitting `Approval` events on the`transferFrom` operation can force the flag to true + * using the following override: + * ``` + * function _approve(address owner, address spender, uint256 amount, bool) internal virtual override { + * super._approve(owner, spender, amount, true); + * } + * ``` + * + * Requirements are the same as {_approve}. + */ + function _approve(address owner, address spender, uint256 amount, bool emitEvent) internal virtual { if (owner == address(0)) { revert ERC20InvalidApprover(address(0)); } @@ -318,7 +339,9 @@ abstract contract ERC20 is Context, IERC20, IERC20Metadata, IERC20Errors { revert ERC20InvalidSpender(address(0)); } _allowances[owner][spender] = amount; - emit Approval(owner, spender, amount); + if (emitEvent) { + emit Approval(owner, spender, amount); + } } /** @@ -336,7 +359,7 @@ abstract contract ERC20 is Context, IERC20, IERC20Metadata, IERC20Errors { revert ERC20InsufficientAllowance(spender, currentAllowance, amount); } unchecked { - _approve(owner, spender, currentAllowance - amount); + _approve(owner, spender, currentAllowance - amount, false); } } } diff --git a/test/token/ERC20/ERC20.behavior.js b/test/token/ERC20/ERC20.behavior.js index 7f547b112b1..bb2efda893c 100644 --- a/test/token/ERC20/ERC20.behavior.js +++ b/test/token/ERC20/ERC20.behavior.js @@ -4,7 +4,10 @@ const { ZERO_ADDRESS, MAX_UINT256 } = constants; const { expectRevertCustomError } = require('../../helpers/customError'); -function shouldBehaveLikeERC20(initialSupply, initialHolder, recipient, anotherAccount) { +function shouldBehaveLikeERC20(initialSupply, accounts, opts = {}) { + const [initialHolder, recipient, anotherAccount] = accounts; + const { forcedApproval } = opts; + describe('total supply', function () { it('returns the total amount of tokens', async function () { expect(await this.token.totalSupply()).to.be.bignumber.equal(initialSupply); @@ -70,13 +73,22 @@ function shouldBehaveLikeERC20(initialSupply, initialHolder, recipient, anotherA }); }); - it('emits an approval event', async function () { - expectEvent(await this.token.transferFrom(tokenOwner, to, amount, { from: spender }), 'Approval', { - owner: tokenOwner, - spender: spender, - value: await this.token.allowance(tokenOwner, spender), + if (forcedApproval) { + it('emits an approval event', async function () { + expectEvent(await this.token.transferFrom(tokenOwner, to, amount, { from: spender }), 'Approval', { + owner: tokenOwner, + spender: spender, + value: await this.token.allowance(tokenOwner, spender), + }); }); - }); + } else { + it('does not emit an approval event', async function () { + expectEvent.notEmitted( + await this.token.transferFrom(tokenOwner, to, amount, { from: spender }), + 'Approval', + ); + }); + } }); describe('when the token owner does not have enough balance', function () { diff --git a/test/token/ERC20/ERC20.test.js b/test/token/ERC20/ERC20.test.js index 7b97c56f1c3..ef6d82f2bd6 100644 --- a/test/token/ERC20/ERC20.test.js +++ b/test/token/ERC20/ERC20.test.js @@ -9,357 +9,357 @@ const { } = require('./ERC20.behavior'); const { expectRevertCustomError } = require('../../helpers/customError'); -const ERC20 = artifacts.require('$ERC20'); -const ERC20Decimals = artifacts.require('$ERC20DecimalsMock'); +const TOKENS = [ + { Token: artifacts.require('$ERC20') }, + { Token: artifacts.require('$ERC20ApprovalMock'), forcedApproval: true }, +]; contract('ERC20', function (accounts) { - const [initialHolder, recipient, anotherAccount] = accounts; + const [initialHolder, recipient] = accounts; const name = 'My Token'; const symbol = 'MTKN'; - const initialSupply = new BN(100); - beforeEach(async function () { - this.token = await ERC20.new(name, symbol); - await this.token.$_mint(initialHolder, initialSupply); - }); - - it('has a name', async function () { - expect(await this.token.name()).to.equal(name); - }); - - it('has a symbol', async function () { - expect(await this.token.symbol()).to.equal(symbol); - }); - - it('has 18 decimals', async function () { - expect(await this.token.decimals()).to.be.bignumber.equal('18'); - }); - - describe('set decimals', function () { - const decimals = new BN(6); - - it('can set decimals during construction', async function () { - const token = await ERC20Decimals.new(name, symbol, decimals); - expect(await token.decimals()).to.be.bignumber.equal(decimals); - }); - }); - - shouldBehaveLikeERC20(initialSupply, initialHolder, recipient, anotherAccount); + for (const { Token, forcedApproval } of TOKENS) { + describe(`using ${Token._json.contractName}`, function () { + beforeEach(async function () { + this.token = await Token.new(name, symbol); + await this.token.$_mint(initialHolder, initialSupply); + }); - describe('decrease allowance', function () { - describe('when the spender is not the zero address', function () { - const spender = recipient; + shouldBehaveLikeERC20(initialSupply, accounts, { forcedApproval }); - function shouldDecreaseApproval(amount) { - describe('when there was no approved amount before', function () { - it('reverts', async function () { - const allowance = await this.token.allowance(initialHolder, spender); - await expectRevertCustomError( - this.token.decreaseAllowance(spender, amount, { from: initialHolder }), - 'ERC20FailedDecreaseAllowance', - [spender, allowance, amount], - ); - }); - }); + it('has a name', async function () { + expect(await this.token.name()).to.equal(name); + }); - describe('when the spender had an approved amount', function () { - const approvedAmount = amount; + it('has a symbol', async function () { + expect(await this.token.symbol()).to.equal(symbol); + }); - beforeEach(async function () { - await this.token.approve(spender, approvedAmount, { from: initialHolder }); - }); + it('has 18 decimals', async function () { + expect(await this.token.decimals()).to.be.bignumber.equal('18'); + }); - it('emits an approval event', async function () { - expectEvent( - await this.token.decreaseAllowance(spender, approvedAmount, { from: initialHolder }), - 'Approval', - { owner: initialHolder, spender: spender, value: new BN(0) }, - ); + describe('decrease allowance', function () { + describe('when the spender is not the zero address', function () { + const spender = recipient; + + function shouldDecreaseApproval(amount) { + describe('when there was no approved amount before', function () { + it('reverts', async function () { + const allowance = await this.token.allowance(initialHolder, spender); + await expectRevertCustomError( + this.token.decreaseAllowance(spender, amount, { from: initialHolder }), + 'ERC20FailedDecreaseAllowance', + [spender, allowance, amount], + ); + }); + }); + + describe('when the spender had an approved amount', function () { + const approvedAmount = amount; + + beforeEach(async function () { + await this.token.approve(spender, approvedAmount, { from: initialHolder }); + }); + + it('emits an approval event', async function () { + expectEvent( + await this.token.decreaseAllowance(spender, approvedAmount, { from: initialHolder }), + 'Approval', + { owner: initialHolder, spender: spender, value: new BN(0) }, + ); + }); + + it('decreases the spender allowance subtracting the requested amount', async function () { + await this.token.decreaseAllowance(spender, approvedAmount.subn(1), { from: initialHolder }); + + expect(await this.token.allowance(initialHolder, spender)).to.be.bignumber.equal('1'); + }); + + it('sets the allowance to zero when all allowance is removed', async function () { + await this.token.decreaseAllowance(spender, approvedAmount, { from: initialHolder }); + expect(await this.token.allowance(initialHolder, spender)).to.be.bignumber.equal('0'); + }); + + it('reverts when more than the full allowance is removed', async function () { + await expectRevertCustomError( + this.token.decreaseAllowance(spender, approvedAmount.addn(1), { from: initialHolder }), + 'ERC20FailedDecreaseAllowance', + [spender, approvedAmount, approvedAmount.addn(1)], + ); + }); + }); + } + + describe('when the sender has enough balance', function () { + const amount = initialSupply; + + shouldDecreaseApproval(amount); }); - it('decreases the spender allowance subtracting the requested amount', async function () { - await this.token.decreaseAllowance(spender, approvedAmount.subn(1), { from: initialHolder }); + describe('when the sender does not have enough balance', function () { + const amount = initialSupply.addn(1); - expect(await this.token.allowance(initialHolder, spender)).to.be.bignumber.equal('1'); + shouldDecreaseApproval(amount); }); + }); - it('sets the allowance to zero when all allowance is removed', async function () { - await this.token.decreaseAllowance(spender, approvedAmount, { from: initialHolder }); - expect(await this.token.allowance(initialHolder, spender)).to.be.bignumber.equal('0'); - }); + describe('when the spender is the zero address', function () { + const amount = initialSupply; + const spender = ZERO_ADDRESS; - it('reverts when more than the full allowance is removed', async function () { + it('reverts', async function () { await expectRevertCustomError( - this.token.decreaseAllowance(spender, approvedAmount.addn(1), { from: initialHolder }), + this.token.decreaseAllowance(spender, amount, { from: initialHolder }), 'ERC20FailedDecreaseAllowance', - [spender, approvedAmount, approvedAmount.addn(1)], + [spender, 0, amount], ); }); }); - } - - describe('when the sender has enough balance', function () { - const amount = initialSupply; - - shouldDecreaseApproval(amount); }); - describe('when the sender does not have enough balance', function () { - const amount = initialSupply.addn(1); + describe('increase allowance', function () { + const amount = initialSupply; - shouldDecreaseApproval(amount); - }); - }); + describe('when the spender is not the zero address', function () { + const spender = recipient; + + describe('when the sender has enough balance', function () { + it('emits an approval event', async function () { + expectEvent(await this.token.increaseAllowance(spender, amount, { from: initialHolder }), 'Approval', { + owner: initialHolder, + spender: spender, + value: amount, + }); + }); + + describe('when there was no approved amount before', function () { + it('approves the requested amount', async function () { + await this.token.increaseAllowance(spender, amount, { from: initialHolder }); + + expect(await this.token.allowance(initialHolder, spender)).to.be.bignumber.equal(amount); + }); + }); + + describe('when the spender had an approved amount', function () { + beforeEach(async function () { + await this.token.approve(spender, new BN(1), { from: initialHolder }); + }); + + it('increases the spender allowance adding the requested amount', async function () { + await this.token.increaseAllowance(spender, amount, { from: initialHolder }); + + expect(await this.token.allowance(initialHolder, spender)).to.be.bignumber.equal(amount.addn(1)); + }); + }); + }); - describe('when the spender is the zero address', function () { - const amount = initialSupply; - const spender = ZERO_ADDRESS; + describe('when the sender does not have enough balance', function () { + const amount = initialSupply.addn(1); - it('reverts', async function () { - await expectRevertCustomError( - this.token.decreaseAllowance(spender, amount, { from: initialHolder }), - 'ERC20FailedDecreaseAllowance', - [spender, 0, amount], - ); - }); - }); - }); + it('emits an approval event', async function () { + expectEvent(await this.token.increaseAllowance(spender, amount, { from: initialHolder }), 'Approval', { + owner: initialHolder, + spender: spender, + value: amount, + }); + }); - describe('increase allowance', function () { - const amount = initialSupply; + describe('when there was no approved amount before', function () { + it('approves the requested amount', async function () { + await this.token.increaseAllowance(spender, amount, { from: initialHolder }); - describe('when the spender is not the zero address', function () { - const spender = recipient; + expect(await this.token.allowance(initialHolder, spender)).to.be.bignumber.equal(amount); + }); + }); - describe('when the sender has enough balance', function () { - it('emits an approval event', async function () { - expectEvent(await this.token.increaseAllowance(spender, amount, { from: initialHolder }), 'Approval', { - owner: initialHolder, - spender: spender, - value: amount, - }); - }); + describe('when the spender had an approved amount', function () { + beforeEach(async function () { + await this.token.approve(spender, new BN(1), { from: initialHolder }); + }); - describe('when there was no approved amount before', function () { - it('approves the requested amount', async function () { - await this.token.increaseAllowance(spender, amount, { from: initialHolder }); + it('increases the spender allowance adding the requested amount', async function () { + await this.token.increaseAllowance(spender, amount, { from: initialHolder }); - expect(await this.token.allowance(initialHolder, spender)).to.be.bignumber.equal(amount); + expect(await this.token.allowance(initialHolder, spender)).to.be.bignumber.equal(amount.addn(1)); + }); + }); }); }); - describe('when the spender had an approved amount', function () { - beforeEach(async function () { - await this.token.approve(spender, new BN(1), { from: initialHolder }); - }); + describe('when the spender is the zero address', function () { + const spender = ZERO_ADDRESS; - it('increases the spender allowance adding the requested amount', async function () { - await this.token.increaseAllowance(spender, amount, { from: initialHolder }); - - expect(await this.token.allowance(initialHolder, spender)).to.be.bignumber.equal(amount.addn(1)); + it('reverts', async function () { + await expectRevertCustomError( + this.token.increaseAllowance(spender, amount, { from: initialHolder }), + 'ERC20InvalidSpender', + [ZERO_ADDRESS], + ); }); }); }); - describe('when the sender does not have enough balance', function () { - const amount = initialSupply.addn(1); + describe('_mint', function () { + const amount = new BN(50); + it('rejects a null account', async function () { + await expectRevertCustomError(this.token.$_mint(ZERO_ADDRESS, amount), 'ERC20InvalidReceiver', [ + ZERO_ADDRESS, + ]); + }); - it('emits an approval event', async function () { - expectEvent(await this.token.increaseAllowance(spender, amount, { from: initialHolder }), 'Approval', { - owner: initialHolder, - spender: spender, - value: amount, - }); + it('rejects overflow', async function () { + const maxUint256 = new BN('2').pow(new BN(256)).subn(1); + await expectRevert( + this.token.$_mint(recipient, maxUint256), + 'reverted with panic code 0x11 (Arithmetic operation underflowed or overflowed outside of an unchecked block)', + ); }); - describe('when there was no approved amount before', function () { - it('approves the requested amount', async function () { - await this.token.increaseAllowance(spender, amount, { from: initialHolder }); + describe('for a non zero account', function () { + beforeEach('minting', async function () { + this.receipt = await this.token.$_mint(recipient, amount); + }); - expect(await this.token.allowance(initialHolder, spender)).to.be.bignumber.equal(amount); + it('increments totalSupply', async function () { + const expectedSupply = initialSupply.add(amount); + expect(await this.token.totalSupply()).to.be.bignumber.equal(expectedSupply); }); - }); - describe('when the spender had an approved amount', function () { - beforeEach(async function () { - await this.token.approve(spender, new BN(1), { from: initialHolder }); + it('increments recipient balance', async function () { + expect(await this.token.balanceOf(recipient)).to.be.bignumber.equal(amount); }); - it('increases the spender allowance adding the requested amount', async function () { - await this.token.increaseAllowance(spender, amount, { from: initialHolder }); + it('emits Transfer event', async function () { + const event = expectEvent(this.receipt, 'Transfer', { from: ZERO_ADDRESS, to: recipient }); - expect(await this.token.allowance(initialHolder, spender)).to.be.bignumber.equal(amount.addn(1)); + expect(event.args.value).to.be.bignumber.equal(amount); }); }); }); - }); - describe('when the spender is the zero address', function () { - const spender = ZERO_ADDRESS; - - it('reverts', async function () { - await expectRevertCustomError( - this.token.increaseAllowance(spender, amount, { from: initialHolder }), - 'ERC20InvalidSpender', - [ZERO_ADDRESS], - ); - }); - }); - }); - - describe('_mint', function () { - const amount = new BN(50); - it('rejects a null account', async function () { - await expectRevertCustomError(this.token.$_mint(ZERO_ADDRESS, amount), 'ERC20InvalidReceiver', [ZERO_ADDRESS]); - }); - - it('rejects overflow', async function () { - const maxUint256 = new BN('2').pow(new BN(256)).subn(1); - await expectRevert( - this.token.$_mint(recipient, maxUint256), - 'reverted with panic code 0x11 (Arithmetic operation underflowed or overflowed outside of an unchecked block)', - ); - }); + describe('_burn', function () { + it('rejects a null account', async function () { + await expectRevertCustomError(this.token.$_burn(ZERO_ADDRESS, new BN(1)), 'ERC20InvalidSender', [ + ZERO_ADDRESS, + ]); + }); - describe('for a non zero account', function () { - beforeEach('minting', async function () { - this.receipt = await this.token.$_mint(recipient, amount); - }); + describe('for a non zero account', function () { + it('rejects burning more than balance', async function () { + await expectRevertCustomError( + this.token.$_burn(initialHolder, initialSupply.addn(1)), + 'ERC20InsufficientBalance', + [initialHolder, initialSupply, initialSupply.addn(1)], + ); + }); - it('increments totalSupply', async function () { - const expectedSupply = initialSupply.add(amount); - expect(await this.token.totalSupply()).to.be.bignumber.equal(expectedSupply); - }); + const describeBurn = function (description, amount) { + describe(description, function () { + beforeEach('burning', async function () { + this.receipt = await this.token.$_burn(initialHolder, amount); + }); - it('increments recipient balance', async function () { - expect(await this.token.balanceOf(recipient)).to.be.bignumber.equal(amount); - }); + it('decrements totalSupply', async function () { + const expectedSupply = initialSupply.sub(amount); + expect(await this.token.totalSupply()).to.be.bignumber.equal(expectedSupply); + }); - it('emits Transfer event', async function () { - const event = expectEvent(this.receipt, 'Transfer', { from: ZERO_ADDRESS, to: recipient }); + it('decrements initialHolder balance', async function () { + const expectedBalance = initialSupply.sub(amount); + expect(await this.token.balanceOf(initialHolder)).to.be.bignumber.equal(expectedBalance); + }); - expect(event.args.value).to.be.bignumber.equal(amount); - }); - }); - }); + it('emits Transfer event', async function () { + const event = expectEvent(this.receipt, 'Transfer', { from: initialHolder, to: ZERO_ADDRESS }); - describe('_burn', function () { - it('rejects a null account', async function () { - await expectRevertCustomError(this.token.$_burn(ZERO_ADDRESS, new BN(1)), 'ERC20InvalidSender', [ZERO_ADDRESS]); - }); + expect(event.args.value).to.be.bignumber.equal(amount); + }); + }); + }; - describe('for a non zero account', function () { - it('rejects burning more than balance', async function () { - await expectRevertCustomError( - this.token.$_burn(initialHolder, initialSupply.addn(1)), - 'ERC20InsufficientBalance', - [initialHolder, initialSupply, initialSupply.addn(1)], - ); + describeBurn('for entire balance', initialSupply); + describeBurn('for less amount than balance', initialSupply.subn(1)); + }); }); - const describeBurn = function (description, amount) { - describe(description, function () { - beforeEach('burning', async function () { - this.receipt = await this.token.$_burn(initialHolder, amount); - }); + describe('_update', function () { + const amount = new BN(1); - it('decrements totalSupply', async function () { - const expectedSupply = initialSupply.sub(amount); - expect(await this.token.totalSupply()).to.be.bignumber.equal(expectedSupply); - }); + it('from is the zero address', async function () { + const balanceBefore = await this.token.balanceOf(initialHolder); + const totalSupply = await this.token.totalSupply(); - it('decrements initialHolder balance', async function () { - const expectedBalance = initialSupply.sub(amount); - expect(await this.token.balanceOf(initialHolder)).to.be.bignumber.equal(expectedBalance); + expectEvent(await this.token.$_update(ZERO_ADDRESS, initialHolder, amount), 'Transfer', { + from: ZERO_ADDRESS, + to: initialHolder, + value: amount, }); + expect(await this.token.totalSupply()).to.be.bignumber.equal(totalSupply.add(amount)); + expect(await this.token.balanceOf(initialHolder)).to.be.bignumber.equal(balanceBefore.add(amount)); + }); - it('emits Transfer event', async function () { - const event = expectEvent(this.receipt, 'Transfer', { from: initialHolder, to: ZERO_ADDRESS }); + it('to is the zero address', async function () { + const balanceBefore = await this.token.balanceOf(initialHolder); + const totalSupply = await this.token.totalSupply(); - expect(event.args.value).to.be.bignumber.equal(amount); + expectEvent(await this.token.$_update(initialHolder, ZERO_ADDRESS, amount), 'Transfer', { + from: initialHolder, + to: ZERO_ADDRESS, + value: amount, }); + expect(await this.token.totalSupply()).to.be.bignumber.equal(totalSupply.sub(amount)); + expect(await this.token.balanceOf(initialHolder)).to.be.bignumber.equal(balanceBefore.sub(amount)); }); - }; - - describeBurn('for entire balance', initialSupply); - describeBurn('for less amount than balance', initialSupply.subn(1)); - }); - }); - describe('_update', function () { - const amount = new BN(1); + it('from and to are the zero address', async function () { + const totalSupply = await this.token.totalSupply(); - it('from is the zero address', async function () { - const balanceBefore = await this.token.balanceOf(initialHolder); - const totalSupply = await this.token.totalSupply(); + await this.token.$_update(ZERO_ADDRESS, ZERO_ADDRESS, amount); - expectEvent(await this.token.$_update(ZERO_ADDRESS, initialHolder, amount), 'Transfer', { - from: ZERO_ADDRESS, - to: initialHolder, - value: amount, - }); - expect(await this.token.totalSupply()).to.be.bignumber.equal(totalSupply.add(amount)); - expect(await this.token.balanceOf(initialHolder)).to.be.bignumber.equal(balanceBefore.add(amount)); - }); - - it('to is the zero address', async function () { - const balanceBefore = await this.token.balanceOf(initialHolder); - const totalSupply = await this.token.totalSupply(); - - expectEvent(await this.token.$_update(initialHolder, ZERO_ADDRESS, amount), 'Transfer', { - from: initialHolder, - to: ZERO_ADDRESS, - value: amount, - }); - expect(await this.token.totalSupply()).to.be.bignumber.equal(totalSupply.sub(amount)); - expect(await this.token.balanceOf(initialHolder)).to.be.bignumber.equal(balanceBefore.sub(amount)); - }); - - it('from and to are the zero address', async function () { - const totalSupply = await this.token.totalSupply(); - - await this.token.$_update(ZERO_ADDRESS, ZERO_ADDRESS, amount); - - expect(await this.token.totalSupply()).to.be.bignumber.equal(totalSupply); - expectEvent(await this.token.$_update(ZERO_ADDRESS, ZERO_ADDRESS, amount), 'Transfer', { - from: ZERO_ADDRESS, - to: ZERO_ADDRESS, - value: amount, + expect(await this.token.totalSupply()).to.be.bignumber.equal(totalSupply); + expectEvent(await this.token.$_update(ZERO_ADDRESS, ZERO_ADDRESS, amount), 'Transfer', { + from: ZERO_ADDRESS, + to: ZERO_ADDRESS, + value: amount, + }); + }); }); - }); - }); - describe('_transfer', function () { - shouldBehaveLikeERC20Transfer(initialHolder, recipient, initialSupply, function (from, to, amount) { - return this.token.$_transfer(from, to, amount); - }); + describe('_transfer', function () { + shouldBehaveLikeERC20Transfer(initialHolder, recipient, initialSupply, function (from, to, amount) { + return this.token.$_transfer(from, to, amount); + }); - describe('when the sender is the zero address', function () { - it('reverts', async function () { - await expectRevertCustomError( - this.token.$_transfer(ZERO_ADDRESS, recipient, initialSupply), - 'ERC20InvalidSender', - [ZERO_ADDRESS], - ); + describe('when the sender is the zero address', function () { + it('reverts', async function () { + await expectRevertCustomError( + this.token.$_transfer(ZERO_ADDRESS, recipient, initialSupply), + 'ERC20InvalidSender', + [ZERO_ADDRESS], + ); + }); + }); }); - }); - }); - describe('_approve', function () { - shouldBehaveLikeERC20Approve(initialHolder, recipient, initialSupply, function (owner, spender, amount) { - return this.token.$_approve(owner, spender, amount); - }); + describe('_approve', function () { + shouldBehaveLikeERC20Approve(initialHolder, recipient, initialSupply, function (owner, spender, amount) { + return this.token.$_approve(owner, spender, amount); + }); - describe('when the owner is the zero address', function () { - it('reverts', async function () { - await expectRevertCustomError( - this.token.$_approve(ZERO_ADDRESS, recipient, initialSupply), - 'ERC20InvalidApprover', - [ZERO_ADDRESS], - ); + describe('when the owner is the zero address', function () { + it('reverts', async function () { + await expectRevertCustomError( + this.token.$_approve(ZERO_ADDRESS, recipient, initialSupply), + 'ERC20InvalidApprover', + [ZERO_ADDRESS], + ); + }); + }); }); }); - }); + } }); diff --git a/test/token/ERC20/extensions/ERC20Wrapper.test.js b/test/token/ERC20/extensions/ERC20Wrapper.test.js index ffb97e8a212..94415d088d9 100644 --- a/test/token/ERC20/extensions/ERC20Wrapper.test.js +++ b/test/token/ERC20/extensions/ERC20Wrapper.test.js @@ -10,7 +10,7 @@ const ERC20Decimals = artifacts.require('$ERC20DecimalsMock'); const ERC20Wrapper = artifacts.require('$ERC20Wrapper'); contract('ERC20Wrapper', function (accounts) { - const [initialHolder, recipient, anotherAccount] = accounts; + const [initialHolder, receiver] = accounts; const name = 'My Token'; const symbol = 'MTKN'; @@ -85,7 +85,7 @@ contract('ERC20Wrapper', function (accounts) { it('to other account', async function () { await this.underlying.approve(this.token.address, initialSupply, { from: initialHolder }); - const { tx } = await this.token.depositFor(anotherAccount, initialSupply, { from: initialHolder }); + const { tx } = await this.token.depositFor(receiver, initialSupply, { from: initialHolder }); await expectEvent.inTransaction(tx, this.underlying, 'Transfer', { from: initialHolder, to: this.token.address, @@ -93,7 +93,7 @@ contract('ERC20Wrapper', function (accounts) { }); await expectEvent.inTransaction(tx, this.token, 'Transfer', { from: ZERO_ADDRESS, - to: anotherAccount, + to: receiver, value: initialSupply, }); }); @@ -144,10 +144,10 @@ contract('ERC20Wrapper', function (accounts) { }); it('to other account', async function () { - const { tx } = await this.token.withdrawTo(anotherAccount, initialSupply, { from: initialHolder }); + const { tx } = await this.token.withdrawTo(receiver, initialSupply, { from: initialHolder }); await expectEvent.inTransaction(tx, this.underlying, 'Transfer', { from: this.token.address, - to: anotherAccount, + to: receiver, value: initialSupply, }); await expectEvent.inTransaction(tx, this.token, 'Transfer', { @@ -163,10 +163,10 @@ contract('ERC20Wrapper', function (accounts) { await this.underlying.approve(this.token.address, initialSupply, { from: initialHolder }); await this.token.depositFor(initialHolder, initialSupply, { from: initialHolder }); - const { tx } = await this.token.$_recover(anotherAccount); + const { tx } = await this.token.$_recover(receiver); await expectEvent.inTransaction(tx, this.token, 'Transfer', { from: ZERO_ADDRESS, - to: anotherAccount, + to: receiver, value: '0', }); }); @@ -174,10 +174,10 @@ contract('ERC20Wrapper', function (accounts) { it('something to recover', async function () { await this.underlying.transfer(this.token.address, initialSupply, { from: initialHolder }); - const { tx } = await this.token.$_recover(anotherAccount); + const { tx } = await this.token.$_recover(receiver); await expectEvent.inTransaction(tx, this.token, 'Transfer', { from: ZERO_ADDRESS, - to: anotherAccount, + to: receiver, value: initialSupply, }); }); @@ -189,6 +189,6 @@ contract('ERC20Wrapper', function (accounts) { await this.token.depositFor(initialHolder, initialSupply, { from: initialHolder }); }); - shouldBehaveLikeERC20(initialSupply, initialHolder, recipient, anotherAccount); + shouldBehaveLikeERC20(initialSupply, accounts); }); }); From 35de3820e0d9f776f34c9bb997b8bff4359a21e3 Mon Sep 17 00:00:00 2001 From: Sergei Tikhomirov Date: Fri, 23 Jun 2023 00:34:53 +0200 Subject: [PATCH 09/17] Update comment to reflect code logic in Ownable.sol (#4369) --- contracts/access/Ownable.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/access/Ownable.sol b/contracts/access/Ownable.sol index f4394378b7f..627226cf715 100644 --- a/contracts/access/Ownable.sol +++ b/contracts/access/Ownable.sol @@ -10,8 +10,8 @@ import "../utils/Context.sol"; * there is an account (an owner) that can be granted exclusive access to * specific functions. * - * By default, the owner account will be the one that deploys the contract. This - * can later be changed with {transferOwnership}. + * The initial owner is set to the address provided by the deployer. This can + * later be changed with {transferOwnership}. * * This module is used through inheritance. It will make available the modifier * `onlyOwner`, which can be applied to your functions to restrict their use to @@ -33,7 +33,7 @@ abstract contract Ownable is Context { event OwnershipTransferred(address indexed previousOwner, address indexed newOwner); /** - * @dev Initializes the contract setting the deployer as the initial owner. + * @dev Initializes the contract setting the address provided by the deployer as the initial owner. */ constructor(address initialOwner) { _transferOwnership(initialOwner); From a75ca266113b7a2b8d29915dd3a2a4d769046fd5 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 23 Jun 2023 18:05:22 +0200 Subject: [PATCH 10/17] Pack Governor's ProposalCore into a single slot (#4268) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García Co-authored-by: Francisco --- .changeset/grumpy-bulldogs-call.md | 5 ++++ contracts/governance/Governor.sol | 30 ++++++++----------- contracts/governance/IGovernor.sol | 7 +++++ .../extensions/GovernorPreventLateQuorum.sol | 14 ++++----- .../extensions/GovernorSettings.sol | 17 ++++++----- .../extensions/GovernorTimelockCompound.sol | 4 +-- 6 files changed, 43 insertions(+), 34 deletions(-) create mode 100644 .changeset/grumpy-bulldogs-call.md diff --git a/.changeset/grumpy-bulldogs-call.md b/.changeset/grumpy-bulldogs-call.md new file mode 100644 index 00000000000..c034587f34a --- /dev/null +++ b/.changeset/grumpy-bulldogs-call.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': major +--- + +`Governor`: Optimized use of storage for proposal data diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 8aaa2bffbb9..2f187981454 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -38,14 +38,9 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 // solhint-disable var-name-mixedcase struct ProposalCore { - // --- start retyped from Timers.BlockNumber at offset 0x00 --- - uint64 voteStart; address proposer; - bytes4 __gap_unused0; - // --- start retyped from Timers.BlockNumber at offset 0x20 --- - uint64 voteEnd; - bytes24 __gap_unused1; - // --- Remaining fields starting at offset 0x40 --------------- + uint48 voteStart; + uint32 voteDuration; bool executed; bool canceled; } @@ -170,7 +165,9 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 * @dev See {IGovernor-state}. */ function state(uint256 proposalId) public view virtual override returns (ProposalState) { - ProposalCore storage proposal = _proposals[proposalId]; + // ProposalCore is just one slot. We can load it from storage to memory with a single sload and use memory + // object as a cache. This avoid duplicating expensive sloads. + ProposalCore memory proposal = _proposals[proposalId]; if (proposal.executed) { return ProposalState.Executed; @@ -223,7 +220,7 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 * @dev See {IGovernor-proposalDeadline}. */ function proposalDeadline(uint256 proposalId) public view virtual override returns (uint256) { - return _proposals[proposalId].voteEnd; + return _proposals[proposalId].voteStart + _proposals[proposalId].voteDuration; } /** @@ -304,16 +301,14 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 } uint256 snapshot = currentTimepoint + votingDelay(); - uint256 deadline = snapshot + votingPeriod(); + uint256 duration = votingPeriod(); _proposals[proposalId] = ProposalCore({ proposer: proposer, - voteStart: SafeCast.toUint64(snapshot), - voteEnd: SafeCast.toUint64(deadline), + voteStart: SafeCast.toUint48(snapshot), + voteDuration: SafeCast.toUint32(duration), executed: false, - canceled: false, - __gap_unused0: 0, - __gap_unused1: 0 + canceled: false }); emit ProposalCreated( @@ -324,7 +319,7 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 new string[](targets.length), calldatas, snapshot, - deadline, + snapshot + duration, description ); @@ -609,13 +604,12 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 string memory reason, bytes memory params ) internal virtual returns (uint256) { - ProposalCore storage proposal = _proposals[proposalId]; ProposalState currentState = state(proposalId); if (currentState != ProposalState.Active) { revert GovernorUnexpectedProposalState(proposalId, currentState, _encodeStateBitmap(ProposalState.Active)); } - uint256 weight = _getVotes(account, proposal.voteStart, params); + uint256 weight = _getVotes(account, proposalSnapshot(proposalId), params); _countVote(proposalId, account, support, weight, params); if (params.length == 0) { diff --git a/contracts/governance/IGovernor.sol b/contracts/governance/IGovernor.sol index 99d66ae36e8..196ed830f7e 100644 --- a/contracts/governance/IGovernor.sol +++ b/contracts/governance/IGovernor.sol @@ -227,6 +227,9 @@ abstract contract IGovernor is IERC165, IERC6372 { * * This can be increased to leave time for users to buy voting power, or delegate it, before the voting of a * proposal starts. + * + * NOTE: While this interface returns a uint256, timepoints are stored as uint48 following the ERC-6372 clock type. + * Consequently this value must fit in a uint48 (when added to the current clock). See {IERC6372-clock}. */ function votingDelay() public view virtual returns (uint256); @@ -237,6 +240,10 @@ abstract contract IGovernor is IERC165, IERC6372 { * * NOTE: The {votingDelay} can delay the start of the vote. This must be considered when setting the voting * duration compared to the voting delay. + * + * NOTE: This value is stored when the proposal is submitted so that possible changes to the value do not affect + * proposals that have already been submitted. The type used to save it is a uint32. Consequently, while this + * interface returns a uint256, the value it returns should fit in a uint32. */ function votingPeriod() public view virtual returns (uint256); diff --git a/contracts/governance/extensions/GovernorPreventLateQuorum.sol b/contracts/governance/extensions/GovernorPreventLateQuorum.sol index abb81128eb1..3e730174e5b 100644 --- a/contracts/governance/extensions/GovernorPreventLateQuorum.sol +++ b/contracts/governance/extensions/GovernorPreventLateQuorum.sol @@ -18,10 +18,10 @@ import "../../utils/math/Math.sol"; * _Available since v4.5._ */ abstract contract GovernorPreventLateQuorum is Governor { - uint64 private _voteExtension; + uint48 private _voteExtension; /// @custom:oz-retyped-from mapping(uint256 => Timers.BlockNumber) - mapping(uint256 => uint64) private _extendedDeadlines; + mapping(uint256 => uint48) private _extendedDeadlines; /// @dev Emitted when a proposal deadline is pushed back due to reaching quorum late in its voting period. event ProposalExtended(uint256 indexed proposalId, uint64 extendedDeadline); @@ -34,7 +34,7 @@ abstract contract GovernorPreventLateQuorum is Governor { * clock mode) that is required to pass since the moment a proposal reaches quorum until its voting period ends. If * necessary the voting period will be extended beyond the one set during proposal creation. */ - constructor(uint64 initialVoteExtension) { + constructor(uint48 initialVoteExtension) { _setLateQuorumVoteExtension(initialVoteExtension); } @@ -62,7 +62,7 @@ abstract contract GovernorPreventLateQuorum is Governor { uint256 result = super._castVote(proposalId, account, support, reason, params); if (_extendedDeadlines[proposalId] == 0 && _quorumReached(proposalId)) { - uint64 extendedDeadline = clock() + lateQuorumVoteExtension(); + uint48 extendedDeadline = clock() + lateQuorumVoteExtension(); if (extendedDeadline > proposalDeadline(proposalId)) { emit ProposalExtended(proposalId, extendedDeadline); @@ -78,7 +78,7 @@ abstract contract GovernorPreventLateQuorum is Governor { * @dev Returns the current value of the vote extension parameter: the number of blocks that are required to pass * from the time a proposal reaches quorum until its voting period ends. */ - function lateQuorumVoteExtension() public view virtual returns (uint64) { + function lateQuorumVoteExtension() public view virtual returns (uint48) { return _voteExtension; } @@ -88,7 +88,7 @@ abstract contract GovernorPreventLateQuorum is Governor { * * Emits a {LateQuorumVoteExtensionSet} event. */ - function setLateQuorumVoteExtension(uint64 newVoteExtension) public virtual onlyGovernance { + function setLateQuorumVoteExtension(uint48 newVoteExtension) public virtual onlyGovernance { _setLateQuorumVoteExtension(newVoteExtension); } @@ -98,7 +98,7 @@ abstract contract GovernorPreventLateQuorum is Governor { * * Emits a {LateQuorumVoteExtensionSet} event. */ - function _setLateQuorumVoteExtension(uint64 newVoteExtension) internal virtual { + function _setLateQuorumVoteExtension(uint48 newVoteExtension) internal virtual { emit LateQuorumVoteExtensionSet(_voteExtension, newVoteExtension); _voteExtension = newVoteExtension; } diff --git a/contracts/governance/extensions/GovernorSettings.sol b/contracts/governance/extensions/GovernorSettings.sol index 64e081cc585..6168689add9 100644 --- a/contracts/governance/extensions/GovernorSettings.sol +++ b/contracts/governance/extensions/GovernorSettings.sol @@ -11,9 +11,12 @@ import "../Governor.sol"; * _Available since v4.4._ */ abstract contract GovernorSettings is Governor { - uint256 private _votingDelay; - uint256 private _votingPeriod; + // amount of token uint256 private _proposalThreshold; + // timepoint: limited to uint48 in core (same as clock() type) + uint48 private _votingDelay; + // duration: limited to uint32 in core + uint32 private _votingPeriod; event VotingDelaySet(uint256 oldVotingDelay, uint256 newVotingDelay); event VotingPeriodSet(uint256 oldVotingPeriod, uint256 newVotingPeriod); @@ -22,7 +25,7 @@ abstract contract GovernorSettings is Governor { /** * @dev Initialize the governance parameters. */ - constructor(uint256 initialVotingDelay, uint256 initialVotingPeriod, uint256 initialProposalThreshold) { + constructor(uint48 initialVotingDelay, uint32 initialVotingPeriod, uint256 initialProposalThreshold) { _setVotingDelay(initialVotingDelay); _setVotingPeriod(initialVotingPeriod); _setProposalThreshold(initialProposalThreshold); @@ -54,7 +57,7 @@ abstract contract GovernorSettings is Governor { * * Emits a {VotingDelaySet} event. */ - function setVotingDelay(uint256 newVotingDelay) public virtual onlyGovernance { + function setVotingDelay(uint48 newVotingDelay) public virtual onlyGovernance { _setVotingDelay(newVotingDelay); } @@ -63,7 +66,7 @@ abstract contract GovernorSettings is Governor { * * Emits a {VotingPeriodSet} event. */ - function setVotingPeriod(uint256 newVotingPeriod) public virtual onlyGovernance { + function setVotingPeriod(uint32 newVotingPeriod) public virtual onlyGovernance { _setVotingPeriod(newVotingPeriod); } @@ -81,7 +84,7 @@ abstract contract GovernorSettings is Governor { * * Emits a {VotingDelaySet} event. */ - function _setVotingDelay(uint256 newVotingDelay) internal virtual { + function _setVotingDelay(uint48 newVotingDelay) internal virtual { emit VotingDelaySet(_votingDelay, newVotingDelay); _votingDelay = newVotingDelay; } @@ -91,7 +94,7 @@ abstract contract GovernorSettings is Governor { * * Emits a {VotingPeriodSet} event. */ - function _setVotingPeriod(uint256 newVotingPeriod) internal virtual { + function _setVotingPeriod(uint32 newVotingPeriod) internal virtual { // voting period must be at least one block long if (newVotingPeriod == 0) { revert GovernorInvalidVotingPeriod(0); diff --git a/contracts/governance/extensions/GovernorTimelockCompound.sol b/contracts/governance/extensions/GovernorTimelockCompound.sol index 21439b4dbc6..ed4d916a661 100644 --- a/contracts/governance/extensions/GovernorTimelockCompound.sol +++ b/contracts/governance/extensions/GovernorTimelockCompound.sol @@ -24,7 +24,7 @@ abstract contract GovernorTimelockCompound is IGovernorTimelock, Governor { ICompoundTimelock private _timelock; /// @custom:oz-retyped-from mapping(uint256 => GovernorTimelockCompound.ProposalTimelock) - mapping(uint256 => uint64) private _proposalTimelocks; + mapping(uint256 => uint256) private _proposalTimelocks; /** * @dev Emitted when the timelock controller used for proposal execution is modified. @@ -100,7 +100,7 @@ abstract contract GovernorTimelockCompound is IGovernorTimelock, Governor { } uint256 eta = block.timestamp + _timelock.delay(); - _proposalTimelocks[proposalId] = SafeCast.toUint64(eta); + _proposalTimelocks[proposalId] = eta; for (uint256 i = 0; i < targets.length; ++i) { if (_timelock.queuedTransactions(keccak256(abi.encode(targets[i], values[i], "", calldatas[i], eta)))) { From b32bfddd8e6fc5585798c33532927107a06d490e Mon Sep 17 00:00:00 2001 From: Renan Souza Date: Fri, 23 Jun 2023 19:37:27 -0300 Subject: [PATCH 11/17] Update Ownable2Step docs (#4384) Co-authored-by: Francisco --- contracts/access/Ownable2Step.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/Ownable2Step.sol b/contracts/access/Ownable2Step.sol index 61005b7e35f..f76eb2bba60 100644 --- a/contracts/access/Ownable2Step.sol +++ b/contracts/access/Ownable2Step.sol @@ -10,7 +10,7 @@ import "./Ownable.sol"; * there is an account (an owner) that can be granted exclusive access to * specific functions. * - * By default, the owner account will be the one that deploys the contract. This + * The initial owner is specified at deployment time in the constructor for `Ownable`. This * can later be changed with {transferOwnership} and {acceptOwnership}. * * This module is used through inheritance. It will make available all functions From d518d658bb29a0376064e235d920349d54d5a6e8 Mon Sep 17 00:00:00 2001 From: Francisco Date: Sat, 24 Jun 2023 00:23:25 -0300 Subject: [PATCH 12/17] Add unreleased disclaimer in readme --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index bbe7502e628..2936877e186 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## Unreleased + +> **Warning** Version 5.0 is under active development and should not be used. Install the releases from npm or use the version tags in the repository. + ### Removals The following contracts, libraries and functions were removed: From 71cd6ba38e98bc66516e35a428b06ead09ecd5db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Mon, 26 Jun 2023 06:20:01 -0600 Subject: [PATCH 13/17] Rename `ERC1155InsufficientApprovalForAll` to `ERC1155MissingApprovalForAll` (#4381) --- contracts/interfaces/draft-IERC6093.sol | 2 +- contracts/token/ERC1155/ERC1155.sol | 4 ++-- contracts/token/ERC1155/extensions/ERC1155Burnable.sol | 4 ++-- test/token/ERC1155/ERC1155.behavior.js | 4 ++-- test/token/ERC1155/extensions/ERC1155Burnable.test.js | 4 ++-- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/contracts/interfaces/draft-IERC6093.sol b/contracts/interfaces/draft-IERC6093.sol index cebda56a3ea..fbe31051a49 100644 --- a/contracts/interfaces/draft-IERC6093.sol +++ b/contracts/interfaces/draft-IERC6093.sol @@ -138,7 +138,7 @@ interface IERC1155Errors { * @param operator Address that may be allowed to operate on tokens without being their owner. * @param owner Address of the current owner of a token. */ - error ERC1155InsufficientApprovalForAll(address operator, address owner); + error ERC1155MissingApprovalForAll(address operator, address owner); /** * @dev Indicates a failure with the `approver` of a token to be approved. Used in approvals. diff --git a/contracts/token/ERC1155/ERC1155.sol b/contracts/token/ERC1155/ERC1155.sol index 9129992f4a1..e4bd74826e6 100644 --- a/contracts/token/ERC1155/ERC1155.sol +++ b/contracts/token/ERC1155/ERC1155.sol @@ -116,7 +116,7 @@ abstract contract ERC1155 is Context, ERC165, IERC1155, IERC1155MetadataURI, IER */ function safeTransferFrom(address from, address to, uint256 id, uint256 amount, bytes memory data) public virtual { if (from != _msgSender() && !isApprovedForAll(from, _msgSender())) { - revert ERC1155InsufficientApprovalForAll(_msgSender(), from); + revert ERC1155MissingApprovalForAll(_msgSender(), from); } _safeTransferFrom(from, to, id, amount, data); } @@ -132,7 +132,7 @@ abstract contract ERC1155 is Context, ERC165, IERC1155, IERC1155MetadataURI, IER bytes memory data ) public virtual { if (from != _msgSender() && !isApprovedForAll(from, _msgSender())) { - revert ERC1155InsufficientApprovalForAll(_msgSender(), from); + revert ERC1155MissingApprovalForAll(_msgSender(), from); } _safeBatchTransferFrom(from, to, ids, amounts, data); } diff --git a/contracts/token/ERC1155/extensions/ERC1155Burnable.sol b/contracts/token/ERC1155/extensions/ERC1155Burnable.sol index ff099cb5867..d7bee05d924 100644 --- a/contracts/token/ERC1155/extensions/ERC1155Burnable.sol +++ b/contracts/token/ERC1155/extensions/ERC1155Burnable.sol @@ -14,7 +14,7 @@ import "../ERC1155.sol"; abstract contract ERC1155Burnable is ERC1155 { function burn(address account, uint256 id, uint256 value) public virtual { if (account != _msgSender() && !isApprovedForAll(account, _msgSender())) { - revert ERC1155InsufficientApprovalForAll(_msgSender(), account); + revert ERC1155MissingApprovalForAll(_msgSender(), account); } _burn(account, id, value); @@ -22,7 +22,7 @@ abstract contract ERC1155Burnable is ERC1155 { function burnBatch(address account, uint256[] memory ids, uint256[] memory values) public virtual { if (account != _msgSender() && !isApprovedForAll(account, _msgSender())) { - revert ERC1155InsufficientApprovalForAll(_msgSender(), account); + revert ERC1155MissingApprovalForAll(_msgSender(), account); } _burnBatch(account, ids, values); diff --git a/test/token/ERC1155/ERC1155.behavior.js b/test/token/ERC1155/ERC1155.behavior.js index 3391be4c8f9..4bf4a7319e7 100644 --- a/test/token/ERC1155/ERC1155.behavior.js +++ b/test/token/ERC1155/ERC1155.behavior.js @@ -254,7 +254,7 @@ function shouldBehaveLikeERC1155([minter, firstTokenHolder, secondTokenHolder, m this.token.safeTransferFrom(multiTokenHolder, recipient, firstTokenId, firstAmount, '0x', { from: proxy, }), - 'ERC1155InsufficientApprovalForAll', + 'ERC1155MissingApprovalForAll', [proxy, multiTokenHolder], ); }); @@ -609,7 +609,7 @@ function shouldBehaveLikeERC1155([minter, firstTokenHolder, secondTokenHolder, m '0x', { from: proxy }, ), - 'ERC1155InsufficientApprovalForAll', + 'ERC1155MissingApprovalForAll', [proxy, multiTokenHolder], ); }); diff --git a/test/token/ERC1155/extensions/ERC1155Burnable.test.js b/test/token/ERC1155/extensions/ERC1155Burnable.test.js index 6af2308f800..65a2f95f4c1 100644 --- a/test/token/ERC1155/extensions/ERC1155Burnable.test.js +++ b/test/token/ERC1155/extensions/ERC1155Burnable.test.js @@ -38,7 +38,7 @@ contract('ERC1155Burnable', function (accounts) { it("unapproved accounts cannot burn the holder's tokens", async function () { await expectRevertCustomError( this.token.burn(holder, tokenIds[0], amounts[0].subn(1), { from: other }), - 'ERC1155InsufficientApprovalForAll', + 'ERC1155MissingApprovalForAll', [other, holder], ); }); @@ -63,7 +63,7 @@ contract('ERC1155Burnable', function (accounts) { it("unapproved accounts cannot burn the holder's tokens", async function () { await expectRevertCustomError( this.token.burnBatch(holder, tokenIds, [amounts[0].subn(1), amounts[1].subn(2)], { from: other }), - 'ERC1155InsufficientApprovalForAll', + 'ERC1155MissingApprovalForAll', [other, holder], ); }); From 48a048c49a93ecd0c470e687329f0ab06cbe5197 Mon Sep 17 00:00:00 2001 From: Francisco Date: Mon, 26 Jun 2023 11:36:46 -0300 Subject: [PATCH 14/17] Add Foundry installation instructions with required warnings (#4389) --- README.md | 12 ++++++++++- scripts/upgradeable/upgradeable.patch | 31 ++++++++++++++++----------- 2 files changed, 29 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index 8b44917fe9f..9d1c405b60e 100644 --- a/README.md +++ b/README.md @@ -23,13 +23,23 @@ ### Installation +#### Hardhat, Truffle (npm) + ``` $ npm install @openzeppelin/contracts ``` OpenZeppelin Contracts features a [stable API](https://docs.openzeppelin.com/contracts/releases-stability#api-stability), which means that your contracts won't break unexpectedly when upgrading to a newer minor version. -An alternative to npm is to use the GitHub repository (`openzeppelin/openzeppelin-contracts`) to retrieve the contracts. When doing this, make sure to specify the tag for a release such as `v4.5.0`, instead of using the `master` branch. +#### Foundry (git) + +> **Warning** When installing via git, it is a common error to use the `master` branch. This is a development branch that should be avoided in favor of tagged releases. The release process involves security measures that the `master` branch does not guarantee. + +> **Warning** Foundry installs the latest version initially, but subsequent `forge update` commands will use the `master` branch. + +``` +$ forge install OpenZeppelin/openzeppelin-contracts +``` ### Usage diff --git a/scripts/upgradeable/upgradeable.patch b/scripts/upgradeable/upgradeable.patch index 554775ffe8e..ecd0a3711d7 100644 --- a/scripts/upgradeable/upgradeable.patch +++ b/scripts/upgradeable/upgradeable.patch @@ -59,10 +59,10 @@ index ff596b0c..00000000 - - diff --git a/README.md b/README.md -index aba99171..6656267b 100644 +index 9d1c405b..c264e29c 100644 --- a/README.md +++ b/README.md -@@ -19,17 +19,20 @@ +@@ -19,6 +19,9 @@ :building_construction: **Want to scale your decentralized application?** Check out [OpenZeppelin Defender](https://openzeppelin.com/defender) — a secure platform for automating and monitoring your operations. @@ -72,6 +72,8 @@ index aba99171..6656267b 100644 ## Overview ### Installation +@@ -26,7 +29,7 @@ + #### Hardhat, Truffle (npm) ``` -$ npm install @openzeppelin/contracts @@ -79,13 +81,16 @@ index aba99171..6656267b 100644 ``` OpenZeppelin Contracts features a [stable API](https://docs.openzeppelin.com/contracts/releases-stability#api-stability), which means that your contracts won't break unexpectedly when upgrading to a newer minor version. +@@ -38,7 +41,7 @@ OpenZeppelin Contracts features a [stable API](https://docs.openzeppelin.com/con + > **Warning** Foundry installs the latest version initially, but subsequent `forge update` commands will use the `master` branch. --An alternative to npm is to use the GitHub repository (`openzeppelin/openzeppelin-contracts`) to retrieve the contracts. When doing this, make sure to specify the tag for a release such as `v4.5.0`, instead of using the `master` branch. -+An alternative to npm is to use the GitHub repository (`openzeppelin/openzeppelin-contracts-upgradeable`) to retrieve the contracts. When doing this, make sure to specify the tag for a release such as `v4.5.0`, instead of using the `master` branch. + ``` +-$ forge install OpenZeppelin/openzeppelin-contracts ++$ forge install OpenZeppelin/openzeppelin-contracts-upgradeable + ``` ### Usage - -@@ -38,10 +41,11 @@ Once installed, you can use the contracts in the library by importing them: +@@ -48,10 +51,11 @@ Once installed, you can use the contracts in the library by importing them: ```solidity pragma solidity ^0.8.19; @@ -101,7 +106,7 @@ index aba99171..6656267b 100644 } ``` diff --git a/contracts/finance/VestingWallet.sol b/contracts/finance/VestingWallet.sol -index 5b7e1b15..1ca745d6 100644 +index ebdf0a33..8888803e 100644 --- a/contracts/finance/VestingWallet.sol +++ b/contracts/finance/VestingWallet.sol @@ -18,6 +18,8 @@ import "../utils/Context.sol"; @@ -127,7 +132,7 @@ index 5d8318f4..ef3cde55 100644 abstract contract GovernorVotes is Governor { IERC5805 public immutable token; diff --git a/contracts/package.json b/contracts/package.json -index 4d0f576b..822fd471 100644 +index df141192..1cf90ad1 100644 --- a/contracts/package.json +++ b/contracts/package.json @@ -1,5 +1,5 @@ @@ -135,7 +140,7 @@ index 4d0f576b..822fd471 100644 - "name": "@openzeppelin/contracts", + "name": "@openzeppelin/contracts-upgradeable", "description": "Secure Smart Contract library for Solidity", - "version": "4.9.0", + "version": "4.9.2", "files": [ @@ -13,7 +13,7 @@ }, @@ -147,7 +152,7 @@ index 4d0f576b..822fd471 100644 "keywords": [ "solidity", diff --git a/contracts/token/ERC20/extensions/ERC20Capped.sol b/contracts/token/ERC20/extensions/ERC20Capped.sol -index cda07265..d314148c 100644 +index 41e9ce5c..1d910dfa 100644 --- a/contracts/token/ERC20/extensions/ERC20Capped.sol +++ b/contracts/token/ERC20/extensions/ERC20Capped.sol @@ -7,6 +7,8 @@ import "../ERC20.sol"; @@ -160,7 +165,7 @@ index cda07265..d314148c 100644 abstract contract ERC20Capped is ERC20 { uint256 private immutable _cap; diff --git a/contracts/token/ERC20/extensions/ERC20Permit.sol b/contracts/token/ERC20/extensions/ERC20Permit.sol -index 9379e445..e02f0644 100644 +index 4378eb7c..1da9e731 100644 --- a/contracts/token/ERC20/extensions/ERC20Permit.sol +++ b/contracts/token/ERC20/extensions/ERC20Permit.sol @@ -18,6 +18,8 @@ import "../../../utils/Nonces.sol"; @@ -173,7 +178,7 @@ index 9379e445..e02f0644 100644 abstract contract ERC20Permit is ERC20, IERC20Permit, EIP712, Nonces { // solhint-disable-next-line var-name-mixedcase diff --git a/contracts/token/ERC20/extensions/ERC20Wrapper.sol b/contracts/token/ERC20/extensions/ERC20Wrapper.sol -index bf2b225c..0e5b3628 100644 +index 389965e9..66436b14 100644 --- a/contracts/token/ERC20/extensions/ERC20Wrapper.sol +++ b/contracts/token/ERC20/extensions/ERC20Wrapper.sol @@ -14,6 +14,8 @@ import "../utils/SafeERC20.sol"; @@ -356,7 +361,7 @@ index 2628014f..7d5193c8 100644 } } diff --git a/package.json b/package.json -index c070915f..9a513cac 100644 +index 37e8f871..d098669f 100644 --- a/package.json +++ b/package.json @@ -33,7 +33,7 @@ From 8a810c97225f03d662a57ad37e30fcc45993aee5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Thu, 29 Jun 2023 10:00:35 -0600 Subject: [PATCH 15/17] Add `ERC2771Forwarder` as an enhanced successor to `MinimalForwarder` (#4346) Co-authored-by: Francisco --- .changeset/blue-horses-do.md | 5 + .github/workflows/checks.yml | 1 + contracts/metatx/ERC2771Forwarder.sol | 289 ++++++++++++++++ contracts/metatx/MinimalForwarder.sol | 104 ------ contracts/metatx/README.adoc | 2 +- contracts/utils/cryptography/EIP712.sol | 2 +- test/metatx/ERC2771Context.test.js | 29 +- test/metatx/ERC2771Forwarder.test.js | 433 ++++++++++++++++++++++++ test/metatx/MinimalForwarder.test.js | 171 ---------- 9 files changed, 749 insertions(+), 287 deletions(-) create mode 100644 .changeset/blue-horses-do.md create mode 100644 contracts/metatx/ERC2771Forwarder.sol delete mode 100644 contracts/metatx/MinimalForwarder.sol create mode 100644 test/metatx/ERC2771Forwarder.test.js delete mode 100644 test/metatx/MinimalForwarder.test.js diff --git a/.changeset/blue-horses-do.md b/.changeset/blue-horses-do.md new file mode 100644 index 00000000000..9df604fe448 --- /dev/null +++ b/.changeset/blue-horses-do.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': major +--- + +`ERC2771Forwarder`: Added `deadline` for expiring transactions, batching, and more secure handling of `msg.value`. diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index 122d3956413..15cc88e9600 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -100,6 +100,7 @@ jobs: - uses: crytic/slither-action@v0.3.0 with: node-version: 18.15 + slither-version: 0.9.3 codespell: runs-on: ubuntu-latest diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol new file mode 100644 index 00000000000..651fdce0b62 --- /dev/null +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -0,0 +1,289 @@ +// SPDX-License-Identifier: MIT +// OpenZeppelin Contracts (last updated v4.9.0) (metatx/ERC2771Forwarder.sol) + +pragma solidity ^0.8.19; + +import "../utils/cryptography/ECDSA.sol"; +import "../utils/cryptography/EIP712.sol"; +import "../utils/Nonces.sol"; +import "../utils/Address.sol"; + +/** + * @dev A forwarder compatible with ERC2771 contracts. See {ERC2771Context}. + * + * This forwarder operates on forward requests that include: + * + * * `from`: An address to operate on behalf of. It is required to be equal to the request signer. + * * `to`: The address that should be called. + * * `value`: The amount of native token to attach with the requested call. + * * `gas`: The amount of gas limit that will be forwarded with the requested call. + * * `nonce`: A unique transaction ordering identifier to avoid replayability and request invalidation. + * * `deadline`: A timestamp after which the request is not executable anymore. + * * `data`: Encoded `msg.data` to send with the requested call. + */ +contract ERC2771Forwarder is EIP712, Nonces { + using ECDSA for bytes32; + + struct ForwardRequestData { + address from; + address to; + uint256 value; + uint256 gas; + uint48 deadline; + bytes data; + bytes signature; + } + + bytes32 private constant _FORWARD_REQUEST_TYPEHASH = + keccak256( + "ForwardRequest(address from,address to,uint256 value,uint256 gas,uint256 nonce,uint48 deadline,bytes data)" + ); + + /** + * @dev Emitted when a `ForwardRequest` is executed. + * + * NOTE: An unsuccessful forward request could be due to an invalid signature, an expired deadline, + * or simply a revert in the requested call. The contract guarantees that the relayer is not able to force + * the requested call to run out of gas. + */ + event ExecutedForwardRequest(address indexed signer, uint256 nonce, bool success); + + /** + * @dev The request `from` doesn't match with the recovered `signer`. + */ + error ERC2771ForwarderInvalidSigner(address signer, address from); + + /** + * @dev The `requestedValue` doesn't match with the available `msgValue`. + */ + error ERC2771ForwarderMismatchedValue(uint256 requestedValue, uint256 msgValue); + + /** + * @dev The request `deadline` has expired. + */ + error ERC2771ForwarderExpiredRequest(uint48 deadline); + + /** + * @dev See {EIP712-constructor}. + */ + constructor(string memory name) EIP712(name, "1") {} + + /** + * @dev Returns `true` if a request is valid for a provided `signature` at the current block timestamp. + * + * A transaction is considered valid when it hasn't expired (deadline is not met), and the signer + * matches the `from` parameter of the signed request. + * + * NOTE: A request may return false here but it won't cause {executeBatch} to revert if a refund + * receiver is provided. + */ + function verify(ForwardRequestData calldata request) public view virtual returns (bool) { + (bool alive, bool signerMatch, ) = _validate(request); + return alive && signerMatch; + } + + /** + * @dev Executes a `request` on behalf of `signature`'s signer using the ERC-2771 protocol. The gas + * provided to the requested call may not be exactly the amount requested, but the call will not run + * out of gas. Will revert if the request is invalid or the call reverts, in this case the nonce is not consumed. + * + * Requirements: + * + * - The request value should be equal to the provided `msg.value`. + * - The request should be valid according to {verify}. + */ + function execute(ForwardRequestData calldata request) public payable virtual { + // We make sure that msg.value and request.value match exactly. + // If the request is invalid or the call reverts, this whole function + // will revert, ensuring value isn't stuck. + if (msg.value != request.value) { + revert ERC2771ForwarderMismatchedValue(request.value, msg.value); + } + + if (!_execute(request, true)) { + revert Address.FailedInnerCall(); + } + } + + /** + * @dev Batch version of {execute} with optional refunding and atomic execution. + * + * In case a batch contains at least one invalid request (see {verify}), the + * request will be skipped and the `refundReceiver` parameter will receive back the + * unused requested value at the end of the execution. This is done to prevent reverting + * the entire batch when a request is invalid or has already been submitted. + * + * If the `refundReceiver` is the `address(0)`, this function will revert when at least + * one of the requests was not valid instead of skipping it. This could be useful if + * a batch is required to get executed atomically (at least at the top-level). For example, + * refunding (and thus atomicity) can be opt-out if the relayer is using a service that avoids + * including reverted transactions. + * + * Requirements: + * + * - The sum of the requests' values should be equal to the provided `msg.value`. + * - All of the requests should be valid (see {verify}) when `refundReceiver` is the zero address. + * + * NOTE: Setting a zero `refundReceiver` guarantees an all-or-nothing requests execution only for + * the first-level forwarded calls. In case a forwarded request calls to a contract with another + * subcall, the second-level call may revert without the top-level call reverting. + */ + function executeBatch( + ForwardRequestData[] calldata requests, + address payable refundReceiver + ) public payable virtual { + bool atomic = refundReceiver == address(0); + + uint256 requestsValue; + uint256 refundValue; + + for (uint256 i; i < requests.length; ++i) { + requestsValue += requests[i].value; + bool success = _execute(requests[i], atomic); + if (!success) { + refundValue += requests[i].value; + } + } + + // The batch should revert if there's a mismatched msg.value provided + // to avoid request value tampering + if (requestsValue != msg.value) { + revert ERC2771ForwarderMismatchedValue(requestsValue, msg.value); + } + + // Some requests with value were invalid (possibly due to frontrunning). + // To avoid leaving ETH in the contract this value is refunded. + if (refundValue != 0) { + // We know refundReceiver != address(0) && requestsValue == msg.value + // meaning we can ensure refundValue is not taken from the original contract's balance + // and refundReceiver is a known account. + Address.sendValue(refundReceiver, refundValue); + } + } + + /** + * @dev Validates if the provided request can be executed at current block timestamp with + * the given `request.signature` on behalf of `request.signer`. + */ + function _validate( + ForwardRequestData calldata request + ) internal view virtual returns (bool alive, bool signerMatch, address signer) { + signer = _recoverForwardRequestSigner(request); + return (request.deadline >= block.timestamp, signer == request.from, signer); + } + + /** + * @dev Recovers the signer of an EIP712 message hash for a forward `request` and its corresponding `signature`. + * See {ECDSA-recover}. + */ + function _recoverForwardRequestSigner(ForwardRequestData calldata request) internal view virtual returns (address) { + return + _hashTypedDataV4( + keccak256( + abi.encode( + _FORWARD_REQUEST_TYPEHASH, + request.from, + request.to, + request.value, + request.gas, + nonces(request.from), + request.deadline, + keccak256(request.data) + ) + ) + ).recover(request.signature); + } + + /** + * @dev Validates and executes a signed request returning the request call `success` value. + * + * Internal function without msg.value validation. + * + * Requirements: + * + * - The caller must have provided enough gas to forward with the call. + * - The request must be valid (see {verify}) if the `requireValidRequest` is true. + * + * Emits an {ExecutedForwardRequest} event. + * + * IMPORTANT: Using this function doesn't check that all the `msg.value` was sent, potentially + * leaving value stuck in the contract. + */ + function _execute( + ForwardRequestData calldata request, + bool requireValidRequest + ) internal virtual returns (bool success) { + (bool alive, bool signerMatch, address signer) = _validate(request); + + // Need to explicitly specify if a revert is required since non-reverting is default for + // batches and reversion is opt-in since it could be useful in some scenarios + if (requireValidRequest) { + if (!alive) { + revert ERC2771ForwarderExpiredRequest(request.deadline); + } + + if (!signerMatch) { + revert ERC2771ForwarderInvalidSigner(signer, request.from); + } + } + + // Ignore an invalid request because requireValidRequest = false + if (signerMatch && alive) { + // Nonce should be used before the call to prevent reusing by reentrancy + uint256 currentNonce = _useNonce(signer); + + (success, ) = request.to.call{gas: request.gas, value: request.value}( + abi.encodePacked(request.data, request.from) + ); + + _checkForwardedGas(request); + + emit ExecutedForwardRequest(signer, currentNonce, success); + } + } + + /** + * @dev Checks if the requested gas was correctly forwarded to the callee. + * + * As a consequence of https://eips.ethereum.org/EIPS/eip-150[EIP-150]: + * - At most `gasleft() - floor(gasleft() / 64)` is forwarded to the callee. + * - At least `floor(gasleft() / 64)` is kept in the caller. + * + * It reverts consuming all the available gas if the forwarded gas is not the requested gas. + * + * IMPORTANT: This function should be called exactly the end of the forwarded call. Any gas consumed + * in between will make room for bypassing this check. + */ + function _checkForwardedGas(ForwardRequestData calldata request) private view { + // To avoid insufficient gas griefing attacks, as referenced in https://ronan.eth.limo/blog/ethereum-gas-dangers/ + // + // A malicious relayer can attempt to shrink the gas forwarded so that the underlying call reverts out-of-gas + // but the forwarding itself still succeeds. In order to make sure that the subcall received sufficient gas, + // we will inspect gasleft() after the forwarding. + // + // Let X be the gas available before the subcall, such that the subcall gets at most X * 63 / 64. + // We can't know X after CALL dynamic costs, but we want it to be such that X * 63 / 64 >= req.gas. + // Let Y be the gas used in the subcall. gasleft() measured immediately after the subcall will be gasleft() = X - Y. + // If the subcall ran out of gas, then Y = X * 63 / 64 and gasleft() = X - Y = X / 64. + // Under this assumption req.gas / 63 > gasleft() is true is true if and only if + // req.gas / 63 > X / 64, or equivalently req.gas > X * 63 / 64. + // This means that if the subcall runs out of gas we are able to detect that insufficient gas was passed. + // + // We will now also see that req.gas / 63 > gasleft() implies that req.gas >= X * 63 / 64. + // The contract guarantees Y <= req.gas, thus gasleft() = X - Y >= X - req.gas. + // - req.gas / 63 > gasleft() + // - req.gas / 63 >= X - req.gas + // - req.gas >= X * 63 / 64 + // In other words if req.gas < X * 63 / 64 then req.gas / 63 <= gasleft(), thus if the relayer behaves honestly + // the forwarding does not revert. + if (gasleft() < request.gas / 63) { + // We explicitly trigger invalid opcode to consume all gas and bubble-up the effects, since + // neither revert or assert consume all gas since Solidity 0.8.0 + // https://docs.soliditylang.org/en/v0.8.0/control-structures.html#panic-via-assert-and-error-via-require + /// @solidity memory-safe-assembly + assembly { + invalid() + } + } + } +} diff --git a/contracts/metatx/MinimalForwarder.sol b/contracts/metatx/MinimalForwarder.sol deleted file mode 100644 index b5267aa108b..00000000000 --- a/contracts/metatx/MinimalForwarder.sol +++ /dev/null @@ -1,104 +0,0 @@ -// SPDX-License-Identifier: MIT -// OpenZeppelin Contracts (last updated v4.9.0) (metatx/MinimalForwarder.sol) - -pragma solidity ^0.8.19; - -import "../utils/cryptography/ECDSA.sol"; -import "../utils/cryptography/EIP712.sol"; - -/** - * @dev Simple minimal forwarder to be used together with an ERC2771 compatible contract. See {ERC2771Context}. - * - * MinimalForwarder is mainly meant for testing, as it is missing features to be a good production-ready forwarder. This - * contract does not intend to have all the properties that are needed for a sound forwarding system. A fully - * functioning forwarding system with good properties requires more complexity. We suggest you look at other projects - * such as the GSN which do have the goal of building a system like that. - */ -contract MinimalForwarder is EIP712 { - using ECDSA for bytes32; - - struct ForwardRequest { - address from; - address to; - uint256 value; - uint256 gas; - uint256 nonce; - bytes data; - } - - bytes32 private constant _TYPEHASH = - keccak256("ForwardRequest(address from,address to,uint256 value,uint256 gas,uint256 nonce,bytes data)"); - - mapping(address => uint256) private _nonces; - - /** - * @dev The request `from` doesn't match with the recovered `signer`. - */ - error MinimalForwarderInvalidSigner(address signer, address from); - - /** - * @dev The request nonce doesn't match with the `current` nonce for the request signer. - */ - error MinimalForwarderInvalidNonce(address signer, uint256 current); - - constructor() EIP712("MinimalForwarder", "0.0.1") {} - - function getNonce(address from) public view returns (uint256) { - return _nonces[from]; - } - - function verify(ForwardRequest calldata req, bytes calldata signature) public view returns (bool) { - address signer = _recover(req, signature); - (bool correctFrom, bool correctNonce) = _validateReq(req, signer); - return correctFrom && correctNonce; - } - - function execute( - ForwardRequest calldata req, - bytes calldata signature - ) public payable returns (bool, bytes memory) { - address signer = _recover(req, signature); - (bool correctFrom, bool correctNonce) = _validateReq(req, signer); - - if (!correctFrom) { - revert MinimalForwarderInvalidSigner(signer, req.from); - } - if (!correctNonce) { - revert MinimalForwarderInvalidNonce(signer, _nonces[req.from]); - } - - _nonces[req.from] = req.nonce + 1; - - (bool success, bytes memory returndata) = req.to.call{gas: req.gas, value: req.value}( - abi.encodePacked(req.data, req.from) - ); - - // Validate that the relayer has sent enough gas for the call. - // See https://ronan.eth.limo/blog/ethereum-gas-dangers/ - if (gasleft() <= req.gas / 63) { - // We explicitly trigger invalid opcode to consume all gas and bubble-up the effects, since - // neither revert or assert consume all gas since Solidity 0.8.0 - // https://docs.soliditylang.org/en/v0.8.0/control-structures.html#panic-via-assert-and-error-via-require - /// @solidity memory-safe-assembly - assembly { - invalid() - } - } - - return (success, returndata); - } - - function _recover(ForwardRequest calldata req, bytes calldata signature) internal view returns (address) { - return - _hashTypedDataV4( - keccak256(abi.encode(_TYPEHASH, req.from, req.to, req.value, req.gas, req.nonce, keccak256(req.data))) - ).recover(signature); - } - - function _validateReq( - ForwardRequest calldata req, - address signer - ) internal view returns (bool correctFrom, bool correctNonce) { - return (signer == req.from, _nonces[req.from] == req.nonce); - } -} diff --git a/contracts/metatx/README.adoc b/contracts/metatx/README.adoc index eccdeaf9740..9f25802e4d8 100644 --- a/contracts/metatx/README.adoc +++ b/contracts/metatx/README.adoc @@ -9,4 +9,4 @@ NOTE: This document is better viewed at https://docs.openzeppelin.com/contracts/ == Utils -{{MinimalForwarder}} +{{ERC2771Forwarder}} diff --git a/contracts/utils/cryptography/EIP712.sol b/contracts/utils/cryptography/EIP712.sol index 2628014f12c..1089de0050d 100644 --- a/contracts/utils/cryptography/EIP712.sol +++ b/contracts/utils/cryptography/EIP712.sol @@ -110,7 +110,7 @@ abstract contract EIP712 is IERC5267 { } /** - * @dev See {EIP-5267}. + * @dev See {IERC-5267}. * * _Available since v4.9._ */ diff --git a/test/metatx/ERC2771Context.test.js b/test/metatx/ERC2771Context.test.js index 6c298d3d9f0..3dd4b41532d 100644 --- a/test/metatx/ERC2771Context.test.js +++ b/test/metatx/ERC2771Context.test.js @@ -6,14 +6,16 @@ const { expectEvent } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); const ERC2771ContextMock = artifacts.require('ERC2771ContextMock'); -const MinimalForwarder = artifacts.require('MinimalForwarder'); +const ERC2771Forwarder = artifacts.require('ERC2771Forwarder'); const ContextMockCaller = artifacts.require('ContextMockCaller'); const { shouldBehaveLikeRegularContext } = require('../utils/Context.behavior'); contract('ERC2771Context', function (accounts) { + const MAX_UINT48 = web3.utils.toBN(1).shln(48).subn(1).toString(); + beforeEach(async function () { - this.forwarder = await MinimalForwarder.new(); + this.forwarder = await ERC2771Forwarder.new('ERC2771Forwarder'); this.recipient = await ERC2771ContextMock.new(this.forwarder.address); this.domain = await getDomain(this.forwarder); @@ -25,6 +27,7 @@ contract('ERC2771Context', function (accounts) { { name: 'value', type: 'uint256' }, { name: 'gas', type: 'uint256' }, { name: 'nonce', type: 'uint256' }, + { name: 'deadline', type: 'uint48' }, { name: 'data', type: 'bytes' }, ], }; @@ -63,14 +66,17 @@ contract('ERC2771Context', function (accounts) { to: this.recipient.address, value: '0', gas: '100000', - nonce: (await this.forwarder.getNonce(this.sender)).toString(), + nonce: (await this.forwarder.nonces(this.sender)).toString(), + deadline: MAX_UINT48, data, }; - const sign = ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { data: { ...this.data, message: req } }); - expect(await this.forwarder.verify(req, sign)).to.equal(true); + req.signature = ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { + data: { ...this.data, message: req }, + }); + expect(await this.forwarder.verify(req)).to.equal(true); - const { tx } = await this.forwarder.execute(req, sign); + const { tx } = await this.forwarder.execute(req); await expectEvent.inTransaction(tx, ERC2771ContextMock, 'Sender', { sender: this.sender }); }); }); @@ -86,14 +92,17 @@ contract('ERC2771Context', function (accounts) { to: this.recipient.address, value: '0', gas: '100000', - nonce: (await this.forwarder.getNonce(this.sender)).toString(), + nonce: (await this.forwarder.nonces(this.sender)).toString(), + deadline: MAX_UINT48, data, }; - const sign = ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { data: { ...this.data, message: req } }); - expect(await this.forwarder.verify(req, sign)).to.equal(true); + req.signature = ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { + data: { ...this.data, message: req }, + }); + expect(await this.forwarder.verify(req)).to.equal(true); - const { tx } = await this.forwarder.execute(req, sign); + const { tx } = await this.forwarder.execute(req); await expectEvent.inTransaction(tx, ERC2771ContextMock, 'Data', { data, integerValue, stringValue }); }); }); diff --git a/test/metatx/ERC2771Forwarder.test.js b/test/metatx/ERC2771Forwarder.test.js new file mode 100644 index 00000000000..fa84ccdc301 --- /dev/null +++ b/test/metatx/ERC2771Forwarder.test.js @@ -0,0 +1,433 @@ +const ethSigUtil = require('eth-sig-util'); +const Wallet = require('ethereumjs-wallet').default; +const { getDomain, domainType } = require('../helpers/eip712'); +const { expectRevertCustomError } = require('../helpers/customError'); + +const { constants, expectRevert, expectEvent, time } = require('@openzeppelin/test-helpers'); +const { expect } = require('chai'); + +const ERC2771Forwarder = artifacts.require('ERC2771Forwarder'); +const CallReceiverMock = artifacts.require('CallReceiverMock'); + +contract('ERC2771Forwarder', function (accounts) { + const [, refundReceiver, another] = accounts; + + const tamperedValues = { + from: another, + to: another, + value: web3.utils.toWei('0.5'), + data: '0x1742', + deadline: 0xdeadbeef, + }; + + beforeEach(async function () { + this.forwarder = await ERC2771Forwarder.new('ERC2771Forwarder'); + + this.domain = await getDomain(this.forwarder); + this.types = { + EIP712Domain: domainType(this.domain), + ForwardRequest: [ + { name: 'from', type: 'address' }, + { name: 'to', type: 'address' }, + { name: 'value', type: 'uint256' }, + { name: 'gas', type: 'uint256' }, + { name: 'nonce', type: 'uint256' }, + { name: 'deadline', type: 'uint48' }, + { name: 'data', type: 'bytes' }, + ], + }; + + this.alice = Wallet.generate(); + this.alice.address = web3.utils.toChecksumAddress(this.alice.getAddressString()); + + this.timestamp = await time.latest(); + this.request = { + from: this.alice.address, + to: constants.ZERO_ADDRESS, + value: '0', + gas: '100000', + data: '0x', + deadline: this.timestamp.toNumber() + 60, // 1 minute + }; + this.requestData = { + ...this.request, + nonce: (await this.forwarder.nonces(this.alice.address)).toString(), + }; + + this.forgeData = request => ({ + types: this.types, + domain: this.domain, + primaryType: 'ForwardRequest', + message: { ...this.requestData, ...request }, + }); + this.sign = (privateKey, request) => + ethSigUtil.signTypedMessage(privateKey, { + data: this.forgeData(request), + }); + + this.requestData.signature = this.sign(this.alice.getPrivateKey()); + }); + + context('verify', function () { + context('with valid signature', function () { + it('returns true without altering the nonce', async function () { + expect(await this.forwarder.nonces(this.requestData.from)).to.be.bignumber.equal( + web3.utils.toBN(this.requestData.nonce), + ); + expect(await this.forwarder.verify(this.requestData)).to.be.equal(true); + expect(await this.forwarder.nonces(this.requestData.from)).to.be.bignumber.equal( + web3.utils.toBN(this.requestData.nonce), + ); + }); + }); + + context('with tampered values', function () { + for (const [key, value] of Object.entries(tamperedValues)) { + it(`returns false with tampered ${key}`, async function () { + expect(await this.forwarder.verify(this.forgeData({ [key]: value }).message)).to.be.equal(false); + }); + } + + it('returns false with tampered signature', async function () { + const tamperedsign = web3.utils.hexToBytes(this.requestData.signature); + tamperedsign[42] ^= 0xff; + this.requestData.signature = web3.utils.bytesToHex(tamperedsign); + expect(await this.forwarder.verify(this.requestData)).to.be.equal(false); + }); + + it('returns false with valid signature for non-current nonce', async function () { + const req = { + ...this.requestData, + nonce: this.requestData.nonce + 1, + }; + req.signature = this.sign(this.alice.getPrivateKey(), req); + expect(await this.forwarder.verify(req)).to.be.equal(false); + }); + + it('returns false with valid signature for expired deadline', async function () { + const req = { + ...this.requestData, + deadline: this.timestamp - 1, + }; + req.signature = this.sign(this.alice.getPrivateKey(), req); + expect(await this.forwarder.verify(req)).to.be.equal(false); + }); + }); + }); + + context('execute', function () { + context('with valid requests', function () { + beforeEach(async function () { + expect(await this.forwarder.nonces(this.requestData.from)).to.be.bignumber.equal( + web3.utils.toBN(this.requestData.nonce), + ); + }); + + it('emits an event and consumes nonce for a successful request', async function () { + const receipt = await this.forwarder.execute(this.requestData); + expectEvent(receipt, 'ExecutedForwardRequest', { + signer: this.requestData.from, + nonce: web3.utils.toBN(this.requestData.nonce), + success: true, + }); + expect(await this.forwarder.nonces(this.requestData.from)).to.be.bignumber.equal( + web3.utils.toBN(this.requestData.nonce + 1), + ); + }); + + it('reverts with an unsuccessful request', async function () { + const receiver = await CallReceiverMock.new(); + const req = { + ...this.requestData, + to: receiver.address, + data: receiver.contract.methods.mockFunctionRevertsNoReason().encodeABI(), + }; + req.signature = this.sign(this.alice.getPrivateKey(), req); + await expectRevertCustomError(this.forwarder.execute(req), 'FailedInnerCall', []); + }); + }); + + context('with tampered request', function () { + for (const [key, value] of Object.entries(tamperedValues)) { + it(`reverts with tampered ${key}`, async function () { + const data = this.forgeData({ [key]: value }); + await expectRevertCustomError( + this.forwarder.execute(data.message, { + value: key == 'value' ? value : 0, // To avoid MismatchedValue error + }), + 'ERC2771ForwarderInvalidSigner', + [ethSigUtil.recoverTypedSignature({ data, sig: this.requestData.signature }), data.message.from], + ); + }); + } + + it('reverts with tampered signature', async function () { + const tamperedSig = web3.utils.hexToBytes(this.requestData.signature); + tamperedSig[42] ^= 0xff; + this.requestData.signature = web3.utils.bytesToHex(tamperedSig); + await expectRevertCustomError(this.forwarder.execute(this.requestData), 'ERC2771ForwarderInvalidSigner', [ + ethSigUtil.recoverTypedSignature({ data: this.forgeData(), sig: tamperedSig }), + this.requestData.from, + ]); + }); + + it('reverts with valid signature for non-current nonce', async function () { + // Execute first a request + await this.forwarder.execute(this.requestData); + + // And then fail due to an already used nonce + await expectRevertCustomError(this.forwarder.execute(this.requestData), 'ERC2771ForwarderInvalidSigner', [ + ethSigUtil.recoverTypedSignature({ + data: this.forgeData({ ...this.requestData, nonce: this.requestData.nonce + 1 }), + sig: this.requestData.signature, + }), + this.requestData.from, + ]); + }); + + it('reverts with valid signature for expired deadline', async function () { + const req = { + ...this.requestData, + deadline: this.timestamp - 1, + }; + req.signature = this.sign(this.alice.getPrivateKey(), req); + await expectRevertCustomError(this.forwarder.execute(req), 'ERC2771ForwarderExpiredRequest', [ + this.timestamp - 1, + ]); + }); + + it('reverts with valid signature but mismatched value', async function () { + const value = 100; + const req = { + ...this.requestData, + value, + }; + req.signature = this.sign(this.alice.getPrivateKey(), req); + await expectRevertCustomError(this.forwarder.execute(req), 'ERC2771ForwarderMismatchedValue', [0, value]); + }); + }); + + it('bubbles out of gas', async function () { + const receiver = await CallReceiverMock.new(); + const gasAvailable = 100000; + this.requestData.to = receiver.address; + this.requestData.data = receiver.contract.methods.mockFunctionOutOfGas().encodeABI(); + this.requestData.gas = 1000000; + + this.requestData.signature = this.sign(this.alice.getPrivateKey()); + + await expectRevert.assertion(this.forwarder.execute(this.requestData, { gas: gasAvailable })); + + const { transactions } = await web3.eth.getBlock('latest'); + const { gasUsed } = await web3.eth.getTransactionReceipt(transactions[0]); + + expect(gasUsed).to.be.equal(gasAvailable); + }); + }); + + context('executeBatch', function () { + const batchValue = requestDatas => requestDatas.reduce((value, request) => value + Number(request.value), 0); + + beforeEach(async function () { + this.bob = Wallet.generate(); + this.bob.address = web3.utils.toChecksumAddress(this.bob.getAddressString()); + + this.eve = Wallet.generate(); + this.eve.address = web3.utils.toChecksumAddress(this.eve.getAddressString()); + + this.signers = [this.alice, this.bob, this.eve]; + + this.requestDatas = await Promise.all( + this.signers.map(async ({ address }) => ({ + ...this.requestData, + from: address, + nonce: (await this.forwarder.nonces(address)).toString(), + value: web3.utils.toWei('10', 'gwei'), + })), + ); + + this.requestDatas = this.requestDatas.map((requestData, i) => ({ + ...requestData, + signature: this.sign(this.signers[i].getPrivateKey(), requestData), + })); + + this.msgValue = batchValue(this.requestDatas); + }); + + context('with valid requests', function () { + beforeEach(async function () { + for (const request of this.requestDatas) { + expect(await this.forwarder.verify(request)).to.be.equal(true); + } + + this.receipt = await this.forwarder.executeBatch(this.requestDatas, another, { value: this.msgValue }); + }); + + it('emits events', async function () { + for (const request of this.requestDatas) { + expectEvent(this.receipt, 'ExecutedForwardRequest', { + signer: request.from, + nonce: web3.utils.toBN(request.nonce), + success: true, + }); + } + }); + + it('increase nonces', async function () { + for (const request of this.requestDatas) { + expect(await this.forwarder.nonces(request.from)).to.be.bignumber.eq(web3.utils.toBN(request.nonce + 1)); + } + }); + }); + + context('with tampered requests', function () { + beforeEach(async function () { + this.idx = 1; // Tampered idx + }); + + it('reverts with mismatched value', async function () { + this.requestDatas[this.idx].value = 100; + this.requestDatas[this.idx].signature = this.sign( + this.signers[this.idx].getPrivateKey(), + this.requestDatas[this.idx], + ); + await expectRevertCustomError( + this.forwarder.executeBatch(this.requestDatas, another, { value: this.msgValue }), + 'ERC2771ForwarderMismatchedValue', + [batchValue(this.requestDatas), this.msgValue], + ); + }); + + context('when the refund receiver is the zero address', function () { + beforeEach(function () { + this.refundReceiver = constants.ZERO_ADDRESS; + }); + + for (const [key, value] of Object.entries(tamperedValues)) { + it(`reverts with at least one tampered request ${key}`, async function () { + const data = this.forgeData({ ...this.requestDatas[this.idx], [key]: value }); + + this.requestDatas[this.idx] = data.message; + + await expectRevertCustomError( + this.forwarder.executeBatch(this.requestDatas, this.refundReceiver, { value: this.msgValue }), + 'ERC2771ForwarderInvalidSigner', + [ + ethSigUtil.recoverTypedSignature({ data, sig: this.requestDatas[this.idx].signature }), + data.message.from, + ], + ); + }); + } + + it('reverts with at least one tampered request signature', async function () { + const tamperedSig = web3.utils.hexToBytes(this.requestDatas[this.idx].signature); + tamperedSig[42] ^= 0xff; + + this.requestDatas[this.idx].signature = web3.utils.bytesToHex(tamperedSig); + + await expectRevertCustomError( + this.forwarder.executeBatch(this.requestDatas, this.refundReceiver, { value: this.msgValue }), + 'ERC2771ForwarderInvalidSigner', + [ + ethSigUtil.recoverTypedSignature({ + data: this.forgeData(this.requestDatas[this.idx]), + sig: this.requestDatas[this.idx].signature, + }), + this.requestDatas[this.idx].from, + ], + ); + }); + + it('reverts with at least one valid signature for non-current nonce', async function () { + // Execute first a request + await this.forwarder.execute(this.requestDatas[this.idx], { value: this.requestDatas[this.idx].value }); + + // And then fail due to an already used nonce + await expectRevertCustomError( + this.forwarder.executeBatch(this.requestDatas, this.refundReceiver, { value: this.msgValue }), + 'ERC2771ForwarderInvalidSigner', + [ + ethSigUtil.recoverTypedSignature({ + data: this.forgeData({ ...this.requestDatas[this.idx], nonce: this.requestDatas[this.idx].nonce + 1 }), + sig: this.requestDatas[this.idx].signature, + }), + this.requestDatas[this.idx].from, + ], + ); + }); + + it('reverts with at least one valid signature for expired deadline', async function () { + this.requestDatas[this.idx].deadline = this.timestamp.toNumber() - 1; + this.requestDatas[this.idx].signature = this.sign( + this.signers[this.idx].getPrivateKey(), + this.requestDatas[this.idx], + ); + await expectRevertCustomError( + this.forwarder.executeBatch(this.requestDatas, this.refundReceiver, { value: this.msgValue }), + 'ERC2771ForwarderExpiredRequest', + [this.timestamp.toNumber() - 1], + ); + }); + }); + + context('when the refund receiver is a known address', function () { + beforeEach(async function () { + this.refundReceiver = refundReceiver; + this.initialRefundReceiverBalance = web3.utils.toBN(await web3.eth.getBalance(this.refundReceiver)); + this.initialTamperedRequestNonce = await this.forwarder.nonces(this.requestDatas[this.idx].from); + }); + + for (const [key, value] of Object.entries(tamperedValues)) { + it(`ignores a request with tampered ${key} and refunds its value`, async function () { + const data = this.forgeData({ ...this.requestDatas[this.idx], [key]: value }); + + this.requestDatas[this.idx] = data.message; + + const receipt = await this.forwarder.executeBatch(this.requestDatas, this.refundReceiver, { + value: batchValue(this.requestDatas), + }); + expect(receipt.logs.filter(({ event }) => event === 'ExecutedForwardRequest').length).to.be.equal(2); + }); + } + + it('ignores a request with a valid signature for non-current nonce', async function () { + // Execute first a request + await this.forwarder.execute(this.requestDatas[this.idx], { value: this.requestDatas[this.idx].value }); + this.initialTamperedRequestNonce++; // Should be already incremented by the individual `execute` + + // And then ignore the same request in a batch due to an already used nonce + const receipt = await this.forwarder.executeBatch(this.requestDatas, this.refundReceiver, { + value: this.msgValue, + }); + expect(receipt.logs.filter(({ event }) => event === 'ExecutedForwardRequest').length).to.be.equal(2); + }); + + it('ignores a request with a valid signature for expired deadline', async function () { + this.requestDatas[this.idx].deadline = this.timestamp.toNumber() - 1; + this.requestDatas[this.idx].signature = this.sign( + this.signers[this.idx].getPrivateKey(), + this.requestDatas[this.idx], + ); + + const receipt = await this.forwarder.executeBatch(this.requestDatas, this.refundReceiver, { + value: this.msgValue, + }); + expect(receipt.logs.filter(({ event }) => event === 'ExecutedForwardRequest').length).to.be.equal(2); + }); + + afterEach(async function () { + // The invalid request value was refunded + expect(await web3.eth.getBalance(this.refundReceiver)).to.be.bignumber.equal( + this.initialRefundReceiverBalance.add(web3.utils.toBN(this.requestDatas[this.idx].value)), + ); + + // The invalid request from's nonce was not incremented + expect(await this.forwarder.nonces(this.requestDatas[this.idx].from)).to.be.bignumber.eq( + web3.utils.toBN(this.initialTamperedRequestNonce), + ); + }); + }); + }); + }); +}); diff --git a/test/metatx/MinimalForwarder.test.js b/test/metatx/MinimalForwarder.test.js deleted file mode 100644 index c775c5e4403..00000000000 --- a/test/metatx/MinimalForwarder.test.js +++ /dev/null @@ -1,171 +0,0 @@ -const ethSigUtil = require('eth-sig-util'); -const Wallet = require('ethereumjs-wallet').default; -const { getDomain, domainType } = require('../helpers/eip712'); -const { expectRevertCustomError } = require('../helpers/customError'); - -const { constants, expectRevert } = require('@openzeppelin/test-helpers'); -const { expect } = require('chai'); - -const MinimalForwarder = artifacts.require('MinimalForwarder'); -const CallReceiverMock = artifacts.require('CallReceiverMock'); - -contract('MinimalForwarder', function (accounts) { - beforeEach(async function () { - this.forwarder = await MinimalForwarder.new(); - - this.domain = await getDomain(this.forwarder); - this.types = { - EIP712Domain: domainType(this.domain), - ForwardRequest: [ - { name: 'from', type: 'address' }, - { name: 'to', type: 'address' }, - { name: 'value', type: 'uint256' }, - { name: 'gas', type: 'uint256' }, - { name: 'nonce', type: 'uint256' }, - { name: 'data', type: 'bytes' }, - ], - }; - }); - - context('with message', function () { - const tamperedValues = { - from: accounts[0], - to: accounts[0], - value: web3.utils.toWei('1'), - nonce: 1234, - data: '0x1742', - }; - - beforeEach(async function () { - this.wallet = Wallet.generate(); - this.sender = web3.utils.toChecksumAddress(this.wallet.getAddressString()); - this.req = { - from: this.sender, - to: constants.ZERO_ADDRESS, - value: '0', - gas: '100000', - nonce: Number(await this.forwarder.getNonce(this.sender)), - data: '0x', - }; - this.forgeData = req => ({ - types: this.types, - domain: this.domain, - primaryType: 'ForwardRequest', - message: { ...this.req, ...req }, - }); - this.sign = req => - ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { - data: this.forgeData(req), - }); - }); - - context('verify', function () { - context('valid signature', function () { - beforeEach(async function () { - expect(await this.forwarder.getNonce(this.req.from)).to.be.bignumber.equal(web3.utils.toBN(this.req.nonce)); - }); - - it('success', async function () { - expect(await this.forwarder.verify(this.req, this.sign())).to.be.equal(true); - }); - - afterEach(async function () { - expect(await this.forwarder.getNonce(this.req.from)).to.be.bignumber.equal(web3.utils.toBN(this.req.nonce)); - }); - }); - - context('with tampered values', function () { - for (const [key, value] of Object.entries(tamperedValues)) { - it(`returns false with tampered ${key}`, async function () { - expect(await this.forwarder.verify(this.forgeData({ [key]: value }).message, this.sign())).to.be.equal( - false, - ); - }); - } - - it('returns false with tampered signature', async function () { - const tamperedsign = web3.utils.hexToBytes(this.sign()); - tamperedsign[42] ^= 0xff; - expect(await this.forwarder.verify(this.req, web3.utils.bytesToHex(tamperedsign))).to.be.equal(false); - }); - - it('returns false with valid signature for non-current nonce', async function () { - const req = { - ...this.req, - nonce: this.req.nonce + 1, - }; - const sig = this.sign(req); - expect(await this.forwarder.verify(req, sig)).to.be.equal(false); - }); - }); - }); - - context('execute', function () { - context('valid signature', function () { - beforeEach(async function () { - expect(await this.forwarder.getNonce(this.req.from)).to.be.bignumber.equal(web3.utils.toBN(this.req.nonce)); - }); - - it('success', async function () { - await this.forwarder.execute(this.req, this.sign()); // expect to not revert - }); - - afterEach(async function () { - expect(await this.forwarder.getNonce(this.req.from)).to.be.bignumber.equal( - web3.utils.toBN(this.req.nonce + 1), - ); - }); - }); - - context('with tampered values', function () { - for (const [key, value] of Object.entries(tamperedValues)) { - it(`reverts with tampered ${key}`, async function () { - const sig = this.sign(); - const data = this.forgeData({ [key]: value }); - await expectRevertCustomError(this.forwarder.execute(data.message, sig), 'MinimalForwarderInvalidSigner', [ - ethSigUtil.recoverTypedSignature({ data, sig }), - data.message.from, - ]); - }); - } - - it('reverts with tampered signature', async function () { - const tamperedSig = web3.utils.hexToBytes(this.sign()); - tamperedSig[42] ^= 0xff; - await expectRevertCustomError( - this.forwarder.execute(this.req, web3.utils.bytesToHex(tamperedSig)), - 'MinimalForwarderInvalidSigner', - [ethSigUtil.recoverTypedSignature({ data: this.forgeData(), sig: tamperedSig }), this.req.from], - ); - }); - - it('reverts with valid signature for non-current nonce', async function () { - const req = { - ...this.req, - nonce: this.req.nonce + 1, - }; - const sig = this.sign(req); - await expectRevertCustomError(this.forwarder.execute(req, sig), 'MinimalForwarderInvalidNonce', [ - this.req.from, - this.req.nonce, - ]); - }); - }); - - it('bubble out of gas', async function () { - const receiver = await CallReceiverMock.new(); - const gasAvailable = 100000; - this.req.to = receiver.address; - this.req.data = receiver.contract.methods.mockFunctionOutOfGas().encodeABI(); - this.req.gas = 1000000; - - await expectRevert.assertion(this.forwarder.execute(this.req, this.sign(), { gas: gasAvailable })); - - const { transactions } = await web3.eth.getBlock('latest'); - const { gasUsed } = await web3.eth.getTransactionReceipt(transactions[0]); - - expect(gasUsed).to.be.equal(gasAvailable); - }); - }); - }); -}); From f808576d7f06d61d20873b7df197747e4a3dc6d9 Mon Sep 17 00:00:00 2001 From: Renan Souza Date: Thu, 29 Jun 2023 13:41:44 -0300 Subject: [PATCH 16/17] Fix details AccessControl-test (#4391) --- test/access/AccessControl.behavior.js | 2 +- test/access/AccessControlEnumerable.test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index b1729c5d6ac..20804f04990 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -382,7 +382,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(delay, defaultAdmin, new it("should revert if defaultAdmin's admin is changed", async function () { await expectRevertCustomError( - this.accessControl.$_setRoleAdmin(DEFAULT_ADMIN_ROLE, defaultAdmin), + this.accessControl.$_setRoleAdmin(DEFAULT_ADMIN_ROLE, OTHER_ROLE), 'AccessControlEnforcedDefaultAdminRules', [], ); diff --git a/test/access/AccessControlEnumerable.test.js b/test/access/AccessControlEnumerable.test.js index 0e1879700d0..429f22f8f1a 100644 --- a/test/access/AccessControlEnumerable.test.js +++ b/test/access/AccessControlEnumerable.test.js @@ -6,7 +6,7 @@ const { const AccessControlEnumerable = artifacts.require('$AccessControlEnumerable'); -contract('AccessControl', function (accounts) { +contract('AccessControlEnumerable', function (accounts) { beforeEach(async function () { this.accessControl = await AccessControlEnumerable.new({ from: accounts[0] }); await this.accessControl.$_grantRole(DEFAULT_ADMIN_ROLE, accounts[0]); From 2ed9be3e3c02d5084c22ddbcdebef4b3aff11bd6 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Thu, 29 Jun 2023 12:57:33 -0600 Subject: [PATCH 17/17] Fix tests --- test/governance/Governor.test.js | 89 ++++++++------- .../extensions/GovernorWithParams.test.js | 101 ++++++++++-------- test/helpers/governance.js | 53 ++++----- 3 files changed, 135 insertions(+), 108 deletions(-) diff --git a/test/governance/Governor.test.js b/test/governance/Governor.test.js index 2c1cbf50a98..f4a352c6d8c 100644 --- a/test/governance/Governor.test.js +++ b/test/governance/Governor.test.js @@ -2,7 +2,7 @@ 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 } = require('ethereumjs-util'); +const { fromRpcSig, toRpcSig } = require('ethereumjs-util'); const Enums = require('../helpers/enums'); const { getDomain, domainType } = require('../helpers/eip712'); @@ -311,22 +311,24 @@ contract('Governor', function (accounts) { this.voterBySig = Wallet.generate(); this.voterBySig.address = web3.utils.toChecksumAddress(this.voterBySig.getAddressString()); + this.data = (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, + })); + this.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, - })) + this.data(contract, message) .then(data => ethSigUtil.signTypedMessage(this.voterBySig.getPrivateKey(), { data })) .then(fromRpcSig); @@ -340,35 +342,46 @@ contract('Governor', function (accounts) { it('if signature does not match signer', async function () { const nonce = await this.mock.nonces(this.voterBySig.address); - expectRevertCustomError( - this.helper.vote({ - support: Enums.VoteType.For, - voter: this.voterBySig.address, - nonce, - signature: (...params) => { - const sig = this.signature(...params); - sig[69] ^= 0xff; - return sig; - }, - }), - 'GovernorInvalidSigner', - [], - ); + 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 { 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, + ]); }); it('if vote nonce is incorrect', async function () { const nonce = await this.mock.nonces(this.voterBySig.address); - expectRevertCustomError( - this.helper.vote({ - support: Enums.VoteType.For, - voter: this.voterBySig.address, - nonce: nonce.addn(1), - signature: this.signature, - }), + const voteParams = { + support: Enums.VoteType.For, + voter: this.voterBySig.address, + nonce: nonce.addn(1), + 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], ); }); }); diff --git a/test/governance/extensions/GovernorWithParams.test.js b/test/governance/extensions/GovernorWithParams.test.js index 1a0d242acda..fb58edaaf57 100644 --- a/test/governance/extensions/GovernorWithParams.test.js +++ b/test/governance/extensions/GovernorWithParams.test.js @@ -2,7 +2,7 @@ const { expectEvent } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); const ethSigUtil = require('eth-sig-util'); const Wallet = require('ethereumjs-wallet').default; -const { fromRpcSig } = require('ethereumjs-util'); +const { fromRpcSig, toRpcSig } = require('ethereumjs-util'); const Enums = require('../../helpers/enums'); const { getDomain, domainType } = require('../../helpers/eip712'); @@ -125,24 +125,26 @@ contract('GovernorWithParams', function (accounts) { this.voterBySig = Wallet.generate(); this.voterBySig.address = web3.utils.toChecksumAddress(this.voterBySig.getAddressString()); + this.data = (contract, message) => + getDomain(contract).then(domain => ({ + primaryType: 'ExtendedBallot', + types: { + EIP712Domain: domainType(domain), + ExtendedBallot: [ + { name: 'proposalId', type: 'uint256' }, + { name: 'support', type: 'uint8' }, + { name: 'voter', type: 'address' }, + { name: 'nonce', type: 'uint256' }, + { name: 'reason', type: 'string' }, + { name: 'params', type: 'bytes' }, + ], + }, + domain, + message, + })); + this.signature = (contract, message) => - getDomain(contract) - .then(domain => ({ - primaryType: 'ExtendedBallot', - types: { - EIP712Domain: domainType(domain), - ExtendedBallot: [ - { name: 'proposalId', type: 'uint256' }, - { name: 'support', type: 'uint8' }, - { name: 'voter', type: 'address' }, - { name: 'nonce', type: 'uint256' }, - { name: 'reason', type: 'string' }, - { name: 'params', type: 'bytes' }, - ], - }, - domain, - message, - })) + this.data(contract, message) .then(data => ethSigUtil.signTypedMessage(this.voterBySig.getPrivateKey(), { data })) .then(fromRpcSig); @@ -185,39 +187,50 @@ contract('GovernorWithParams', function (accounts) { it('reverts if signature does not match signer', async function () { const nonce = await this.mock.nonces(this.voterBySig.address); - expectRevertCustomError( - this.helper.vote({ - support: Enums.VoteType.For, - voter: this.voterBySig.address, - nonce, - signature: (...params) => { - const sig = this.signature(...params); - sig[69] ^= 0xff; - return sig; - }, - reason: 'no particular reason', - params: encodedParams, - }), - 'GovernorInvalidSigner', - [], - ); + 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; + }, + 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, + ]); }); it('reverts if vote nonce is incorrect', async function () { const nonce = await this.mock.nonces(this.voterBySig.address); - expectRevertCustomError( - this.helper.vote({ - support: Enums.VoteType.For, - voter: this.voterBySig.address, - nonce: nonce.addn(1), - signature: this.signature, - reason: 'no particular reason', - params: encodedParams, - }), + const voteParams = { + support: Enums.VoteType.For, + voter: this.voterBySig.address, + nonce: nonce.addn(1), + signature: this.signature, + 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], ); }); }); diff --git a/test/helpers/governance.js b/test/helpers/governance.js index 333904dd8c1..588aeb258c0 100644 --- a/test/helpers/governance.js +++ b/test/helpers/governance.js @@ -91,33 +91,17 @@ class GovernorHelper { return vote.signature ? // if signature, and either params or reason → vote.params || vote.reason - ? vote - .signature(this.governor, { - proposalId: proposal.id, - support: vote.support, - voter: vote.voter, - nonce: vote.nonce, - reason: vote.reason || '', - params: vote.params || '', - }) - .then(({ v, r, s }) => - this.governor.castVoteWithReasonAndParamsBySig( - ...concatOpts( - [proposal.id, vote.support, vote.voter, vote.reason || '', vote.params || '', v, r, s], - opts, - ), + ? this.sign(vote).then(({ v, r, s }) => + this.governor.castVoteWithReasonAndParamsBySig( + ...concatOpts( + [proposal.id, vote.support, vote.voter, vote.reason || '', vote.params || '', v, r, s], + opts, ), - ) - : vote - .signature(this.governor, { - proposalId: proposal.id, - support: vote.support, - voter: vote.voter, - nonce: vote.nonce, - }) - .then(({ v, r, s }) => - this.governor.castVoteBySig(...concatOpts([proposal.id, vote.support, vote.voter, v, r, s], opts)), - ) + ), + ) + : this.sign(vote).then(({ v, r, s }) => + this.governor.castVoteBySig(...concatOpts([proposal.id, vote.support, vote.voter, v, r, s], opts)), + ) : vote.params ? // otherwise if params this.governor.castVoteWithReasonAndParams( @@ -129,6 +113,23 @@ class GovernorHelper { : this.governor.castVote(...concatOpts([proposal.id, vote.support], opts)); } + sign(vote = {}) { + return vote.signature(this.governor, this.forgeMessage(vote)); + } + + forgeMessage(vote = {}) { + const proposal = this.currentProposal; + + const message = { proposalId: proposal.id, support: vote.support, voter: vote.voter, nonce: vote.nonce }; + + if (vote.params || vote.reason) { + message.reason = vote.reason || ''; + message.params = vote.params || ''; + } + + return message; + } + async waitForSnapshot(offset = 0) { const proposal = this.currentProposal; const timepoint = await this.governor.proposalSnapshot(proposal.id);