diff --git a/CHANGELOG.md b/CHANGELOG.md index bf92d9b4395..9450563faf5 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/contracts/token/ERC20/ERC20.sol b/contracts/token/ERC20/ERC20.sol index c5c52691de1..c10f659d019 100644 --- a/contracts/token/ERC20/ERC20.sol +++ b/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 {} } diff --git a/contracts/token/ERC20/extensions/ERC20Capped.sol b/contracts/token/ERC20/extensions/ERC20Capped.sol index 16f830d18a5..3be9ff805d7 100644 --- a/contracts/token/ERC20/extensions/ERC20Capped.sol +++ b/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); } } diff --git a/contracts/token/ERC20/extensions/ERC20Pausable.sol b/contracts/token/ERC20/extensions/ERC20Pausable.sol index e448e96a640..92d764932a9 100644 --- a/contracts/token/ERC20/extensions/ERC20Pausable.sol +++ b/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); } } diff --git a/contracts/token/ERC20/extensions/ERC20Snapshot.sol b/contracts/token/ERC20/extensions/ERC20Snapshot.sol index 0b46fc660bf..2c34e226809 100644 --- a/contracts/token/ERC20/extensions/ERC20Snapshot.sol +++ b/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) { diff --git a/contracts/token/ERC20/extensions/ERC20Votes.sol b/contracts/token/ERC20/extensions/ERC20Votes.sol index e2a070921c6..7f9f36b7603 100644 --- a/contracts/token/ERC20/extensions/ERC20Votes.sol +++ b/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"); - } } diff --git a/test/token/ERC20/ERC20.behavior.js b/test/token/ERC20/ERC20.behavior.js index 8bc54762ac8..701ec2e6d5e 100644 --- a/test/token/ERC20/ERC20.behavior.js +++ b/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) { diff --git a/test/token/ERC20/ERC20.test.js b/test/token/ERC20/ERC20.test.js index 992edf93b17..46ee5ab6373 100644 --- a/test/token/ERC20/ERC20.test.js +++ b/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 () { diff --git a/test/token/ERC20/extensions/ERC20FlashMint.test.js b/test/token/ERC20/extensions/ERC20FlashMint.test.js index d08464decfb..4354dd90c2b 100644 --- a/test/token/ERC20/extensions/ERC20FlashMint.test.js +++ b/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'); diff --git a/test/token/ERC20/extensions/ERC20Votes.test.js b/test/token/ERC20/extensions/ERC20Votes.test.js index 0d3ab74aadc..d89a7b9fb54 100644 --- a/test/token/ERC20/extensions/ERC20Votes.test.js +++ b/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", ); });