Skip to content

Commit

Permalink
Fix decrease allowance and add non-zero address precondition to appro…
Browse files Browse the repository at this point in the history 
…ve (#1293)

* rename {increase,decrease}Approval to {increase,decrease}Allowance

* add non-zero spender check to approve and {increase,decrease}Allowance

* Updated tests to reflect the new behavior.

* fix wrong test description

* fix old function names

* Fixed linter error.

* Fixed typo.
  • Loading branch information
@frangio
frangio authored Sep 7, 2018
1 parent e7e8d8e commit 84e63bb
Show file tree
Hide file tree
Showing 4 changed files with 60 additions and 121 deletions.
18 changes: 10 additions & 8 deletions contracts/token/ERC20/ERC20.sol
View file
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
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
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
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

0 comments on commit 84e63bb

Please sign in to comment.