Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix decrease allowance and add non-zero address precondition to approve #1293

Merged
merged 8 commits into from
Sep 7, 2018
Merged

Fix decrease allowance and add non-zero address precondition to approve #1293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f306cc8
rename {increase,decrease}Approval to {increase,decrease}Allowance
frangio Sep 6, 2018
2470b65
add non-zero spender check to approve and {increase,decrease}Allowance
frangio Sep 6, 2018
79fb00c
Updated tests to reflect the new behavior.
nventuro Sep 7, 2018
00f124e
fix wrong test description
frangio Sep 7, 2018
267c384
fix old function names
frangio Sep 7, 2018
f7d18fe
Fixed linter error.
nventuro Sep 7, 2018
5d87d9a
Merge branch 'master' into fix-increase-decrease-allowance
frangio Sep 7, 2018
f98cdfa
Fixed typo.
nventuro Sep 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions contracts/token/ERC20/ERC20.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@ contract ERC20 is IERC20 {
* @param _value The amount of tokens to be spent.
*/
function approve(address _spender, uint256 _value) public returns (bool) {
require(_spender != address(0));

allowed_[msg.sender][_spender] = _value;
emit Approval(msg.sender, _spender, _value);
return true;
Expand Down Expand Up @@ -117,13 +119,15 @@ contract ERC20 is IERC20 {
* @param _spender The address which will spend the funds.
* @param _addedValue The amount of tokens to increase the allowance by.
*/
function increaseApproval(
function increaseAllowance(
address _spender,
uint256 _addedValue
)
public
returns (bool)
{
require(_spender != address(0));

allowed_[msg.sender][_spender] = (
allowed_[msg.sender][_spender].add(_addedValue));
emit Approval(msg.sender, _spender, allowed_[msg.sender][_spender]);
Expand All @@ -139,19 +143,17 @@ contract ERC20 is IERC20 {
* @param _spender The address which will spend the funds.
* @param _subtractedValue The amount of tokens to decrease the allowance by.
*/
function decreaseApproval(
function decreaseAllowance(
address _spender,
uint256 _subtractedValue
)
public
returns (bool)
{
uint256 oldValue = allowed_[msg.sender][_spender];
if (_subtractedValue >= oldValue) {
allowed_[msg.sender][_spender] = 0;
} else {
allowed_[msg.sender][_spender] = oldValue.sub(_subtractedValue);
}
require(_spender != address(0));

allowed_[msg.sender][_spender] = (
allowed_[msg.sender][_spender].sub(_subtractedValue));
emit Approval(msg.sender, _spender, allowed_[msg.sender][_spender]);
return true;
}
Expand Down
8 changes: 4 additions & 4 deletions contracts/token/ERC20/ERC20Pausable.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,25 +44,25 @@ contract ERC20Pausable is ERC20, Pausable {
return super.approve(_spender, _value);
}

function increaseApproval(
function increaseAllowance(
address _spender,
uint _addedValue
)
public
whenNotPaused
returns (bool success)
{
return super.increaseApproval(_spender, _addedValue);
return super.increaseAllowance(_spender, _addedValue);
}

function decreaseApproval(
function decreaseAllowance(
address _spender,
uint _subtractedValue
)
public
whenNotPaused
returns (bool success)
{
return super.decreaseApproval(_spender, _subtractedValue);
return super.decreaseAllowance(_spender, _subtractedValue);
}
}
143 changes: 40 additions & 103 deletions test/token/ERC20/ERC20.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,20 +158,8 @@ contract('ERC20', function ([_, owner, recipient, anotherAccount]) {
const amount = 100;
const spender = ZERO_ADDRESS;

it('approves the requested amount', async function () {
await this.token.approve(spender, amount, { from: owner });

(await this.token.allowance(owner, spender)).should.be.bignumber.equal(amount);
});

it('emits an approval event', async function () {
const { logs } = await this.token.approve(spender, amount, { from: owner });

logs.length.should.equal(1);
logs[0].event.should.equal('Approval');
logs[0].args.owner.should.equal(owner);
logs[0].args.spender.should.equal(spender);
logs[0].args.value.should.be.bignumber.equal(amount);
it('reverts', async function () {
await assertRevert(this.token.approve(spender, amount, { from: owner }));
});
});
});
Expand Down Expand Up @@ -261,122 +249,83 @@ contract('ERC20', function ([_, owner, recipient, anotherAccount]) {
});
});

describe('decrease approval', function () {
describe('decrease allowance', function () {
describe('when the spender is not the zero address', function () {
const spender = recipient;

describe('when the sender has enough balance', function () {
const amount = 100;

it('emits an approval event', async function () {
const { logs } = await this.token.decreaseApproval(spender, amount, { from: owner });

logs.length.should.equal(1);
logs[0].event.should.equal('Approval');
logs[0].args.owner.should.equal(owner);
logs[0].args.spender.should.equal(spender);
logs[0].args.value.should.be.bignumber.equal(0);
});

function shouldDecreaseApproval (amount) {
describe('when there was no approved amount before', function () {
it('keeps the allowance to zero', async function () {
await this.token.decreaseApproval(spender, amount, { from: owner });

(await this.token.allowance(owner, spender)).should.be.bignumber.equal(0);
it('reverts', async function () {
await assertRevert(this.token.decreaseAllowance(spender, amount, { from: owner }));
});
});

describe('when the spender had an approved amount', function () {
const approvedAmount = amount;

beforeEach(async function () {
await this.token.approve(spender, approvedAmount, { from: owner });
({ logs: this.logs } = await this.token.approve(spender, approvedAmount, { from: owner }));
});

it('emits an approval event', async function () {
const { logs } = await this.token.decreaseAllowance(spender, approvedAmount, { from: owner });

logs.length.should.equal(1);
logs[0].event.should.equal('Approval');
logs[0].args.owner.should.equal(owner);
logs[0].args.spender.should.equal(spender);
logs[0].args.value.should.be.bignumber.equal(0);
});

it('decreases the spender allowance subtracting the requested amount', async function () {
await this.token.decreaseApproval(spender, approvedAmount - 5, { from: owner });
await this.token.decreaseAllowance(spender, approvedAmount - 1, { from: owner });

(await this.token.allowance(owner, spender)).should.be.bignumber.equal(5);
(await this.token.allowance(owner, spender)).should.be.bignumber.equal(1);
});

it('sets the allowance to zero when all allowance is removed', async function () {
await this.token.decreaseApproval(spender, approvedAmount, { from: owner });
await this.token.decreaseAllowance(spender, approvedAmount, { from: owner });
(await this.token.allowance(owner, spender)).should.be.bignumber.equal(0);
});

it('sets the allowance to zero when more than the full allowance is removed', async function () {
await this.token.decreaseApproval(spender, approvedAmount + 5, { from: owner });
(await this.token.allowance(owner, spender)).should.be.bignumber.equal(0);
it('reverts when more than the full allowance is removed', async function () {
await assertRevert(this.token.decreaseAllowance(spender, approvedAmount + 1, { from: owner }));
});
});
}

describe('when the sender has enough balance', function () {
const amount = 100;

shouldDecreaseApproval(amount);
});

describe('when the sender does not have enough balance', function () {
const amount = 101;

it('emits an approval event', async function () {
const { logs } = await this.token.decreaseApproval(spender, amount, { from: owner });

logs.length.should.equal(1);
logs[0].event.should.equal('Approval');
logs[0].args.owner.should.equal(owner);
logs[0].args.spender.should.equal(spender);
logs[0].args.value.should.be.bignumber.equal(0);
});

describe('when there was no approved amount before', function () {
it('keeps the allowance to zero', async function () {
await this.token.decreaseApproval(spender, amount, { from: owner });

(await this.token.allowance(owner, spender)).should.be.bignumber.equal(0);
});
});

describe('when the spender had an approved amount', function () {
beforeEach(async function () {
await this.token.approve(spender, amount + 1, { from: owner });
});

it('decreases the spender allowance subtracting the requested amount', async function () {
await this.token.decreaseApproval(spender, amount, { from: owner });

(await this.token.allowance(owner, spender)).should.be.bignumber.equal(1);
});
});
shouldDecreaseApproval(amount);
});
});

describe('when the spender is the zero address', function () {
const amount = 100;
const spender = ZERO_ADDRESS;

it('decreases the requested amount', async function () {
await this.token.decreaseApproval(spender, amount, { from: owner });

(await this.token.allowance(owner, spender)).should.be.bignumber.equal(0);
});

it('emits an approval event', async function () {
const { logs } = await this.token.decreaseApproval(spender, amount, { from: owner });

logs.length.should.equal(1);
logs[0].event.should.equal('Approval');
logs[0].args.owner.should.equal(owner);
logs[0].args.spender.should.equal(spender);
logs[0].args.value.should.be.bignumber.equal(0);
it('reverts', async function () {
await assertRevert(this.token.decreaseAllowance(spender, amount, { from: owner }));
});
});
});

describe('increase approval', function () {
describe('increase allowance', function () {
const amount = 100;

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 () {
const { logs } = await this.token.increaseApproval(spender, amount, { from: owner });
const { logs } = await this.token.increaseAllowance(spender, amount, { from: owner });

logs.length.should.equal(1);
logs[0].event.should.equal('Approval');
Expand All @@ -387,7 +336,7 @@ contract('ERC20', function ([_, owner, recipient, anotherAccount]) {

describe('when there was no approved amount before', function () {
it('approves the requested amount', async function () {
await this.token.increaseApproval(spender, amount, { from: owner });
await this.token.increaseAllowance(spender, amount, { from: owner });

(await this.token.allowance(owner, spender)).should.be.bignumber.equal(amount);
});
Expand All @@ -399,7 +348,7 @@ contract('ERC20', function ([_, owner, recipient, anotherAccount]) {
});

it('increases the spender allowance adding the requested amount', async function () {
await this.token.increaseApproval(spender, amount, { from: owner });
await this.token.increaseAllowance(spender, amount, { from: owner });

(await this.token.allowance(owner, spender)).should.be.bignumber.equal(amount + 1);
});
Expand All @@ -410,7 +359,7 @@ contract('ERC20', function ([_, owner, recipient, anotherAccount]) {
const amount = 101;

it('emits an approval event', async function () {
const { logs } = await this.token.increaseApproval(spender, amount, { from: owner });
const { logs } = await this.token.increaseAllowance(spender, amount, { from: owner });

logs.length.should.equal(1);
logs[0].event.should.equal('Approval');
Expand All @@ -421,7 +370,7 @@ contract('ERC20', function ([_, owner, recipient, anotherAccount]) {

describe('when there was no approved amount before', function () {
it('approves the requested amount', async function () {
await this.token.increaseApproval(spender, amount, { from: owner });
await this.token.increaseAllowance(spender, amount, { from: owner });

(await this.token.allowance(owner, spender)).should.be.bignumber.equal(amount);
});
Expand All @@ -433,7 +382,7 @@ contract('ERC20', function ([_, owner, recipient, anotherAccount]) {
});

it('increases the spender allowance adding the requested amount', async function () {
await this.token.increaseApproval(spender, amount, { from: owner });
await this.token.increaseAllowance(spender, amount, { from: owner });

(await this.token.allowance(owner, spender)).should.be.bignumber.equal(amount + 1);
});
Expand All @@ -444,20 +393,8 @@ contract('ERC20', function ([_, owner, recipient, anotherAccount]) {
describe('when the spender is the zero address', function () {
const spender = ZERO_ADDRESS;

it('approves the requested amount', async function () {
await this.token.increaseApproval(spender, amount, { from: owner });

(await this.token.allowance(owner, spender)).should.be.bignumber.equal(amount);
});

it('emits an approval event', async function () {
const { logs } = await this.token.increaseApproval(spender, amount, { from: owner });

logs.length.should.equal(1);
logs[0].event.should.equal('Approval');
logs[0].args.owner.should.equal(owner);
logs[0].args.spender.should.equal(spender);
logs[0].args.value.should.be.bignumber.equal(amount);
it('reverts', async function () {
await assertRevert(this.token.increaseAllowance(spender, amount, { from: owner }));
});
});
});
Expand Down
12 changes: 6 additions & 6 deletions test/token/ERC20/ERC20Pausable.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ contract('ERC20Pausable', function ([_, pauser, otherPauser, recipient, anotherA
});

it('allows to decrease approval when unpaused', async function () {
await this.token.decreaseApproval(anotherAccount, 40, { from: pauser });
await this.token.decreaseAllowance(anotherAccount, 40, { from: pauser });

(await this.token.allowance(pauser, anotherAccount)).should.be.bignumber.equal(60);
});
Expand All @@ -205,15 +205,15 @@ contract('ERC20Pausable', function ([_, pauser, otherPauser, recipient, anotherA
await this.token.pause({ from: pauser });
await this.token.unpause({ from: pauser });

await this.token.decreaseApproval(anotherAccount, 40, { from: pauser });
await this.token.decreaseAllowance(anotherAccount, 40, { from: pauser });

(await this.token.allowance(pauser, anotherAccount)).should.be.bignumber.equal(60);
});

it('reverts when trying to transfer when paused', async function () {
await this.token.pause({ from: pauser });

await assertRevert(this.token.decreaseApproval(anotherAccount, 40, { from: pauser }));
await assertRevert(this.token.decreaseAllowance(anotherAccount, 40, { from: pauser }));
});
});

Expand All @@ -223,7 +223,7 @@ contract('ERC20Pausable', function ([_, pauser, otherPauser, recipient, anotherA
});

it('allows to increase approval when unpaused', async function () {
await this.token.increaseApproval(anotherAccount, 40, { from: pauser });
await this.token.increaseAllowance(anotherAccount, 40, { from: pauser });

(await this.token.allowance(pauser, anotherAccount)).should.be.bignumber.equal(140);
});
Expand All @@ -232,15 +232,15 @@ contract('ERC20Pausable', function ([_, pauser, otherPauser, recipient, anotherA
await this.token.pause({ from: pauser });
await this.token.unpause({ from: pauser });

await this.token.increaseApproval(anotherAccount, 40, { from: pauser });
await this.token.increaseAllowance(anotherAccount, 40, { from: pauser });

(await this.token.allowance(pauser, anotherAccount)).should.be.bignumber.equal(140);
});

it('reverts when trying to increase approval when paused', async function () {
await this.token.pause({ from: pauser });

await assertRevert(this.token.increaseApproval(anotherAccount, 40, { from: pauser }));
await assertRevert(this.token.increaseAllowance(anotherAccount, 40, { from: pauser }));
});
});
});
Expand Down