diff --git a/CHANGELOG.md b/CHANGELOG.md index b531bfbc6..be54f9160 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ * `ERC721Votes`: Added an extension of ERC721 enabled with vote tracking and delegation. ([#2944](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2944)) * `ERC2771Context`: use immutable storage to store the forwarder address, no longer an issue since Solidity >=0.8.8 allows reading immutable variables in the constructor. ([#2917](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2917)) * `Base64`: add a library to parse bytes into base64 strings using `encode(bytes memory)` function, and provide examples to show how to use to build URL-safe `tokenURIs` ([#2884](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#2884)) + * `ERC20`: reduce allowance before triggering transfer. ## 4.4.1 (2021-12-14) diff --git a/contracts/token/ERC20/ERC20.sol b/contracts/token/ERC20/ERC20.sol index 99e455f8e..054159e3d 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/contracts/token/ERC777/ERC777.sol b/contracts/token/ERC777/ERC777.sol index b3c0ca495..f89e1e024 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; diff --git a/test/token/ERC20/ERC20.behavior.js b/test/token/ERC20/ERC20.behavior.js index 5d23fa9ee..55bcc3608 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 allowance', function () { beforeEach(async function () { await this.token.approve(spender, initialSupply, { from: initialHolder }); }); @@ -63,58 +63,67 @@ 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) }, + ); }); }); 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 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, 2, { 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 +152,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 }), + 'from the zero address', ); }); }); @@ -183,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 }, + ); }); }); @@ -205,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 }, + ); }); }); }); @@ -231,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 () { @@ -265,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 c10041504..992edf93b 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); });