From 361292b4a7b73b42298947598d64a7a77deafd9d Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 18 Jan 2023 19:06:08 +0100 Subject: [PATCH 01/18] add ERC5267 interface --- contracts/interfaces/ERC5267.sol | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 contracts/interfaces/ERC5267.sol diff --git a/contracts/interfaces/ERC5267.sol b/contracts/interfaces/ERC5267.sol new file mode 100644 index 00000000000..e45b867ef46 --- /dev/null +++ b/contracts/interfaces/ERC5267.sol @@ -0,0 +1,24 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +interface ERC5267 { + /** + * @dev MAY be emitted to signal that the domain could have changed. + */ + event EIP712DomainChanged(); + + /** + * @dev returns the fields and values that describe the domain separator used by this contract for EIP-712 + * signature. + */ + function eip712Domain() external view returns ( + bytes1 fields, + string memory name, + string memory version, + uint256 chainId, + address verifyingContract, + bytes32 salt, + uint256[] memory extensions + ); +} From 4f71bf15d1144120e3acd4299929faa83275bcfb Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 18 Jan 2023 19:06:40 +0100 Subject: [PATCH 02/18] expose ERC5267 in EIP712.sol --- contracts/utils/cryptography/EIP712.sol | 39 ++++++++++++++++++++----- 1 file changed, 32 insertions(+), 7 deletions(-) diff --git a/contracts/utils/cryptography/EIP712.sol b/contracts/utils/cryptography/EIP712.sol index eb211a7e27f..f3a8db68ba5 100644 --- a/contracts/utils/cryptography/EIP712.sol +++ b/contracts/utils/cryptography/EIP712.sol @@ -4,6 +4,7 @@ pragma solidity ^0.8.0; import "./ECDSA.sol"; +import "../../interfaces/ERC5267.sol"; /** * @dev https://eips.ethereum.org/EIPS/eip-712[EIP 712] is a standard for hashing and signing of typed structured data. @@ -24,7 +25,7 @@ import "./ECDSA.sol"; * * _Available since v3.4._ */ -abstract contract EIP712 { +abstract contract EIP712 is ERC5267 { /* solhint-disable var-name-mixedcase */ // Cache the domain separator as an immutable value, but also store the chain id that it corresponds to, in order to // invalidate the cached domain separator if the chain id changes. @@ -32,9 +33,12 @@ abstract contract EIP712 { uint256 private immutable _CACHED_CHAIN_ID; address private immutable _CACHED_THIS; + string private _NAME; + string private _VERSION; + bytes32 private immutable _HASHED_NAME; bytes32 private immutable _HASHED_VERSION; - bytes32 private immutable _TYPE_HASH; + bytes32 private immutable _TYPE_HASH = keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"); /* solhint-enable var-name-mixedcase */ @@ -53,15 +57,13 @@ abstract contract EIP712 { constructor(string memory name, string memory version) { bytes32 hashedName = keccak256(bytes(name)); bytes32 hashedVersion = keccak256(bytes(version)); - bytes32 typeHash = keccak256( - "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)" - ); + _NAME = name; + _VERSION = version; _HASHED_NAME = hashedName; _HASHED_VERSION = hashedVersion; _CACHED_CHAIN_ID = block.chainid; - _CACHED_DOMAIN_SEPARATOR = _buildDomainSeparator(typeHash, hashedName, hashedVersion); + _CACHED_DOMAIN_SEPARATOR = _buildDomainSeparator(_TYPE_HASH, hashedName, hashedVersion); _CACHED_THIS = address(this); - _TYPE_HASH = typeHash; } /** @@ -101,4 +103,27 @@ abstract contract EIP712 { function _hashTypedDataV4(bytes32 structHash) internal view virtual returns (bytes32) { return ECDSA.toTypedDataHash(_domainSeparatorV4(), structHash); } + + /** + * @dev See {EIP-5267}. + */ + function eip712Domain() public view virtual override returns ( + bytes1 fields, + string memory name, + string memory version, + uint256 chainId, + address verifyingContract, + bytes32 salt, + uint256[] memory extensions + ) { + return ( + hex"0f", // 01111 + _NAME, + _VERSION, + block.chainid, + address(this), + bytes32(0), + new uint256[](0) + ); + } } From 9aa3246dc379b1bd937e57e7f7cb7b50fdb19fda Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 18 Jan 2023 19:07:03 +0100 Subject: [PATCH 03/18] update tests to use extract domain from EIP5267 --- test/governance/Governor.test.js | 35 +++--- .../extensions/GovernorWithParams.test.js | 39 +++---- test/governance/utils/Votes.behavior.js | 110 +++++++----------- test/helpers/eip712.js | 31 ++++- test/helpers/governance.js | 4 +- test/metatx/ERC2771Context.test.js | 15 +-- test/metatx/MinimalForwarder.test.js | 17 +-- .../token/ERC20/extensions/ERC20Votes.test.js | 105 +++++++---------- .../ERC20/extensions/ERC20VotesComp.test.js | 105 +++++++---------- .../extensions/draft-ERC20Permit.test.js | 37 +++--- test/token/ERC20/utils/SafeERC20.test.js | 13 +-- test/utils/cryptography/EIP712.test.js | 19 ++- 12 files changed, 244 insertions(+), 286 deletions(-) diff --git a/test/governance/Governor.test.js b/test/governance/Governor.test.js index cc3cc17ef45..2256997050b 100644 --- a/test/governance/Governor.test.js +++ b/test/governance/Governor.test.js @@ -4,7 +4,7 @@ const ethSigUtil = require('eth-sig-util'); const Wallet = require('ethereumjs-wallet').default; const { fromRpcSig } = require('ethereumjs-util'); const Enums = require('../helpers/enums'); -const { EIP712Domain } = require('../helpers/eip712'); +const { getDomain, domainType } = require('../helpers/eip712'); const { GovernorHelper } = require('../helpers/governance'); const { shouldSupportInterfaces } = require('../utils/introspection/SupportsInterface.behavior'); @@ -148,24 +148,21 @@ contract('Governor', function (accounts) { const voterBySig = Wallet.generate(); const voterBySigAddress = web3.utils.toChecksumAddress(voterBySig.getAddressString()); - const signature = async message => { - return fromRpcSig( - ethSigUtil.signTypedMessage(voterBySig.getPrivateKey(), { - data: { - types: { - EIP712Domain, - Ballot: [ - { name: 'proposalId', type: 'uint256' }, - { name: 'support', type: 'uint8' }, - ], - }, - domain: { name, version, chainId: this.chainId, verifyingContract: this.mock.address }, - primaryType: 'Ballot', - message, - }, - }), - ); - }; + const signature = (contract, message) => getDomain(contract) + .then(domain => ({ + primaryType: 'Ballot', + types: { + EIP712Domain: domainType(domain), + Ballot: [ + { name: 'proposalId', type: 'uint256' }, + { name: 'support', type: 'uint8' }, + ], + }, + domain, + message, + })) + .then(data => ethSigUtil.signTypedMessage(voterBySig.getPrivateKey(), { data })) + .then(fromRpcSig); await this.token.delegate(voterBySigAddress, { from: voter1 }); diff --git a/test/governance/extensions/GovernorWithParams.test.js b/test/governance/extensions/GovernorWithParams.test.js index 26e94854439..ae2f42636fd 100644 --- a/test/governance/extensions/GovernorWithParams.test.js +++ b/test/governance/extensions/GovernorWithParams.test.js @@ -4,7 +4,7 @@ const ethSigUtil = require('eth-sig-util'); const Wallet = require('ethereumjs-wallet').default; const { fromRpcSig } = require('ethereumjs-util'); const Enums = require('../../helpers/enums'); -const { EIP712Domain } = require('../../helpers/eip712'); +const { getDomain, domainType } = require('../../helpers/eip712'); const { GovernorHelper } = require('../../helpers/governance'); const Token = artifacts.require('$ERC20VotesComp'); @@ -116,26 +116,23 @@ contract('GovernorWithParams', function (accounts) { const voterBySig = Wallet.generate(); const voterBySigAddress = web3.utils.toChecksumAddress(voterBySig.getAddressString()); - const signature = async message => { - return fromRpcSig( - ethSigUtil.signTypedMessage(voterBySig.getPrivateKey(), { - data: { - types: { - EIP712Domain, - ExtendedBallot: [ - { name: 'proposalId', type: 'uint256' }, - { name: 'support', type: 'uint8' }, - { name: 'reason', type: 'string' }, - { name: 'params', type: 'bytes' }, - ], - }, - domain: { name, version, chainId: this.chainId, verifyingContract: this.mock.address }, - primaryType: 'ExtendedBallot', - message, - }, - }), - ); - }; + 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); await this.token.delegate(voterBySigAddress, { from: voter2 }); diff --git a/test/governance/utils/Votes.behavior.js b/test/governance/utils/Votes.behavior.js index 5062a9c9701..d83b1691ab6 100644 --- a/test/governance/utils/Votes.behavior.js +++ b/test/governance/utils/Votes.behavior.js @@ -6,7 +6,7 @@ const { fromRpcSig } = require('ethereumjs-util'); const ethSigUtil = require('eth-sig-util'); const Wallet = require('ethereumjs-wallet').default; -const { EIP712Domain, domainSeparator } = require('../../helpers/eip712'); +const { getDomain, domainType, domainSeparator } = require('../../helpers/eip712'); const Delegation = [ { name: 'delegatee', type: 'address' }, @@ -24,12 +24,7 @@ function shouldBehaveLikeVotes() { it('domain separator', async function () { expect(await this.votes.DOMAIN_SEPARATOR()).to.equal( - await domainSeparator({ - name: this.name, - version, - chainId: this.chainId, - verifyingContract: this.votes.address, - }), + await getDomain(this.votes).then(domainSeparator), ); }); @@ -38,30 +33,25 @@ function shouldBehaveLikeVotes() { const delegatorAddress = web3.utils.toChecksumAddress(delegator.getAddressString()); const nonce = 0; - const buildData = (chainId, verifyingContract, name, message) => ({ - data: { - primaryType: 'Delegation', - types: { EIP712Domain, Delegation }, - domain: { name, version, chainId, verifyingContract }, - message, - }, - }); + const buildData = (contract, message) => getDomain(contract).then(domain => ({ + primaryType: 'Delegation', + types: { EIP712Domain: domainType(domain), Delegation }, + domain, + message, + })); beforeEach(async function () { await this.votes.$_mint(delegatorAddress, this.NFT0); }); it('accept signed delegation', async function () { - const { v, r, s } = fromRpcSig( - ethSigUtil.signTypedMessage( - delegator.getPrivateKey(), - buildData(this.chainId, this.votes.address, this.name, { - delegatee: delegatorAddress, - nonce, - expiry: MAX_UINT256, - }), - ), - ); + const { v, r, s } = await buildData(this.votes, { + delegatee: delegatorAddress, + nonce, + expiry: MAX_UINT256, + }) + .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) + .then(fromRpcSig); expect(await this.votes.delegates(delegatorAddress)).to.be.equal(ZERO_ADDRESS); @@ -86,16 +76,13 @@ function shouldBehaveLikeVotes() { }); it('rejects reused signature', async function () { - const { v, r, s } = fromRpcSig( - ethSigUtil.signTypedMessage( - delegator.getPrivateKey(), - buildData(this.chainId, this.votes.address, this.name, { - delegatee: delegatorAddress, - nonce, - expiry: MAX_UINT256, - }), - ), - ); + const { v, r, s } = await buildData(this.votes, { + delegatee: delegatorAddress, + nonce, + expiry: MAX_UINT256, + }) + .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) + .then(fromRpcSig); await this.votes.delegateBySig(delegatorAddress, nonce, MAX_UINT256, v, r, s); @@ -106,16 +93,13 @@ function shouldBehaveLikeVotes() { }); it('rejects bad delegatee', async function () { - const { v, r, s } = fromRpcSig( - ethSigUtil.signTypedMessage( - delegator.getPrivateKey(), - buildData(this.chainId, this.votes.address, this.name, { - delegatee: delegatorAddress, - nonce, - expiry: MAX_UINT256, - }), - ), - ); + const { v, r, s } = await buildData(this.votes, { + delegatee: delegatorAddress, + nonce, + expiry: MAX_UINT256, + }) + .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) + .then(fromRpcSig); const receipt = await this.votes.delegateBySig(this.account1Delegatee, nonce, MAX_UINT256, v, r, s); const { args } = receipt.logs.find(({ event }) => event === 'DelegateChanged'); @@ -125,16 +109,14 @@ function shouldBehaveLikeVotes() { }); it('rejects bad nonce', async function () { - const { v, r, s } = fromRpcSig( - ethSigUtil.signTypedMessage( - delegator.getPrivateKey(), - buildData(this.chainId, this.votes.address, this.name, { - delegatee: delegatorAddress, - nonce, - expiry: MAX_UINT256, - }), - ), - ); + const { v, r, s } = await buildData(this.votes, { + delegatee: delegatorAddress, + nonce, + expiry: MAX_UINT256, + }) + .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) + .then(fromRpcSig); + await expectRevert( this.votes.delegateBySig(delegatorAddress, nonce + 1, MAX_UINT256, v, r, s), 'Votes: invalid nonce', @@ -143,16 +125,14 @@ function shouldBehaveLikeVotes() { it('rejects expired permit', async function () { const expiry = (await time.latest()) - time.duration.weeks(1); - const { v, r, s } = fromRpcSig( - ethSigUtil.signTypedMessage( - delegator.getPrivateKey(), - buildData(this.chainId, this.votes.address, this.name, { - delegatee: delegatorAddress, - nonce, - expiry, - }), - ), - ); + + const { v, r, s } = await buildData(this.votes, { + delegatee: delegatorAddress, + nonce, + expiry, + }) + .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) + .then(fromRpcSig); await expectRevert( this.votes.delegateBySig(delegatorAddress, nonce, expiry, v, r, s), diff --git a/test/helpers/eip712.js b/test/helpers/eip712.js index 9851df66205..fc2ebbd12c2 100644 --- a/test/helpers/eip712.js +++ b/test/helpers/eip712.js @@ -6,6 +6,7 @@ const EIP712Domain = [ { name: 'version', type: 'string' }, { name: 'chainId', type: 'uint256' }, { name: 'verifyingContract', type: 'address' }, + { name: 'salt', type: 'bytes32' }, ]; const Permit = [ @@ -24,12 +25,33 @@ function hexStringToBuffer(hexstr) { return Buffer.from(hexstr.replace(/^0x/, ''), 'hex'); } -async function domainSeparator({ name, version, chainId, verifyingContract }) { +async function getDomain(contract) { + const { fields, name, version, chainId, verifyingContract, salt, extensions } = await contract.eip712Domain(); + + if (extensions.length > 0) { + throw Error("Extensions not implemented"); + } + + const domain = { name, version, chainId, verifyingContract, salt }; + for (const [i, { name }] of EIP712Domain.entries()) { + if (!(fields & (1 << i))) { + delete domain[name]; + } + } + + return domain; +} + +function domainType(domain) { + return EIP712Domain.filter(({ name }) => domain[name] !== undefined); +} + +async function domainSeparator(domain) { return bufferToHexString( ethSigUtil.TypedDataUtils.hashStruct( 'EIP712Domain', - { name, version, chainId, verifyingContract }, - { EIP712Domain }, + domain, + { EIP712Domain: domainType(domain) }, ), ); } @@ -41,8 +63,9 @@ async function hashTypedData(domain, structHash) { } module.exports = { - EIP712Domain, Permit, + getDomain, + domainType, domainSeparator, hashTypedData, }; diff --git a/test/helpers/governance.js b/test/helpers/governance.js index ff341aa12f3..c3a087be940 100644 --- a/test/helpers/governance.js +++ b/test/helpers/governance.js @@ -79,7 +79,7 @@ class GovernorHelper { ? // if signature, and either params or reason → vote.params || vote.reason ? vote - .signature({ + .signature(this.governor, { proposalId: proposal.id, support: vote.support, reason: vote.reason || '', @@ -91,7 +91,7 @@ class GovernorHelper { ), ) : vote - .signature({ + .signature(this.governor, { proposalId: proposal.id, support: vote.support, }) diff --git a/test/metatx/ERC2771Context.test.js b/test/metatx/ERC2771Context.test.js index 78877772663..6c298d3d9f0 100644 --- a/test/metatx/ERC2771Context.test.js +++ b/test/metatx/ERC2771Context.test.js @@ -1,6 +1,6 @@ const ethSigUtil = require('eth-sig-util'); const Wallet = require('ethereumjs-wallet').default; -const { EIP712Domain } = require('../helpers/eip712'); +const { getDomain, domainType } = require('../helpers/eip712'); const { expectEvent } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); @@ -10,24 +10,15 @@ const MinimalForwarder = artifacts.require('MinimalForwarder'); const ContextMockCaller = artifacts.require('ContextMockCaller'); const { shouldBehaveLikeRegularContext } = require('../utils/Context.behavior'); -const { getChainId } = require('../helpers/chainid'); - -const name = 'MinimalForwarder'; -const version = '0.0.1'; contract('ERC2771Context', function (accounts) { beforeEach(async function () { this.forwarder = await MinimalForwarder.new(); this.recipient = await ERC2771ContextMock.new(this.forwarder.address); - this.domain = { - name, - version, - chainId: await getChainId(), - verifyingContract: this.forwarder.address, - }; + this.domain = await getDomain(this.forwarder); this.types = { - EIP712Domain, + EIP712Domain: domainType(this.domain), ForwardRequest: [ { name: 'from', type: 'address' }, { name: 'to', type: 'address' }, diff --git a/test/metatx/MinimalForwarder.test.js b/test/metatx/MinimalForwarder.test.js index 24de1719d31..4884cc760bb 100644 --- a/test/metatx/MinimalForwarder.test.js +++ b/test/metatx/MinimalForwarder.test.js @@ -1,29 +1,20 @@ const ethSigUtil = require('eth-sig-util'); const Wallet = require('ethereumjs-wallet').default; -const { EIP712Domain } = require('../helpers/eip712'); +const { getDomain, domainType } = require('../helpers/eip712'); const { expectRevert, constants } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); -const { getChainId } = require('../helpers/chainid'); - const MinimalForwarder = artifacts.require('MinimalForwarder'); const CallReceiverMock = artifacts.require('CallReceiverMock'); -const name = 'MinimalForwarder'; -const version = '0.0.1'; - contract('MinimalForwarder', function (accounts) { beforeEach(async function () { this.forwarder = await MinimalForwarder.new(); - this.domain = { - name, - version, - chainId: await getChainId(), - verifyingContract: this.forwarder.address, - }; + + this.domain = await getDomain(this.forwarder); this.types = { - EIP712Domain, + EIP712Domain: domainType(this.domain), ForwardRequest: [ { name: 'from', type: 'address' }, { name: 'to', type: 'address' }, diff --git a/test/token/ERC20/extensions/ERC20Votes.test.js b/test/token/ERC20/extensions/ERC20Votes.test.js index a340e71a775..625262a1a0f 100644 --- a/test/token/ERC20/extensions/ERC20Votes.test.js +++ b/test/token/ERC20/extensions/ERC20Votes.test.js @@ -11,7 +11,7 @@ const Wallet = require('ethereumjs-wallet').default; const ERC20Votes = artifacts.require('$ERC20Votes'); const { batchInBlock } = require('../../../helpers/txpool'); -const { EIP712Domain, domainSeparator } = require('../../../helpers/eip712'); +const { getDomain, domainType, domainSeparator } = require('../../../helpers/eip712'); const { getChainId } = require('../../../helpers/chainid'); const Delegation = [ @@ -39,7 +39,7 @@ contract('ERC20Votes', function (accounts) { it('domain separator', async function () { expect(await this.token.DOMAIN_SEPARATOR()).to.equal( - await domainSeparator({ name, version, chainId: this.chainId, verifyingContract: this.token.address }), + await getDomain(this.token).then(domainSeparator), ); }); @@ -107,30 +107,25 @@ contract('ERC20Votes', function (accounts) { const delegatorAddress = web3.utils.toChecksumAddress(delegator.getAddressString()); const nonce = 0; - const buildData = (chainId, verifyingContract, message) => ({ - data: { - primaryType: 'Delegation', - types: { EIP712Domain, Delegation }, - domain: { name, version, chainId, verifyingContract }, - message, - }, - }); + const buildData = (contract, message) => getDomain(contract).then(domain => ({ + primaryType: 'Delegation', + types: { EIP712Domain: domainType(domain), Delegation }, + domain, + message, + })); beforeEach(async function () { await this.token.$_mint(delegatorAddress, supply); }); it('accept signed delegation', async function () { - const { v, r, s } = fromRpcSig( - ethSigUtil.signTypedMessage( - delegator.getPrivateKey(), - buildData(this.chainId, this.token.address, { - delegatee: delegatorAddress, - nonce, - expiry: MAX_UINT256, - }), - ), - ); + const { v, r, s } = await buildData(this.token, { + delegatee: delegatorAddress, + nonce, + expiry: MAX_UINT256, + }) + .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) + .then(fromRpcSig); expect(await this.token.delegates(delegatorAddress)).to.be.equal(ZERO_ADDRESS); @@ -155,16 +150,13 @@ contract('ERC20Votes', function (accounts) { }); it('rejects reused signature', async function () { - const { v, r, s } = fromRpcSig( - ethSigUtil.signTypedMessage( - delegator.getPrivateKey(), - buildData(this.chainId, this.token.address, { - delegatee: delegatorAddress, - nonce, - expiry: MAX_UINT256, - }), - ), - ); + const { v, r, s } = await buildData(this.token, { + delegatee: delegatorAddress, + nonce, + expiry: MAX_UINT256, + }) + .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) + .then(fromRpcSig); await this.token.delegateBySig(delegatorAddress, nonce, MAX_UINT256, v, r, s); @@ -175,16 +167,13 @@ contract('ERC20Votes', function (accounts) { }); it('rejects bad delegatee', async function () { - const { v, r, s } = fromRpcSig( - ethSigUtil.signTypedMessage( - delegator.getPrivateKey(), - buildData(this.chainId, this.token.address, { - delegatee: delegatorAddress, - nonce, - expiry: MAX_UINT256, - }), - ), - ); + const { v, r, s } = await buildData(this.token, { + delegatee: delegatorAddress, + nonce, + expiry: MAX_UINT256, + }) + .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) + .then(fromRpcSig); const receipt = await this.token.delegateBySig(holderDelegatee, nonce, MAX_UINT256, v, r, s); const { args } = receipt.logs.find(({ event }) => event == 'DelegateChanged'); @@ -194,16 +183,14 @@ contract('ERC20Votes', function (accounts) { }); it('rejects bad nonce', async function () { - const { v, r, s } = fromRpcSig( - ethSigUtil.signTypedMessage( - delegator.getPrivateKey(), - buildData(this.chainId, this.token.address, { - delegatee: delegatorAddress, - nonce, - expiry: MAX_UINT256, - }), - ), - ); + const { v, r, s } = await buildData(this.token, { + delegatee: delegatorAddress, + nonce, + expiry: MAX_UINT256, + }) + .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) + .then(fromRpcSig); + await expectRevert( this.token.delegateBySig(delegatorAddress, nonce + 1, MAX_UINT256, v, r, s), 'ERC20Votes: invalid nonce', @@ -212,16 +199,14 @@ contract('ERC20Votes', function (accounts) { it('rejects expired permit', async function () { const expiry = (await time.latest()) - time.duration.weeks(1); - const { v, r, s } = fromRpcSig( - ethSigUtil.signTypedMessage( - delegator.getPrivateKey(), - buildData(this.chainId, this.token.address, { - delegatee: delegatorAddress, - nonce, - expiry, - }), - ), - ); + + const { v, r, s } = await buildData(this.token, { + delegatee: delegatorAddress, + nonce, + expiry, + }) + .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) + .then(fromRpcSig); await expectRevert( this.token.delegateBySig(delegatorAddress, nonce, expiry, v, r, s), diff --git a/test/token/ERC20/extensions/ERC20VotesComp.test.js b/test/token/ERC20/extensions/ERC20VotesComp.test.js index 6a7c0001344..02f51a63cfc 100644 --- a/test/token/ERC20/extensions/ERC20VotesComp.test.js +++ b/test/token/ERC20/extensions/ERC20VotesComp.test.js @@ -11,7 +11,7 @@ const Wallet = require('ethereumjs-wallet').default; const ERC20VotesComp = artifacts.require('$ERC20VotesComp'); const { batchInBlock } = require('../../../helpers/txpool'); -const { EIP712Domain, domainSeparator } = require('../../../helpers/eip712'); +const { getDomain, domainType, domainSeparator } = require('../../../helpers/eip712'); const { getChainId } = require('../../../helpers/chainid'); const Delegation = [ @@ -39,7 +39,7 @@ contract('ERC20VotesComp', function (accounts) { it('domain separator', async function () { expect(await this.token.DOMAIN_SEPARATOR()).to.equal( - await domainSeparator({ name, version, chainId: this.chainId, verifyingContract: this.token.address }), + await getDomain(this.token).then(domainSeparator), ); }); @@ -94,30 +94,25 @@ contract('ERC20VotesComp', function (accounts) { const delegatorAddress = web3.utils.toChecksumAddress(delegator.getAddressString()); const nonce = 0; - const buildData = (chainId, verifyingContract, message) => ({ - data: { - primaryType: 'Delegation', - types: { EIP712Domain, Delegation }, - domain: { name, version, chainId, verifyingContract }, - message, - }, - }); + const buildData = (contract, message) => getDomain(contract).then(domain => ({ + primaryType: 'Delegation', + types: { EIP712Domain: domainType(domain), Delegation }, + domain, + message, + })); beforeEach(async function () { await this.token.$_mint(delegatorAddress, supply); }); it('accept signed delegation', async function () { - const { v, r, s } = fromRpcSig( - ethSigUtil.signTypedMessage( - delegator.getPrivateKey(), - buildData(this.chainId, this.token.address, { - delegatee: delegatorAddress, - nonce, - expiry: MAX_UINT256, - }), - ), - ); + const { v, r, s } = await buildData(this.token, { + delegatee: delegatorAddress, + nonce, + expiry: MAX_UINT256, + }) + .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) + .then(fromRpcSig); expect(await this.token.delegates(delegatorAddress)).to.be.equal(ZERO_ADDRESS); @@ -142,16 +137,13 @@ contract('ERC20VotesComp', function (accounts) { }); it('rejects reused signature', async function () { - const { v, r, s } = fromRpcSig( - ethSigUtil.signTypedMessage( - delegator.getPrivateKey(), - buildData(this.chainId, this.token.address, { - delegatee: delegatorAddress, - nonce, - expiry: MAX_UINT256, - }), - ), - ); + const { v, r, s } = await buildData(this.token, { + delegatee: delegatorAddress, + nonce, + expiry: MAX_UINT256, + }) + .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) + .then(fromRpcSig); await this.token.delegateBySig(delegatorAddress, nonce, MAX_UINT256, v, r, s); @@ -162,16 +154,13 @@ contract('ERC20VotesComp', function (accounts) { }); it('rejects bad delegatee', async function () { - const { v, r, s } = fromRpcSig( - ethSigUtil.signTypedMessage( - delegator.getPrivateKey(), - buildData(this.chainId, this.token.address, { - delegatee: delegatorAddress, - nonce, - expiry: MAX_UINT256, - }), - ), - ); + const { v, r, s } = await buildData(this.token, { + delegatee: delegatorAddress, + nonce, + expiry: MAX_UINT256, + }) + .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) + .then(fromRpcSig); const receipt = await this.token.delegateBySig(holderDelegatee, nonce, MAX_UINT256, v, r, s); const { args } = receipt.logs.find(({ event }) => event == 'DelegateChanged'); @@ -181,16 +170,14 @@ contract('ERC20VotesComp', function (accounts) { }); it('rejects bad nonce', async function () { - const { v, r, s } = fromRpcSig( - ethSigUtil.signTypedMessage( - delegator.getPrivateKey(), - buildData(this.chainId, this.token.address, { - delegatee: delegatorAddress, - nonce, - expiry: MAX_UINT256, - }), - ), - ); + const { v, r, s } = await buildData(this.token, { + delegatee: delegatorAddress, + nonce, + expiry: MAX_UINT256, + }) + .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) + .then(fromRpcSig); + await expectRevert( this.token.delegateBySig(delegatorAddress, nonce + 1, MAX_UINT256, v, r, s), 'ERC20Votes: invalid nonce', @@ -199,16 +186,14 @@ contract('ERC20VotesComp', function (accounts) { it('rejects expired permit', async function () { const expiry = (await time.latest()) - time.duration.weeks(1); - const { v, r, s } = fromRpcSig( - ethSigUtil.signTypedMessage( - delegator.getPrivateKey(), - buildData(this.chainId, this.token.address, { - delegatee: delegatorAddress, - nonce, - expiry, - }), - ), - ); + + const { v, r, s } = await buildData(this.token, { + delegatee: delegatorAddress, + nonce, + expiry, + }) + .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) + .then(fromRpcSig); await expectRevert( this.token.delegateBySig(delegatorAddress, nonce, expiry, v, r, s), diff --git a/test/token/ERC20/extensions/draft-ERC20Permit.test.js b/test/token/ERC20/extensions/draft-ERC20Permit.test.js index eb673782699..18f0282317b 100644 --- a/test/token/ERC20/extensions/draft-ERC20Permit.test.js +++ b/test/token/ERC20/extensions/draft-ERC20Permit.test.js @@ -10,7 +10,7 @@ const Wallet = require('ethereumjs-wallet').default; const ERC20Permit = artifacts.require('$ERC20Permit'); -const { EIP712Domain, Permit, domainSeparator } = require('../../../helpers/eip712'); +const { Permit, getDomain, domainType, domainSeparator } = require('../../../helpers/eip712'); const { getChainId } = require('../../../helpers/chainid'); contract('ERC20Permit', function (accounts) { @@ -35,7 +35,7 @@ contract('ERC20Permit', function (accounts) { it('domain separator', async function () { expect(await this.token.DOMAIN_SEPARATOR()).to.equal( - await domainSeparator({ name, version, chainId: this.chainId, verifyingContract: this.token.address }), + await getDomain(this.token).then(domainSeparator), ); }); @@ -47,17 +47,17 @@ contract('ERC20Permit', function (accounts) { const nonce = 0; const maxDeadline = MAX_UINT256; - const buildData = (chainId, verifyingContract, deadline = maxDeadline) => ({ + const buildData = (contract, deadline = maxDeadline) => getDomain(contract).then(domain => ({ primaryType: 'Permit', - types: { EIP712Domain, Permit }, - domain: { name, version, chainId, verifyingContract }, + types: { EIP712Domain: domainType(domain), Permit }, + domain, message: { owner, spender, value, nonce, deadline }, - }); + })); it('accepts owner signature', async function () { - const data = buildData(this.chainId, this.token.address); - const signature = ethSigUtil.signTypedMessage(wallet.getPrivateKey(), { data }); - const { v, r, s } = fromRpcSig(signature); + const { v, r, s } = await buildData(this.token) + .then(data => ethSigUtil.signTypedMessage(wallet.getPrivateKey(), { data })) + .then(fromRpcSig); await this.token.permit(owner, spender, value, maxDeadline, v, r, s); @@ -66,9 +66,9 @@ contract('ERC20Permit', function (accounts) { }); it('rejects reused signature', async function () { - const data = buildData(this.chainId, this.token.address); - const signature = ethSigUtil.signTypedMessage(wallet.getPrivateKey(), { data }); - const { v, r, s } = fromRpcSig(signature); + const { v, r, s } = await buildData(this.token) + .then(data => ethSigUtil.signTypedMessage(wallet.getPrivateKey(), { data })) + .then(fromRpcSig); await this.token.permit(owner, spender, value, maxDeadline, v, r, s); @@ -80,9 +80,10 @@ contract('ERC20Permit', function (accounts) { it('rejects other signature', async function () { const otherWallet = Wallet.generate(); - const data = buildData(this.chainId, this.token.address); - const signature = ethSigUtil.signTypedMessage(otherWallet.getPrivateKey(), { data }); - const { v, r, s } = fromRpcSig(signature); + + const { v, r, s } = await buildData(this.token) + .then(data => ethSigUtil.signTypedMessage(otherWallet.getPrivateKey(), { data })) + .then(fromRpcSig); await expectRevert( this.token.permit(owner, spender, value, maxDeadline, v, r, s), @@ -93,9 +94,9 @@ contract('ERC20Permit', function (accounts) { it('rejects expired permit', async function () { const deadline = (await time.latest()) - time.duration.weeks(1); - const data = buildData(this.chainId, this.token.address, deadline); - const signature = ethSigUtil.signTypedMessage(wallet.getPrivateKey(), { data }); - const { v, r, s } = fromRpcSig(signature); + const { v, r, s } = await buildData(this.token, deadline) + .then(data => ethSigUtil.signTypedMessage(wallet.getPrivateKey(), { data })) + .then(fromRpcSig); await expectRevert(this.token.permit(owner, spender, value, deadline, v, r, s), 'ERC20Permit: expired deadline'); }); diff --git a/test/token/ERC20/utils/SafeERC20.test.js b/test/token/ERC20/utils/SafeERC20.test.js index 878989cb171..62edc604708 100644 --- a/test/token/ERC20/utils/SafeERC20.test.js +++ b/test/token/ERC20/utils/SafeERC20.test.js @@ -6,7 +6,7 @@ const ERC20ReturnTrueMock = artifacts.require('ERC20ReturnTrueMock'); const ERC20NoReturnMock = artifacts.require('ERC20NoReturnMock'); const ERC20PermitNoRevertMock = artifacts.require('ERC20PermitNoRevertMock'); -const { EIP712Domain, Permit } = require('../../../helpers/eip712'); +const { getDomain, domainType, Permit } = require('../../../helpers/eip712'); const { getChainId } = require('../../../helpers/chainid'); const { fromRpcSig } = require('ethereumjs-util'); @@ -58,16 +58,15 @@ contract('SafeERC20', function (accounts) { const spender = hasNoCode; beforeEach(async function () { - const chainId = await getChainId(); - this.token = await ERC20PermitNoRevertMock.new(); - this.data = { + this.data = await getDomain(this.token).then(domain => ({ primaryType: 'Permit', - types: { EIP712Domain, Permit }, - domain: { name: 'ERC20PermitNoRevertMock', version: '1', chainId, verifyingContract: this.token.address }, + types: { EIP712Domain: domainType(domain), Permit }, + domain, message: { owner, spender, value: '42', nonce: '0', deadline: constants.MAX_UINT256 }, - }; + })); + this.signature = fromRpcSig(ethSigUtil.signTypedMessage(wallet.getPrivateKey(), { data: this.data })); }); diff --git a/test/utils/cryptography/EIP712.test.js b/test/utils/cryptography/EIP712.test.js index 8c20b91badc..1e152cbee87 100644 --- a/test/utils/cryptography/EIP712.test.js +++ b/test/utils/cryptography/EIP712.test.js @@ -1,8 +1,9 @@ const ethSigUtil = require('eth-sig-util'); const Wallet = require('ethereumjs-wallet').default; -const { EIP712Domain, domainSeparator, hashTypedData } = require('../../helpers/eip712'); +const { getDomain, domainType, domainSeparator, hashTypedData } = require('../../helpers/eip712'); const { getChainId } = require('../../helpers/chainid'); +const { mapValues } = require('../../helpers/map-values'); const EIP712Verifier = artifacts.require('$EIP712Verifier'); @@ -21,12 +22,20 @@ contract('EIP712', function (accounts) { chainId: await getChainId(), verifyingContract: this.eip712.address, }; + this.domainType = domainType(this.domain); }); - it('domain separator', async function () { - const expected = await domainSeparator(this.domain); + describe('domain separator', function () { + it('is internally available', async function () { + const expected = await domainSeparator(this.domain); - expect(await this.eip712.$_domainSeparatorV4()).to.equal(expected); + expect(await this.eip712.$_domainSeparatorV4()).to.equal(expected); + }); + + it("can be rebuilt using EIP-5267's eip712Domain", async function () { + const rebuildDomain = await getDomain(this.eip712); + expect(mapValues(rebuildDomain, String)).to.be.deep.equal(mapValues(this.domain, String)); + }); }); it('hash digest', async function () { @@ -44,7 +53,7 @@ contract('EIP712', function (accounts) { const data = { types: { - EIP712Domain, + EIP712Domain: this.domainType, Mail: [ { name: 'to', type: 'address' }, { name: 'contents', type: 'string' }, From 5d794f117536265540d2402ddbf15d6c0667d31c Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 18 Jan 2023 19:29:28 +0100 Subject: [PATCH 04/18] fix lint --- contracts/interfaces/ERC5267.sol | 21 +++++---- contracts/utils/cryptography/EIP712.sol | 43 +++++++++++-------- test/governance/Governor.test.js | 33 +++++++------- .../extensions/GovernorWithParams.test.js | 37 ++++++++-------- test/governance/utils/Votes.behavior.js | 39 ++++++++--------- test/helpers/eip712.js | 8 +--- .../token/ERC20/extensions/ERC20Votes.test.js | 37 ++++++++-------- .../ERC20/extensions/ERC20VotesComp.test.js | 37 ++++++++-------- .../extensions/draft-ERC20Permit.test.js | 21 +++++---- test/token/ERC20/utils/SafeERC20.test.js | 1 - 10 files changed, 139 insertions(+), 138 deletions(-) diff --git a/contracts/interfaces/ERC5267.sol b/contracts/interfaces/ERC5267.sol index e45b867ef46..79dec66e223 100644 --- a/contracts/interfaces/ERC5267.sol +++ b/contracts/interfaces/ERC5267.sol @@ -12,13 +12,16 @@ interface ERC5267 { * @dev returns the fields and values that describe the domain separator used by this contract for EIP-712 * signature. */ - function eip712Domain() external view returns ( - bytes1 fields, - string memory name, - string memory version, - uint256 chainId, - address verifyingContract, - bytes32 salt, - uint256[] memory extensions - ); + function eip712Domain() + external + view + returns ( + bytes1 fields, + string memory name, + string memory version, + uint256 chainId, + address verifyingContract, + bytes32 salt, + uint256[] memory extensions + ); } diff --git a/contracts/utils/cryptography/EIP712.sol b/contracts/utils/cryptography/EIP712.sol index f3a8db68ba5..52cdaf5c9e1 100644 --- a/contracts/utils/cryptography/EIP712.sol +++ b/contracts/utils/cryptography/EIP712.sol @@ -38,7 +38,8 @@ abstract contract EIP712 is ERC5267 { bytes32 private immutable _HASHED_NAME; bytes32 private immutable _HASHED_VERSION; - bytes32 private immutable _TYPE_HASH = keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"); + bytes32 private immutable _TYPE_HASH = + keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"); /* solhint-enable var-name-mixedcase */ @@ -107,23 +108,29 @@ abstract contract EIP712 is ERC5267 { /** * @dev See {EIP-5267}. */ - function eip712Domain() public view virtual override returns ( - bytes1 fields, - string memory name, - string memory version, - uint256 chainId, - address verifyingContract, - bytes32 salt, - uint256[] memory extensions - ) { + function eip712Domain() + public + view + virtual + override + returns ( + bytes1 fields, + string memory name, + string memory version, + uint256 chainId, + address verifyingContract, + bytes32 salt, + uint256[] memory extensions + ) + { return ( - hex"0f", // 01111 - _NAME, - _VERSION, - block.chainid, - address(this), - bytes32(0), - new uint256[](0) - ); + hex"0f", // 01111 + _NAME, + _VERSION, + block.chainid, + address(this), + bytes32(0), + new uint256[](0) + ); } } diff --git a/test/governance/Governor.test.js b/test/governance/Governor.test.js index 2256997050b..1f105a5a6d7 100644 --- a/test/governance/Governor.test.js +++ b/test/governance/Governor.test.js @@ -20,7 +20,7 @@ contract('Governor', function (accounts) { const empty = web3.utils.toChecksumAddress(web3.utils.randomHex(20)); const name = 'OZ-Governor'; - const version = '1'; + // const version = '1'; const tokenName = 'MockToken'; const tokenSymbol = 'MTKN'; const tokenSupply = web3.utils.toWei('100'); @@ -148,21 +148,22 @@ contract('Governor', function (accounts) { const voterBySig = Wallet.generate(); const voterBySigAddress = web3.utils.toChecksumAddress(voterBySig.getAddressString()); - const signature = (contract, message) => getDomain(contract) - .then(domain => ({ - primaryType: 'Ballot', - types: { - EIP712Domain: domainType(domain), - Ballot: [ - { name: 'proposalId', type: 'uint256' }, - { name: 'support', type: 'uint8' }, - ], - }, - domain, - message, - })) - .then(data => ethSigUtil.signTypedMessage(voterBySig.getPrivateKey(), { data })) - .then(fromRpcSig); + const signature = (contract, message) => + getDomain(contract) + .then(domain => ({ + primaryType: 'Ballot', + types: { + EIP712Domain: domainType(domain), + Ballot: [ + { name: 'proposalId', type: 'uint256' }, + { name: 'support', type: 'uint8' }, + ], + }, + domain, + message, + })) + .then(data => ethSigUtil.signTypedMessage(voterBySig.getPrivateKey(), { data })) + .then(fromRpcSig); await this.token.delegate(voterBySigAddress, { from: voter1 }); diff --git a/test/governance/extensions/GovernorWithParams.test.js b/test/governance/extensions/GovernorWithParams.test.js index ae2f42636fd..82b3f38a7e3 100644 --- a/test/governance/extensions/GovernorWithParams.test.js +++ b/test/governance/extensions/GovernorWithParams.test.js @@ -22,7 +22,7 @@ contract('GovernorWithParams', function (accounts) { const [owner, proposer, voter1, voter2, voter3, voter4] = accounts; const name = 'OZ-Governor'; - const version = '1'; + // const version = '1'; const tokenName = 'MockToken'; const tokenSymbol = 'MTKN'; const tokenSupply = web3.utils.toWei('100'); @@ -116,23 +116,24 @@ contract('GovernorWithParams', function (accounts) { 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); + 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); await this.token.delegate(voterBySigAddress, { from: voter2 }); diff --git a/test/governance/utils/Votes.behavior.js b/test/governance/utils/Votes.behavior.js index d83b1691ab6..dee9a51d707 100644 --- a/test/governance/utils/Votes.behavior.js +++ b/test/governance/utils/Votes.behavior.js @@ -14,8 +14,6 @@ const Delegation = [ { name: 'expiry', type: 'uint256' }, ]; -const version = '1'; - function shouldBehaveLikeVotes() { describe('run votes workflow', function () { it('initial nonce is 0', async function () { @@ -23,9 +21,7 @@ function shouldBehaveLikeVotes() { }); it('domain separator', async function () { - expect(await this.votes.DOMAIN_SEPARATOR()).to.equal( - await getDomain(this.votes).then(domainSeparator), - ); + expect(await this.votes.DOMAIN_SEPARATOR()).to.equal(await getDomain(this.votes).then(domainSeparator)); }); describe('delegation with signature', function () { @@ -33,12 +29,13 @@ function shouldBehaveLikeVotes() { const delegatorAddress = web3.utils.toChecksumAddress(delegator.getAddressString()); const nonce = 0; - const buildData = (contract, message) => getDomain(contract).then(domain => ({ - primaryType: 'Delegation', - types: { EIP712Domain: domainType(domain), Delegation }, - domain, - message, - })); + const buildData = (contract, message) => + getDomain(contract).then(domain => ({ + primaryType: 'Delegation', + types: { EIP712Domain: domainType(domain), Delegation }, + domain, + message, + })); beforeEach(async function () { await this.votes.$_mint(delegatorAddress, this.NFT0); @@ -50,8 +47,8 @@ function shouldBehaveLikeVotes() { nonce, expiry: MAX_UINT256, }) - .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) - .then(fromRpcSig); + .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) + .then(fromRpcSig); expect(await this.votes.delegates(delegatorAddress)).to.be.equal(ZERO_ADDRESS); @@ -81,8 +78,8 @@ function shouldBehaveLikeVotes() { nonce, expiry: MAX_UINT256, }) - .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) - .then(fromRpcSig); + .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) + .then(fromRpcSig); await this.votes.delegateBySig(delegatorAddress, nonce, MAX_UINT256, v, r, s); @@ -98,8 +95,8 @@ function shouldBehaveLikeVotes() { nonce, expiry: MAX_UINT256, }) - .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) - .then(fromRpcSig); + .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) + .then(fromRpcSig); const receipt = await this.votes.delegateBySig(this.account1Delegatee, nonce, MAX_UINT256, v, r, s); const { args } = receipt.logs.find(({ event }) => event === 'DelegateChanged'); @@ -114,8 +111,8 @@ function shouldBehaveLikeVotes() { nonce, expiry: MAX_UINT256, }) - .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) - .then(fromRpcSig); + .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) + .then(fromRpcSig); await expectRevert( this.votes.delegateBySig(delegatorAddress, nonce + 1, MAX_UINT256, v, r, s), @@ -131,8 +128,8 @@ function shouldBehaveLikeVotes() { nonce, expiry, }) - .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) - .then(fromRpcSig); + .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) + .then(fromRpcSig); await expectRevert( this.votes.delegateBySig(delegatorAddress, nonce, expiry, v, r, s), diff --git a/test/helpers/eip712.js b/test/helpers/eip712.js index fc2ebbd12c2..69e7366db51 100644 --- a/test/helpers/eip712.js +++ b/test/helpers/eip712.js @@ -29,7 +29,7 @@ async function getDomain(contract) { const { fields, name, version, chainId, verifyingContract, salt, extensions } = await contract.eip712Domain(); if (extensions.length > 0) { - throw Error("Extensions not implemented"); + throw Error('Extensions not implemented'); } const domain = { name, version, chainId, verifyingContract, salt }; @@ -48,11 +48,7 @@ function domainType(domain) { async function domainSeparator(domain) { return bufferToHexString( - ethSigUtil.TypedDataUtils.hashStruct( - 'EIP712Domain', - domain, - { EIP712Domain: domainType(domain) }, - ), + ethSigUtil.TypedDataUtils.hashStruct('EIP712Domain', domain, { EIP712Domain: domainType(domain) }), ); } diff --git a/test/token/ERC20/extensions/ERC20Votes.test.js b/test/token/ERC20/extensions/ERC20Votes.test.js index 625262a1a0f..9759c11629b 100644 --- a/test/token/ERC20/extensions/ERC20Votes.test.js +++ b/test/token/ERC20/extensions/ERC20Votes.test.js @@ -38,9 +38,7 @@ contract('ERC20Votes', function (accounts) { }); it('domain separator', async function () { - expect(await this.token.DOMAIN_SEPARATOR()).to.equal( - await getDomain(this.token).then(domainSeparator), - ); + expect(await this.token.DOMAIN_SEPARATOR()).to.equal(await getDomain(this.token).then(domainSeparator)); }); it('minting restriction', async function () { @@ -107,12 +105,13 @@ contract('ERC20Votes', function (accounts) { const delegatorAddress = web3.utils.toChecksumAddress(delegator.getAddressString()); const nonce = 0; - const buildData = (contract, message) => getDomain(contract).then(domain => ({ - primaryType: 'Delegation', - types: { EIP712Domain: domainType(domain), Delegation }, - domain, - message, - })); + const buildData = (contract, message) => + getDomain(contract).then(domain => ({ + primaryType: 'Delegation', + types: { EIP712Domain: domainType(domain), Delegation }, + domain, + message, + })); beforeEach(async function () { await this.token.$_mint(delegatorAddress, supply); @@ -124,8 +123,8 @@ contract('ERC20Votes', function (accounts) { nonce, expiry: MAX_UINT256, }) - .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) - .then(fromRpcSig); + .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) + .then(fromRpcSig); expect(await this.token.delegates(delegatorAddress)).to.be.equal(ZERO_ADDRESS); @@ -155,8 +154,8 @@ contract('ERC20Votes', function (accounts) { nonce, expiry: MAX_UINT256, }) - .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) - .then(fromRpcSig); + .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) + .then(fromRpcSig); await this.token.delegateBySig(delegatorAddress, nonce, MAX_UINT256, v, r, s); @@ -172,8 +171,8 @@ contract('ERC20Votes', function (accounts) { nonce, expiry: MAX_UINT256, }) - .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) - .then(fromRpcSig); + .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) + .then(fromRpcSig); const receipt = await this.token.delegateBySig(holderDelegatee, nonce, MAX_UINT256, v, r, s); const { args } = receipt.logs.find(({ event }) => event == 'DelegateChanged'); @@ -188,8 +187,8 @@ contract('ERC20Votes', function (accounts) { nonce, expiry: MAX_UINT256, }) - .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) - .then(fromRpcSig); + .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) + .then(fromRpcSig); await expectRevert( this.token.delegateBySig(delegatorAddress, nonce + 1, MAX_UINT256, v, r, s), @@ -205,8 +204,8 @@ contract('ERC20Votes', function (accounts) { nonce, expiry, }) - .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) - .then(fromRpcSig); + .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) + .then(fromRpcSig); await expectRevert( this.token.delegateBySig(delegatorAddress, nonce, expiry, v, r, s), diff --git a/test/token/ERC20/extensions/ERC20VotesComp.test.js b/test/token/ERC20/extensions/ERC20VotesComp.test.js index 02f51a63cfc..660ed2124ce 100644 --- a/test/token/ERC20/extensions/ERC20VotesComp.test.js +++ b/test/token/ERC20/extensions/ERC20VotesComp.test.js @@ -38,9 +38,7 @@ contract('ERC20VotesComp', function (accounts) { }); it('domain separator', async function () { - expect(await this.token.DOMAIN_SEPARATOR()).to.equal( - await getDomain(this.token).then(domainSeparator), - ); + expect(await this.token.DOMAIN_SEPARATOR()).to.equal(await getDomain(this.token).then(domainSeparator)); }); it('minting restriction', async function () { @@ -94,12 +92,13 @@ contract('ERC20VotesComp', function (accounts) { const delegatorAddress = web3.utils.toChecksumAddress(delegator.getAddressString()); const nonce = 0; - const buildData = (contract, message) => getDomain(contract).then(domain => ({ - primaryType: 'Delegation', - types: { EIP712Domain: domainType(domain), Delegation }, - domain, - message, - })); + const buildData = (contract, message) => + getDomain(contract).then(domain => ({ + primaryType: 'Delegation', + types: { EIP712Domain: domainType(domain), Delegation }, + domain, + message, + })); beforeEach(async function () { await this.token.$_mint(delegatorAddress, supply); @@ -111,8 +110,8 @@ contract('ERC20VotesComp', function (accounts) { nonce, expiry: MAX_UINT256, }) - .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) - .then(fromRpcSig); + .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) + .then(fromRpcSig); expect(await this.token.delegates(delegatorAddress)).to.be.equal(ZERO_ADDRESS); @@ -142,8 +141,8 @@ contract('ERC20VotesComp', function (accounts) { nonce, expiry: MAX_UINT256, }) - .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) - .then(fromRpcSig); + .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) + .then(fromRpcSig); await this.token.delegateBySig(delegatorAddress, nonce, MAX_UINT256, v, r, s); @@ -159,8 +158,8 @@ contract('ERC20VotesComp', function (accounts) { nonce, expiry: MAX_UINT256, }) - .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) - .then(fromRpcSig); + .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) + .then(fromRpcSig); const receipt = await this.token.delegateBySig(holderDelegatee, nonce, MAX_UINT256, v, r, s); const { args } = receipt.logs.find(({ event }) => event == 'DelegateChanged'); @@ -175,8 +174,8 @@ contract('ERC20VotesComp', function (accounts) { nonce, expiry: MAX_UINT256, }) - .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) - .then(fromRpcSig); + .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) + .then(fromRpcSig); await expectRevert( this.token.delegateBySig(delegatorAddress, nonce + 1, MAX_UINT256, v, r, s), @@ -192,8 +191,8 @@ contract('ERC20VotesComp', function (accounts) { nonce, expiry, }) - .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) - .then(fromRpcSig); + .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) + .then(fromRpcSig); await expectRevert( this.token.delegateBySig(delegatorAddress, nonce, expiry, v, r, s), diff --git a/test/token/ERC20/extensions/draft-ERC20Permit.test.js b/test/token/ERC20/extensions/draft-ERC20Permit.test.js index 18f0282317b..33c43c479fd 100644 --- a/test/token/ERC20/extensions/draft-ERC20Permit.test.js +++ b/test/token/ERC20/extensions/draft-ERC20Permit.test.js @@ -34,9 +34,7 @@ contract('ERC20Permit', function (accounts) { }); it('domain separator', async function () { - expect(await this.token.DOMAIN_SEPARATOR()).to.equal( - await getDomain(this.token).then(domainSeparator), - ); + expect(await this.token.DOMAIN_SEPARATOR()).to.equal(await getDomain(this.token).then(domainSeparator)); }); describe('permit', function () { @@ -47,12 +45,13 @@ contract('ERC20Permit', function (accounts) { const nonce = 0; const maxDeadline = MAX_UINT256; - const buildData = (contract, deadline = maxDeadline) => getDomain(contract).then(domain => ({ - primaryType: 'Permit', - types: { EIP712Domain: domainType(domain), Permit }, - domain, - message: { owner, spender, value, nonce, deadline }, - })); + const buildData = (contract, deadline = maxDeadline) => + getDomain(contract).then(domain => ({ + primaryType: 'Permit', + types: { EIP712Domain: domainType(domain), Permit }, + domain, + message: { owner, spender, value, nonce, deadline }, + })); it('accepts owner signature', async function () { const { v, r, s } = await buildData(this.token) @@ -82,8 +81,8 @@ contract('ERC20Permit', function (accounts) { const otherWallet = Wallet.generate(); const { v, r, s } = await buildData(this.token) - .then(data => ethSigUtil.signTypedMessage(otherWallet.getPrivateKey(), { data })) - .then(fromRpcSig); + .then(data => ethSigUtil.signTypedMessage(otherWallet.getPrivateKey(), { data })) + .then(fromRpcSig); await expectRevert( this.token.permit(owner, spender, value, maxDeadline, v, r, s), diff --git a/test/token/ERC20/utils/SafeERC20.test.js b/test/token/ERC20/utils/SafeERC20.test.js index 62edc604708..a6b10fae3a4 100644 --- a/test/token/ERC20/utils/SafeERC20.test.js +++ b/test/token/ERC20/utils/SafeERC20.test.js @@ -7,7 +7,6 @@ const ERC20NoReturnMock = artifacts.require('ERC20NoReturnMock'); const ERC20PermitNoRevertMock = artifacts.require('ERC20PermitNoRevertMock'); const { getDomain, domainType, Permit } = require('../../../helpers/eip712'); -const { getChainId } = require('../../../helpers/chainid'); const { fromRpcSig } = require('ethereumjs-util'); const ethSigUtil = require('eth-sig-util'); From 658c172ff826830d48552601cf59772b46d96e69 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 18 Jan 2023 21:34:19 +0100 Subject: [PATCH 05/18] Update EIP712.sol --- contracts/utils/cryptography/EIP712.sol | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/contracts/utils/cryptography/EIP712.sol b/contracts/utils/cryptography/EIP712.sol index 52cdaf5c9e1..8e0d4e1d481 100644 --- a/contracts/utils/cryptography/EIP712.sol +++ b/contracts/utils/cryptography/EIP712.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: MIT // OpenZeppelin Contracts (last updated v4.8.0) (utils/cryptography/EIP712.sol) -pragma solidity ^0.8.0; +pragma solidity ^0.8.8; import "./ECDSA.sol"; import "../../interfaces/ERC5267.sol"; @@ -56,14 +56,12 @@ abstract contract EIP712 is ERC5267 { * contract upgrade]. */ constructor(string memory name, string memory version) { - bytes32 hashedName = keccak256(bytes(name)); - bytes32 hashedVersion = keccak256(bytes(version)); _NAME = name; _VERSION = version; - _HASHED_NAME = hashedName; - _HASHED_VERSION = hashedVersion; + _HASHED_NAME = keccak256(bytes(name)); + _HASHED_VERSION = keccak256(bytes(version)); _CACHED_CHAIN_ID = block.chainid; - _CACHED_DOMAIN_SEPARATOR = _buildDomainSeparator(_TYPE_HASH, hashedName, hashedVersion); + _CACHED_DOMAIN_SEPARATOR = _buildDomainSeparator(); _CACHED_THIS = address(this); } @@ -74,16 +72,12 @@ abstract contract EIP712 is ERC5267 { if (address(this) == _CACHED_THIS && block.chainid == _CACHED_CHAIN_ID) { return _CACHED_DOMAIN_SEPARATOR; } else { - return _buildDomainSeparator(_TYPE_HASH, _HASHED_NAME, _HASHED_VERSION); + return _buildDomainSeparator(); } } - function _buildDomainSeparator( - bytes32 typeHash, - bytes32 nameHash, - bytes32 versionHash - ) private view returns (bytes32) { - return keccak256(abi.encode(typeHash, nameHash, versionHash, block.chainid, address(this))); + function _buildDomainSeparator() private view returns (bytes32) { + return keccak256(abi.encode(_TYPE_HASH, _HASHED_NAME, _HASHED_VERSION, block.chainid, address(this))); } /** From f9e31dd591df9264aa631b4b92d54a7fd426b9af Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 19 Jan 2023 15:02:45 +0100 Subject: [PATCH 06/18] add changeset entry --- .changeset/short-roses-judge.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/short-roses-judge.md diff --git a/.changeset/short-roses-judge.md b/.changeset/short-roses-judge.md new file mode 100644 index 00000000000..fa2a13f96bb --- /dev/null +++ b/.changeset/short-roses-judge.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`EIP721`: add EIP-5267 support for better domain discovery. ([#3969](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3969)) From 709d52580df33ed447438cca8f07e829c72a1005 Mon Sep 17 00:00:00 2001 From: Francisco Date: Thu, 19 Jan 2023 17:21:41 -0300 Subject: [PATCH 07/18] Update short-roses-judge.md --- .changeset/short-roses-judge.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/short-roses-judge.md b/.changeset/short-roses-judge.md index fa2a13f96bb..d1779bd4e4c 100644 --- a/.changeset/short-roses-judge.md +++ b/.changeset/short-roses-judge.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': minor --- -`EIP721`: add EIP-5267 support for better domain discovery. ([#3969](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3969)) +`EIP721`: add EIP-5267 support for better domain discovery. From db8c0862790ccee16233acc3c5b2d215fe853b58 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 6 Feb 2023 10:09:23 +0100 Subject: [PATCH 08/18] use shortstrings --- contracts/utils/cryptography/EIP712.sol | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/contracts/utils/cryptography/EIP712.sol b/contracts/utils/cryptography/EIP712.sol index 8e0d4e1d481..46c8f39b555 100644 --- a/contracts/utils/cryptography/EIP712.sol +++ b/contracts/utils/cryptography/EIP712.sol @@ -4,6 +4,7 @@ pragma solidity ^0.8.8; import "./ECDSA.sol"; +import "../ShortStrings.sol"; import "../../interfaces/ERC5267.sol"; /** @@ -26,6 +27,8 @@ import "../../interfaces/ERC5267.sol"; * _Available since v3.4._ */ abstract contract EIP712 is ERC5267 { + using ShortStrings for *; + /* solhint-disable var-name-mixedcase */ // Cache the domain separator as an immutable value, but also store the chain id that it corresponds to, in order to // invalidate the cached domain separator if the chain id changes. @@ -33,14 +36,15 @@ abstract contract EIP712 is ERC5267 { uint256 private immutable _CACHED_CHAIN_ID; address private immutable _CACHED_THIS; - string private _NAME; - string private _VERSION; - bytes32 private immutable _HASHED_NAME; bytes32 private immutable _HASHED_VERSION; bytes32 private immutable _TYPE_HASH = keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"); + ShortString private immutable _NAME; + ShortString private immutable _VERSION; + string private _NAME_FALLBACK; + string private _VERSION_FALLBACK; /* solhint-enable var-name-mixedcase */ /** @@ -56,8 +60,9 @@ abstract contract EIP712 is ERC5267 { * contract upgrade]. */ constructor(string memory name, string memory version) { - _NAME = name; - _VERSION = version; + _NAME = name.toShortStringWithFallback(_NAME_FALLBACK); + _VERSION = version.toShortStringWithFallback(_VERSION_FALLBACK); + _HASHED_NAME = keccak256(bytes(name)); _HASHED_VERSION = keccak256(bytes(version)); _CACHED_CHAIN_ID = block.chainid; @@ -119,8 +124,8 @@ abstract contract EIP712 is ERC5267 { { return ( hex"0f", // 01111 - _NAME, - _VERSION, + _NAME.toStringWithFallback(_NAME_FALLBACK), + _VERSION.toStringWithFallback(_VERSION_FALLBACK), block.chainid, address(this), bytes32(0), From a065f3061afa5f9fe8c394baf32a43288e069511 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 6 Feb 2023 10:12:14 +0100 Subject: [PATCH 09/18] fix lint --- contracts/utils/cryptography/EIP712.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/contracts/utils/cryptography/EIP712.sol b/contracts/utils/cryptography/EIP712.sol index 46c8f39b555..7b510d1734a 100644 --- a/contracts/utils/cryptography/EIP712.sol +++ b/contracts/utils/cryptography/EIP712.sol @@ -45,6 +45,7 @@ abstract contract EIP712 is ERC5267 { ShortString private immutable _VERSION; string private _NAME_FALLBACK; string private _VERSION_FALLBACK; + /* solhint-enable var-name-mixedcase */ /** From b9ec95f052911b0b4b5d7067b3110f07dc74a029 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 7 Feb 2023 14:31:06 +0100 Subject: [PATCH 10/18] Update .changeset/short-roses-judge.md Co-authored-by: Francisco --- .changeset/short-roses-judge.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/short-roses-judge.md b/.changeset/short-roses-judge.md index d1779bd4e4c..002aebb11e3 100644 --- a/.changeset/short-roses-judge.md +++ b/.changeset/short-roses-judge.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': minor --- -`EIP721`: add EIP-5267 support for better domain discovery. +`EIP712`: add EIP-5267 support for better domain discovery. From 0f6f3aa5e7669127158fc697d9d1e23bc653be85 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 7 Feb 2023 15:00:46 +0100 Subject: [PATCH 11/18] rename IERC5267 and improve tests --- .../interfaces/{ERC5267.sol => IERC5267.sol} | 2 +- test/governance/Governor.test.js | 1 - .../extensions/GovernorWithParams.test.js | 1 - test/governance/utils/Votes.behavior.js | 86 +++++++++++-------- test/helpers/eip712.js | 2 +- 5 files changed, 51 insertions(+), 41 deletions(-) rename contracts/interfaces/{ERC5267.sol => IERC5267.sol} (96%) diff --git a/contracts/interfaces/ERC5267.sol b/contracts/interfaces/IERC5267.sol similarity index 96% rename from contracts/interfaces/ERC5267.sol rename to contracts/interfaces/IERC5267.sol index 79dec66e223..3adc4a70355 100644 --- a/contracts/interfaces/ERC5267.sol +++ b/contracts/interfaces/IERC5267.sol @@ -2,7 +2,7 @@ pragma solidity ^0.8.0; -interface ERC5267 { +interface IERC5267 { /** * @dev MAY be emitted to signal that the domain could have changed. */ diff --git a/test/governance/Governor.test.js b/test/governance/Governor.test.js index f0b99f6565e..2018959348c 100644 --- a/test/governance/Governor.test.js +++ b/test/governance/Governor.test.js @@ -20,7 +20,6 @@ contract('Governor', function (accounts) { const empty = web3.utils.toChecksumAddress(web3.utils.randomHex(20)); const name = 'OZ-Governor'; - // const version = '1'; const tokenName = 'MockToken'; const tokenSymbol = 'MTKN'; const tokenSupply = web3.utils.toWei('100'); diff --git a/test/governance/extensions/GovernorWithParams.test.js b/test/governance/extensions/GovernorWithParams.test.js index 82b3f38a7e3..86ebacc087b 100644 --- a/test/governance/extensions/GovernorWithParams.test.js +++ b/test/governance/extensions/GovernorWithParams.test.js @@ -22,7 +22,6 @@ contract('GovernorWithParams', function (accounts) { const [owner, proposer, voter1, voter2, voter3, voter4] = accounts; const name = 'OZ-Governor'; - // const version = '1'; const tokenName = 'MockToken'; const tokenSymbol = 'MTKN'; const tokenSupply = web3.utils.toWei('100'); diff --git a/test/governance/utils/Votes.behavior.js b/test/governance/utils/Votes.behavior.js index dee9a51d707..40388872111 100644 --- a/test/governance/utils/Votes.behavior.js +++ b/test/governance/utils/Votes.behavior.js @@ -29,26 +29,30 @@ function shouldBehaveLikeVotes() { const delegatorAddress = web3.utils.toChecksumAddress(delegator.getAddressString()); const nonce = 0; - const buildData = (contract, message) => - getDomain(contract).then(domain => ({ + const buildAndSignData = async (contract, message, pk) => { + const data = await getDomain(contract).then(domain => ({ primaryType: 'Delegation', types: { EIP712Domain: domainType(domain), Delegation }, domain, message, })); + return fromRpcSig(ethSigUtil.signTypedMessage(pk, { data })); + }; beforeEach(async function () { await this.votes.$_mint(delegatorAddress, this.NFT0); }); it('accept signed delegation', async function () { - const { v, r, s } = await buildData(this.votes, { - delegatee: delegatorAddress, - nonce, - expiry: MAX_UINT256, - }) - .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) - .then(fromRpcSig); + const { v, r, s } = await buildAndSignData( + this.votes, + { + delegatee: delegatorAddress, + nonce, + expiry: MAX_UINT256, + }, + delegator.getPrivateKey(), + ); expect(await this.votes.delegates(delegatorAddress)).to.be.equal(ZERO_ADDRESS); @@ -73,13 +77,15 @@ function shouldBehaveLikeVotes() { }); it('rejects reused signature', async function () { - const { v, r, s } = await buildData(this.votes, { - delegatee: delegatorAddress, - nonce, - expiry: MAX_UINT256, - }) - .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) - .then(fromRpcSig); + const { v, r, s } = await buildAndSignData( + this.votes, + { + delegatee: delegatorAddress, + nonce, + expiry: MAX_UINT256, + }, + delegator.getPrivateKey(), + ); await this.votes.delegateBySig(delegatorAddress, nonce, MAX_UINT256, v, r, s); @@ -90,13 +96,15 @@ function shouldBehaveLikeVotes() { }); it('rejects bad delegatee', async function () { - const { v, r, s } = await buildData(this.votes, { - delegatee: delegatorAddress, - nonce, - expiry: MAX_UINT256, - }) - .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) - .then(fromRpcSig); + const { v, r, s } = await buildAndSignData( + this.votes, + { + delegatee: delegatorAddress, + nonce, + expiry: MAX_UINT256, + }, + delegator.getPrivateKey(), + ); const receipt = await this.votes.delegateBySig(this.account1Delegatee, nonce, MAX_UINT256, v, r, s); const { args } = receipt.logs.find(({ event }) => event === 'DelegateChanged'); @@ -106,13 +114,15 @@ function shouldBehaveLikeVotes() { }); it('rejects bad nonce', async function () { - const { v, r, s } = await buildData(this.votes, { - delegatee: delegatorAddress, - nonce, - expiry: MAX_UINT256, - }) - .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) - .then(fromRpcSig); + const { v, r, s } = await buildAndSignData( + this.votes, + { + delegatee: delegatorAddress, + nonce, + expiry: MAX_UINT256, + }, + delegator.getPrivateKey(), + ); await expectRevert( this.votes.delegateBySig(delegatorAddress, nonce + 1, MAX_UINT256, v, r, s), @@ -123,13 +133,15 @@ function shouldBehaveLikeVotes() { it('rejects expired permit', async function () { const expiry = (await time.latest()) - time.duration.weeks(1); - const { v, r, s } = await buildData(this.votes, { - delegatee: delegatorAddress, - nonce, - expiry, - }) - .then(data => ethSigUtil.signTypedMessage(delegator.getPrivateKey(), { data })) - .then(fromRpcSig); + const { v, r, s } = await buildAndSignData( + this.votes, + { + delegatee: delegatorAddress, + nonce, + expiry, + }, + delegator.getPrivateKey(), + ); await expectRevert( this.votes.delegateBySig(delegatorAddress, nonce, expiry, v, r, s), diff --git a/test/helpers/eip712.js b/test/helpers/eip712.js index 69e7366db51..057452aaf24 100644 --- a/test/helpers/eip712.js +++ b/test/helpers/eip712.js @@ -46,7 +46,7 @@ function domainType(domain) { return EIP712Domain.filter(({ name }) => domain[name] !== undefined); } -async function domainSeparator(domain) { +function domainSeparator(domain) { return bufferToHexString( ethSigUtil.TypedDataUtils.hashStruct('EIP712Domain', domain, { EIP712Domain: domainType(domain) }), ); From aed5659368fdd8f02c23ccd1794872a261e3d792 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 7 Feb 2023 15:01:07 +0100 Subject: [PATCH 12/18] use immutable storage in EIP712 --- contracts/utils/cryptography/EIP712.sol | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/contracts/utils/cryptography/EIP712.sol b/contracts/utils/cryptography/EIP712.sol index 7b510d1734a..721a9f14e2f 100644 --- a/contracts/utils/cryptography/EIP712.sol +++ b/contracts/utils/cryptography/EIP712.sol @@ -5,7 +5,7 @@ pragma solidity ^0.8.8; import "./ECDSA.sol"; import "../ShortStrings.sol"; -import "../../interfaces/ERC5267.sol"; +import "../../interfaces/IERC5267.sol"; /** * @dev https://eips.ethereum.org/EIPS/eip-712[EIP 712] is a standard for hashing and signing of typed structured data. @@ -24,9 +24,15 @@ import "../../interfaces/ERC5267.sol"; * NOTE: This contract implements the version of the encoding known as "v4", as implemented by the JSON RPC method * https://docs.metamask.io/guide/signing-data.html[`eth_signTypedDataV4` in MetaMask]. * + * NOTE: In the upgradeable version of this contract, the cached values will correspond to the address, and the domain + * separator of the implementation contract. This will cause the `_domainSeparatorV4` function to always rebuild the + * separator from the immutable values, which is cheaper than accessing a cached version in cold storage. + * * _Available since v3.4._ + * + * @custom:oz-upgrades-unsafe-allow state-variable-immutable state-variable-assignment */ -abstract contract EIP712 is ERC5267 { +abstract contract EIP712 is IERC5267 { using ShortStrings for *; /* solhint-disable var-name-mixedcase */ @@ -38,7 +44,7 @@ abstract contract EIP712 is ERC5267 { bytes32 private immutable _HASHED_NAME; bytes32 private immutable _HASHED_VERSION; - bytes32 private immutable _TYPE_HASH = + bytes32 private constant _TYPE_HASH = keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"); ShortString private immutable _NAME; From 44387ae62920a48a4b499788094192310ffdd475 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 7 Feb 2023 15:20:14 +0100 Subject: [PATCH 13/18] fix tests --- test/helpers/eip712.js | 6 +++--- test/utils/cryptography/EIP712.test.js | 4 +--- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/test/helpers/eip712.js b/test/helpers/eip712.js index 057452aaf24..b12a6233ec9 100644 --- a/test/helpers/eip712.js +++ b/test/helpers/eip712.js @@ -52,9 +52,9 @@ function domainSeparator(domain) { ); } -async function hashTypedData(domain, structHash) { - return domainSeparator(domain).then(separator => - bufferToHexString(keccak256(Buffer.concat(['0x1901', separator, structHash].map(str => hexStringToBuffer(str))))), +function hashTypedData(domain, structHash) { + return bufferToHexString( + keccak256(Buffer.concat(['0x1901', domainSeparator(domain), structHash].map(str => hexStringToBuffer(str)))), ); } diff --git a/test/utils/cryptography/EIP712.test.js b/test/utils/cryptography/EIP712.test.js index 1e152cbee87..f12f226735b 100644 --- a/test/utils/cryptography/EIP712.test.js +++ b/test/utils/cryptography/EIP712.test.js @@ -40,9 +40,7 @@ contract('EIP712', function (accounts) { it('hash digest', async function () { const structhash = web3.utils.randomHex(32); - const expected = await hashTypedData(this.domain, structhash); - - expect(await this.eip712.$_hashTypedDataV4(structhash)).to.be.equal(expected); + expect(await this.eip712.$_hashTypedDataV4(structhash)).to.be.equal(hashTypedData(this.domain, structhash)); }); it('digest', async function () { From d01fdb2916c245390ae13278952f8360c85e09ca Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 7 Feb 2023 15:22:58 +0100 Subject: [PATCH 14/18] breaking changes entry --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e60161db0ea..526452ea088 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ # Changelog +### Breaking changes + +- `ERC712`: Addition of ERC5267 support requires UDVT support, which was released in solidity version 0.8.8. This requires a pragma change from `^0.8.0` to `^0.8.8`. +- `ERC712`: Optimisation of the cache for the upgradeable version affects the way `name` and `version` are set. This is no longer done through an initializer, and is instead part of the implementation's constructor. As a consequence, all proxies using the same implementation will necessarily share the same `name` and `version`. Additionnaly, an implementation upgrade risks changing the EIP712 domain unless the same `name` and `version` are used when deploying the new implementation contract. + ### Deprecations - `ERC20Permit`: Added the file `IERC20Permit.sol` and `ERC20Permit.sol` and deprecated `draft-IERC20Permit.sol` and `draft-ERC20Permit.sol` since [EIP-2612](https://eips.ethereum.org/EIPS/eip-2612) is no longer a Draft. Developers are encouraged to update their imports. ([#3793](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3793)) From 6837f8df47667f52407bd2d509ecadf48eed72aa Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 7 Feb 2023 15:30:11 +0100 Subject: [PATCH 15/18] spelling --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 526452ea088..27b09aee876 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,7 @@ ### Breaking changes - `ERC712`: Addition of ERC5267 support requires UDVT support, which was released in solidity version 0.8.8. This requires a pragma change from `^0.8.0` to `^0.8.8`. -- `ERC712`: Optimisation of the cache for the upgradeable version affects the way `name` and `version` are set. This is no longer done through an initializer, and is instead part of the implementation's constructor. As a consequence, all proxies using the same implementation will necessarily share the same `name` and `version`. Additionnaly, an implementation upgrade risks changing the EIP712 domain unless the same `name` and `version` are used when deploying the new implementation contract. +- `ERC712`: Optimisation of the cache for the upgradeable version affects the way `name` and `version` are set. This is no longer done through an initializer, and is instead part of the implementation's constructor. As a consequence, all proxies using the same implementation will necessarily share the same `name` and `version`. Additionally, an implementation upgrade risks changing the EIP712 domain unless the same `name` and `version` are used when deploying the new implementation contract. ### Deprecations From 16754dc57e9f0e69dbe775b48ebf202d48ef7c99 Mon Sep 17 00:00:00 2001 From: Francisco Date: Wed, 8 Feb 2023 00:35:04 -0300 Subject: [PATCH 16/18] Update CHANGELOG.md --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 27b09aee876..ccd279d53b1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,8 +2,8 @@ ### Breaking changes -- `ERC712`: Addition of ERC5267 support requires UDVT support, which was released in solidity version 0.8.8. This requires a pragma change from `^0.8.0` to `^0.8.8`. -- `ERC712`: Optimisation of the cache for the upgradeable version affects the way `name` and `version` are set. This is no longer done through an initializer, and is instead part of the implementation's constructor. As a consequence, all proxies using the same implementation will necessarily share the same `name` and `version`. Additionally, an implementation upgrade risks changing the EIP712 domain unless the same `name` and `version` are used when deploying the new implementation contract. +- `EIP712`: Addition of ERC5267 support requires support for user defined value types, which was released in Solidity version 0.8.8. This requires a pragma change from `^0.8.0` to `^0.8.8`. +- `EIP712`: Optimization of the cache for the upgradeable version affects the way `name` and `version` are set. This is no longer done through an initializer, and is instead part of the implementation's constructor. As a consequence, all proxies using the same implementation will necessarily share the same `name` and `version`. Additionally, an implementation upgrade risks changing the EIP712 domain unless the same `name` and `version` are used when deploying the new implementation contract. ### Deprecations From f6e54ab523bee89bce94beb27b1b33422bf0e918 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 8 Feb 2023 00:44:11 -0300 Subject: [PATCH 17/18] rename variables to camel case --- contracts/utils/cryptography/EIP712.sol | 47 +++++++++++++------------ 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/contracts/utils/cryptography/EIP712.sol b/contracts/utils/cryptography/EIP712.sol index 721a9f14e2f..d0e52c39658 100644 --- a/contracts/utils/cryptography/EIP712.sol +++ b/contracts/utils/cryptography/EIP712.sol @@ -35,22 +35,23 @@ import "../../interfaces/IERC5267.sol"; abstract contract EIP712 is IERC5267 { using ShortStrings for *; + bytes32 private constant _TYPE_HASH = + keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"); + /* solhint-disable var-name-mixedcase */ // Cache the domain separator as an immutable value, but also store the chain id that it corresponds to, in order to // invalidate the cached domain separator if the chain id changes. - bytes32 private immutable _CACHED_DOMAIN_SEPARATOR; - uint256 private immutable _CACHED_CHAIN_ID; - address private immutable _CACHED_THIS; + bytes32 private immutable _cachedDomainSeparator; + uint256 private immutable _cachedChainId; + address private immutable _cachedThis; - bytes32 private immutable _HASHED_NAME; - bytes32 private immutable _HASHED_VERSION; - bytes32 private constant _TYPE_HASH = - keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"); + ShortString private immutable _name; + ShortString private immutable _version; + string private _nameFallback; + string private _versionFallback; - ShortString private immutable _NAME; - ShortString private immutable _VERSION; - string private _NAME_FALLBACK; - string private _VERSION_FALLBACK; + bytes32 private immutable _hashedName; + bytes32 private immutable _hashedVersion; /* solhint-enable var-name-mixedcase */ @@ -67,29 +68,29 @@ abstract contract EIP712 is IERC5267 { * contract upgrade]. */ constructor(string memory name, string memory version) { - _NAME = name.toShortStringWithFallback(_NAME_FALLBACK); - _VERSION = version.toShortStringWithFallback(_VERSION_FALLBACK); + _name = name.toShortStringWithFallback(_nameFallback); + _version = version.toShortStringWithFallback(_versionFallback); + _hashedName = keccak256(bytes(name)); + _hashedVersion = keccak256(bytes(version)); - _HASHED_NAME = keccak256(bytes(name)); - _HASHED_VERSION = keccak256(bytes(version)); - _CACHED_CHAIN_ID = block.chainid; - _CACHED_DOMAIN_SEPARATOR = _buildDomainSeparator(); - _CACHED_THIS = address(this); + _cachedChainId = block.chainid; + _cachedDomainSeparator = _buildDomainSeparator(); + _cachedThis = address(this); } /** * @dev Returns the domain separator for the current chain. */ function _domainSeparatorV4() internal view returns (bytes32) { - if (address(this) == _CACHED_THIS && block.chainid == _CACHED_CHAIN_ID) { - return _CACHED_DOMAIN_SEPARATOR; + if (address(this) == _cachedThis && block.chainid == _cachedChainId) { + return _cachedDomainSeparator; } else { return _buildDomainSeparator(); } } function _buildDomainSeparator() private view returns (bytes32) { - return keccak256(abi.encode(_TYPE_HASH, _HASHED_NAME, _HASHED_VERSION, block.chainid, address(this))); + return keccak256(abi.encode(_TYPE_HASH, _hashedName, _hashedVersion, block.chainid, address(this))); } /** @@ -131,8 +132,8 @@ abstract contract EIP712 is IERC5267 { { return ( hex"0f", // 01111 - _NAME.toStringWithFallback(_NAME_FALLBACK), - _VERSION.toStringWithFallback(_VERSION_FALLBACK), + _name.toStringWithFallback(_nameFallback), + _version.toStringWithFallback(_versionFallback), block.chainid, address(this), bytes32(0), From c6b34fb8ecd460f13946e71d0e2eae29ba116b06 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 8 Feb 2023 09:46:38 +0100 Subject: [PATCH 18/18] Update test/governance/utils/Votes.behavior.js Co-authored-by: Francisco --- test/governance/utils/Votes.behavior.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/governance/utils/Votes.behavior.js b/test/governance/utils/Votes.behavior.js index 40388872111..8fe34dc47e5 100644 --- a/test/governance/utils/Votes.behavior.js +++ b/test/governance/utils/Votes.behavior.js @@ -21,7 +21,7 @@ function shouldBehaveLikeVotes() { }); it('domain separator', async function () { - expect(await this.votes.DOMAIN_SEPARATOR()).to.equal(await getDomain(this.votes).then(domainSeparator)); + expect(await this.votes.DOMAIN_SEPARATOR()).to.equal(domainSeparator(await getDomain(this.votes))); }); describe('delegation with signature', function () {