From 17211867b6691820f7a733e1ebee6634d6bb699a Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Fri, 17 Aug 2018 15:04:00 -0300 Subject: [PATCH 1/3] fix off by one error in _burn and _burnFrom --- contracts/token/ERC20/StandardToken.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/token/ERC20/StandardToken.sol b/contracts/token/ERC20/StandardToken.sol index 52e125a2771..4eada707a8b 100644 --- a/contracts/token/ERC20/StandardToken.sol +++ b/contracts/token/ERC20/StandardToken.sol @@ -178,7 +178,7 @@ contract StandardToken is ERC20 { */ function _burn(address _account, uint256 _amount) internal { require(_account != 0); - require(balances[_account] > _amount); + require(_amount <= balances[_account]); totalSupply_ = totalSupply_.sub(_amount); balances[_account] = balances[_account].sub(_amount); @@ -193,7 +193,7 @@ contract StandardToken is ERC20 { * @param _amount The amount that will be burnt. */ function _burnFrom(address _account, uint256 _amount) internal { - require(allowed[_account][msg.sender] > _amount); + require(_amount <= allowed[_account][msg.sender]); // Should https://github.com/OpenZeppelin/zeppelin-solidity/issues/707 be accepted, // this function needs to emit an event with the updated approval. From b384386c175d444fc332560d6242b1f85c708437 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Fri, 17 Aug 2018 17:38:19 -0300 Subject: [PATCH 2/3] add tests for off-by-one error --- test/token/ERC20/StandardToken.test.js | 104 +++++++++++++------------ 1 file changed, 56 insertions(+), 48 deletions(-) diff --git a/test/token/ERC20/StandardToken.test.js b/test/token/ERC20/StandardToken.test.js index 9487d107a35..075175a818e 100644 --- a/test/token/ERC20/StandardToken.test.js +++ b/test/token/ERC20/StandardToken.test.js @@ -526,10 +526,9 @@ contract('StandardToken', function ([_, owner, recipient, anotherAccount]) { describe('_burn', function () { const initialSupply = new BigNumber(100); - const amount = new BigNumber(50); it('rejects a null account', async function () { - await assertRevert(this.token.burn(ZERO_ADDRESS, amount)); + await assertRevert(this.token.burn(ZERO_ADDRESS, 1)); }); describe('for a non null account', function () { @@ -537,38 +536,42 @@ contract('StandardToken', function ([_, owner, recipient, anotherAccount]) { await assertRevert(this.token.burn(owner, initialSupply.plus(1))); }); - describe('for less amount than balance', function () { - beforeEach('burning', async function () { - const { logs } = await this.token.burn(owner, amount); - this.logs = logs; - }); - - it('decrements totalSupply', async function () { - const expectedSupply = initialSupply.minus(amount); - (await this.token.totalSupply()).should.be.bignumber.equal(expectedSupply); - }); + const describeBurn = function (description, amount) { + describe(description, function () { + beforeEach('burning', async function () { + const { logs } = await this.token.burn(owner, amount); + this.logs = logs; + }); - it('decrements owner balance', async function () { - const expectedBalance = initialSupply.minus(amount); - (await this.token.balanceOf(owner)).should.be.bignumber.equal(expectedBalance); - }); + it('decrements totalSupply', async function () { + const expectedSupply = initialSupply.minus(amount); + (await this.token.totalSupply()).should.be.bignumber.equal(expectedSupply); + }); - it('emits Transfer event', async function () { - const event = expectEvent.inLogs(this.logs, 'Transfer', { - from: owner, - to: ZERO_ADDRESS, + it('decrements owner balance', async function () { + const expectedBalance = initialSupply.minus(amount); + (await this.token.balanceOf(owner)).should.be.bignumber.equal(expectedBalance); }); - event.args.value.should.be.bignumber.equal(amount); + it('emits Transfer event', async function () { + const event = expectEvent.inLogs(this.logs, 'Transfer', { + from: owner, + to: ZERO_ADDRESS, + }); + + event.args.value.should.be.bignumber.equal(amount); + }); }); - }); + }; + + describeBurn('for entire balance', initialSupply); + describeBurn('for less amount than balance', new BigNumber(50)); }); }); describe('_burnFrom', function () { const initialSupply = new BigNumber(100); const allowance = new BigNumber(70); - const amount = new BigNumber(50); const spender = anotherAccount; @@ -577,7 +580,7 @@ contract('StandardToken', function ([_, owner, recipient, anotherAccount]) { }); it('rejects a null account', async function () { - await assertRevert(this.token.burnFrom(ZERO_ADDRESS, amount)); + await assertRevert(this.token.burnFrom(ZERO_ADDRESS, 1)); }); describe('for a non null account', function () { @@ -589,36 +592,41 @@ contract('StandardToken', function ([_, owner, recipient, anotherAccount]) { await assertRevert(this.token.burnFrom(owner, initialSupply.plus(1))); }); - describe('for less amount than allowance', function () { - beforeEach('burning', async function () { - const { logs } = await this.token.burnFrom(owner, amount, { from: spender }); - this.logs = logs; - }); - - it('decrements totalSupply', async function () { - const expectedSupply = initialSupply.minus(amount); - (await this.token.totalSupply()).should.be.bignumber.equal(expectedSupply); - }); + const describeBurnFrom = function (description, amount) { + describe(description, function () { + beforeEach('burning', async function () { + const { logs } = await this.token.burnFrom(owner, amount, { from: spender }); + this.logs = logs; + }); - it('decrements owner balance', async function () { - const expectedBalance = initialSupply.minus(amount); - (await this.token.balanceOf(owner)).should.be.bignumber.equal(expectedBalance); - }); + it('decrements totalSupply', async function () { + const expectedSupply = initialSupply.minus(amount); + (await this.token.totalSupply()).should.be.bignumber.equal(expectedSupply); + }); - it('decrements spender allowance', async function () { - const expectedAllowance = allowance.minus(amount); - (await this.token.allowance(owner, spender)).should.be.bignumber.equal(expectedAllowance); - }); + it('decrements owner balance', async function () { + const expectedBalance = initialSupply.minus(amount); + (await this.token.balanceOf(owner)).should.be.bignumber.equal(expectedBalance); + }); - it('emits Transfer event', async function () { - const event = expectEvent.inLogs(this.logs, 'Transfer', { - from: owner, - to: ZERO_ADDRESS, + it('decrements spender allowance', async function () { + const expectedAllowance = allowance.minus(amount); + (await this.token.allowance(owner, spender)).should.be.bignumber.equal(expectedAllowance); }); - event.args.value.should.be.bignumber.equal(amount); + it('emits Transfer event', async function () { + const event = expectEvent.inLogs(this.logs, 'Transfer', { + from: owner, + to: ZERO_ADDRESS, + }); + + event.args.value.should.be.bignumber.equal(amount); + }); }); - }); + }; + + describeBurnFrom('for entire allowance', allowance); + describeBurnFrom('for less amount than allowance', new BigNumber(50)); }); }); }); From f88c2739fd816cfad7ff5989aed69fc78c276ee9 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Fri, 17 Aug 2018 18:52:02 -0300 Subject: [PATCH 3/3] change amount used --- test/token/ERC20/StandardToken.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/token/ERC20/StandardToken.test.js b/test/token/ERC20/StandardToken.test.js index 075175a818e..f7bc1a3e177 100644 --- a/test/token/ERC20/StandardToken.test.js +++ b/test/token/ERC20/StandardToken.test.js @@ -565,7 +565,7 @@ contract('StandardToken', function ([_, owner, recipient, anotherAccount]) { }; describeBurn('for entire balance', initialSupply); - describeBurn('for less amount than balance', new BigNumber(50)); + describeBurn('for less amount than balance', initialSupply.sub(1)); }); }); @@ -626,7 +626,7 @@ contract('StandardToken', function ([_, owner, recipient, anotherAccount]) { }; describeBurnFrom('for entire allowance', allowance); - describeBurnFrom('for less amount than allowance', new BigNumber(50)); + describeBurnFrom('for less amount than allowance', allowance.sub(1)); }); }); });