Merge branch 'fix/OpenZeppelin#1404' of https://github.com/Aniket-Eng…

…g/zeppelin-solidity
Aniket-Engg · Oct 16, 2018 · 998dfd7 · 998dfd7
2 parents 52c7870 + e2fb20a
commit 998dfd7
Show file tree

Hide file tree

Showing 13 changed files with 127 additions and 26 deletions.
diff --git a/contracts/mocks/SafeERC20Helper.sol b/contracts/mocks/SafeERC20Helper.sol
@@ -4,6 +4,7 @@ import "../token/ERC20/IERC20.sol";
 import "../token/ERC20/SafeERC20.sol";
 
 contract ERC20FailingMock is IERC20 {
+  uint256 private _allowance;
   function totalSupply() public view returns (uint256) {
     return 0;
   }
@@ -20,16 +21,30 @@ contract ERC20FailingMock is IERC20 {
     return false;
   }
 
+  function increaseAllowance(address, uint256) public returns (bool){
+    return false;
+  }
+
+  function decreaseAllowance(address, uint256) public returns (bool){
+    return false;
+  }
+
   function balanceOf(address) public view returns (uint256) {
     return 0;
   }
 
   function allowance(address, address) public view returns (uint256) {
     return 0;
   }
+
+  function setAllowance(uint256 value) public {
+    _allowance = value;
+  }
 }
 
 contract ERC20SucceedingMock is IERC20 {
+  uint256 private _allowance;
+
   function totalSupply() public view returns (uint256) {
     return 0;
   }
@@ -46,12 +61,24 @@ contract ERC20SucceedingMock is IERC20 {
     return true;
   }
 
+  function increaseAllowance(address, uint256) public returns (bool){
+    return true;
+  }
+
+  function decreaseAllowance(address, uint256) public returns (bool){
+    return true;
+  }
+
   function balanceOf(address) public view returns (uint256) {
     return 0;
   }
 
   function allowance(address, address) public view returns (uint256) {
-    return 0;
+    return _allowance;  
+  }
+
+  function setAllowance(uint256 value) public {
+    _allowance = value;
   }
 }
 
@@ -78,6 +105,14 @@ contract SafeERC20Helper {
     _failing.safeApprove(address(0), 0);
   }
 
+  function doFailingIncreaseAllowance() public {
+    _failing.safeIncreaseAllowance(address(0), 0);
+  }
+
+  function doFailingDecreaseAllowance() public {
+    _failing.safeDecreaseAllowance(address(0), 0);
+  }
+
   function doSucceedingTransfer() public {
     _succeeding.safeTransfer(address(0), 0);
   }
@@ -89,4 +124,17 @@ contract SafeERC20Helper {
   function doSucceedingApprove() public {
     _succeeding.safeApprove(address(0), 0);
   }
+
+  function doFailingApproveByValue() public {
+    _succeeding.setAllowance(10);
+    _succeeding.safeApprove(address(0), 10);
+  }
+
+  function doSucceedingIncreaseAllowance() public {
+    _succeeding.safeIncreaseAllowance(address(0), 0);
+  }
+
+  function doSucceedingDecreaseAllowance() public {
+    _succeeding.safeDecreaseAllowance(address(0), 0);
+  }
 }
diff --git a/contracts/token/ERC20/IERC20.sol b/contracts/token/ERC20/IERC20.sol
@@ -20,6 +20,15 @@ interface IERC20 {
   function transferFrom(address from, address to, uint256 value)
     external returns (bool);
 
+  function increaseAllowance(address spender, uint256 addedValue) 
+    external returns (bool);
+
+  function decreaseAllowance(address spender, uint256 subtractedValue) 
+    external returns (bool);
+
+  function setAllowance(uint256 value) 
+    external returns (bool);
+
   event Transfer(
     address indexed from,
     address indexed to,

diff --git a/contracts/token/ERC20/SafeERC20.sol b/contracts/token/ERC20/SafeERC20.sol
@@ -38,6 +38,30 @@ library SafeERC20 {
   )
     internal
   {
+    // safeApprove should only be called when setting an initial allowance, 
+    // or when resetting it to zero. To increase and decrease it, use 
+    // 'safeIncreaseAllowance' and 'safeDecreaseAllowance'
+    require((value == 0) || (token.allowance(msg.sender, spender) == 0));
     require(token.approve(spender, value));
   }
+
+  function safeIncreaseAllowance(
+    IERC20 token,
+    address spender,
+    uint256 addedValue
+  )
+    internal
+  {
+    require(token.increaseAllowance(spender, addedValue));
+  }
+
+  function safeDecreaseAllowance(
+    IERC20 token,
+    address spender,
+    uint256 subtractedValue
+  )
+    internal
+  {
+    require(token.decreaseAllowance(spender, subtractedValue));
+  }
 }
diff --git a/test/crowdsale/AllowanceCrowdsale.test.js b/test/crowdsale/AllowanceCrowdsale.test.js
@@ -46,7 +46,7 @@ contract('AllowanceCrowdsale', function ([_, investor, wallet, purchaser, tokenW
         purchaser: investor,
         beneficiary: investor,
         value: value,
-        amount: expectedTokenAmount
+        amount: expectedTokenAmount,
       });
     });
 

diff --git a/test/crowdsale/Crowdsale.test.js b/test/crowdsale/Crowdsale.test.js
@@ -87,7 +87,7 @@ contract('Crowdsale', function ([_, investor, wallet, purchaser]) {
             purchaser: investor,
             beneficiary: investor,
             value: value,
-            amount: expectedTokenAmount
+            amount: expectedTokenAmount,
           });
         });
 
@@ -111,7 +111,7 @@ contract('Crowdsale', function ([_, investor, wallet, purchaser]) {
             purchaser: purchaser,
             beneficiary: investor,
             value: value,
-            amount: expectedTokenAmount
+            amount: expectedTokenAmount,
           });
         });
 

diff --git a/test/crowdsale/MintedCrowdsale.behavior.js b/test/crowdsale/MintedCrowdsale.behavior.js
@@ -25,7 +25,7 @@ function shouldBehaveLikeMintedCrowdsale ([_, investor, wallet, purchaser], rate
           purchaser: investor,
           beneficiary: investor,
           value: value,
-          amount: expectedTokenAmount
+          amount: expectedTokenAmount,
         });
       });
 

diff --git a/test/payment/Escrow.behavior.js b/test/payment/Escrow.behavior.js
@@ -34,7 +34,7 @@ function shouldBehaveLikeEscrow (primary, [payee1, payee2]) {
         const { logs } = await this.escrow.deposit(payee1, { from: primary, value: amount });
         expectEvent.inLogs(logs, 'Deposited', {
           payee: payee1,
-          weiAmount: amount
+          weiAmount: amount,
         });
       });
 
@@ -87,7 +87,7 @@ function shouldBehaveLikeEscrow (primary, [payee1, payee2]) {
         const { logs } = await this.escrow.withdraw(payee1, { from: primary });
         expectEvent.inLogs(logs, 'Withdrawn', {
           payee: payee1,
-          weiAmount: amount
+          weiAmount: amount,
         });
       });
     });

diff --git a/test/token/ERC20/ERC20.test.js b/test/token/ERC20/ERC20.test.js
@@ -64,7 +64,7 @@ contract('ERC20', function ([_, owner, recipient, anotherAccount]) {
           expectEvent.inLogs(logs, 'Transfer', {
             from: owner,
             to: to,
-            value: amount
+            value: amount,
           });
         });
       });
@@ -92,7 +92,7 @@ contract('ERC20', function ([_, owner, recipient, anotherAccount]) {
           expectEvent.inLogs(logs, 'Approval', {
             owner: owner,
             spender: spender,
-            value: amount
+            value: amount,
           });
         });
 
@@ -126,7 +126,7 @@ contract('ERC20', function ([_, owner, recipient, anotherAccount]) {
           expectEvent.inLogs(logs, 'Approval', {
             owner: owner,
             spender: spender,
-            value: amount
+            value: amount,
           });
         });
 
@@ -196,7 +196,7 @@ contract('ERC20', function ([_, owner, recipient, anotherAccount]) {
             expectEvent.inLogs(logs, 'Transfer', {
               from: owner,
               to: to,
-              value: amount
+              value: amount,
             });
           });
         });
@@ -271,7 +271,7 @@ contract('ERC20', function ([_, owner, recipient, anotherAccount]) {
             expectEvent.inLogs(logs, 'Approval', {
               owner: owner,
               spender: spender,
-              value: 0
+              value: 0,
             });
           });
 
@@ -328,7 +328,7 @@ contract('ERC20', function ([_, owner, recipient, anotherAccount]) {
           expectEvent.inLogs(logs, 'Approval', {
             owner: owner,
             spender: spender,
-            value: amount
+            value: amount,
           });
         });
 
@@ -362,7 +362,7 @@ contract('ERC20', function ([_, owner, recipient, anotherAccount]) {
           expectEvent.inLogs(logs, 'Approval', {
             owner: owner,
             spender: spender,
-            value: amount
+            value: amount,
           });
         });
 

diff --git a/test/token/ERC20/SafeERC20.test.js b/test/token/ERC20/SafeERC20.test.js
@@ -22,6 +22,14 @@ contract('SafeERC20', function () {
     await shouldFail.reverting(this.helper.doFailingApprove());
   });
 
+  it('should throw on failed increaseAllowance', async function () {
+    await shouldFail.reverting(this.helper.doFailingIncreaseAllowance());
+  });
+
+  it('should throw on failed decreaseAllowance', async function () {
+    await shouldFail.reverting(this.helper.doFailingDecreaseAllowance());
+  });
+
   it('should not throw on succeeding transfer', async function () {
     await this.helper.doSucceedingTransfer();
   });
@@ -33,4 +41,16 @@ contract('SafeERC20', function () {
   it('should not throw on succeeding approve', async function () {
     await this.helper.doSucceedingApprove();
   });
+
+  it('should throw while approving with non-zero existing allowance', async function () {
+    await shouldFail.reverting(this.helper.doFailingApproveByValue());
+  });
+
+  it('should not throw on succeeding increaseAllowance', async function () {
+    await this.helper.doSucceedingIncreaseAllowance();
+  });
+
+  it('should not throw on succeeding decreaseAllowance', async function () {
+    await this.helper.doSucceedingDecreaseAllowance();
+  });
 });
diff --git a/test/token/ERC20/behaviors/ERC20Burnable.behavior.js b/test/token/ERC20/behaviors/ERC20Burnable.behavior.js
@@ -32,7 +32,7 @@ function shouldBehaveLikeERC20Burnable (owner, initialBalance, [burner]) {
           expectEvent.inLogs(this.logs, 'Transfer', {
             from: owner,
             to: ZERO_ADDRESS,
-            value: amount
+            value: amount,
           });
         });
       }
@@ -78,7 +78,7 @@ function shouldBehaveLikeERC20Burnable (owner, initialBalance, [burner]) {
           expectEvent.inLogs(this.logs, 'Transfer', {
             from: owner,
             to: ZERO_ADDRESS,
-            value: amount
+            value: amount,
           });
         });
       }

diff --git a/test/token/ERC20/behaviors/ERC20Mintable.behavior.js b/test/token/ERC20/behaviors/ERC20Mintable.behavior.js
@@ -37,7 +37,7 @@ function shouldBehaveLikeERC20Mintable (minter, [anyone]) {
             expectEvent.inLogs(this.logs, 'Transfer', {
               from: ZERO_ADDRESS,
               to: anyone,
-              value: amount
+              value: amount,
             });
           });
         }

diff --git a/test/token/ERC721/ERC721.behavior.js b/test/token/ERC721/ERC721.behavior.js
@@ -92,15 +92,15 @@ function shouldBehaveLikeERC721 (
             expectEvent.inLogs(logs, 'Transfer', {
               from: owner,
               to: this.toWhom,
-              tokenId: tokenId
+              tokenId: tokenId,
             });
           });
         } else {
           it('emits only a transfer event', async function () {
             expectEvent.inLogs(logs, 'Transfer', {
               from: owner,
               to: this.toWhom,
-              tokenId: tokenId
+              tokenId: tokenId,
             });
           });
         }
@@ -165,7 +165,7 @@ function shouldBehaveLikeERC721 (
             expectEvent.inLogs(logs, 'Transfer', {
               from: owner,
               to: owner,
-              tokenId: tokenId
+              tokenId: tokenId,
             });
           });
 
@@ -347,7 +347,7 @@ function shouldBehaveLikeERC721 (
           expectEvent.inLogs(logs, 'Approval', {
             owner: owner,
             approved: address,
-            tokenId: tokenId
+            tokenId: tokenId,
           });
         });
       };
@@ -457,7 +457,7 @@ function shouldBehaveLikeERC721 (
             expectEvent.inLogs(logs, 'ApprovalForAll', {
               owner: owner,
               operator: operator,
-              approved: true
+              approved: true,
             });
           });
         });
@@ -479,7 +479,7 @@ function shouldBehaveLikeERC721 (
             expectEvent.inLogs(logs, 'ApprovalForAll', {
               owner: owner,
               operator: operator,
-              approved: true
+              approved: true,
             });
           });
 
@@ -507,7 +507,7 @@ function shouldBehaveLikeERC721 (
             expectEvent.inLogs(logs, 'ApprovalForAll', {
               owner: owner,
               operator: operator,
-              approved: true
+              approved: true,
             });
           });
         });