From f7ff3e7e678347322218514c406a77b4b4f09ef7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Thu, 16 May 2019 11:50:54 -0300 Subject: [PATCH] Disallow ERC20._transfer from the zero address. (#1752) * Add requirement of non-zero from to ERC20 transfer. * Add test for transferFrom zero address to behavior. * Create ERC20.transfer behavior. * Add tests for _transfer. * Add changelog entry. * Fix linter error. * Delete repeated test. * Fix hardcoded error prefix. * Update CHANGELOG.md Co-Authored-By: Francisco Giordano * Address review comments. (cherry picked from commit ad18098d65bbb5e41dcf1a286b8550470039d056) --- CHANGELOG.md | 1 + contracts/mocks/ERC20Mock.sol | 4 + contracts/token/ERC20/ERC20.sol | 1 + test/token/ERC20/ERC20.behavior.js | 253 +++++++++++++++++------------ test/token/ERC20/ERC20.test.js | 15 ++ 5 files changed, 169 insertions(+), 105 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2eb69798846..f85c53153df 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)) + * `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) 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/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); diff --git a/test/token/ERC20/ERC20.behavior.js b/test/token/ERC20/ERC20.behavior.js index 2536df005ec..8f4c0c32ae1 100644 --- a/test/token/ERC20/ERC20.behavior.js +++ b/test/token/ERC20/ERC20.behavior.js @@ -23,151 +23,127 @@ 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 }); + shouldBehaveLikeERC20Transfer(errorPrefix, initialHolder, recipient, initialSupply, + function (from, to, value) { + return this.token.transfer(to, value, { from }); + } + ); + }); - (await this.token.balanceOf(initialHolder)).should.be.bignumber.equal('0'); + describe('transfer from', function () { + const spender = recipient; - (await this.token.balanceOf(to)).should.be.bignumber.equal(amount); - }); + describe('when the token owner is not the zero address', function () { + const tokenOwner = initialHolder; - it('emits a transfer event', async function () { - const { logs } = await this.token.transfer(to, amount, { from: initialHolder }); + describe('when the recipient is not the zero address', function () { + const to = anotherAccount; - expectEvent.inLogs(logs, 'Transfer', { - from: initialHolder, - to: to, - value: amount, + 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 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` - ); - }); - }); - }); + describe('when the token owner has enough balance', function () { + const amount = initialSupply; - describe('transfer from', function () { - const spender = recipient; + it('transfers the requested amount', async function () { + await this.token.transferFrom(tokenOwner, to, amount, { from: spender }); - describe('when the recipient is not the zero address', function () { - const to = anotherAccount; + (await this.token.balanceOf(tokenOwner)).should.be.bignumber.equal('0'); - describe('when the spender has enough approved balance', function () { - beforeEach(async function () { - await this.token.approve(spender, initialSupply, { from: initialHolder }); - }); + (await this.token.balanceOf(to)).should.be.bignumber.equal(amount); + }); - describe('when the initial holder has enough balance', function () { - const amount = initialSupply; + it('decreases the spender allowance', async function () { + await this.token.transferFrom(tokenOwner, to, amount, { from: spender }); - it('transfers the requested amount', async function () { - await this.token.transferFrom(initialHolder, to, amount, { from: spender }); + (await this.token.allowance(tokenOwner, spender)).should.be.bignumber.equal('0'); + }); - (await this.token.balanceOf(initialHolder)).should.be.bignumber.equal('0'); + it('emits a transfer event', async function () { + const { logs } = await this.token.transferFrom(tokenOwner, to, amount, { from: spender }); - (await this.token.balanceOf(to)).should.be.bignumber.equal(amount); - }); + expectEvent.inLogs(logs, 'Transfer', { + from: tokenOwner, + to: to, + value: amount, + }); + }); - it('decreases the spender allowance', async function () { - 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 }); - (await this.token.allowance(initialHolder, spender)).should.be.bignumber.equal('0'); + expectEvent.inLogs(logs, 'Approval', { + owner: tokenOwner, + spender: spender, + value: await this.token.allowance(tokenOwner, spender), + }); + }); }); - it('emits a transfer event', async function () { - const { logs } = await this.token.transferFrom(initialHolder, to, amount, { from: spender }); + describe('when the token owner does not have enough balance', function () { + const amount = initialSupply.addn(1); - expectEvent.inLogs(logs, 'Transfer', { - from: initialHolder, - to: to, - value: amount, + 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: tokenOwner }); + }); - it('emits an approval event', async function () { - const { logs } = await this.token.transferFrom(initialHolder, to, amount, { from: spender }); + describe('when the token owner has enough balance', function () { + const amount = initialSupply; - expectEvent.inLogs(logs, 'Approval', { - owner: initialHolder, - spender: spender, - value: await this.token.allowance(initialHolder, spender), + 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 spender does not have enough approved balance', function () { - beforeEach(async function () { - await this.token.approve(spender, initialSupply.subn(1), { from: initialHolder }); - }); - - describe('when the initial holder has enough balance', function () { - const amount = initialSupply; + describe('when the recipient is the zero address', function () { + const amount = initialSupply; + const to = ZERO_ADDRESS; - it('reverts', async function () { - await shouldFail.reverting.withMessage(this.token.transferFrom( - initialHolder, to, amount, { from: spender }), 'SafeMath: subtraction overflow' - ); - }); + beforeEach(async function () { + await this.token.approve(spender, amount, { from: tokenOwner }); }); - describe('when the initial holder 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 }), `${errorPrefix}: transfer to the 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: initialHolder }); - }); + 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( - initialHolder, to, amount, { from: spender }), `${errorPrefix}: transfer to the zero address` + tokenOwner, to, amount, { from: spender }), `${errorPrefix}: transfer from the zero address` ); }); }); @@ -182,6 +158,72 @@ 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 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 () { + 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 () { @@ -264,5 +306,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..847bce2df14 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,20 @@ contract('ERC20', function ([_, initialHolder, recipient, anotherAccount]) { }); }); + 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);