From 43093125034e806be061a61079955a8e7d7d8f16 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 8 Dec 2021 17:08:32 -0300 Subject: [PATCH 1/7] Reduce ERC20 allowance before triggering transfer --- CHANGELOG.md | 1 + contracts/token/ERC20/ERC20.sol | 4 +-- test/token/ERC20/ERC20.behavior.js | 40 ++++++++++++++++++++---------- 3 files changed, 30 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9cee4746924..5cad656014e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ * Preset contracts are now deprecated in favor of [Contracts Wizard](https://wizard.openzeppelin.com). ([#2986](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2986)) * `Governor`: add a relay function to help recover assets sent to a governor that is not its own executor (e.g. when using a timelock). ([#2926](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2926)) * `GovernorExtendedVoting`: add new module to ensure a minimum voting duration is available after the quorum is reached. ([#2973](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2973)) + * `ERC20`: reduce allowance before triggering transfer. ## 4.4.0 (2021-11-25) diff --git a/contracts/token/ERC20/ERC20.sol b/contracts/token/ERC20/ERC20.sol index a8c60e5956d..55d635cf3a4 100644 --- a/contracts/token/ERC20/ERC20.sol +++ b/contracts/token/ERC20/ERC20.sol @@ -152,14 +152,14 @@ contract ERC20 is Context, IERC20, IERC20Metadata { address recipient, uint256 amount ) public virtual override returns (bool) { - _transfer(sender, recipient, amount); - uint256 currentAllowance = _allowances[sender][_msgSender()]; require(currentAllowance >= amount, "ERC20: transfer amount exceeds allowance"); unchecked { _approve(sender, _msgSender(), currentAllowance - amount); } + _transfer(sender, recipient, amount); + return true; } diff --git a/test/token/ERC20/ERC20.behavior.js b/test/token/ERC20/ERC20.behavior.js index 5d23fa9eedb..e2da08b1706 100644 --- a/test/token/ERC20/ERC20.behavior.js +++ b/test/token/ERC20/ERC20.behavior.js @@ -40,7 +40,7 @@ function shouldBehaveLikeERC20 (errorPrefix, initialSupply, initialHolder, recip describe('when the recipient is not the zero address', function () { const to = anotherAccount; - describe('when the spender has enough approved balance', function () { + describe('when the spender has enough approved allowance', function () { beforeEach(async function () { await this.token.approve(spender, initialSupply, { from: initialHolder }); }); @@ -84,37 +84,50 @@ function shouldBehaveLikeERC20 (errorPrefix, initialSupply, initialHolder, recip }); describe('when the token owner does not have enough balance', function () { - const amount = initialSupply.addn(1); + const amount = initialSupply; + + beforeEach('reducing balance', async function () { + await this.token.transfer(to, 1, { from: tokenOwner }); + }); it('reverts', async function () { - await expectRevert(this.token.transferFrom( - tokenOwner, to, amount, { from: spender }), `${errorPrefix}: transfer amount exceeds balance`, + await expectRevert( + this.token.transferFrom(tokenOwner, to, amount, { from: spender }), + `${errorPrefix}: transfer amount exceeds balance`, ); }); }); }); - describe('when the spender does not have enough approved balance', function () { + describe('when the spender does not have enough approved allowance', function () { + const allowance = initialSupply.subn(1); + beforeEach(async function () { - await this.token.approve(spender, initialSupply.subn(1), { from: tokenOwner }); + await this.token.approve(spender, allowance, { from: tokenOwner }); }); describe('when the token owner has enough balance', function () { const amount = initialSupply; it('reverts', async function () { - await expectRevert(this.token.transferFrom( - tokenOwner, to, amount, { from: spender }), `${errorPrefix}: transfer amount exceeds allowance`, + await expectRevert( + this.token.transferFrom(tokenOwner, to, amount, { from: spender }), + `${errorPrefix}: transfer amount exceeds allowance`, ); }); }); describe('when the token owner does not have enough balance', function () { - const amount = initialSupply.addn(1); + const amount = allowance; + + beforeEach('reducing balance', async function () { + await this.token.transfer(to, initialSupply, { from: tokenOwner }); + }); it('reverts', async function () { - await expectRevert(this.token.transferFrom( - tokenOwner, to, amount, { from: spender }), `${errorPrefix}: transfer amount exceeds balance`, + await expectRevert( + this.token.transferFrom(tokenOwner, to, amount, { from: spender }), + `${errorPrefix}: transfer amount exceeds balance`, ); }); }); @@ -143,8 +156,9 @@ function shouldBehaveLikeERC20 (errorPrefix, initialSupply, initialHolder, recip const to = recipient; it('reverts', async function () { - await expectRevert(this.token.transferFrom( - tokenOwner, to, amount, { from: spender }), `${errorPrefix}: transfer from the zero address`, + await expectRevert( + this.token.transferFrom(tokenOwner, to, amount, { from: spender }), + `${errorPrefix}: approve from the zero address`, ); }); }); From abd682317d96ebee0dba0a6581cc960f67aaf661 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 8 Dec 2021 17:16:57 -0300 Subject: [PATCH 2/7] adapt ERC777 to reduce allowance before transfer --- contracts/token/ERC777/ERC777.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/token/ERC777/ERC777.sol b/contracts/token/ERC777/ERC777.sol index 8c53a3a5ffd..5eef111ac16 100644 --- a/contracts/token/ERC777/ERC777.sol +++ b/contracts/token/ERC777/ERC777.sol @@ -287,12 +287,12 @@ contract ERC777 is Context, IERC777, IERC20 { _callTokensToSend(spender, holder, recipient, amount, "", ""); - _move(spender, holder, recipient, amount, "", ""); - uint256 currentAllowance = _allowances[holder][spender]; require(currentAllowance >= amount, "ERC777: transfer amount exceeds allowance"); _approve(holder, spender, currentAllowance - amount); + _move(spender, holder, recipient, amount, "", ""); + _callTokensReceived(spender, holder, recipient, amount, "", "", false); return true; From af7425aa2a697a5537a9225117c93eee9d620aed Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 8 Dec 2021 17:26:00 -0300 Subject: [PATCH 3/7] fix test for ERC777 --- test/token/ERC20/ERC20.behavior.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/token/ERC20/ERC20.behavior.js b/test/token/ERC20/ERC20.behavior.js index e2da08b1706..e23b4259f97 100644 --- a/test/token/ERC20/ERC20.behavior.js +++ b/test/token/ERC20/ERC20.behavior.js @@ -158,7 +158,7 @@ function shouldBehaveLikeERC20 (errorPrefix, initialSupply, initialHolder, recip it('reverts', async function () { await expectRevert( this.token.transferFrom(tokenOwner, to, amount, { from: spender }), - `${errorPrefix}: approve from the zero address`, + `from the zero address`, ); }); }); From 1a7de84e767e35e671d147a6d5d55e72146d7c40 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 29 Dec 2021 13:21:18 -0300 Subject: [PATCH 4/7] use smaller number to reduce balance --- test/token/ERC20/ERC20.behavior.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/token/ERC20/ERC20.behavior.js b/test/token/ERC20/ERC20.behavior.js index e23b4259f97..090030ee1a7 100644 --- a/test/token/ERC20/ERC20.behavior.js +++ b/test/token/ERC20/ERC20.behavior.js @@ -121,7 +121,7 @@ function shouldBehaveLikeERC20 (errorPrefix, initialSupply, initialHolder, recip const amount = allowance; beforeEach('reducing balance', async function () { - await this.token.transfer(to, initialSupply, { from: tokenOwner }); + await this.token.transfer(to, 2, { from: tokenOwner }); }); it('reverts', async function () { From 8b3be05d307b95f247b7d814c8691d06d2b2e5dc Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 29 Dec 2021 13:22:00 -0300 Subject: [PATCH 5/7] simplify test description --- test/token/ERC20/ERC20.behavior.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/token/ERC20/ERC20.behavior.js b/test/token/ERC20/ERC20.behavior.js index 090030ee1a7..80697135ffd 100644 --- a/test/token/ERC20/ERC20.behavior.js +++ b/test/token/ERC20/ERC20.behavior.js @@ -40,7 +40,7 @@ function shouldBehaveLikeERC20 (errorPrefix, initialSupply, initialHolder, recip describe('when the recipient is not the zero address', function () { const to = anotherAccount; - describe('when the spender has enough approved allowance', function () { + describe('when the spender has enough allowance', function () { beforeEach(async function () { await this.token.approve(spender, initialSupply, { from: initialHolder }); }); @@ -99,7 +99,7 @@ function shouldBehaveLikeERC20 (errorPrefix, initialSupply, initialHolder, recip }); }); - describe('when the spender does not have enough approved allowance', function () { + describe('when the spender does not have enough allowance', function () { const allowance = initialSupply.subn(1); beforeEach(async function () { From 787cc5f1d286c1f279daf6fd73cb673217c07f07 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 29 Dec 2021 17:28:59 +0100 Subject: [PATCH 6/7] don't use deprecated expectEvents.inLogs --- test/token/ERC20/ERC20.behavior.js | 74 +++++++++++++----------------- test/token/ERC20/ERC20.test.js | 62 +++++++++++-------------- 2 files changed, 59 insertions(+), 77 deletions(-) diff --git a/test/token/ERC20/ERC20.behavior.js b/test/token/ERC20/ERC20.behavior.js index e23b4259f97..f1f211fd153 100644 --- a/test/token/ERC20/ERC20.behavior.js +++ b/test/token/ERC20/ERC20.behavior.js @@ -63,23 +63,19 @@ function shouldBehaveLikeERC20 (errorPrefix, initialSupply, initialHolder, recip }); it('emits a transfer event', async function () { - const { logs } = await this.token.transferFrom(tokenOwner, to, amount, { from: spender }); - - expectEvent.inLogs(logs, 'Transfer', { - from: tokenOwner, - to: to, - value: amount, - }); + expectEvent( + await this.token.transferFrom(tokenOwner, to, amount, { from: spender }), + 'Transfer', + { from: tokenOwner, to: to, value: amount }, + ); }); it('emits an approval event', async function () { - const { logs } = await this.token.transferFrom(tokenOwner, to, amount, { from: spender }); - - expectEvent.inLogs(logs, 'Approval', { - owner: tokenOwner, - spender: spender, - value: await this.token.allowance(tokenOwner, spender), - }); + expectEvent( + await this.token.transferFrom(tokenOwner, to, amount, { from: spender }), + 'Approval', + { owner: tokenOwner, spender: spender, value: await this.token.allowance(tokenOwner, spender) }, + ); }); }); @@ -158,7 +154,7 @@ function shouldBehaveLikeERC20 (errorPrefix, initialSupply, initialHolder, recip it('reverts', async function () { await expectRevert( this.token.transferFrom(tokenOwner, to, amount, { from: spender }), - `from the zero address`, + `${errorPrefix}: approve from the zero address`, ); }); }); @@ -197,13 +193,11 @@ function shouldBehaveLikeERC20Transfer (errorPrefix, from, to, balance, transfer }); it('emits a transfer event', async function () { - const { logs } = await transfer.call(this, from, to, amount); - - expectEvent.inLogs(logs, 'Transfer', { - from, - to, - value: amount, - }); + expectEvent( + await transfer.call(this, from, to, amount), + 'Transfer', + { from, to, value: amount }, + ); }); }); @@ -219,13 +213,11 @@ function shouldBehaveLikeERC20Transfer (errorPrefix, from, to, balance, transfer }); it('emits a transfer event', async function () { - const { logs } = await transfer.call(this, from, to, amount); - - expectEvent.inLogs(logs, 'Transfer', { - from, - to, - value: amount, - }); + expectEvent( + await transfer.call(this, from, to, amount), + 'Transfer', + { from, to, value: amount }, + ); }); }); }); @@ -245,13 +237,11 @@ function shouldBehaveLikeERC20Approve (errorPrefix, owner, spender, supply, appr const amount = supply; it('emits an approval event', async function () { - const { logs } = await approve.call(this, owner, spender, amount); - - expectEvent.inLogs(logs, 'Approval', { - owner: owner, - spender: spender, - value: amount, - }); + expectEvent( + await approve.call(this, owner, spender, amount), + 'Approval', + { owner: owner, spender: spender, value: amount }, + ); }); describe('when there was no approved amount before', function () { @@ -279,13 +269,11 @@ function shouldBehaveLikeERC20Approve (errorPrefix, owner, spender, supply, appr const amount = supply.addn(1); it('emits an approval event', async function () { - const { logs } = await approve.call(this, owner, spender, amount); - - expectEvent.inLogs(logs, 'Approval', { - owner: owner, - spender: spender, - value: amount, - }); + expectEvent( + await approve.call(this, owner, spender, amount), + 'Approval', + { owner: owner, spender: spender, value: amount }, + ); }); describe('when there was no approved amount before', function () { diff --git a/test/token/ERC20/ERC20.test.js b/test/token/ERC20/ERC20.test.js index c1004150411..992edf93b17 100644 --- a/test/token/ERC20/ERC20.test.js +++ b/test/token/ERC20/ERC20.test.js @@ -63,17 +63,15 @@ contract('ERC20', function (accounts) { const approvedAmount = amount; beforeEach(async function () { - ({ logs: this.logs } = await this.token.approve(spender, approvedAmount, { from: initialHolder })); + await this.token.approve(spender, approvedAmount, { from: initialHolder }); }); it('emits an approval event', async function () { - const { logs } = await this.token.decreaseAllowance(spender, approvedAmount, { from: initialHolder }); - - expectEvent.inLogs(logs, 'Approval', { - owner: initialHolder, - spender: spender, - value: new BN(0), - }); + expectEvent( + await this.token.decreaseAllowance(spender, approvedAmount, { from: initialHolder }), + 'Approval', + { owner: initialHolder, spender: spender, value: new BN(0) }, + ); }); it('decreases the spender allowance subtracting the requested amount', async function () { @@ -129,13 +127,11 @@ contract('ERC20', function (accounts) { describe('when the sender has enough balance', function () { it('emits an approval event', async function () { - const { logs } = await this.token.increaseAllowance(spender, amount, { from: initialHolder }); - - expectEvent.inLogs(logs, 'Approval', { - owner: initialHolder, - spender: spender, - value: amount, - }); + expectEvent( + await this.token.increaseAllowance(spender, amount, { from: initialHolder }), + 'Approval', + { owner: initialHolder, spender: spender, value: amount }, + ); }); describe('when there was no approved amount before', function () { @@ -163,13 +159,11 @@ contract('ERC20', function (accounts) { const amount = initialSupply.addn(1); it('emits an approval event', async function () { - const { logs } = await this.token.increaseAllowance(spender, amount, { from: initialHolder }); - - expectEvent.inLogs(logs, 'Approval', { - owner: initialHolder, - spender: spender, - value: amount, - }); + expectEvent( + await this.token.increaseAllowance(spender, amount, { from: initialHolder }), + 'Approval', + { owner: initialHolder, spender: spender, value: amount }, + ); }); describe('when there was no approved amount before', function () { @@ -215,8 +209,7 @@ contract('ERC20', function (accounts) { describe('for a non zero account', function () { beforeEach('minting', async function () { - const { logs } = await this.token.mint(recipient, amount); - this.logs = logs; + this.receipt = await this.token.mint(recipient, amount); }); it('increments totalSupply', async function () { @@ -229,10 +222,11 @@ contract('ERC20', function (accounts) { }); it('emits Transfer event', async function () { - const event = expectEvent.inLogs(this.logs, 'Transfer', { - from: ZERO_ADDRESS, - to: recipient, - }); + const event = expectEvent( + this.receipt, + 'Transfer', + { from: ZERO_ADDRESS, to: recipient }, + ); expect(event.args.value).to.be.bignumber.equal(amount); }); @@ -255,8 +249,7 @@ contract('ERC20', function (accounts) { const describeBurn = function (description, amount) { describe(description, function () { beforeEach('burning', async function () { - const { logs } = await this.token.burn(initialHolder, amount); - this.logs = logs; + this.receipt = await this.token.burn(initialHolder, amount); }); it('decrements totalSupply', async function () { @@ -270,10 +263,11 @@ contract('ERC20', function (accounts) { }); it('emits Transfer event', async function () { - const event = expectEvent.inLogs(this.logs, 'Transfer', { - from: initialHolder, - to: ZERO_ADDRESS, - }); + const event = expectEvent( + this.receipt, + 'Transfer', + { from: initialHolder, to: ZERO_ADDRESS }, + ); expect(event.args.value).to.be.bignumber.equal(amount); }); From 798992e2d5004f937a26b2d6052808120926aa83 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 29 Dec 2021 17:38:58 +0100 Subject: [PATCH 7/7] fix test --- test/token/ERC20/ERC20.behavior.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/token/ERC20/ERC20.behavior.js b/test/token/ERC20/ERC20.behavior.js index 57246ac5aa9..55bcc36084f 100644 --- a/test/token/ERC20/ERC20.behavior.js +++ b/test/token/ERC20/ERC20.behavior.js @@ -154,7 +154,7 @@ function shouldBehaveLikeERC20 (errorPrefix, initialSupply, initialHolder, recip it('reverts', async function () { await expectRevert( this.token.transferFrom(tokenOwner, to, amount, { from: spender }), - `${errorPrefix}: approve from the zero address`, + 'from the zero address', ); }); });