Remove hooks from ERC20 (#3838)

Co-authored-by: Francisco <frangio.1@gmail.com>

CHANGELOG.md

@@ -8,6 +8,30 @@
  * 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, added a new internal `_update` function for customizations, and refactored all extensions using those hooks to use `_update` instead. ([#3838](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3838))
+
+### How to upgrade from 4.x
+
+#### ERC20, ERC721, and ERC1155
+
+These breaking changes will require modifications to ERC20, ERC721, and ERC1155 contracts, since the `_afterTokenTransfer` and `_beforeTokenTransfer` functions were removed. Any customization made through those hooks should now be done overriding the new `_update` function instead.
+
+Minting and burning are implemented by `_update` and customizations should be done by overriding this function as well. `_mint` and `_burn` are no longer virtual (meaning they are not overridable) to guard against possible inconsistencies.
+
+For example, a contract using `ERC20`'s `_beforeTokenTransfer` hook would have to be changed in the following way.
+
+```diff
+- function _beforeTokenTransfer(
++ function _update(
+      address from,
+      address to,
+      uint256 amount
+  ) internal virtual override {
+-     super._beforeTokenTransfer(from, to, amount);
+      require(!condition(), "ERC20: wrong condition");
++     super._update(from, to, amount);
+  }
+```
 
 ## Unreleased
 

contracts/token/ERC20/ERC20.sol

@@ -216,87 +216,81 @@ contract ERC20 is Context, IERC20, IERC20Metadata {
      *
      * Emits a {Transfer} event.
      *
-     * Requirements:
-     *
-     * - `from` cannot be the zero address.
-     * - `to` cannot be the zero address.
-     * - `from` must have a balance of at least `amount`.
+     * NOTE: This function is not virtual, {_update} should be overridden instead.
      */
     function _transfer(
         address from,
         address to,
         uint256 amount
-    ) internal virtual {
+    ) internal {
         require(from != address(0), "ERC20: transfer from the zero address");
         require(to != address(0), "ERC20: transfer to the zero address");
+        _update(from, to, amount);
+    }
 
-        _beforeTokenTransfer(from, to, amount);
-
-        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.
-            _balances[to] += amount;
+    /**
+     * @dev Transfers `amount` of tokens from `from` to `to`, or alternatively mints (or burns) if `from` (or `to`) is
+     * the zero address. All customizations to transfers, mints, and burns should be done by overriding this function.
+     *
+     * Emits a {Transfer} event.
+     */
+    function _update(
+        address from,
+        address to,
+        uint256 amount
+    ) internal virtual {
+        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)) {
+            uint256 fromBalance = _balances[from];
+            require(fromBalance >= amount, "ERC20: burn amount exceeds balance");
+            _totalSupply -= amount;
+            unchecked {
+                // Overflow not possible: amount <= fromBalance <= totalSupply.
+                _balances[from] = fromBalance - amount;
+            }
+        } else {
+            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.
+                _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 `_update` mechanism
      *
      * Emits a {Transfer} event with `from` set to the zero address.
      *
-     * Requirements:
-     *
-     * - `account` cannot be the zero address.
+     * NOTE: This function is not virtual, {_update} should be overridden instead.
      */
-    function _mint(address account, uint256 amount) internal virtual {
+    function _mint(address account, uint256 amount) internal {
         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);
+        _update(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 `_update` mechanism.
      *
      * Emits a {Transfer} event with `to` set to the zero address.
      *
-     * Requirements:
-     *
-     * - `account` cannot be the zero address.
-     * - `account` must have at least `amount` tokens.
+     * NOTE: This function is not virtual, {_update} should be overridden instead
      */
-    function _burn(address account, uint256 amount) internal virtual {
+    function _burn(address account, uint256 amount) internal {
         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);
+        _update(account, address(0), amount);
     }
 
     /**
@@ -345,44 +339,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,17 @@ abstract contract ERC20Capped is ERC20 {
     }
 
     /**
-     * @dev See {ERC20-_mint}.
+     * @dev See {ERC20-_transfer}.
      */
-    function _mint(address account, uint256 amount) internal virtual override {
-        require(ERC20.totalSupply() + amount <= cap(), "ERC20Capped: cap exceeded");
-        super._mint(account, amount);
+    function _update(
+        address from,
+        address to,
+        uint256 amount
+    ) internal virtual override {
+        if (from == address(0)) {
+            require(ERC20.totalSupply() + amount <= cap(), "ERC20Capped: cap exceeded");
+        }
+
+        super._update(from, to, 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-_update}.
      *
      * Requirements:
      *
      * - the contract must not be paused.
      */
-    function _beforeTokenTransfer(
+    function _update(
         address from,
         address to,
         uint256 amount
     ) internal virtual override {
-        super._beforeTokenTransfer(from, to, amount);
-
         require(!paused(), "ERC20Pausable: token transfer while paused");
+        super._update(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 _update(
         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._update(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 _update(
         address from,
         address to,
         uint256 amount
     ) internal virtual override {
+        super._update(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

@@ -158,8 +158,9 @@ function shouldBehaveLikeERC20 (errorPrefix, initialSupply, initialHolder, recip
         });
 
         it('reverts', async function () {
-          await expectRevert(this.token.transferFrom(
-            tokenOwner, to, amount, { from: spender }), `${errorPrefix}: transfer to the zero address`,
+          await expectRevert(
+            this.token.transferFrom(tokenOwner, to, amount, { from: spender }),
+            `${errorPrefix}: transfer to the zero address`,
           );
         });
       });
@@ -240,14 +241,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

@@ -207,6 +207,14 @@ contract('ERC20', function (accounts) {
       );
     });
 
+    it('rejects overflow', async function () {
+      const maxUint256 = new BN('2').pow(new BN(256)).subn(1);
+      await expectRevert(
+        this.token.mint(recipient, maxUint256),
+        'reverted with panic code 0x11 (Arithmetic operation underflowed or overflowed outside of an unchecked block)',
+      );
+    });
+
     describe('for a non zero account', function () {
       beforeEach('minting', async function () {
         this.receipt = await this.token.mint(recipient, amount);
@@ -283,14 +291,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",
     );
   });