From abbbaefaefa7d0add58f12ce452306898c45d98d Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 22 Jun 2023 18:41:56 +0200 Subject: [PATCH] Do not emit Approval event when calling transferFrom (#4370) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García Co-authored-by: Francisco --- contracts/mocks/token/ERC20ApprovalMock.sol | 2 +- contracts/token/ERC20/ERC20.sol | 16 +-- test/token/ERC20/ERC20.behavior.js | 4 +- test/token/ERC20/ERC20.test.js | 140 ++++++++++---------- 4 files changed, 82 insertions(+), 80 deletions(-) diff --git a/contracts/mocks/token/ERC20ApprovalMock.sol b/contracts/mocks/token/ERC20ApprovalMock.sol index 3caa7d0d2d0..da8f51e1ecc 100644 --- a/contracts/mocks/token/ERC20ApprovalMock.sol +++ b/contracts/mocks/token/ERC20ApprovalMock.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.19; -import {ERC20} from "../../token/ERC20/ERC20.sol"; +import "../../token/ERC20/ERC20.sol"; abstract contract ERC20ApprovalMock is ERC20 { function _approve(address owner, address spender, uint256 amount, bool) internal virtual override { diff --git a/contracts/token/ERC20/ERC20.sol b/contracts/token/ERC20/ERC20.sol index abaf258c815..536d5d8d1b0 100644 --- a/contracts/token/ERC20/ERC20.sol +++ b/contracts/token/ERC20/ERC20.sol @@ -314,8 +314,8 @@ abstract contract ERC20 is Context, IERC20, IERC20Metadata, IERC20Errors { * - `owner` cannot be the zero address. * - `spender` cannot be the zero address. */ - function _approve(address owner, address spender, uint256 value) internal virtual { - _approve(owner, spender, value, true); + function _approve(address owner, address spender, uint256 amount) internal virtual { + _approve(owner, spender, amount, true); } /** @@ -328,23 +328,23 @@ abstract contract ERC20 is Context, IERC20, IERC20Metadata, IERC20Errors { * Anyone who wishes to continue emitting `Approval` events on the`transferFrom` operation can force the flag to true * using the following override: * ``` - * function _approve(address owner, address spender, uint256 value, bool) internal virtual override { - * super._approve(owner, spender, value, true); + * function _approve(address owner, address spender, uint256 amount, bool) internal virtual override { + * super._approve(owner, spender, amount, true); * } * ``` * * Requirements are the same as {_approve}. */ - function _approve(address owner, address spender, uint256 value, bool emitEvent) internal virtual { + function _approve(address owner, address spender, uint256 amount, bool emitEvent) internal virtual { if (owner == address(0)) { revert ERC20InvalidApprover(address(0)); } if (spender == address(0)) { revert ERC20InvalidSpender(address(0)); } - _allowances[owner][spender] = value; + _allowances[owner][spender] = amount; if (emitEvent) { - emit Approval(owner, spender, value); + emit Approval(owner, spender, amount); } } @@ -363,7 +363,7 @@ abstract contract ERC20 is Context, IERC20, IERC20Metadata, IERC20Errors { revert ERC20InsufficientAllowance(spender, currentAllowance, value); } unchecked { - _approve(owner, spender, currentAllowance - value, false); + _approve(owner, spender, currentAllowance - amount, false); } } } diff --git a/test/token/ERC20/ERC20.behavior.js b/test/token/ERC20/ERC20.behavior.js index b6f8617b247..370ba533cd0 100644 --- a/test/token/ERC20/ERC20.behavior.js +++ b/test/token/ERC20/ERC20.behavior.js @@ -75,7 +75,7 @@ function shouldBehaveLikeERC20(initialSupply, accounts, opts = {}) { if (forcedApproval) { it('emits an approval event', async function () { - expectEvent(await this.token.transferFrom(tokenOwner, to, value, { from: spender }), 'Approval', { + expectEvent(await this.token.transferFrom(tokenOwner, to, amount, { from: spender }), 'Approval', { owner: tokenOwner, spender: spender, value: await this.token.allowance(tokenOwner, spender), @@ -84,7 +84,7 @@ function shouldBehaveLikeERC20(initialSupply, accounts, opts = {}) { } else { it('does not emit an approval event', async function () { expectEvent.notEmitted( - await this.token.transferFrom(tokenOwner, to, value, { from: spender }), + await this.token.transferFrom(tokenOwner, to, amount, { from: spender }), 'Approval', ); }); diff --git a/test/token/ERC20/ERC20.test.js b/test/token/ERC20/ERC20.test.js index a63df523958..6f3d3c558e1 100644 --- a/test/token/ERC20/ERC20.test.js +++ b/test/token/ERC20/ERC20.test.js @@ -46,145 +46,145 @@ contract('ERC20', function (accounts) { describe('when the spender is not the zero address', function () { const spender = recipient; - function shouldDecreaseApproval(value) { - describe('when there was no approved value before', function () { + function shouldDecreaseApproval(amount) { + describe('when there was no approved amount before', function () { it('reverts', async function () { const allowance = await this.token.allowance(initialHolder, spender); await expectRevertCustomError( - this.token.decreaseAllowance(spender, value, { from: initialHolder }), + this.token.decreaseAllowance(spender, amount, { from: initialHolder }), 'ERC20FailedDecreaseAllowance', - [spender, allowance, value], + [spender, allowance, amount], ); }); }); - describe('when the spender had an approved value', function () { - const approvedValue = value; + describe('when the spender had an approved amount', function () { + const approvedAmount = amount; beforeEach(async function () { - await this.token.approve(spender, approvedValue, { from: initialHolder }); + await this.token.approve(spender, approvedAmount, { from: initialHolder }); }); it('emits an approval event', async function () { expectEvent( - await this.token.decreaseAllowance(spender, approvedValue, { from: initialHolder }), + 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 value', async function () { - await this.token.decreaseAllowance(spender, approvedValue.subn(1), { from: initialHolder }); + it('decreases the spender allowance subtracting the requested amount', async function () { + await this.token.decreaseAllowance(spender, approvedAmount.subn(1), { from: initialHolder }); expect(await this.token.allowance(initialHolder, spender)).to.be.bignumber.equal('1'); }); it('sets the allowance to zero when all allowance is removed', async function () { - await this.token.decreaseAllowance(spender, approvedValue, { from: initialHolder }); + await this.token.decreaseAllowance(spender, approvedAmount, { from: initialHolder }); expect(await this.token.allowance(initialHolder, spender)).to.be.bignumber.equal('0'); }); it('reverts when more than the full allowance is removed', async function () { await expectRevertCustomError( - this.token.decreaseAllowance(spender, approvedValue.addn(1), { from: initialHolder }), + this.token.decreaseAllowance(spender, approvedAmount.addn(1), { from: initialHolder }), 'ERC20FailedDecreaseAllowance', - [spender, approvedValue, approvedValue.addn(1)], + [spender, approvedAmount, approvedAmount.addn(1)], ); }); }); } describe('when the sender has enough balance', function () { - const value = initialSupply; + const amount = initialSupply; - shouldDecreaseApproval(value); + shouldDecreaseApproval(amount); }); describe('when the sender does not have enough balance', function () { - const value = initialSupply.addn(1); + const amount = initialSupply.addn(1); - shouldDecreaseApproval(value); + shouldDecreaseApproval(amount); }); }); describe('when the spender is the zero address', function () { - const value = initialSupply; + const amount = initialSupply; const spender = ZERO_ADDRESS; it('reverts', async function () { await expectRevertCustomError( this.token.decreaseAllowance(spender, value, { from: initialHolder }), 'ERC20FailedDecreaseAllowance', - [spender, 0, value], + [spender, 0, amount], ); }); }); }); describe('increase allowance', function () { - const value = initialSupply; + const amount = initialSupply; describe('when the spender is not the zero address', function () { const spender = recipient; describe('when the sender has enough balance', function () { it('emits an approval event', async function () { - expectEvent(await this.token.increaseAllowance(spender, value, { from: initialHolder }), 'Approval', { + expectEvent(await this.token.increaseAllowance(spender, amount, { from: initialHolder }), 'Approval', { owner: initialHolder, spender: spender, - value: value, + value: amount, }); }); - describe('when there was no approved value before', function () { - it('approves the requested value', async function () { - await this.token.increaseAllowance(spender, value, { from: initialHolder }); + describe('when there was no approved amount before', function () { + it('approves the requested amount', async function () { + await this.token.increaseAllowance(spender, amount, { from: initialHolder }); - expect(await this.token.allowance(initialHolder, spender)).to.be.bignumber.equal(value); + expect(await this.token.allowance(initialHolder, spender)).to.be.bignumber.equal(amount); }); }); - describe('when the spender had an approved value', function () { + describe('when the spender had an approved amount', function () { beforeEach(async function () { await this.token.approve(spender, new BN(1), { from: initialHolder }); }); - it('increases the spender allowance adding the requested value', async function () { - await this.token.increaseAllowance(spender, value, { from: initialHolder }); + it('increases the spender allowance adding the requested amount', async function () { + await this.token.increaseAllowance(spender, amount, { from: initialHolder }); - expect(await this.token.allowance(initialHolder, spender)).to.be.bignumber.equal(value.addn(1)); + expect(await this.token.allowance(initialHolder, spender)).to.be.bignumber.equal(amount.addn(1)); }); }); }); describe('when the sender does not have enough balance', function () { - const value = initialSupply.addn(1); + const amount = initialSupply.addn(1); it('emits an approval event', async function () { - expectEvent(await this.token.increaseAllowance(spender, value, { from: initialHolder }), 'Approval', { + expectEvent(await this.token.increaseAllowance(spender, amount, { from: initialHolder }), 'Approval', { owner: initialHolder, spender: spender, - value: value, + value: amount, }); }); - describe('when there was no approved value before', function () { - it('approves the requested value', async function () { - await this.token.increaseAllowance(spender, value, { from: initialHolder }); + describe('when there was no approved amount before', function () { + it('approves the requested amount', async function () { + await this.token.increaseAllowance(spender, amount, { from: initialHolder }); - expect(await this.token.allowance(initialHolder, spender)).to.be.bignumber.equal(value); + expect(await this.token.allowance(initialHolder, spender)).to.be.bignumber.equal(amount); }); }); - describe('when the spender had an approved value', function () { + describe('when the spender had an approved amount', function () { beforeEach(async function () { await this.token.approve(spender, new BN(1), { from: initialHolder }); }); - it('increases the spender allowance adding the requested value', async function () { - await this.token.increaseAllowance(spender, value, { from: initialHolder }); + it('increases the spender allowance adding the requested amount', async function () { + await this.token.increaseAllowance(spender, amount, { from: initialHolder }); - expect(await this.token.allowance(initialHolder, spender)).to.be.bignumber.equal(value.addn(1)); + expect(await this.token.allowance(initialHolder, spender)).to.be.bignumber.equal(amount.addn(1)); }); }); }); @@ -195,7 +195,7 @@ contract('ERC20', function (accounts) { it('reverts', async function () { await expectRevertCustomError( - this.token.increaseAllowance(spender, value, { from: initialHolder }), + this.token.increaseAllowance(spender, amount, { from: initialHolder }), 'ERC20InvalidSpender', [ZERO_ADDRESS], ); @@ -204,9 +204,11 @@ contract('ERC20', function (accounts) { }); describe('_mint', function () { - const value = new BN(50); + const amount = new BN(50); it('rejects a null account', async function () { - await expectRevertCustomError(this.token.$_mint(ZERO_ADDRESS, value), 'ERC20InvalidReceiver', [ZERO_ADDRESS]); + await expectRevertCustomError(this.token.$_mint(ZERO_ADDRESS, amount), 'ERC20InvalidReceiver', [ + ZERO_ADDRESS, + ]); }); it('rejects overflow', async function () { @@ -219,16 +221,16 @@ contract('ERC20', function (accounts) { describe('for a non zero account', function () { beforeEach('minting', async function () { - this.receipt = await this.token.$_mint(recipient, value); + this.receipt = await this.token.$_mint(recipient, amount); }); it('increments totalSupply', async function () { - const expectedSupply = initialSupply.add(value); + const expectedSupply = initialSupply.add(amount); expect(await this.token.totalSupply()).to.be.bignumber.equal(expectedSupply); }); it('increments recipient balance', async function () { - expect(await this.token.balanceOf(recipient)).to.be.bignumber.equal(value); + expect(await this.token.balanceOf(recipient)).to.be.bignumber.equal(amount); }); it('emits Transfer event', async function () { @@ -255,81 +257,81 @@ contract('ERC20', function (accounts) { ); }); - const describeBurn = function (description, value) { + const describeBurn = function (description, amount) { describe(description, function () { beforeEach('burning', async function () { - this.receipt = await this.token.$_burn(initialHolder, value); + this.receipt = await this.token.$_burn(initialHolder, amount); }); it('decrements totalSupply', async function () { - const expectedSupply = initialSupply.sub(value); + const expectedSupply = initialSupply.sub(amount); expect(await this.token.totalSupply()).to.be.bignumber.equal(expectedSupply); }); it('decrements initialHolder balance', async function () { - const expectedBalance = initialSupply.sub(value); + const expectedBalance = initialSupply.sub(amount); expect(await this.token.balanceOf(initialHolder)).to.be.bignumber.equal(expectedBalance); }); it('emits Transfer event', async function () { const event = expectEvent(this.receipt, 'Transfer', { from: initialHolder, to: ZERO_ADDRESS }); - expect(event.args.value).to.be.bignumber.equal(value); + expect(event.args.value).to.be.bignumber.equal(amount); }); }); }; describeBurn('for entire balance', initialSupply); - describeBurn('for less value than balance', initialSupply.subn(1)); + describeBurn('for less amount than balance', initialSupply.subn(1)); }); }); describe('_update', function () { - const value = new BN(1); + 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, value), 'Transfer', { + expectEvent(await this.token.$_update(ZERO_ADDRESS, initialHolder, amount), 'Transfer', { from: ZERO_ADDRESS, to: initialHolder, - value: value, + value: amount, }); - expect(await this.token.totalSupply()).to.be.bignumber.equal(totalSupply.add(value)); - expect(await this.token.balanceOf(initialHolder)).to.be.bignumber.equal(balanceBefore.add(value)); + 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, value), 'Transfer', { + expectEvent(await this.token.$_update(initialHolder, ZERO_ADDRESS, amount), 'Transfer', { from: initialHolder, to: ZERO_ADDRESS, - value: value, + value: amount, }); - expect(await this.token.totalSupply()).to.be.bignumber.equal(totalSupply.sub(value)); - expect(await this.token.balanceOf(initialHolder)).to.be.bignumber.equal(balanceBefore.sub(value)); + 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, value); + 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, value), 'Transfer', { + expectEvent(await this.token.$_update(ZERO_ADDRESS, ZERO_ADDRESS, amount), 'Transfer', { from: ZERO_ADDRESS, to: ZERO_ADDRESS, - value: value, + value: amount, }); }); }); describe('_transfer', function () { - shouldBehaveLikeERC20Transfer(initialHolder, recipient, initialSupply, function (from, to, value) { - return this.token.$_transfer(from, to, value); + shouldBehaveLikeERC20Transfer(initialHolder, recipient, initialSupply, function (from, to, amount) { + return this.token.$_transfer(from, to, amount); }); describe('when the sender is the zero address', function () { @@ -344,8 +346,8 @@ contract('ERC20', function (accounts) { }); describe('_approve', function () { - shouldBehaveLikeERC20Approve(initialHolder, recipient, initialSupply, function (owner, spender, value) { - return this.token.$_approve(owner, spender, value); + shouldBehaveLikeERC20Approve(initialHolder, recipient, initialSupply, function (owner, spender, amount) { + return this.token.$_approve(owner, spender, amount); }); describe('when the owner is the zero address', function () {