fixes OpenZeppelin#1404

Aniket-Engg · Dec 21, 2018 · 9bb5670 · 9bb5670
1 parent 10abc85
commit 9bb5670
Show file tree

Hide file tree

Showing 13 changed files with 148 additions and 75 deletions.
diff --git a/contracts/mocks/SafeERC20Helper.sol b/contracts/mocks/SafeERC20Helper.sol
@@ -3,24 +3,38 @@ pragma solidity ^0.4.24;
 import "../token/ERC20/IERC20.sol";
 import "../token/ERC20/SafeERC20.sol";
 
-contract ERC20FailingMock {
-    uint256 private _allowance;
-
-    function transfer(address, uint256) public returns (bool) {
-        return false;
-    }
-
-    function transferFrom(address, address, uint256) public returns (bool) {
-        return false;
-    }
-
-    function approve(address, uint256) public returns (bool) {
-        return false;
-    }
-
-    function allowance(address, address) public view returns (uint256) {
-        return 0;
-    }
+contract ERC20FailingMock is IERC20 {
+  function totalSupply() public view returns (uint256) {
+    return 0;
+  }
+
+  function transfer(address, uint256) public returns (bool) {
+    return false;
+  }
+
+  function transferFrom(address, address, uint256) public returns (bool) {
+    return false;
+  }
+
+  function approve(address, uint256) public returns (bool) {
+    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;
+  }
 }
 
 contract ERC20SucceedingMock {
@@ -38,9 +52,17 @@ contract ERC20SucceedingMock {
         return true;
     }
 
-    function setAllowance(uint256 allowance_) public {
-        _allowance = allowance_;
-    }
+  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 _allowance;
@@ -98,15 +120,31 @@ contract SafeERC20Helper {
         _succeeding.safeIncreaseAllowance(address(0), amount);
     }
 
-    function doSucceedingDecreaseAllowance(uint256 amount) public {
-        _succeeding.safeDecreaseAllowance(address(0), amount);
-    }
+  function doFailingIncreaseAllowance() public {
+    _failing.safeIncreaseAllowance(address(0), 0);
+  }
+
+  function doFailingDecreaseAllowance() public {
+    _failing.safeDecreaseAllowance(address(0), 0);
+  }
+
+  function doSucceedingTransfer() public {
+    _succeeding.safeTransfer(address(0), 0);
+  }
 
     function setAllowance(uint256 allowance_) public {
         ERC20SucceedingMock(_succeeding).setAllowance(allowance_);
     }
 
-    function allowance() public view returns (uint256) {
-        return _succeeding.allowance(address(0), address(0));
-    }
+  function doSucceedingApprove() public {
+    _succeeding.safeApprove(address(0), 0);
+  }
+
+  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
@@ -17,7 +17,17 @@ interface IERC20 {
 
     function transferFrom(address from, address to, uint256 value) external returns (bool);
 
-    event Transfer(address indexed from, address indexed to, uint256 value);
+  function increaseAllowance(address spender, uint256 addedValue) 
+    external returns (bool);
+
+  function decreaseAllowance(address spender, uint256 subtractedValue) 
+    external returns (bool);
+
+  event Transfer(
+    address indexed from,
+    address indexed to,
+    uint256 value
+  );
 
     event Approval(address indexed owner, address indexed spender, uint256 value);
 }
diff --git a/contracts/token/ERC20/SafeERC20.sol b/contracts/token/ERC20/SafeERC20.sol
@@ -16,25 +16,34 @@ library SafeERC20 {
         require(token.transfer(to, value));
     }
 
-    function safeTransferFrom(IERC20 token, address from, address to, uint256 value) internal {
-        require(token.transferFrom(from, to, value));
-    }
+  function safeApprove(
+    IERC20 token,
+    address spender,
+    uint256 value
+  )
+    internal
+  {
+    require((value == 0) || (token.allowance(msg.sender, spender) == 0));
+    require(token.approve(spender, value));
+  }
 
-    function safeApprove(IERC20 token, address spender, uint256 value) 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 safeIncreaseAllowance(IERC20 token, address spender, uint256 value) internal {
-        uint256 newAllowance = token.allowance(address(this), spender).add(value);
-        require(token.approve(spender, newAllowance));
-    }
-
-    function safeDecreaseAllowance(IERC20 token, address spender, uint256 value) internal {
-        uint256 newAllowance = token.allowance(address(this), spender).sub(value);
-        require(token.approve(spender, newAllowance));
-    }
+  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
@@ -42,7 +42,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
@@ -83,7 +83,7 @@ contract('Crowdsale', function ([_, investor, wallet, purchaser]) {
             purchaser: investor,
             beneficiary: investor,
             value: value,
-            amount: expectedTokenAmount
+            amount: expectedTokenAmount,
           });
         });
 
@@ -106,7 +106,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
@@ -21,7 +21,7 @@ function shouldBehaveLikeMintedCrowdsale ([_, investor, wallet, purchaser], rate
           purchaser: investor,
           beneficiary: investor,
           value: value,
-          amount: expectedTokenAmount
+          amount: expectedTokenAmount,
         });
       });
 

diff --git a/test/payment/escrow/Escrow.behavior.js b/test/payment/escrow/Escrow.behavior.js
@@ -31,7 +31,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,
         });
       });
 
@@ -80,7 +80,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
@@ -60,7 +60,7 @@ contract('ERC20', function ([_, owner, recipient, anotherAccount]) {
           expectEvent.inLogs(logs, 'Transfer', {
             from: owner,
             to: to,
-            value: amount
+            value: amount,
           });
         });
       });
@@ -88,7 +88,7 @@ contract('ERC20', function ([_, owner, recipient, anotherAccount]) {
           expectEvent.inLogs(logs, 'Approval', {
             owner: owner,
             spender: spender,
-            value: amount
+            value: amount,
           });
         });
 
@@ -122,7 +122,7 @@ contract('ERC20', function ([_, owner, recipient, anotherAccount]) {
           expectEvent.inLogs(logs, 'Approval', {
             owner: owner,
             spender: spender,
-            value: amount
+            value: amount,
           });
         });
 
@@ -192,7 +192,7 @@ contract('ERC20', function ([_, owner, recipient, anotherAccount]) {
             expectEvent.inLogs(logs, 'Transfer', {
               from: owner,
               to: to,
-              value: amount
+              value: amount,
             });
           });
 
@@ -277,7 +277,7 @@ contract('ERC20', function ([_, owner, recipient, anotherAccount]) {
             expectEvent.inLogs(logs, 'Approval', {
               owner: owner,
               spender: spender,
-              value: 0
+              value: 0,
             });
           });
 
@@ -334,7 +334,7 @@ contract('ERC20', function ([_, owner, recipient, anotherAccount]) {
           expectEvent.inLogs(logs, 'Approval', {
             owner: owner,
             spender: spender,
-            value: amount
+            value: amount,
           });
         });
 
@@ -368,7 +368,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,9 +22,17 @@ contract('SafeERC20', function () {
       await shouldFail.reverting(this.helper.doFailingApprove());
     });
 
-    it('reverts on increaseAllowance', async function () {
-      await shouldFail.reverting(this.helper.doFailingIncreaseAllowance());
-    });
+  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();
+  });
 
     it('reverts on decreaseAllowance', async function () {
       await shouldFail.reverting(this.helper.doFailingDecreaseAllowance());
@@ -90,4 +98,12 @@ contract('SafeERC20', function () {
       });
     });
   });
+
+  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
@@ -28,7 +28,7 @@ function shouldBehaveLikeERC20Burnable (owner, initialBalance, [burner]) {
           expectEvent.inLogs(this.logs, 'Transfer', {
             from: owner,
             to: ZERO_ADDRESS,
-            value: amount
+            value: amount,
           });
         });
       }
@@ -74,7 +74,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
@@ -33,7 +33,7 @@ function shouldBehaveLikeERC20Mintable (minter, [anyone]) {
             expectEvent.inLogs(this.logs, 'Transfer', {
               from: ZERO_ADDRESS,
               to: anyone,
-              value: amount
+              value: amount,
             });
           });
         }