From aa4cd0b76463d80b691effe11222aa45314f8227 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Wed, 15 May 2019 13:33:21 -0300 Subject: [PATCH 01/10] Add requirement of non-zero from to ERC20 transfer. --- contracts/token/ERC20/ERC20.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/contracts/token/ERC20/ERC20.sol b/contracts/token/ERC20/ERC20.sol index 30039d274bd..7c7d36fec10 100644 --- a/contracts/token/ERC20/ERC20.sol +++ b/contracts/token/ERC20/ERC20.sol @@ -125,6 +125,7 @@ contract ERC20 is IERC20 { * @param value The amount to be transferred. */ function _transfer(address from, address to, uint256 value) internal { + require(from != address(0), "ERC20: transfer from the zero address"); require(to != address(0), "ERC20: transfer to the zero address"); _balances[from] = _balances[from].sub(value); From 8a747938c6e1d0294709fd4a67c0108cffca69bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Wed, 15 May 2019 13:34:43 -0300 Subject: [PATCH 02/10] Add test for transferFrom zero address to behavior. --- test/token/ERC20/ERC20.behavior.js | 139 ++++++++++++++++------------- 1 file changed, 77 insertions(+), 62 deletions(-) diff --git a/test/token/ERC20/ERC20.behavior.js b/test/token/ERC20/ERC20.behavior.js index 2536df005ec..95fb61c8c0f 100644 --- a/test/token/ERC20/ERC20.behavior.js +++ b/test/token/ERC20/ERC20.behavior.js @@ -73,101 +73,116 @@ function shouldBehaveLikeERC20 (errorPrefix, initialSupply, initialHolder, recip describe('transfer from', function () { const spender = recipient; - describe('when the recipient is not the zero address', function () { - const to = anotherAccount; + describe('when the token owner is not the zero address', function () { + const tokenOwner = initialHolder; - describe('when the spender has enough approved balance', function () { - beforeEach(async function () { - await this.token.approve(spender, initialSupply, { from: initialHolder }); - }); + describe('when the recipient is not the zero address', function () { + const to = anotherAccount; + + describe('when the spender has enough approved balance', function () { + beforeEach(async function () { + await this.token.approve(spender, initialSupply, { from: initialHolder }); + }); - describe('when the initial holder has enough balance', function () { - const amount = initialSupply; + describe('when the token owner has enough balance', function () { + const amount = initialSupply; - it('transfers the requested amount', async function () { - await this.token.transferFrom(initialHolder, to, amount, { from: spender }); + it('transfers the requested amount', async function () { + await this.token.transferFrom(tokenOwner, to, amount, { from: spender }); - (await this.token.balanceOf(initialHolder)).should.be.bignumber.equal('0'); + (await this.token.balanceOf(tokenOwner)).should.be.bignumber.equal('0'); - (await this.token.balanceOf(to)).should.be.bignumber.equal(amount); - }); + (await this.token.balanceOf(to)).should.be.bignumber.equal(amount); + }); - it('decreases the spender allowance', async function () { - await this.token.transferFrom(initialHolder, to, amount, { from: spender }); + it('decreases the spender allowance', async function () { + await this.token.transferFrom(tokenOwner, to, amount, { from: spender }); - (await this.token.allowance(initialHolder, spender)).should.be.bignumber.equal('0'); - }); + (await this.token.allowance(tokenOwner, spender)).should.be.bignumber.equal('0'); + }); - it('emits a transfer event', async function () { - const { logs } = await this.token.transferFrom(initialHolder, to, amount, { from: spender }); + it('emits a transfer event', async function () { + const { logs } = await this.token.transferFrom(tokenOwner, to, amount, { from: spender }); - expectEvent.inLogs(logs, 'Transfer', { - from: initialHolder, - to: to, - value: amount, + expectEvent.inLogs(logs, 'Transfer', { + from: tokenOwner, + to: to, + value: amount, + }); }); - }); - it('emits an approval event', async function () { - const { logs } = await this.token.transferFrom(initialHolder, to, amount, { from: spender }); + it('emits an approval event', async function () { + const { logs } = await this.token.transferFrom(tokenOwner, to, amount, { from: spender }); - expectEvent.inLogs(logs, 'Approval', { - owner: initialHolder, - spender: spender, - value: await this.token.allowance(initialHolder, spender), + expectEvent.inLogs(logs, 'Approval', { + owner: tokenOwner, + spender: spender, + value: await this.token.allowance(tokenOwner, spender), + }); }); }); - }); - describe('when the initial holder does not have enough balance', function () { - const amount = initialSupply.addn(1); + describe('when the token owner does not have enough balance', function () { + const amount = initialSupply.addn(1); - it('reverts', async function () { - await shouldFail.reverting.withMessage(this.token.transferFrom( - initialHolder, to, amount, { from: spender }), 'SafeMath: subtraction overflow' - ); + it('reverts', async function () { + await shouldFail.reverting.withMessage(this.token.transferFrom( + tokenOwner, to, amount, { from: spender }), 'SafeMath: subtraction overflow' + ); + }); }); }); - }); - describe('when the spender does not have enough approved balance', function () { - beforeEach(async function () { - await this.token.approve(spender, initialSupply.subn(1), { from: initialHolder }); - }); + describe('when the spender does not have enough approved balance', function () { + beforeEach(async function () { + await this.token.approve(spender, initialSupply.subn(1), { from: tokenOwner }); + }); - describe('when the initial holder has enough balance', function () { - const amount = initialSupply; + describe('when the token owner has enough balance', function () { + const amount = initialSupply; - it('reverts', async function () { - await shouldFail.reverting.withMessage(this.token.transferFrom( - initialHolder, to, amount, { from: spender }), 'SafeMath: subtraction overflow' - ); + it('reverts', async function () { + await shouldFail.reverting.withMessage(this.token.transferFrom( + tokenOwner, to, amount, { from: spender }), 'SafeMath: subtraction overflow' + ); + }); }); - }); - describe('when the initial holder does not have enough balance', function () { - const amount = initialSupply.addn(1); + describe('when the token owner does not have enough balance', function () { + const amount = initialSupply.addn(1); - it('reverts', async function () { - await shouldFail.reverting.withMessage(this.token.transferFrom( - initialHolder, to, amount, { from: spender }), 'SafeMath: subtraction overflow' - ); + it('reverts', async function () { + await shouldFail.reverting.withMessage(this.token.transferFrom( + tokenOwner, to, amount, { from: spender }), 'SafeMath: subtraction overflow' + ); + }); }); }); }); - }); - describe('when the recipient is the zero address', function () { - const amount = initialSupply; - const to = ZERO_ADDRESS; + describe('when the recipient is the zero address', function () { + const amount = initialSupply; + const to = ZERO_ADDRESS; + + beforeEach(async function () { + await this.token.approve(spender, amount, { from: tokenOwner }); + }); - beforeEach(async function () { - await this.token.approve(spender, amount, { from: initialHolder }); + it('reverts', async function () { + await shouldFail.reverting.withMessage(this.token.transferFrom( + tokenOwner, to, amount, { from: spender }), `${errorPrefix}: transfer to the zero address` + ); + }); }); + }); + + describe('when the token owner is the zero address', function () { + const amount = 0; + const to = recipient; it('reverts', async function () { await shouldFail.reverting.withMessage(this.token.transferFrom( - initialHolder, to, amount, { from: spender }), `${errorPrefix}: transfer to the zero address` + ZERO_ADDRESS, to, amount, { from: to }), `${errorPrefix}: transfer from the zero address` ); }); }); From 3940149e8d8208873ced1f4aab34217b9a1502ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Wed, 15 May 2019 13:44:57 -0300 Subject: [PATCH 03/10] Create ERC20.transfer behavior. --- test/token/ERC20/ERC20.behavior.js | 94 ++++++++++++++++-------------- 1 file changed, 49 insertions(+), 45 deletions(-) diff --git a/test/token/ERC20/ERC20.behavior.js b/test/token/ERC20/ERC20.behavior.js index 95fb61c8c0f..d22244c91e6 100644 --- a/test/token/ERC20/ERC20.behavior.js +++ b/test/token/ERC20/ERC20.behavior.js @@ -23,51 +23,11 @@ function shouldBehaveLikeERC20 (errorPrefix, initialSupply, initialHolder, recip }); describe('transfer', function () { - describe('when the recipient is not the zero address', function () { - const to = recipient; - - describe('when the sender does not have enough balance', function () { - const amount = initialSupply.addn(1); - - it('reverts', async function () { - await shouldFail.reverting.withMessage(this.token.transfer(to, amount, { from: initialHolder }), - 'SafeMath: subtraction overflow' - ); - }); - }); - - describe('when the sender has enough balance', function () { - const amount = initialSupply; - - it('transfers the requested amount', async function () { - await this.token.transfer(to, amount, { from: initialHolder }); - - (await this.token.balanceOf(initialHolder)).should.be.bignumber.equal('0'); - - (await this.token.balanceOf(to)).should.be.bignumber.equal(amount); - }); - - it('emits a transfer event', async function () { - const { logs } = await this.token.transfer(to, amount, { from: initialHolder }); - - expectEvent.inLogs(logs, 'Transfer', { - from: initialHolder, - to: to, - value: amount, - }); - }); - }); - }); - - describe('when the recipient is the zero address', function () { - const to = ZERO_ADDRESS; - - it('reverts', async function () { - await shouldFail.reverting.withMessage(this.token.transfer(to, initialSupply, { from: initialHolder }), - `${errorPrefix}: transfer to the zero address` - ); - }); - }); + shouldBehaveLikeERC20Transfer('ERC20', initialHolder, recipient, initialSupply, + function(from, to, value) { + return this.token.transfer(to, value, { from }); + } + ); }); describe('transfer from', function () { @@ -197,6 +157,50 @@ function shouldBehaveLikeERC20 (errorPrefix, initialSupply, initialHolder, recip }); } +function shouldBehaveLikeERC20Transfer (errorPrefix, from, to, balance, transfer) { + describe('when the recipient is not the zero address', function () { + describe('when the sender does not have enough balance', function () { + const amount = balance.addn(1); + + it('reverts', async function () { + await shouldFail.reverting.withMessage(transfer.call(this, from, to, amount), + 'SafeMath: subtraction overflow' + ); + }); + }); + + describe('when the sender transfers all balance', function () { + const amount = balance; + + it('transfers the requested amount', async function () { + await transfer.call(this, from, to, amount); + + (await this.token.balanceOf(from)).should.be.bignumber.equal('0'); + + (await this.token.balanceOf(to)).should.be.bignumber.equal(amount); + }); + + it('emits a transfer event', async function () { + const { logs } = await transfer.call(this, from, to, amount); + + expectEvent.inLogs(logs, 'Transfer', { + from, + to, + value: amount, + }); + }); + }); + }); + + describe('when the recipient is the zero address', function () { + it('reverts', async function () { + await shouldFail.reverting.withMessage(transfer.call(this, from, ZERO_ADDRESS, balance), + `${errorPrefix}: transfer to the zero address` + ); + }); + }); +} + function shouldBehaveLikeERC20Approve (errorPrefix, owner, spender, supply, approve) { describe('when the spender is not the zero address', function () { describe('when the sender has enough balance', function () { From cf011e14ec3e1a9f400fa19ab59160f1a9e4bf90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Wed, 15 May 2019 14:58:48 -0300 Subject: [PATCH 04/10] Add tests for _transfer. --- contracts/mocks/ERC20Mock.sol | 4 ++++ test/token/ERC20/ERC20.behavior.js | 23 +++++++++++++++++++++++ test/token/ERC20/ERC20.test.js | 29 +++++++++++++++++++++++++++++ 3 files changed, 56 insertions(+) diff --git a/contracts/mocks/ERC20Mock.sol b/contracts/mocks/ERC20Mock.sol index b5ed2fc7e9d..0fdfa695378 100644 --- a/contracts/mocks/ERC20Mock.sol +++ b/contracts/mocks/ERC20Mock.sol @@ -20,6 +20,10 @@ contract ERC20Mock is ERC20 { _burnFrom(account, amount); } + function transferInternal(address from, address to, uint256 value) public { + _transfer(from, to, value); + } + function approveInternal(address owner, address spender, uint256 value) public { _approve(owner, spender, value); } diff --git a/test/token/ERC20/ERC20.behavior.js b/test/token/ERC20/ERC20.behavior.js index d22244c91e6..be4985eae74 100644 --- a/test/token/ERC20/ERC20.behavior.js +++ b/test/token/ERC20/ERC20.behavior.js @@ -190,6 +190,28 @@ function shouldBehaveLikeERC20Transfer (errorPrefix, from, to, balance, transfer }); }); }); + + describe('when the sender transfers zero tokens', function () { + const amount = new BN('0'); + + it('transfers the requested amount', async function () { + await transfer.call(this, from, to, amount); + + (await this.token.balanceOf(from)).should.be.bignumber.equal(balance); + + (await this.token.balanceOf(to)).should.be.bignumber.equal('0'); + }); + + it('emits a transfer event', async function () { + const { logs } = await transfer.call(this, from, to, amount); + + expectEvent.inLogs(logs, 'Transfer', { + from, + to, + value: amount, + }); + }); + }); }); describe('when the recipient is the zero address', function () { @@ -283,5 +305,6 @@ function shouldBehaveLikeERC20Approve (errorPrefix, owner, spender, supply, appr module.exports = { shouldBehaveLikeERC20, + shouldBehaveLikeERC20Transfer, shouldBehaveLikeERC20Approve, }; diff --git a/test/token/ERC20/ERC20.test.js b/test/token/ERC20/ERC20.test.js index be772655908..41f78332397 100644 --- a/test/token/ERC20/ERC20.test.js +++ b/test/token/ERC20/ERC20.test.js @@ -3,6 +3,7 @@ const { ZERO_ADDRESS } = constants; const { shouldBehaveLikeERC20, + shouldBehaveLikeERC20Transfer, shouldBehaveLikeERC20Approve, } = require('./ERC20.behavior'); @@ -330,6 +331,34 @@ contract('ERC20', function ([_, initialHolder, recipient, anotherAccount]) { }); }); + describe('_transfer', function () { + shouldBehaveLikeERC20Approve('ERC20', initialHolder, recipient, initialSupply, function (owner, spender, amount) { + return this.token.approveInternal(owner, spender, amount); + }); + + describe('when the owner is the zero address', function () { + it('reverts', async function () { + await shouldFail.reverting.withMessage(this.token.approveInternal(ZERO_ADDRESS, recipient, initialSupply), + 'ERC20: approve from the zero address' + ); + }); + }); + }); + + describe('_transfer', function () { + shouldBehaveLikeERC20Transfer('ERC20', initialHolder, recipient, initialSupply, function (from, to, amount) { + return this.token.transferInternal(from, to, amount); + }); + + describe('when the sender is the zero address', function () { + it('reverts', async function () { + await shouldFail.reverting.withMessage(this.token.transferInternal(ZERO_ADDRESS, recipient, initialSupply), + 'ERC20: transfer from the zero address' + ); + }); + }); + }); + describe('_approve', function () { shouldBehaveLikeERC20Approve('ERC20', initialHolder, recipient, initialSupply, function (owner, spender, amount) { return this.token.approveInternal(owner, spender, amount); From 73f7fbb7262d4ee1f16a4334ff663ce694228306 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Wed, 15 May 2019 15:12:15 -0300 Subject: [PATCH 05/10] Add changelog entry. --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2eb69798846..7b7cfee3312 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ ### Bugfixes: * `PostDeliveryCrowdsale`: some validations where skipped when paired with other crowdsale flavors, such as `AllowanceCrowdsale`, or `MintableCrowdsale` and `ERC20Capped`, which could cause buyers to not be able to claim their purchased tokens. ([#1721](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1721)) + * (minor): `ERC20._transfer` allowed the `from` parameter to be the zero address, so it was possible to trigger a transfer of 0 tokens from it to any other address. The zero address is not a valid destinatary of transfers, not can it emit or receive allowance, so this behavior was inconsistent. It now reverts. ([#1752](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1752)) ## 2.2.0 (2019-03-14) From 2a90883f5257b9ee65964ca9ee0fbbfccc42fd35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Wed, 15 May 2019 15:13:09 -0300 Subject: [PATCH 06/10] Fix linter error. --- 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 be4985eae74..f5c46f65774 100644 --- a/test/token/ERC20/ERC20.behavior.js +++ b/test/token/ERC20/ERC20.behavior.js @@ -24,7 +24,7 @@ function shouldBehaveLikeERC20 (errorPrefix, initialSupply, initialHolder, recip describe('transfer', function () { shouldBehaveLikeERC20Transfer('ERC20', initialHolder, recipient, initialSupply, - function(from, to, value) { + function (from, to, value) { return this.token.transfer(to, value, { from }); } ); From 5fe5dd18bcfbd64c9c531a5b43cb2f181d4b5d67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Wed, 15 May 2019 16:24:35 -0300 Subject: [PATCH 07/10] Delete repeated test. --- test/token/ERC20/ERC20.test.js | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/test/token/ERC20/ERC20.test.js b/test/token/ERC20/ERC20.test.js index 41f78332397..847bce2df14 100644 --- a/test/token/ERC20/ERC20.test.js +++ b/test/token/ERC20/ERC20.test.js @@ -331,20 +331,6 @@ contract('ERC20', function ([_, initialHolder, recipient, anotherAccount]) { }); }); - describe('_transfer', function () { - shouldBehaveLikeERC20Approve('ERC20', initialHolder, recipient, initialSupply, function (owner, spender, amount) { - return this.token.approveInternal(owner, spender, amount); - }); - - describe('when the owner is the zero address', function () { - it('reverts', async function () { - await shouldFail.reverting.withMessage(this.token.approveInternal(ZERO_ADDRESS, recipient, initialSupply), - 'ERC20: approve from the zero address' - ); - }); - }); - }); - describe('_transfer', function () { shouldBehaveLikeERC20Transfer('ERC20', initialHolder, recipient, initialSupply, function (from, to, amount) { return this.token.transferInternal(from, to, amount); From e618dce726c4d2ddc1a28f59691af85a37311508 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Wed, 15 May 2019 16:28:19 -0300 Subject: [PATCH 08/10] Fix hardcoded error prefix. --- 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 f5c46f65774..dab98718a11 100644 --- a/test/token/ERC20/ERC20.behavior.js +++ b/test/token/ERC20/ERC20.behavior.js @@ -23,7 +23,7 @@ function shouldBehaveLikeERC20 (errorPrefix, initialSupply, initialHolder, recip }); describe('transfer', function () { - shouldBehaveLikeERC20Transfer('ERC20', initialHolder, recipient, initialSupply, + shouldBehaveLikeERC20Transfer(errorPrefix, initialHolder, recipient, initialSupply, function (from, to, value) { return this.token.transfer(to, value, { from }); } From dd36523625a4d87e9949913b92066b55c66ef3fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Thu, 16 May 2019 10:44:03 -0300 Subject: [PATCH 09/10] Update CHANGELOG.md Co-Authored-By: Francisco Giordano --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b7cfee3312..f85c53153df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ ### Bugfixes: * `PostDeliveryCrowdsale`: some validations where skipped when paired with other crowdsale flavors, such as `AllowanceCrowdsale`, or `MintableCrowdsale` and `ERC20Capped`, which could cause buyers to not be able to claim their purchased tokens. ([#1721](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1721)) - * (minor): `ERC20._transfer` allowed the `from` parameter to be the zero address, so it was possible to trigger a transfer of 0 tokens from it to any other address. The zero address is not a valid destinatary of transfers, not can it emit or receive allowance, so this behavior was inconsistent. It now reverts. ([#1752](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1752)) + * `ERC20._transfer`: the `from` argument was allowed to be the zero address, so it was possible to internally trigger a transfer of 0 tokens from the zero address. This address is not a valid destinatary of transfers, nor can it give or receive allowance, so this behavior was inconsistent. It now reverts. ([#1752](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1752)) ## 2.2.0 (2019-03-14) From 8989f4c2f103d43b699ee0ea02611ac87e975a01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Thu, 16 May 2019 11:12:04 -0300 Subject: [PATCH 10/10] Address review comments. --- test/token/ERC20/ERC20.behavior.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/token/ERC20/ERC20.behavior.js b/test/token/ERC20/ERC20.behavior.js index dab98718a11..8f4c0c32ae1 100644 --- a/test/token/ERC20/ERC20.behavior.js +++ b/test/token/ERC20/ERC20.behavior.js @@ -138,11 +138,12 @@ function shouldBehaveLikeERC20 (errorPrefix, initialSupply, initialHolder, recip describe('when the token owner is the zero address', function () { const amount = 0; + const tokenOwner = ZERO_ADDRESS; const to = recipient; it('reverts', async function () { await shouldFail.reverting.withMessage(this.token.transferFrom( - ZERO_ADDRESS, to, amount, { from: to }), `${errorPrefix}: transfer from the zero address` + tokenOwner, to, amount, { from: spender }), `${errorPrefix}: transfer from the zero address` ); }); });