Fix decrease allowance and add non-zero address precondition to approve (#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.

contracts/token/ERC20/ERC20.sol

@@ -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;
@@ -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]);
@@ -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;
   }

contracts/token/ERC20/ERC20Pausable.sol

@@ -44,7 +44,7 @@ contract ERC20Pausable is ERC20, Pausable {
     return super.approve(_spender, _value);
   }
 
-  function increaseApproval(
+  function increaseAllowance(
     address _spender,
     uint _addedValue
   )
@@ -52,10 +52,10 @@ contract ERC20Pausable is ERC20, Pausable {
     whenNotPaused
     returns (bool success)
   {
-    return super.increaseApproval(_spender, _addedValue);
+    return super.increaseAllowance(_spender, _addedValue);
   }
 
-  function decreaseApproval(
+  function decreaseAllowance(
     address _spender,
     uint _subtractedValue
   )
@@ -63,6 +63,6 @@ contract ERC20Pausable is ERC20, Pausable {
     whenNotPaused
     returns (bool success)
   {
-    return super.decreaseApproval(_spender, _subtractedValue);
+    return super.decreaseAllowance(_spender, _subtractedValue);
   }
 }

test/token/ERC20/ERC20.test.js

@@ -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 }));
       });
     });
   });
@@ -261,28 +249,14 @@ 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 }));
           });
         });
 
@@ -290,59 +264,46 @@ contract('ERC20', function ([_, owner, recipient, anotherAccount]) {
           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);
       });
     });
 
@@ -350,25 +311,13 @@ contract('ERC20', function ([_, owner, recipient, anotherAccount]) {
       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 () {
@@ -376,7 +325,7 @@ contract('ERC20', function ([_, owner, recipient, anotherAccount]) {
 
       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');
@@ -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);
           });
@@ -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);
           });
@@ -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');
@@ -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);
           });
@@ -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);
           });
@@ -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 }));
       });
     });
   });

test/token/ERC20/ERC20Pausable.test.js

@@ -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);
       });
@@ -205,7 +205,7 @@ 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);
       });
@@ -213,7 +213,7 @@ contract('ERC20Pausable', function ([_, pauser, otherPauser, recipient, anotherA
       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 }));
       });
     });
 
@@ -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);
       });
@@ -232,7 +232,7 @@ 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);
       });
@@ -240,7 +240,7 @@ contract('ERC20Pausable', function ([_, pauser, otherPauser, recipient, anotherA
       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 }));
       });
     });
   });