Skip to content

Commit

Permalink
Disallow ERC20._transfer from the zero address. (#1752)
Browse files Browse the repository at this point in the history 
* 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 <frangio.1@gmail.com>

* Address review comments.
  • Loading branch information
@nventuro
nventuro authored May 16, 2019
1 parent dd6ec21 commit ad18098
Show file tree
Hide file tree
Showing 5 changed files with 169 additions and 105 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
4 changes: 4 additions & 0 deletions contracts/mocks/ERC20Mock.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
1 change: 1 addition & 0 deletions contracts/token/ERC20/ERC20.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
253 changes: 148 additions & 105 deletions test/token/ERC20/ERC20.behavior.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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`
);
});
});
Expand All @@ -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 () {
Expand Down Expand Up @@ -264,5 +306,6 @@ function shouldBehaveLikeERC20Approve (errorPrefix, owner, spender, supply, appr

module.exports = {
shouldBehaveLikeERC20,
shouldBehaveLikeERC20Transfer,
shouldBehaveLikeERC20Approve,
};
15 changes: 15 additions & 0 deletions test/token/ERC20/ERC20.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ const { ZERO_ADDRESS } = constants;

const {
shouldBehaveLikeERC20,
shouldBehaveLikeERC20Transfer,
shouldBehaveLikeERC20Approve,
} = require('./ERC20.behavior');

Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit ad18098

Please sign in to comment.