From 5171e46c47bd6be781aa92315944ca37126d4a73 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 10 Mar 2021 16:24:12 +0100 Subject: [PATCH 1/4] Add an internal _useNonce(address) function in ERC20Permit (#2565) --- CHANGELOG.md | 1 + .../token/ERC20/extensions/draft-ERC20Permit.sol | 14 +++++++++++--- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index db5f434a1f5..0ff8a86584f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ * `IERC20Metadata`: add a new extended interface that includes the optional `name()`, `symbol()` and `decimals()` functions. ([#2561](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2561)) * `ERC777`: make reception acquirement optional in `_mint`. ([#2552](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2552)) + * `ERC20Permit`: add a `_useNonce` to enable further usage of ERC712 signatures. ([#2565](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2565)) ## Unreleased diff --git a/contracts/token/ERC20/extensions/draft-ERC20Permit.sol b/contracts/token/ERC20/extensions/draft-ERC20Permit.sol index 1f5cdc2cb71..4dcd9007a9c 100644 --- a/contracts/token/ERC20/extensions/draft-ERC20Permit.sol +++ b/contracts/token/ERC20/extensions/draft-ERC20Permit.sol @@ -47,7 +47,7 @@ abstract contract ERC20Permit is ERC20, IERC20Permit, EIP712 { owner, spender, value, - _nonces[owner].current(), + _useNonce(owner), deadline ) ); @@ -57,14 +57,13 @@ abstract contract ERC20Permit is ERC20, IERC20Permit, EIP712 { address signer = ECDSA.recover(hash, v, r, s); require(signer == owner, "ERC20Permit: invalid signature"); - _nonces[owner].increment(); _approve(owner, spender, value); } /** * @dev See {IERC20Permit-nonces}. */ - function nonces(address owner) public view override returns (uint256) { + function nonces(address owner) public view virtual override returns (uint256) { return _nonces[owner].current(); } @@ -75,4 +74,13 @@ abstract contract ERC20Permit is ERC20, IERC20Permit, EIP712 { function DOMAIN_SEPARATOR() external view override returns (bytes32) { return _domainSeparatorV4(); } + + /** + * @dev "Consume a nonce": return the current value and increment. + */ + function _useNonce(address owner) internal virtual returns (uint256 current) { + Counters.Counter storage nonce = _nonces[owner]; + current = nonce.current(); + nonce.increment(); + } } From 508a879ef0590314188a5a93e15ccee03918ecca Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 10 Mar 2021 14:30:16 -0300 Subject: [PATCH 2/4] Remove merkleTree.js in favor of merkletreejs dependency (#2578) --- package-lock.json | 173 +++++++++++++++++++- package.json | 5 +- test/helpers/merkleTree.js | 135 --------------- test/utils/cryptography/MerkleProof.test.js | 24 +-- 4 files changed, 187 insertions(+), 150 deletions(-) delete mode 100644 test/helpers/merkleTree.js diff --git a/package-lock.json b/package-lock.json index c048e46e5c1..d46edab8d33 100644 --- a/package-lock.json +++ b/package-lock.json @@ -30,8 +30,10 @@ "ethereumjs-wallet": "^1.0.1", "hardhat": "^2.0.6", "hardhat-gas-reporter": "^1.0.4", + "keccak256": "^1.0.2", "lodash.startcase": "^4.4.0", "lodash.zip": "^4.2.0", + "merkletreejs": "^0.2.13", "micromatch": "^4.0.2", "mocha": "^8.0.1", "rimraf": "^3.0.2", @@ -4818,6 +4820,12 @@ "integrity": "sha512-MQcXEUbCKtEo7bhqEs6560Hyd4XaovZlO/k9V3hjVUF/zwW7KBVdSK4gIt/bzwS9MbR5qob+F5jusZsb0YQK2A==", "dev": true }, + "node_modules/buffer-reverse": { + "version": "1.0.1", + "resolved": "https://registry.npmjs.org/buffer-reverse/-/buffer-reverse-1.0.1.tgz", + "integrity": "sha1-SSg8jvpvkBvAH6MwTQYCeXGuL2A=", + "dev": true + }, "node_modules/buffer-to-arraybuffer": { "version": "0.0.5", "resolved": "https://registry.npmjs.org/buffer-to-arraybuffer/-/buffer-to-arraybuffer-0.0.5.tgz", @@ -8869,7 +8877,104 @@ "bundleDependencies": [ "source-map-support", "yargs", - "ethereumjs-util" + "ethereumjs-util", + "@types/bn.js", + "@types/node", + "@types/pbkdf2", + "@types/secp256k1", + "ansi-regex", + "ansi-styles", + "base-x", + "blakejs", + "bn.js", + "brorand", + "browserify-aes", + "bs58", + "bs58check", + "buffer-from", + "buffer-xor", + "camelcase", + "cipher-base", + "cliui", + "color-convert", + "color-name", + "create-hash", + "create-hmac", + "cross-spawn", + "decamelize", + "elliptic", + "emoji-regex", + "end-of-stream", + "ethereum-cryptography", + "ethjs-util", + "evp_bytestokey", + "execa", + "find-up", + "get-caller-file", + "get-stream", + "hash-base", + "hash.js", + "hmac-drbg", + "inherits", + "invert-kv", + "is-fullwidth-code-point", + "is-hex-prefixed", + "is-stream", + "isexe", + "keccak", + "lcid", + "locate-path", + "map-age-cleaner", + "md5.js", + "mem", + "mimic-fn", + "minimalistic-assert", + "minimalistic-crypto-utils", + "nice-try", + "node-addon-api", + "node-gyp-build", + "npm-run-path", + "once", + "os-locale", + "p-defer", + "p-finally", + "p-is-promise", + "p-limit", + "p-locate", + "p-try", + "path-exists", + "path-key", + "pbkdf2", + "pump", + "randombytes", + "readable-stream", + "require-directory", + "require-main-filename", + "ripemd160", + "rlp", + "safe-buffer", + "scrypt-js", + "secp256k1", + "semver", + "set-blocking", + "setimmediate", + "sha.js", + "shebang-command", + "shebang-regex", + "signal-exit", + "source-map", + "string_decoder", + "string-width", + "strip-ansi", + "strip-eof", + "strip-hex-prefix", + "util-deprecate", + "which", + "which-module", + "wrap-ansi", + "wrappy", + "y18n", + "yargs-parser" ], "dev": true, "dependencies": { @@ -12219,6 +12324,16 @@ "node": ">=10.0.0" } }, + "node_modules/keccak256": { + "version": "1.0.2", + "resolved": "https://registry.npmjs.org/keccak256/-/keccak256-1.0.2.tgz", + "integrity": "sha512-f2EncSgmHmmQOkgxZ+/f2VaWTNkFL6f39VIrpoX+p8cEXJVyyCs/3h9GNz/ViHgwchxvv7oG5mjT2Tk4ZqInag==", + "dev": true, + "dependencies": { + "bn.js": "^4.11.8", + "keccak": "^3.0.1" + } + }, "node_modules/keyv": { "version": "3.1.0", "resolved": "https://registry.npmjs.org/keyv/-/keyv-3.1.0.tgz", @@ -13185,6 +13300,20 @@ "safe-buffer": "^5.1.1" } }, + "node_modules/merkletreejs": { + "version": "0.2.13", + "resolved": "https://registry.npmjs.org/merkletreejs/-/merkletreejs-0.2.13.tgz", + "integrity": "sha512-hnM1XX0C+3yfAytRiX7FKC+bYg+GC83aQq7EytAp6nbcUBRdXU6/AVkmNdsAaJJ9IaKZt0w76r0QeWY/Fq+uFw==", + "dev": true, + "dependencies": { + "buffer-reverse": "^1.0.1", + "crypto-js": "^3.1.9-1", + "treeify": "^1.1.0" + }, + "engines": { + "node": ">= 7.6.0" + } + }, "node_modules/methods": { "version": "1.1.2", "resolved": "https://registry.npmjs.org/methods/-/methods-1.1.2.tgz", @@ -17567,6 +17696,15 @@ "node": ">=6" } }, + "node_modules/treeify": { + "version": "1.1.0", + "resolved": "https://registry.npmjs.org/treeify/-/treeify-1.1.0.tgz", + "integrity": "sha512-1m4RA7xVAJrSGrrXGs0L3YTwyvBs2S8PbRHaLZAkFw7JR8oIFwYtysxlBZhYIa7xSyiYJKZ3iGrrk55cGA3i9A==", + "dev": true, + "engines": { + "node": ">=0.6" + } + }, "node_modules/true-case-path": { "version": "2.2.1", "resolved": "https://registry.npmjs.org/true-case-path/-/true-case-path-2.2.1.tgz", @@ -23031,6 +23169,12 @@ "integrity": "sha512-MQcXEUbCKtEo7bhqEs6560Hyd4XaovZlO/k9V3hjVUF/zwW7KBVdSK4gIt/bzwS9MbR5qob+F5jusZsb0YQK2A==", "dev": true }, + "buffer-reverse": { + "version": "1.0.1", + "resolved": "https://registry.npmjs.org/buffer-reverse/-/buffer-reverse-1.0.1.tgz", + "integrity": "sha1-SSg8jvpvkBvAH6MwTQYCeXGuL2A=", + "dev": true + }, "buffer-to-arraybuffer": { "version": "0.0.5", "resolved": "https://registry.npmjs.org/buffer-to-arraybuffer/-/buffer-to-arraybuffer-0.0.5.tgz", @@ -28994,6 +29138,16 @@ "node-gyp-build": "^4.2.0" } }, + "keccak256": { + "version": "1.0.2", + "resolved": "https://registry.npmjs.org/keccak256/-/keccak256-1.0.2.tgz", + "integrity": "sha512-f2EncSgmHmmQOkgxZ+/f2VaWTNkFL6f39VIrpoX+p8cEXJVyyCs/3h9GNz/ViHgwchxvv7oG5mjT2Tk4ZqInag==", + "dev": true, + "requires": { + "bn.js": "^4.11.8", + "keccak": "^3.0.1" + } + }, "keyv": { "version": "3.1.0", "resolved": "https://registry.npmjs.org/keyv/-/keyv-3.1.0.tgz", @@ -29827,6 +29981,17 @@ } } }, + "merkletreejs": { + "version": "0.2.13", + "resolved": "https://registry.npmjs.org/merkletreejs/-/merkletreejs-0.2.13.tgz", + "integrity": "sha512-hnM1XX0C+3yfAytRiX7FKC+bYg+GC83aQq7EytAp6nbcUBRdXU6/AVkmNdsAaJJ9IaKZt0w76r0QeWY/Fq+uFw==", + "dev": true, + "requires": { + "buffer-reverse": "^1.0.1", + "crypto-js": "^3.1.9-1", + "treeify": "^1.1.0" + } + }, "methods": { "version": "1.1.2", "resolved": "https://registry.npmjs.org/methods/-/methods-1.1.2.tgz", @@ -33311,6 +33476,12 @@ } } }, + "treeify": { + "version": "1.1.0", + "resolved": "https://registry.npmjs.org/treeify/-/treeify-1.1.0.tgz", + "integrity": "sha512-1m4RA7xVAJrSGrrXGs0L3YTwyvBs2S8PbRHaLZAkFw7JR8oIFwYtysxlBZhYIa7xSyiYJKZ3iGrrk55cGA3i9A==", + "dev": true + }, "true-case-path": { "version": "2.2.1", "resolved": "https://registry.npmjs.org/true-case-path/-/true-case-path-2.2.1.tgz", diff --git a/package.json b/package.json index c3c3c0e4124..2be7e0966b3 100644 --- a/package.json +++ b/package.json @@ -66,8 +66,10 @@ "ethereumjs-wallet": "^1.0.1", "hardhat": "^2.0.6", "hardhat-gas-reporter": "^1.0.4", + "keccak256": "^1.0.2", "lodash.startcase": "^4.4.0", "lodash.zip": "^4.2.0", + "merkletreejs": "^0.2.13", "micromatch": "^4.0.2", "mocha": "^8.0.1", "rimraf": "^3.0.2", @@ -75,6 +77,5 @@ "solidity-coverage": "^0.7.11", "solidity-docgen": "^0.5.3", "web3": "^1.3.0" - }, - "dependencies": {} + } } diff --git a/test/helpers/merkleTree.js b/test/helpers/merkleTree.js deleted file mode 100644 index 339954429c8..00000000000 --- a/test/helpers/merkleTree.js +++ /dev/null @@ -1,135 +0,0 @@ -const { keccak256, keccakFromString, bufferToHex } = require('ethereumjs-util'); - -class MerkleTree { - constructor (elements) { - // Filter empty strings and hash elements - this.elements = elements.filter(el => el).map(el => keccakFromString(el)); - - // Sort elements - this.elements.sort(Buffer.compare); - // Deduplicate elements - this.elements = this.bufDedup(this.elements); - - // Create layers - this.layers = this.getLayers(this.elements); - } - - getLayers (elements) { - if (elements.length === 0) { - return [['']]; - } - - const layers = []; - layers.push(elements); - - // Get next layer until we reach the root - while (layers[layers.length - 1].length > 1) { - layers.push(this.getNextLayer(layers[layers.length - 1])); - } - - return layers; - } - - getNextLayer (elements) { - return elements.reduce((layer, el, idx, arr) => { - if (idx % 2 === 0) { - // Hash the current element with its pair element - layer.push(this.combinedHash(el, arr[idx + 1])); - } - - return layer; - }, []); - } - - combinedHash (first, second) { - if (!first) { return second; } - if (!second) { return first; } - - return keccak256(this.sortAndConcat(first, second)); - } - - getRoot () { - return this.layers[this.layers.length - 1][0]; - } - - getHexRoot () { - return bufferToHex(this.getRoot()); - } - - getProof (el) { - let idx = this.bufIndexOf(el, this.elements); - - if (idx === -1) { - throw new Error('Element does not exist in Merkle tree'); - } - - return this.layers.reduce((proof, layer) => { - const pairElement = this.getPairElement(idx, layer); - - if (pairElement) { - proof.push(pairElement); - } - - idx = Math.floor(idx / 2); - - return proof; - }, []); - } - - getHexProof (el) { - const proof = this.getProof(el); - - return this.bufArrToHexArr(proof); - } - - getPairElement (idx, layer) { - const pairIdx = idx % 2 === 0 ? idx + 1 : idx - 1; - - if (pairIdx < layer.length) { - return layer[pairIdx]; - } else { - return null; - } - } - - bufIndexOf (el, arr) { - let hash; - - // Convert element to 32 byte hash if it is not one already - if (el.length !== 32 || !Buffer.isBuffer(el)) { - hash = keccakFromString(el); - } else { - hash = el; - } - - for (let i = 0; i < arr.length; i++) { - if (hash.equals(arr[i])) { - return i; - } - } - - return -1; - } - - bufDedup (elements) { - return elements.filter((el, idx) => { - return idx === 0 || !elements[idx - 1].equals(el); - }); - } - - bufArrToHexArr (arr) { - if (arr.some(el => !Buffer.isBuffer(el))) { - throw new Error('Array is not an array of buffers'); - } - - return arr.map(el => '0x' + el.toString('hex')); - } - - sortAndConcat (...args) { - return Buffer.concat([...args].sort(Buffer.compare)); - } -} - -module.exports = { - MerkleTree, -}; diff --git a/test/utils/cryptography/MerkleProof.test.js b/test/utils/cryptography/MerkleProof.test.js index 0ef5b3b08e9..c85b2b9251a 100644 --- a/test/utils/cryptography/MerkleProof.test.js +++ b/test/utils/cryptography/MerkleProof.test.js @@ -1,7 +1,7 @@ require('@openzeppelin/test-helpers'); -const { MerkleTree } = require('../../helpers/merkleTree.js'); -const { keccakFromString, bufferToHex } = require('ethereumjs-util'); +const { MerkleTree } = require('merkletreejs'); +const keccak256 = require('keccak256'); const { expect } = require('chai'); @@ -15,43 +15,43 @@ contract('MerkleProof', function (accounts) { describe('verify', function () { it('returns true for a valid Merkle proof', async function () { const elements = ['a', 'b', 'c', 'd']; - const merkleTree = new MerkleTree(elements); + const merkleTree = new MerkleTree(elements, keccak256, { hashLeaves: true }); const root = merkleTree.getHexRoot(); - const proof = merkleTree.getHexProof(elements[0]); + const leaf = keccak256(elements[0]); - const leaf = bufferToHex(keccakFromString(elements[0])); + const proof = merkleTree.getHexProof(leaf); expect(await this.merkleProof.verify(proof, root, leaf)).to.equal(true); }); it('returns false for an invalid Merkle proof', async function () { const correctElements = ['a', 'b', 'c']; - const correctMerkleTree = new MerkleTree(correctElements); + const correctMerkleTree = new MerkleTree(correctElements, keccak256, { hashLeaves: true }); const correctRoot = correctMerkleTree.getHexRoot(); - const correctLeaf = bufferToHex(keccakFromString(correctElements[0])); + const correctLeaf = keccak256(correctElements[0]); const badElements = ['d', 'e', 'f']; const badMerkleTree = new MerkleTree(badElements); - const badProof = badMerkleTree.getHexProof(badElements[0]); + const badProof = badMerkleTree.getHexProof(badElements[0], keccak256, { hashLeaves: true }); expect(await this.merkleProof.verify(badProof, correctRoot, correctLeaf)).to.equal(false); }); it('returns false for a Merkle proof of invalid length', async function () { const elements = ['a', 'b', 'c']; - const merkleTree = new MerkleTree(elements); + const merkleTree = new MerkleTree(elements, keccak256, { hashLeaves: true }); const root = merkleTree.getHexRoot(); - const proof = merkleTree.getHexProof(elements[0]); - const badProof = proof.slice(0, proof.length - 5); + const leaf = keccak256(elements[0]); - const leaf = bufferToHex(keccakFromString(elements[0])); + const proof = merkleTree.getHexProof(leaf); + const badProof = proof.slice(0, proof.length - 5); expect(await this.merkleProof.verify(badProof, root, leaf)).to.equal(false); }); From 96aece07f359867f00bba5e3ba1ff78e15479c5e Mon Sep 17 00:00:00 2001 From: Ross Campbell <41117279+Ro5s@users.noreply.github.com> Date: Wed, 10 Mar 2021 17:48:22 -0500 Subject: [PATCH 3/4] Fix docs formatting in IERC20Permit (#2579) Co-authored-by: Francisco Giordano --- contracts/token/ERC20/extensions/draft-IERC20Permit.sol | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/token/ERC20/extensions/draft-IERC20Permit.sol b/contracts/token/ERC20/extensions/draft-IERC20Permit.sol index dac94af447a..1a8731a5879 100644 --- a/contracts/token/ERC20/extensions/draft-IERC20Permit.sol +++ b/contracts/token/ERC20/extensions/draft-IERC20Permit.sol @@ -7,13 +7,13 @@ pragma solidity ^0.8.0; * https://eips.ethereum.org/EIPS/eip-2612[EIP-2612]. * * Adds the {permit} method, which can be used to change an account's ERC20 allowance (see {IERC20-allowance}) by - * presenting a message signed by the account. By not relying on `{IERC20-approve}`, the token holder account doesn't + * presenting a message signed by the account. By not relying on {IERC20-approve}, the token holder account doesn't * need to send a transaction, and thus is not required to hold Ether at all. */ interface IERC20Permit { /** - * @dev Sets `value` as the allowance of `spender` over `owner`'s tokens, - * given `owner`'s signed approval. + * @dev Sets `value` as the allowance of `spender` over ``owner``'s tokens, + * given ``owner``'s signed approval. * * IMPORTANT: The same issues {IERC20-approve} has related to transaction * ordering also apply here. @@ -44,7 +44,7 @@ interface IERC20Permit { function nonces(address owner) external view returns (uint256); /** - * @dev Returns the domain separator used in the encoding of the signature for `permit`, as defined by {EIP712}. + * @dev Returns the domain separator used in the encoding of the signature for {permit}, as defined by {EIP712}. */ // solhint-disable-next-line func-name-mixedcase function DOMAIN_SEPARATOR() external view returns (bytes32); From 682def9f898d0177aeb7b2567a4039af7aac4d6e Mon Sep 17 00:00:00 2001 From: Evert0x <35029353+Evert0x@users.noreply.github.com> Date: Thu, 11 Mar 2021 17:28:26 +0100 Subject: [PATCH 4/4] Typo in ERC20 constructor docstring (#2581) --- contracts/token/ERC20/ERC20.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/token/ERC20/ERC20.sol b/contracts/token/ERC20/ERC20.sol index edb5778bd57..1c3fe308f56 100644 --- a/contracts/token/ERC20/ERC20.sol +++ b/contracts/token/ERC20/ERC20.sol @@ -46,7 +46,7 @@ contract ERC20 is Context, IERC20, IERC20Metadata { * The defaut value of {decimals} is 18. To select a different value for * {decimals} you should overload it. * - * All three of these values are immutable: they can only be set once during + * All two of these values are immutable: they can only be set once during * construction. */ constructor (string memory name_, string memory symbol_) {