From 6de0be4c8a45c6736a8a52228ec5292622c8ae52 Mon Sep 17 00:00:00 2001 From: Claude <0xclaudeshannon@protonmail.com> Date: Thu, 6 Jan 2022 12:11:19 -0500 Subject: [PATCH 01/11] add feature request #3084 --- contracts/token/ERC20/ERC20.sol | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/contracts/token/ERC20/ERC20.sol b/contracts/token/ERC20/ERC20.sol index 054159e3dda..4d3f84b011e 100644 --- a/contracts/token/ERC20/ERC20.sol +++ b/contracts/token/ERC20/ERC20.sol @@ -153,9 +153,11 @@ contract ERC20 is Context, IERC20, IERC20Metadata { uint256 amount ) public virtual override returns (bool) { uint256 currentAllowance = _allowances[sender][_msgSender()]; - require(currentAllowance >= amount, "ERC20: transfer amount exceeds allowance"); - unchecked { - _approve(sender, _msgSender(), currentAllowance - amount); + if (currentAllowance != 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff) { + require(currentAllowance >= amount, "ERC20: transfer amount exceeds allowance"); + unchecked { + _approve(sender, _msgSender(), currentAllowance - amount); + } } _transfer(sender, recipient, amount); From b186eb5e5d633f706e482ebd3ab68c2fa99dcbcf Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 7 Jan 2022 23:56:29 +0100 Subject: [PATCH 02/11] Update contracts/token/ERC20/ERC20.sol Co-authored-by: Francisco Giordano --- 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 4d3f84b011e..bfe0fd1bc19 100644 --- a/contracts/token/ERC20/ERC20.sol +++ b/contracts/token/ERC20/ERC20.sol @@ -153,7 +153,7 @@ contract ERC20 is Context, IERC20, IERC20Metadata { uint256 amount ) public virtual override returns (bool) { uint256 currentAllowance = _allowances[sender][_msgSender()]; - if (currentAllowance != 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff) { + if (currentAllowance != type(uint256).max) { require(currentAllowance >= amount, "ERC20: transfer amount exceeds allowance"); unchecked { _approve(sender, _msgSender(), currentAllowance - amount); From b34920b2e400bd74732f11d70f615cf3c0f0a9c9 Mon Sep 17 00:00:00 2001 From: Claude <0xclaudeshannon@protonmail.com> Date: Fri, 7 Jan 2022 21:05:47 -0500 Subject: [PATCH 03/11] Add changelog note --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index be54f9160b5..fb449bacedc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ * `ERC2771Context`: use immutable storage to store the forwarder address, no longer an issue since Solidity >=0.8.8 allows reading immutable variables in the constructor. ([#2917](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2917)) * `Base64`: add a library to parse bytes into base64 strings using `encode(bytes memory)` function, and provide examples to show how to use to build URL-safe `tokenURIs` ([#2884](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#2884)) * `ERC20`: reduce allowance before triggering transfer. + * `ERC20`: do not change ERC20 allowance on `transferFrom` when allowance is `type(uint256).max`. ## 4.4.1 (2021-12-14) From 04d88ddb6cf21e8adb25007dc4a637d270aa0ae4 Mon Sep 17 00:00:00 2001 From: Claude <0xclaudeshannon@protonmail.com> Date: Sat, 8 Jan 2022 13:23:49 -0500 Subject: [PATCH 04/11] add documentation --- contracts/token/ERC20/IERC20.sol | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/contracts/token/ERC20/IERC20.sol b/contracts/token/ERC20/IERC20.sol index c89cd48daf6..16bdd000828 100644 --- a/contracts/token/ERC20/IERC20.sol +++ b/contracts/token/ERC20/IERC20.sol @@ -40,6 +40,9 @@ interface IERC20 { * * Returns a boolean value indicating whether the operation succeeded. * + * Note If `amount` is the maximum `uint`, the allowance is not updated on + * `transferFrom`. This is semantically equivalent to an infinite approval. + * * IMPORTANT: Beware that changing an allowance with this method brings the risk * that someone may use both the old and the new allowance by unfortunate * transaction ordering. One possible solution to mitigate this race @@ -55,6 +58,9 @@ interface IERC20 { * @dev Moves `amount` tokens from `sender` to `recipient` using the * allowance mechanism. `amount` is then deducted from the caller's * allowance. + * + * Note Does not update the allowance if the current allowance + * is the maximum `uint`. * * Returns a boolean value indicating whether the operation succeeded. * From 4b23042dafbde2f26bd57c9ef498f1dd675386e4 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Sun, 9 Jan 2022 17:47:02 +0100 Subject: [PATCH 05/11] test unlimitted allowance and add ERC777 unlimitted allowance --- CHANGELOG.md | 3 ++- contracts/token/ERC20/ERC20.sol | 6 ++++++ contracts/token/ERC20/IERC20.sol | 6 ------ contracts/token/ERC777/ERC777.sol | 14 ++++++++++++-- test/token/ERC20/ERC20.behavior.js | 21 ++++++++++++++++++++- 5 files changed, 40 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fb449bacedc..421edcb01af 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,8 @@ * `ERC2771Context`: use immutable storage to store the forwarder address, no longer an issue since Solidity >=0.8.8 allows reading immutable variables in the constructor. ([#2917](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2917)) * `Base64`: add a library to parse bytes into base64 strings using `encode(bytes memory)` function, and provide examples to show how to use to build URL-safe `tokenURIs` ([#2884](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#2884)) * `ERC20`: reduce allowance before triggering transfer. - * `ERC20`: do not change ERC20 allowance on `transferFrom` when allowance is `type(uint256).max`. + * `ERC20`: do not update allowance on `transferFrom` when allowance is `type(uint256).max`. + * `ERC777`: do not update allowance on `transferFrom` when allowance is `type(uint256).max`. ## 4.4.1 (2021-12-14) diff --git a/contracts/token/ERC20/ERC20.sol b/contracts/token/ERC20/ERC20.sol index bfe0fd1bc19..a695a827b93 100644 --- a/contracts/token/ERC20/ERC20.sol +++ b/contracts/token/ERC20/ERC20.sol @@ -125,6 +125,9 @@ contract ERC20 is Context, IERC20, IERC20Metadata { /** * @dev See {IERC20-approve}. * + * Note If `amount` is the maximum `uint256`, the allowance is not updated on + * `transferFrom`. This is semantically equivalent to an infinite approval. + * * Requirements: * * - `spender` cannot be the zero address. @@ -140,6 +143,9 @@ contract ERC20 is Context, IERC20, IERC20Metadata { * Emits an {Approval} event indicating the updated allowance. This is not * required by the EIP. See the note at the beginning of {ERC20}. * + * Note Does not update the allowance if the current allowance + * is the maximum `uint256`. + * * Requirements: * * - `sender` and `recipient` cannot be the zero address. diff --git a/contracts/token/ERC20/IERC20.sol b/contracts/token/ERC20/IERC20.sol index 16bdd000828..c89cd48daf6 100644 --- a/contracts/token/ERC20/IERC20.sol +++ b/contracts/token/ERC20/IERC20.sol @@ -40,9 +40,6 @@ interface IERC20 { * * Returns a boolean value indicating whether the operation succeeded. * - * Note If `amount` is the maximum `uint`, the allowance is not updated on - * `transferFrom`. This is semantically equivalent to an infinite approval. - * * IMPORTANT: Beware that changing an allowance with this method brings the risk * that someone may use both the old and the new allowance by unfortunate * transaction ordering. One possible solution to mitigate this race @@ -58,9 +55,6 @@ interface IERC20 { * @dev Moves `amount` tokens from `sender` to `recipient` using the * allowance mechanism. `amount` is then deducted from the caller's * allowance. - * - * Note Does not update the allowance if the current allowance - * is the maximum `uint`. * * Returns a boolean value indicating whether the operation succeeded. * diff --git a/contracts/token/ERC777/ERC777.sol b/contracts/token/ERC777/ERC777.sol index f89e1e0242c..314cbde4d19 100644 --- a/contracts/token/ERC777/ERC777.sol +++ b/contracts/token/ERC777/ERC777.sol @@ -258,6 +258,9 @@ contract ERC777 is Context, IERC777, IERC20 { /** * @dev See {IERC20-approve}. * + * Note If `value` is the maximum `uint256`, the allowance is not updated on + * `transferFrom`. This is semantically equivalent to an infinite approval. + * * Note that accounts cannot have allowance issued by their operators. */ function approve(address spender, uint256 value) public virtual override returns (bool) { @@ -269,6 +272,9 @@ contract ERC777 is Context, IERC777, IERC20 { /** * @dev See {IERC20-transferFrom}. * + * Note Does not update the allowance if the current allowance + * is the maximum `uint256`. + * * Note that operator and allowance concepts are orthogonal: operators cannot * call `transferFrom` (unless they have allowance), and accounts with * allowance cannot call `operatorSend` (unless they are operators). @@ -288,8 +294,12 @@ contract ERC777 is Context, IERC777, IERC20 { _callTokensToSend(spender, holder, recipient, amount, "", ""); uint256 currentAllowance = _allowances[holder][spender]; - require(currentAllowance >= amount, "ERC777: transfer amount exceeds allowance"); - _approve(holder, spender, currentAllowance - amount); + if (currentAllowance != type(uint256).max) { + require(currentAllowance >= amount, "ERC777: transfer amount exceeds allowance"); + unchecked { + _approve(holder, spender, currentAllowance - amount); + } + } _move(spender, holder, recipient, amount, "", ""); diff --git a/test/token/ERC20/ERC20.behavior.js b/test/token/ERC20/ERC20.behavior.js index 55bcc36084f..ecef092dba3 100644 --- a/test/token/ERC20/ERC20.behavior.js +++ b/test/token/ERC20/ERC20.behavior.js @@ -1,6 +1,6 @@ const { BN, constants, expectEvent, expectRevert } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); -const { ZERO_ADDRESS } = constants; +const { ZERO_ADDRESS, MAX_UINT256 } = constants; function shouldBehaveLikeERC20 (errorPrefix, initialSupply, initialHolder, recipient, anotherAccount) { describe('total supply', function () { @@ -128,6 +128,25 @@ function shouldBehaveLikeERC20 (errorPrefix, initialSupply, initialHolder, recip }); }); }); + + describe('when the spender has unlimited allowance', function () { + beforeEach(async function () { + await this.token.approve(spender, MAX_UINT256, { from: initialHolder }); + }); + + it('does nott decreases the spender allowance', async function () { + await this.token.transferFrom(tokenOwner, to, 1, { from: spender }); + + expect(await this.token.allowance(tokenOwner, spender)).to.be.bignumber.equal(MAX_UINT256); + }); + + it('does not emits an approval event', async function () { + expectEvent.notEmitted( + await this.token.transferFrom(tokenOwner, to, 1, { from: spender }), + 'Approval', + ); + }); + }); }); describe('when the recipient is the zero address', function () { From 514ed38a41d161c9ffe2a12ece8a741b1f4c6a3d Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Sun, 9 Jan 2022 17:49:01 +0100 Subject: [PATCH 06/11] reference PR in changelog --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 421edcb01af..64de2033033 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,8 +13,8 @@ * `ERC2771Context`: use immutable storage to store the forwarder address, no longer an issue since Solidity >=0.8.8 allows reading immutable variables in the constructor. ([#2917](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2917)) * `Base64`: add a library to parse bytes into base64 strings using `encode(bytes memory)` function, and provide examples to show how to use to build URL-safe `tokenURIs` ([#2884](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#2884)) * `ERC20`: reduce allowance before triggering transfer. - * `ERC20`: do not update allowance on `transferFrom` when allowance is `type(uint256).max`. - * `ERC777`: do not update allowance on `transferFrom` when allowance is `type(uint256).max`. + * `ERC20`: do not update allowance on `transferFrom` when allowance is `type(uint256).max`. ([#3085](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#3085)) + * `ERC777`: do not update allowance on `transferFrom` when allowance is `type(uint256).max`. ([#3085](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#3085)) ## 4.4.1 (2021-12-14) From 51d9fce0ed44f440a8a7c1362e7de0414da7f64d Mon Sep 17 00:00:00 2001 From: Claude <0xclaudeshannon@protonmail.com> Date: Mon, 10 Jan 2022 13:15:23 -0500 Subject: [PATCH 07/11] documentation IERC20 -> ERC20 --- contracts/token/ERC20/ERC20.sol | 6 ++++++ contracts/token/ERC20/IERC20.sol | 6 ------ 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/contracts/token/ERC20/ERC20.sol b/contracts/token/ERC20/ERC20.sol index bfe0fd1bc19..97ec62648ae 100644 --- a/contracts/token/ERC20/ERC20.sol +++ b/contracts/token/ERC20/ERC20.sol @@ -125,6 +125,9 @@ contract ERC20 is Context, IERC20, IERC20Metadata { /** * @dev See {IERC20-approve}. * + * Note If `amount` is the maximum `uint256`, the allowance is not updated. + * on `transferFrom`. This is semantically equivalent to an infinite approval. + * * Requirements: * * - `spender` cannot be the zero address. @@ -140,6 +143,9 @@ contract ERC20 is Context, IERC20, IERC20Metadata { * Emits an {Approval} event indicating the updated allowance. This is not * required by the EIP. See the note at the beginning of {ERC20}. * + * Note Does not update the allowance if the current allowance + * is the maximum `uint256`. + * * Requirements: * * - `sender` and `recipient` cannot be the zero address. diff --git a/contracts/token/ERC20/IERC20.sol b/contracts/token/ERC20/IERC20.sol index 16bdd000828..c89cd48daf6 100644 --- a/contracts/token/ERC20/IERC20.sol +++ b/contracts/token/ERC20/IERC20.sol @@ -40,9 +40,6 @@ interface IERC20 { * * Returns a boolean value indicating whether the operation succeeded. * - * Note If `amount` is the maximum `uint`, the allowance is not updated on - * `transferFrom`. This is semantically equivalent to an infinite approval. - * * IMPORTANT: Beware that changing an allowance with this method brings the risk * that someone may use both the old and the new allowance by unfortunate * transaction ordering. One possible solution to mitigate this race @@ -58,9 +55,6 @@ interface IERC20 { * @dev Moves `amount` tokens from `sender` to `recipient` using the * allowance mechanism. `amount` is then deducted from the caller's * allowance. - * - * Note Does not update the allowance if the current allowance - * is the maximum `uint`. * * Returns a boolean value indicating whether the operation succeeded. * From 8024e723c68c8229aaa1c4c440d7a751195553be Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 10 Jan 2022 19:43:24 -0300 Subject: [PATCH 08/11] use asciidoc note syntax --- contracts/token/ERC20/ERC20.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/token/ERC20/ERC20.sol b/contracts/token/ERC20/ERC20.sol index a695a827b93..fa30163bad2 100644 --- a/contracts/token/ERC20/ERC20.sol +++ b/contracts/token/ERC20/ERC20.sol @@ -125,7 +125,7 @@ contract ERC20 is Context, IERC20, IERC20Metadata { /** * @dev See {IERC20-approve}. * - * Note If `amount` is the maximum `uint256`, the allowance is not updated on + * NOTE: If `amount` is the maximum `uint256`, the allowance is not updated on * `transferFrom`. This is semantically equivalent to an infinite approval. * * Requirements: @@ -143,7 +143,7 @@ contract ERC20 is Context, IERC20, IERC20Metadata { * Emits an {Approval} event indicating the updated allowance. This is not * required by the EIP. See the note at the beginning of {ERC20}. * - * Note Does not update the allowance if the current allowance + * NOTE: Does not update the allowance if the current allowance * is the maximum `uint256`. * * Requirements: From b3cc6000fe5ad28c1ea6cab8c9112049e9825ea0 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 10 Jan 2022 19:43:52 -0300 Subject: [PATCH 09/11] use asciidoc note syntax --- contracts/token/ERC777/ERC777.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/token/ERC777/ERC777.sol b/contracts/token/ERC777/ERC777.sol index 314cbde4d19..302e3f1722e 100644 --- a/contracts/token/ERC777/ERC777.sol +++ b/contracts/token/ERC777/ERC777.sol @@ -258,7 +258,7 @@ contract ERC777 is Context, IERC777, IERC20 { /** * @dev See {IERC20-approve}. * - * Note If `value` is the maximum `uint256`, the allowance is not updated on + * NOTE: If `value` is the maximum `uint256`, the allowance is not updated on * `transferFrom`. This is semantically equivalent to an infinite approval. * * Note that accounts cannot have allowance issued by their operators. @@ -272,7 +272,7 @@ contract ERC777 is Context, IERC777, IERC20 { /** * @dev See {IERC20-transferFrom}. * - * Note Does not update the allowance if the current allowance + * NOTE: Does not update the allowance if the current allowance * is the maximum `uint256`. * * Note that operator and allowance concepts are orthogonal: operators cannot From 54bb56e71a15e2ebe4834eb36ed942ff00373aa0 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 10 Jan 2022 19:44:25 -0300 Subject: [PATCH 10/11] typo --- test/token/ERC20/ERC20.behavior.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/token/ERC20/ERC20.behavior.js b/test/token/ERC20/ERC20.behavior.js index ecef092dba3..b687de8afab 100644 --- a/test/token/ERC20/ERC20.behavior.js +++ b/test/token/ERC20/ERC20.behavior.js @@ -134,7 +134,7 @@ function shouldBehaveLikeERC20 (errorPrefix, initialSupply, initialHolder, recip await this.token.approve(spender, MAX_UINT256, { from: initialHolder }); }); - it('does nott decreases the spender allowance', async function () { + it('does not decreases the spender allowance', async function () { await this.token.transferFrom(tokenOwner, to, 1, { from: spender }); expect(await this.token.allowance(tokenOwner, spender)).to.be.bignumber.equal(MAX_UINT256); From 74e9a22f837d9527d67e76729e753f0da317649c Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 10 Jan 2022 19:44:52 -0300 Subject: [PATCH 11/11] typos --- test/token/ERC20/ERC20.behavior.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/token/ERC20/ERC20.behavior.js b/test/token/ERC20/ERC20.behavior.js index b687de8afab..3489ab05390 100644 --- a/test/token/ERC20/ERC20.behavior.js +++ b/test/token/ERC20/ERC20.behavior.js @@ -134,13 +134,13 @@ function shouldBehaveLikeERC20 (errorPrefix, initialSupply, initialHolder, recip await this.token.approve(spender, MAX_UINT256, { from: initialHolder }); }); - it('does not decreases the spender allowance', async function () { + it('does not decrease the spender allowance', async function () { await this.token.transferFrom(tokenOwner, to, 1, { from: spender }); expect(await this.token.allowance(tokenOwner, spender)).to.be.bignumber.equal(MAX_UINT256); }); - it('does not emits an approval event', async function () { + it('does not emit an approval event', async function () { expectEvent.notEmitted( await this.token.transferFrom(tokenOwner, to, 1, { from: spender }), 'Approval',