Do not emit Approval event when calling transferFrom (#4370)

Co-authored-by: Ernesto García <ernestognw@gmail.com>
Co-authored-by: Francisco <fg@frang.io>

.changeset/heavy-drinks-fail.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': major
+---
+
+`ERC20`: Remove `Approval` event previously emitted in `transferFrom` to indicate that part of the allowance was consumed. With this change, allowances are no longer reconstructible from events. See the code for guidelines on how to re-enable this event if needed.

contracts/mocks/token/ERC20ApprovalMock.sol

@@ -0,0 +1,10 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.19;
+
+import "../../token/ERC20/ERC20.sol";
+
+abstract contract ERC20ApprovalMock is ERC20 {
+    function _approve(address owner, address spender, uint256 amount, bool) internal virtual override {
+        super._approve(owner, spender, amount, true);
+    }
+}

contracts/token/ERC20/ERC20.sol

@@ -311,6 +311,27 @@ abstract contract ERC20 is Context, IERC20, IERC20Metadata, IERC20Errors {
      * - `spender` cannot be the zero address.
      */
     function _approve(address owner, address spender, uint256 amount) internal virtual {
+        _approve(owner, spender, amount, true);
+    }
+
+    /**
+     * @dev Alternative version of {_approve} with an optional flag that can enable or disable the Approval event.
+     *
+     * By default (when calling {_approve}) the flag is set to true. On the other hand, approval changes made by
+     * `_spendAllowance` during the `transferFrom` operation set the flag to false. This saves gas by not emitting any
+     * `Approval` event during `transferFrom` operations.
+     *
+     * Anyone who wishes to continue emitting `Approval` events on the`transferFrom` operation can force the flag to true
+     * using the following override:
+     * ```
+     * function _approve(address owner, address spender, uint256 amount, bool) internal virtual override {
+     *     super._approve(owner, spender, amount, true);
+     * }
+     * ```
+     *
+     * Requirements are the same as {_approve}.
+     */
+    function _approve(address owner, address spender, uint256 amount, bool emitEvent) internal virtual {
         if (owner == address(0)) {
             revert ERC20InvalidApprover(address(0));
         }
@@ -318,8 +339,10 @@ abstract contract ERC20 is Context, IERC20, IERC20Metadata, IERC20Errors {
             revert ERC20InvalidSpender(address(0));
         }
         _allowances[owner][spender] = amount;
+        if (emitEvent) {
             emit Approval(owner, spender, amount);
         }
+    }
 
     /**
      * @dev Updates `owner` s allowance for `spender` based on spent `amount`.
@@ -336,7 +359,7 @@ abstract contract ERC20 is Context, IERC20, IERC20Metadata, IERC20Errors {
                 revert ERC20InsufficientAllowance(spender, currentAllowance, amount);
             }
             unchecked {
-                _approve(owner, spender, currentAllowance - amount);
+                _approve(owner, spender, currentAllowance - amount, false);
             }
         }
     }

test/token/ERC20/ERC20.behavior.js

@@ -4,7 +4,10 @@ const { ZERO_ADDRESS, MAX_UINT256 } = constants;
 
 const { expectRevertCustomError } = require('../../helpers/customError');
 
-function shouldBehaveLikeERC20(initialSupply, initialHolder, recipient, anotherAccount) {
+function shouldBehaveLikeERC20(initialSupply, accounts, opts = {}) {
+  const [initialHolder, recipient, anotherAccount] = accounts;
+  const { forcedApproval } = opts;
+
   describe('total supply', function () {
     it('returns the total amount of tokens', async function () {
       expect(await this.token.totalSupply()).to.be.bignumber.equal(initialSupply);
@@ -70,6 +73,7 @@ function shouldBehaveLikeERC20(initialSupply, initialHolder, recipient, anotherA
               });
             });
 
+            if (forcedApproval) {
               it('emits an approval event', async function () {
                 expectEvent(await this.token.transferFrom(tokenOwner, to, amount, { from: spender }), 'Approval', {
                   owner: tokenOwner,
@@ -77,6 +81,14 @@ function shouldBehaveLikeERC20(initialSupply, initialHolder, recipient, anotherA
                   value: await this.token.allowance(tokenOwner, spender),
                 });
               });
+            } else {
+              it('does not emit an approval event', async function () {
+                expectEvent.notEmitted(
+                  await this.token.transferFrom(tokenOwner, to, amount, { from: spender }),
+                  'Approval',
+                );
+              });
+            }
           });
 
           describe('when the token owner does not have enough balance', function () {

test/token/ERC20/ERC20.test.js

@@ -9,22 +9,27 @@ const {
 } = require('./ERC20.behavior');
 const { expectRevertCustomError } = require('../../helpers/customError');
 
-const ERC20 = artifacts.require('$ERC20');
+const TOKENS = [
-const ERC20Decimals = artifacts.require('$ERC20DecimalsMock');
+  { Token: artifacts.require('$ERC20') },
+  { Token: artifacts.require('$ERC20ApprovalMock'), forcedApproval: true },
+];
 
 contract('ERC20', function (accounts) {
-  const [initialHolder, recipient, anotherAccount] = accounts;
+  const [initialHolder, recipient] = accounts;
 
   const name = 'My Token';
   const symbol = 'MTKN';
-
   const initialSupply = new BN(100);
 
+  for (const { Token, forcedApproval } of TOKENS) {
+    describe(`using ${Token._json.contractName}`, function () {
       beforeEach(async function () {
-    this.token = await ERC20.new(name, symbol);
+        this.token = await Token.new(name, symbol);
         await this.token.$_mint(initialHolder, initialSupply);
       });
 
+      shouldBehaveLikeERC20(initialSupply, accounts, { forcedApproval });
+
       it('has a name', async function () {
         expect(await this.token.name()).to.equal(name);
       });
@@ -37,17 +42,6 @@ contract('ERC20', function (accounts) {
         expect(await this.token.decimals()).to.be.bignumber.equal('18');
       });
 
-  describe('set decimals', function () {
-    const decimals = new BN(6);
-
-    it('can set decimals during construction', async function () {
-      const token = await ERC20Decimals.new(name, symbol, decimals);
-      expect(await token.decimals()).to.be.bignumber.equal(decimals);
-    });
-  });
-
-  shouldBehaveLikeERC20(initialSupply, initialHolder, recipient, anotherAccount);
-
       describe('decrease allowance', function () {
         describe('when the spender is not the zero address', function () {
           const spender = recipient;
@@ -212,7 +206,9 @@ contract('ERC20', function (accounts) {
       describe('_mint', function () {
         const amount = new BN(50);
         it('rejects a null account', async function () {
-      await expectRevertCustomError(this.token.$_mint(ZERO_ADDRESS, amount), 'ERC20InvalidReceiver', [ZERO_ADDRESS]);
+          await expectRevertCustomError(this.token.$_mint(ZERO_ADDRESS, amount), 'ERC20InvalidReceiver', [
+            ZERO_ADDRESS,
+          ]);
         });
 
         it('rejects overflow', async function () {
@@ -247,7 +243,9 @@ contract('ERC20', function (accounts) {
 
       describe('_burn', function () {
         it('rejects a null account', async function () {
-      await expectRevertCustomError(this.token.$_burn(ZERO_ADDRESS, new BN(1)), 'ERC20InvalidSender', [ZERO_ADDRESS]);
+          await expectRevertCustomError(this.token.$_burn(ZERO_ADDRESS, new BN(1)), 'ERC20InvalidSender', [
+            ZERO_ADDRESS,
+          ]);
         });
 
         describe('for a non zero account', function () {
@@ -363,3 +361,5 @@ contract('ERC20', function (accounts) {
         });
       });
     });
+  }
+});

test/token/ERC20/extensions/ERC20Wrapper.test.js

@@ -10,7 +10,7 @@ const ERC20Decimals = artifacts.require('$ERC20DecimalsMock');
 const ERC20Wrapper = artifacts.require('$ERC20Wrapper');
 
 contract('ERC20Wrapper', function (accounts) {
-  const [initialHolder, recipient, anotherAccount] = accounts;
+  const [initialHolder, receiver] = accounts;
 
   const name = 'My Token';
   const symbol = 'MTKN';
@@ -85,7 +85,7 @@ contract('ERC20Wrapper', function (accounts) {
 
     it('to other account', async function () {
       await this.underlying.approve(this.token.address, initialSupply, { from: initialHolder });
-      const { tx } = await this.token.depositFor(anotherAccount, initialSupply, { from: initialHolder });
+      const { tx } = await this.token.depositFor(receiver, initialSupply, { from: initialHolder });
       await expectEvent.inTransaction(tx, this.underlying, 'Transfer', {
         from: initialHolder,
         to: this.token.address,
@@ -93,7 +93,7 @@ contract('ERC20Wrapper', function (accounts) {
       });
       await expectEvent.inTransaction(tx, this.token, 'Transfer', {
         from: ZERO_ADDRESS,
-        to: anotherAccount,
+        to: receiver,
         value: initialSupply,
       });
     });
@@ -144,10 +144,10 @@ contract('ERC20Wrapper', function (accounts) {
     });
 
     it('to other account', async function () {
-      const { tx } = await this.token.withdrawTo(anotherAccount, initialSupply, { from: initialHolder });
+      const { tx } = await this.token.withdrawTo(receiver, initialSupply, { from: initialHolder });
       await expectEvent.inTransaction(tx, this.underlying, 'Transfer', {
         from: this.token.address,
-        to: anotherAccount,
+        to: receiver,
         value: initialSupply,
       });
       await expectEvent.inTransaction(tx, this.token, 'Transfer', {
@@ -163,10 +163,10 @@ contract('ERC20Wrapper', function (accounts) {
       await this.underlying.approve(this.token.address, initialSupply, { from: initialHolder });
       await this.token.depositFor(initialHolder, initialSupply, { from: initialHolder });
 
-      const { tx } = await this.token.$_recover(anotherAccount);
+      const { tx } = await this.token.$_recover(receiver);
       await expectEvent.inTransaction(tx, this.token, 'Transfer', {
         from: ZERO_ADDRESS,
-        to: anotherAccount,
+        to: receiver,
         value: '0',
       });
     });
@@ -174,10 +174,10 @@ contract('ERC20Wrapper', function (accounts) {
     it('something to recover', async function () {
       await this.underlying.transfer(this.token.address, initialSupply, { from: initialHolder });
 
-      const { tx } = await this.token.$_recover(anotherAccount);
+      const { tx } = await this.token.$_recover(receiver);
       await expectEvent.inTransaction(tx, this.token, 'Transfer', {
         from: ZERO_ADDRESS,
-        to: anotherAccount,
+        to: receiver,
         value: initialSupply,
       });
     });
@@ -189,6 +189,6 @@ contract('ERC20Wrapper', function (accounts) {
       await this.token.depositFor(initialHolder, initialSupply, { from: initialHolder });
     });
 
-    shouldBehaveLikeERC20(initialSupply, initialHolder, recipient, anotherAccount);
+    shouldBehaveLikeERC20(initialSupply, accounts);
   });
 });