Remove hooks from ERC20

CHANGELOG.md

@@ -8,6 +8,7 @@
  * Removed presets in favor of [OpenZeppelin Contracts Wizard](https://wizard.openzeppelin.com/).
  * `TransparentUpgradeableProxy`: Removed `admin` and `implementation` getters, which were only callable by the proxy owner and thus not very useful. ([#3820](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3820))
  * `ProxyAdmin`: Removed `getProxyAdmin` and `getProxyImplementation` getters. ([#3820](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3820))
+ * `ERC20`: Deleted `_beforeTokenTransfer` and `_afterTokenTransfer` hooks, and refactor all extensions using those hooks for customization, by using `transfer` instead. ([#3838](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3838))
 
 ## Unreleased
 

contracts/mocks/ERC20SnapshotMock.sol

@@ -19,10 +19,10 @@ contract ERC20SnapshotMock is ERC20Snapshot {
     }
 
     function mint(address account, uint256 amount) public {
-        _mint(account, amount);
+        _transfer(address(0), account, amount);
     }
 
     function burn(address account, uint256 amount) public {
-        _burn(account, amount);
+        _transfer(account, address(0), amount);
     }
 }

contracts/mocks/ERC20VotesMock.sol

@@ -8,11 +8,11 @@ contract ERC20VotesMock is ERC20Votes {
     constructor(string memory name, string memory symbol) ERC20(name, symbol) EIP712(name, "1") {}
 
     function mint(address account, uint256 amount) public {
-        _mint(account, amount);
+        _transfer(address(0), account, amount);
     }
 
     function burn(address account, uint256 amount) public {
-        _burn(account, amount);
+        _transfer(account, address(0), amount);
     }
 
     function getChainId() external view returns (uint256) {

contracts/token/ERC20/ERC20.sol

@@ -214,89 +214,63 @@ contract ERC20 is Context, IERC20, IERC20Metadata {
      * This internal function is equivalent to {transfer}, and can be used to
      * e.g. implement automatic token fees, slashing mechanisms, etc.
      *
-     * Emits a {Transfer} event.
+     * Will mint (or burn) if `from` (or `to`) is address(0).
      *
-     * Requirements:
+     * Emits a {Transfer} event.
      *
-     * - `from` cannot be the zero address.
-     * - `to` cannot be the zero address.
-     * - `from` must have a balance of at least `amount`.
      */
     function _transfer(
         address from,
         address to,
         uint256 amount
     ) internal virtual {
-        require(from != address(0), "ERC20: transfer from the zero address");
-        require(to != address(0), "ERC20: transfer to the zero address");
-
-        _beforeTokenTransfer(from, to, amount);
+        require(from != address(0) || to != address(0), "ERC20: invalid transfer operation");
 
         uint256 fromBalance = _balances[from];
-        require(fromBalance >= amount, "ERC20: transfer amount exceeds balance");
-        unchecked {
-            _balances[from] = fromBalance - amount;
-            // Overflow not possible: the sum of all balances is capped by totalSupply, and the sum is preserved by
-            // decrementing then incrementing.
+        if (from == address(0)) {
+            _totalSupply += amount;
+            unchecked {
+                // Overflow not possible: balance + amount is at most totalSupply + amount, which is checked above.
+                _balances[to] += amount;
+            }
+        } else if (to == address(0)) {
+            require(fromBalance >= amount, "ERC20: burn amount exceeds balance");
+            _totalSupply -= amount;
+            unchecked {
+                _balances[from] -= amount;
+            }
+        } else {
+            require(fromBalance >= amount, "ERC20: transfer amount exceeds balance");
+            _balances[from] -= amount;
             _balances[to] += amount;
         }
 
         emit Transfer(from, to, amount);
-
-        _afterTokenTransfer(from, to, amount);
     }
 
-    /** @dev Creates `amount` tokens and assigns them to `account`, increasing
-     * the total supply.
+    /** @dev Creates `amount` tokens and assigns them to `account`, by transferring it from address(0).
+     * Relies on the `_transfer` mechanism
      *
      * Emits a {Transfer} event with `from` set to the zero address.
      *
-     * Requirements:
+     * Note: this function is not virtual, {_transfer} should be overridden instead.
      *
-     * - `account` cannot be the zero address.
      */
-    function _mint(address account, uint256 amount) internal virtual {
-        require(account != address(0), "ERC20: mint to the zero address");
-
-        _beforeTokenTransfer(address(0), account, amount);
-
-        _totalSupply += amount;
-        unchecked {
-            // Overflow not possible: balance + amount is at most totalSupply + amount, which is checked above.
-            _balances[account] += amount;
-        }
-        emit Transfer(address(0), account, amount);
-
-        _afterTokenTransfer(address(0), account, amount);
+    function _mint(address account, uint256 amount) internal {
+        _transfer(address(0), account, amount);
     }
 
     /**
-     * @dev Destroys `amount` tokens from `account`, reducing the
-     * total supply.
+     * @dev Destroys `amount` tokens from `account`, by transferring it to address(0).
+     * Relies on the `_transfer` mechanism.
      *
-     * Emits a {Transfer} event with `to` set to the zero address.
+     * Note: this function is not virtual, {_transfer} should be overridden instead
      *
-     * Requirements:
+     * Emits a {Transfer} event with `to` set to the zero address.
      *
-     * - `account` cannot be the zero address.
-     * - `account` must have at least `amount` tokens.
      */
-    function _burn(address account, uint256 amount) internal virtual {
-        require(account != address(0), "ERC20: burn from the zero address");
-
-        _beforeTokenTransfer(account, address(0), amount);
-
-        uint256 accountBalance = _balances[account];
-        require(accountBalance >= amount, "ERC20: burn amount exceeds balance");
-        unchecked {
-            _balances[account] = accountBalance - amount;
-            // Overflow not possible: amount <= accountBalance <= totalSupply.
-            _totalSupply -= amount;
-        }
-
-        emit Transfer(account, address(0), amount);
-
-        _afterTokenTransfer(account, address(0), amount);
+    function _burn(address account, uint256 amount) internal {
+        _transfer(account, address(0), amount);
     }
 
     /**
@@ -345,44 +319,4 @@ contract ERC20 is Context, IERC20, IERC20Metadata {
             }
         }
     }
-
-    /**
-     * @dev Hook that is called before any transfer of tokens. This includes
-     * minting and burning.
-     *
-     * Calling conditions:
-     *
-     * - when `from` and `to` are both non-zero, `amount` of ``from``'s tokens
-     * will be transferred to `to`.
-     * - when `from` is zero, `amount` tokens will be minted for `to`.
-     * - when `to` is zero, `amount` of ``from``'s tokens will be burned.
-     * - `from` and `to` are never both zero.
-     *
-     * To learn more about hooks, head to xref:ROOT:extending-contracts.adoc#using-hooks[Using Hooks].
-     */
-    function _beforeTokenTransfer(
-        address from,
-        address to,
-        uint256 amount
-    ) internal virtual {}
-
-    /**
-     * @dev Hook that is called after any transfer of tokens. This includes
-     * minting and burning.
-     *
-     * Calling conditions:
-     *
-     * - when `from` and `to` are both non-zero, `amount` of ``from``'s tokens
-     * has been transferred to `to`.
-     * - when `from` is zero, `amount` tokens have been minted for `to`.
-     * - when `to` is zero, `amount` of ``from``'s tokens have been burned.
-     * - `from` and `to` are never both zero.
-     *
-     * To learn more about hooks, head to xref:ROOT:extending-contracts.adoc#using-hooks[Using Hooks].
-     */
-    function _afterTokenTransfer(
-        address from,
-        address to,
-        uint256 amount
-    ) internal virtual {}
 }

contracts/token/ERC20/extensions/ERC20Capped.sol

@@ -28,10 +28,14 @@ abstract contract ERC20Capped is ERC20 {
     }
 
     /**
-     * @dev See {ERC20-_mint}.
+     * @dev See {ERC20-_transfer}.
      */
-    function _mint(address account, uint256 amount) internal virtual override {
+    function _transfer(
+        address,
+        address account,
+        uint256 amount
+    ) internal virtual override {
         require(ERC20.totalSupply() + amount <= cap(), "ERC20Capped: cap exceeded");
-        super._mint(account, amount);
+        super._transfer(address(0), account, amount);
     }
 }

contracts/token/ERC20/extensions/ERC20Pausable.sol

@@ -15,19 +15,18 @@ import "../../../security/Pausable.sol";
  */
 abstract contract ERC20Pausable is ERC20, Pausable {
     /**
-     * @dev See {ERC20-_beforeTokenTransfer}.
+     * @dev See {ERC20-_transfer}.
      *
      * Requirements:
      *
      * - the contract must not be paused.
      */
-    function _beforeTokenTransfer(
+    function _transfer(
         address from,
         address to,
         uint256 amount
     ) internal virtual override {
-        super._beforeTokenTransfer(from, to, amount);
-
         require(!paused(), "ERC20Pausable: token transfer while paused");
+        super._transfer(from, to, amount);
     }
 }

contracts/token/ERC20/extensions/ERC20Snapshot.sol

@@ -118,15 +118,13 @@ abstract contract ERC20Snapshot is ERC20 {
         return snapshotted ? value : totalSupply();
     }
 
-    // Update balance and/or total supply snapshots before the values are modified. This is implemented
-    // in the _beforeTokenTransfer hook, which is executed for _mint, _burn, and _transfer operations.
-    function _beforeTokenTransfer(
+    // Update balance and/or total supply snapshots before the values are modified. This is executed
+    // for _mint, _burn, and _transfer operations.
+    function _transfer(
         address from,
         address to,
         uint256 amount
     ) internal virtual override {
-        super._beforeTokenTransfer(from, to, amount);
-
         if (from == address(0)) {
             // mint
             _updateAccountSnapshot(to);
@@ -140,6 +138,7 @@ abstract contract ERC20Snapshot is ERC20 {
             _updateAccountSnapshot(from);
             _updateAccountSnapshot(to);
         }
+        super._transfer(from, to, amount);
     }
 
     function _valueAt(uint256 snapshotId, Snapshots storage snapshots) private view returns (bool, uint256) {

contracts/token/ERC20/extensions/ERC20Votes.sol

@@ -35,13 +35,16 @@ abstract contract ERC20Votes is ERC20, Votes {
      *
      * Emits a {IVotes-DelegateVotesChanged} event.
      */
-    function _afterTokenTransfer(
+    function _transfer(
         address from,
         address to,
         uint256 amount
     ) internal virtual override {
+        super._transfer(from, to, amount);
+        if (from == address(0)) {
+            require(totalSupply() <= _maxSupply(), "ERC20Votes: total supply risks overflowing votes");
+        }
         _transferVotingUnits(from, to, amount);
-        super._afterTokenTransfer(from, to, amount);
     }
 
     /**
@@ -64,12 +67,4 @@ abstract contract ERC20Votes is ERC20, Votes {
     function _getVotingUnits(address account) internal view virtual override returns (uint256) {
         return balanceOf(account);
     }
-
-    /**
-     * @dev Snapshots the totalSupply after it has been increased.
-     */
-    function _mint(address account, uint256 amount) internal virtual override {
-        super._mint(account, amount);
-        require(totalSupply() <= _maxSupply(), "ERC20Votes: total supply risks overflowing votes");
-    }
 }

test/token/ERC20/ERC20.behavior.js

@@ -148,21 +148,6 @@ function shouldBehaveLikeERC20 (errorPrefix, initialSupply, initialHolder, recip
           });
         });
       });
-
-      describe('when the recipient is the zero address', function () {
-        const amount = initialSupply;
-        const to = ZERO_ADDRESS;
-
-        beforeEach(async function () {
-          await this.token.approve(spender, amount, { from: tokenOwner });
-        });
-
-        it('reverts', async function () {
-          await expectRevert(this.token.transferFrom(
-            tokenOwner, to, amount, { from: spender }), `${errorPrefix}: transfer to the zero address`,
-          );
-        });
-      });
     });
 
     describe('when the token owner is the zero address', function () {
@@ -240,14 +225,6 @@ function shouldBehaveLikeERC20Transfer (errorPrefix, from, to, balance, transfer
       });
     });
   });
-
-  describe('when the recipient is the zero address', function () {
-    it('reverts', async function () {
-      await expectRevert(transfer.call(this, from, ZERO_ADDRESS, balance),
-        `${errorPrefix}: transfer to the zero address`,
-      );
-    });
-  });
 }
 
 function shouldBehaveLikeERC20Approve (errorPrefix, owner, spender, supply, approve) {

test/token/ERC20/ERC20.test.js

@@ -203,7 +203,14 @@ contract('ERC20', function (accounts) {
     const amount = new BN(50);
     it('rejects a null account', async function () {
       await expectRevert(
-        this.token.mint(ZERO_ADDRESS, amount), 'ERC20: mint to the zero address',
+        this.token.mint(ZERO_ADDRESS, amount), 'ERC20: invalid transfer operation',
+      );
+    });
+
+    it('rejects overflow', async function () {
+      const maxUint256 = new BN('2').pow(new BN(256)).subn(1);
+      await expectRevert.unspecified(
+        this.token.mint(recipient, maxUint256),
       );
     });
 
@@ -236,7 +243,7 @@ contract('ERC20', function (accounts) {
   describe('_burn', function () {
     it('rejects a null account', async function () {
       await expectRevert(this.token.burn(ZERO_ADDRESS, new BN(1)),
-        'ERC20: burn from the zero address');
+        'ERC20: invalid transfer operation');
     });
 
     describe('for a non zero account', function () {
@@ -283,14 +290,6 @@ contract('ERC20', function (accounts) {
     shouldBehaveLikeERC20Transfer('ERC20', initialHolder, recipient, initialSupply, function (from, to, amount) {
       return this.token.transferInternal(from, to, amount);
     });
-
-    describe('when the sender is the zero address', function () {
-      it('reverts', async function () {
-        await expectRevert(this.token.transferInternal(ZERO_ADDRESS, recipient, initialSupply),
-          'ERC20: transfer from the zero address',
-        );
-      });
-    });
   });
 
   describe('_approve', function () {

test/token/ERC20/extensions/ERC20FlashMint.test.js

@@ -1,8 +1,8 @@
 /* eslint-disable */
 
-const { BN, constants, expectEvent, expectRevert, time } = require('@openzeppelin/test-helpers');
+const { BN, constants, expectEvent, expectRevert } = require('@openzeppelin/test-helpers');
 const { expect } = require('chai');
-const { MAX_UINT256, ZERO_ADDRESS, ZERO_BYTES32 } = constants;
+const { MAX_UINT256, ZERO_ADDRESS } = constants;
 
 const ERC20FlashMintMock = artifacts.require('ERC20FlashMintMock');
 const ERC3156FlashBorrowerMock = artifacts.require('ERC3156FlashBorrowerMock');

test/token/ERC20/extensions/ERC20Votes.test.js

@@ -53,7 +53,7 @@ contract('ERC20Votes', function (accounts) {
     const amount = new BN('2').pow(new BN('224'));
     await expectRevert(
       this.token.mint(holder, amount),
-      "SafeCast: value doesn't fit in 224 bits",
+      "ERC20Votes: total supply risks overflowing votes",
     );
   });