Do not reduce approval on transferFrom if current allowance is type(u…

…int256).max (#3085) * add feature request #3084 * Update contracts/token/ERC20/ERC20.sol Co-authored-by: Francisco Giordano <[email protected]> * Add changelog note * add documentation * test unlimitted allowance and add ERC777 unlimitted allowance * reference PR in changelog * documentation IERC20 -> ERC20 * use asciidoc note syntax * use asciidoc note syntax * typo * typos Co-authored-by: Hadrien Croubois <[email protected]> Co-authored-by: Francisco Giordano <[email protected]>
OpenZeppelin · Jan 10, 2022 · 80d8da0 · 80d8da0
1 parent fd7c4ba
commit 80d8da0
Show file tree

Hide file tree

Showing 4 changed files with 45 additions and 6 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -14,6 +14,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. ([#3056](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#3056))
+ * `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))
 
 ### Breaking change
 

diff --git 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.
@@ -153,9 +159,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 != type(uint256).max) {
+            require(currentAllowance >= amount, "ERC20: transfer amount exceeds allowance");
+            unchecked {
+                _approve(sender, _msgSender(), currentAllowance - amount);
+            }
         }
 
         _transfer(sender, recipient, amount);

diff --git 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
@@ -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 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 emit 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 () {