Reduce ERC20 allowance before triggering transfer (#3056)

* Reduce ERC20 allowance before triggering transfer

* adapt ERC777 to reduce allowance before transfer

* fix test for ERC777

* use smaller number to reduce balance

* simplify test description

* don't use deprecated expectEvents.inLogs

* fix test

Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>

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)
 

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;
     }
 

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;

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 () {

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);
           });