From d210847e28e7f81265155e465b139cdda50d0c21 Mon Sep 17 00:00:00 2001 From: JulissaDantes Date: Tue, 10 Jan 2023 15:21:35 -0500 Subject: [PATCH] Fix ERC20._update (#3921) Co-authored-by: Francisco Co-authored-by: Hadrien Croubois --- contracts/mocks/ERC20Mock.sol | 5 +++ contracts/token/ERC20/ERC20.sol | 26 +++++------ .../token/ERC20/extensions/ERC20Capped.sol | 2 +- .../token/ERC20/extensions/ERC20Snapshot.sol | 13 +++--- test/token/ERC20/ERC20.test.js | 45 ++++++++++++++++++- .../extensions/ERC20Burnable.behavior.js | 4 +- .../ERC20/extensions/ERC20FlashMint.test.js | 2 +- .../ERC20/extensions/ERC20Wrapper.test.js | 2 +- 8 files changed, 72 insertions(+), 27 deletions(-) diff --git a/contracts/mocks/ERC20Mock.sol b/contracts/mocks/ERC20Mock.sol index 16ffad19f..2530eafc2 100644 --- a/contracts/mocks/ERC20Mock.sol +++ b/contracts/mocks/ERC20Mock.sol @@ -30,4 +30,9 @@ contract ERC20Mock is ERC20 { function approveInternal(address owner, address spender, uint256 value) public { _approve(owner, spender, value); } + + // Exposed for testing purposes + function update(address from, address to, uint256 amount) public { + _update(from, to, amount); + } } diff --git a/contracts/token/ERC20/ERC20.sol b/contracts/token/ERC20/ERC20.sol index f9be8b689..ee310e9f2 100644 --- a/contracts/token/ERC20/ERC20.sol +++ b/contracts/token/ERC20/ERC20.sol @@ -229,25 +229,23 @@ contract ERC20 is Context, IERC20, IERC20Metadata { 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 { + // Overflow not possible: amount <= fromBalance <= totalSupply. _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 (to == address(0)) { + unchecked { + // Overflow not possible: amount <= totalSupply or amount <= fromBalance <= totalSupply. + _totalSupply -= amount; + } + } else { + unchecked { + // Overflow not possible: balance + amount is at most totalSupply, which we know fits into a uint256. _balances[to] += amount; } } diff --git a/contracts/token/ERC20/extensions/ERC20Capped.sol b/contracts/token/ERC20/extensions/ERC20Capped.sol index d80fb2a4c..b7ddd788c 100644 --- a/contracts/token/ERC20/extensions/ERC20Capped.sol +++ b/contracts/token/ERC20/extensions/ERC20Capped.sol @@ -32,7 +32,7 @@ abstract contract ERC20Capped is ERC20 { */ function _update(address from, address to, uint256 amount) internal virtual override { if (from == address(0)) { - require(ERC20.totalSupply() + amount <= cap(), "ERC20Capped: cap exceeded"); + require(totalSupply() + amount <= cap(), "ERC20Capped: cap exceeded"); } super._update(from, to, amount); diff --git a/contracts/token/ERC20/extensions/ERC20Snapshot.sol b/contracts/token/ERC20/extensions/ERC20Snapshot.sol index 3bc1a74ee..4eae931bc 100644 --- a/contracts/token/ERC20/extensions/ERC20Snapshot.sol +++ b/contracts/token/ERC20/extensions/ERC20Snapshot.sol @@ -122,18 +122,17 @@ abstract contract ERC20Snapshot is ERC20 { // for _mint, _burn, and _transfer operations. function _update(address from, address to, uint256 amount) internal virtual override { if (from == address(0)) { - // mint - _updateAccountSnapshot(to); - _updateTotalSupplySnapshot(); - } else if (to == address(0)) { - // burn - _updateAccountSnapshot(from); _updateTotalSupplySnapshot(); } else { - // transfer _updateAccountSnapshot(from); + } + + if (to == address(0)) { + _updateTotalSupplySnapshot(); + } else { _updateAccountSnapshot(to); } + super._update(from, to, amount); } diff --git a/test/token/ERC20/ERC20.test.js b/test/token/ERC20/ERC20.test.js index 46ee5ab63..cfe803d74 100644 --- a/test/token/ERC20/ERC20.test.js +++ b/test/token/ERC20/ERC20.test.js @@ -250,7 +250,7 @@ contract('ERC20', function (accounts) { describe('for a non zero account', function () { it('rejects burning more than balance', async function () { await expectRevert(this.token.burn( - initialHolder, initialSupply.addn(1)), 'ERC20: burn amount exceeds balance', + initialHolder, initialSupply.addn(1)), 'ERC20: transfer amount exceeds balance', ); }); @@ -287,6 +287,49 @@ contract('ERC20', function (accounts) { }); }); + describe('_update', function () { + const amount = new BN(1); + + it('from is the zero address', async function () { + const balanceBefore = await this.token.balanceOf(initialHolder); + const totalSupply = await this.token.totalSupply(); + + expectEvent( + await this.token.update(ZERO_ADDRESS, initialHolder, amount), + 'Transfer', + { from: ZERO_ADDRESS, to: initialHolder, value: amount }, + ); + expect(await this.token.totalSupply()).to.be.bignumber.equal(totalSupply.add(amount)); + expect(await this.token.balanceOf(initialHolder)).to.be.bignumber.equal(balanceBefore.add(amount)); + }); + + it('to is the zero address', async function () { + const balanceBefore = await this.token.balanceOf(initialHolder); + const totalSupply = await this.token.totalSupply(); + + expectEvent( + await this.token.update(initialHolder, ZERO_ADDRESS, amount), + 'Transfer', + { from: initialHolder, to: ZERO_ADDRESS, value: amount }, + ); + expect(await this.token.totalSupply()).to.be.bignumber.equal(totalSupply.sub(amount)); + expect(await this.token.balanceOf(initialHolder)).to.be.bignumber.equal(balanceBefore.sub(amount)); + }); + + it('from and to are the zero address', async function () { + const totalSupply = await this.token.totalSupply(); + + await this.token.update(ZERO_ADDRESS, ZERO_ADDRESS, amount); + + expect(await this.token.totalSupply()).to.be.bignumber.equal(totalSupply); + expectEvent( + await this.token.update(ZERO_ADDRESS, ZERO_ADDRESS, amount), + 'Transfer', + { from: ZERO_ADDRESS, to: ZERO_ADDRESS, value: amount }, + ); + }); + }); + describe('_transfer', function () { shouldBehaveLikeERC20Transfer('ERC20', initialHolder, recipient, initialSupply, function (from, to, amount) { return this.token.transferInternal(from, to, amount); diff --git a/test/token/ERC20/extensions/ERC20Burnable.behavior.js b/test/token/ERC20/extensions/ERC20Burnable.behavior.js index a931bf60d..80e71d1a0 100644 --- a/test/token/ERC20/extensions/ERC20Burnable.behavior.js +++ b/test/token/ERC20/extensions/ERC20Burnable.behavior.js @@ -38,7 +38,7 @@ function shouldBehaveLikeERC20Burnable (owner, initialBalance, [burner]) { it('reverts', async function () { await expectRevert(this.token.burn(amount, { from: owner }), - 'ERC20: burn amount exceeds balance', + 'ERC20: transfer amount exceeds balance', ); }); }); @@ -86,7 +86,7 @@ function shouldBehaveLikeERC20Burnable (owner, initialBalance, [burner]) { it('reverts', async function () { await this.token.approve(burner, amount, { from: owner }); await expectRevert(this.token.burnFrom(owner, amount, { from: burner }), - 'ERC20: burn amount exceeds balance', + 'ERC20: transfer amount exceeds balance', ); }); }); diff --git a/test/token/ERC20/extensions/ERC20FlashMint.test.js b/test/token/ERC20/extensions/ERC20FlashMint.test.js index 4354dd90c..ac242a6bf 100644 --- a/test/token/ERC20/extensions/ERC20FlashMint.test.js +++ b/test/token/ERC20/extensions/ERC20FlashMint.test.js @@ -82,7 +82,7 @@ contract('ERC20FlashMint', function (accounts) { const data = this.token.contract.methods.transfer(other, 10).encodeABI(); await expectRevert( this.token.flashLoan(receiver.address, this.token.address, loanAmount, data), - 'ERC20: burn amount exceeds balance', + 'ERC20: transfer amount exceeds balance', ); }); diff --git a/test/token/ERC20/extensions/ERC20Wrapper.test.js b/test/token/ERC20/extensions/ERC20Wrapper.test.js index ceb813e08..291b3615d 100644 --- a/test/token/ERC20/extensions/ERC20Wrapper.test.js +++ b/test/token/ERC20/extensions/ERC20Wrapper.test.js @@ -105,7 +105,7 @@ contract('ERC20', function (accounts) { it('missing balance', async function () { await expectRevert( this.token.withdrawTo(initialHolder, MAX_UINT256, { from: initialHolder }), - 'ERC20: burn amount exceeds balance', + 'ERC20: transfer amount exceeds balance', ); });