Improved SafeERC20 allowance handling (#1407)

* signing prefix added * Minor improvement * Tests changed * Successfully tested * Minor improvements * Minor improvements * Revert "Dangling commas are now required. (#1359)" This reverts commit a688977. * updates * fixes #1404 * approve failing test * suggested changes done * ISafeERC20 removed * allowance methods in library * Improved SafeERC20 tests. * Fixed test coverage. (cherry picked from commit 315f426)
OpenZeppelin · Oct 20, 2018 · 1b27b4b · 1b27b4b
1 parent 4115686
commit 1b27b4b
Show file tree

Hide file tree

Showing 3 changed files with 142 additions and 35 deletions.
diff --git a/contracts/mocks/SafeERC20Helper.sol b/contracts/mocks/SafeERC20Helper.sol
@@ -3,10 +3,8 @@ pragma solidity ^0.4.24;
 import "../token/ERC20/IERC20.sol";
 import "../token/ERC20/SafeERC20.sol";
 
-contract ERC20FailingMock is IERC20 {
-  function totalSupply() public view returns (uint256) {
-    return 0;
-  }
+contract ERC20FailingMock {
+  uint256 private _allowance;
 
   function transfer(address, uint256) public returns (bool) {
     return false;
@@ -20,19 +18,13 @@ contract ERC20FailingMock is IERC20 {
     return false;
   }
 
-  function balanceOf(address) public view returns (uint256) {
-    return 0;
-  }
-
   function allowance(address, address) public view returns (uint256) {
     return 0;
   }
 }
 
-contract ERC20SucceedingMock is IERC20 {
-  function totalSupply() public view returns (uint256) {
-    return 0;
-  }
+contract ERC20SucceedingMock {
+  uint256 private _allowance;
 
   function transfer(address, uint256) public returns (bool) {
     return true;
@@ -46,12 +38,12 @@ contract ERC20SucceedingMock is IERC20 {
     return true;
   }
 
-  function balanceOf(address) public view returns (uint256) {
-    return 0;
+  function setAllowance(uint256 allowance_) public {
+    _allowance = allowance_;
   }
 
   function allowance(address, address) public view returns (uint256) {
-    return 0;
+    return _allowance;
   }
 }
 
@@ -62,10 +54,12 @@ contract SafeERC20Helper {
   IERC20 private _succeeding;
 
   constructor() public {
-    _failing = new ERC20FailingMock();
-    _succeeding = new ERC20SucceedingMock();
+    _failing = IERC20(new ERC20FailingMock());
+    _succeeding = IERC20(new ERC20SucceedingMock());
   }
 
+  // Using _failing
+
   function doFailingTransfer() public {
     _failing.safeTransfer(address(0), 0);
   }
@@ -78,6 +72,16 @@ contract SafeERC20Helper {
     _failing.safeApprove(address(0), 0);
   }
 
+  function doFailingIncreaseAllowance() public {
+    _failing.safeIncreaseAllowance(address(0), 0);
+  }
+
+  function doFailingDecreaseAllowance() public {
+    _failing.safeDecreaseAllowance(address(0), 0);
+  }
+
+  // Using _succeeding
+
   function doSucceedingTransfer() public {
     _succeeding.safeTransfer(address(0), 0);
   }
@@ -86,7 +90,23 @@ contract SafeERC20Helper {
     _succeeding.safeTransferFrom(address(0), address(0), 0);
   }
 
-  function doSucceedingApprove() public {
-    _succeeding.safeApprove(address(0), 0);
+  function doSucceedingApprove(uint256 amount) public {
+    _succeeding.safeApprove(address(0), amount);
+  }
+
+  function doSucceedingIncreaseAllowance(uint256 amount) public {
+    _succeeding.safeIncreaseAllowance(address(0), amount);
+  }
+
+  function doSucceedingDecreaseAllowance(uint256 amount) public {
+    _succeeding.safeDecreaseAllowance(address(0), amount);
+  }
+
+  function setAllowance(uint256 allowance_) public {
+    ERC20SucceedingMock(_succeeding).setAllowance(allowance_);
+  }
+
+  function allowance() public view returns (uint256) {
+    return _succeeding.allowance(address(0), address(0));
   }
 }
diff --git a/contracts/token/ERC20/SafeERC20.sol b/contracts/token/ERC20/SafeERC20.sol
@@ -10,6 +10,9 @@ import "./IERC20.sol";
  * which allows you to call the safe operations as `token.safeTransfer(...)`, etc.
  */
 library SafeERC20 {
+
+  using SafeMath for uint256;
+
   function safeTransfer(
     IERC20 token,
     address to,
@@ -38,6 +41,32 @@ 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 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));
+  }
 }
diff --git a/test/token/ERC20/SafeERC20.test.js b/test/token/ERC20/SafeERC20.test.js
@@ -10,27 +10,85 @@ contract('SafeERC20', function () {
     this.helper = await SafeERC20Helper.new();
   });
 
-  it('should throw on failed transfer', async function () {
-    await shouldFail.reverting(this.helper.doFailingTransfer());
-  });
+  describe('with token that returns false on all calls', function () {
+    it('reverts on transfer', async function () {
+      await shouldFail.reverting(this.helper.doFailingTransfer());
+    });
 
-  it('should throw on failed transferFrom', async function () {
-    await shouldFail.reverting(this.helper.doFailingTransferFrom());
-  });
+    it('reverts on transferFrom', async function () {
+      await shouldFail.reverting(this.helper.doFailingTransferFrom());
+    });
 
-  it('should throw on failed approve', async function () {
-    await shouldFail.reverting(this.helper.doFailingApprove());
-  });
+    it('reverts on approve', async function () {
+      await shouldFail.reverting(this.helper.doFailingApprove());
+    });
 
-  it('should not throw on succeeding transfer', async function () {
-    await this.helper.doSucceedingTransfer();
-  });
+    it('reverts on increaseAllowance', async function () {
+      await shouldFail.reverting(this.helper.doFailingIncreaseAllowance());
+    });
 
-  it('should not throw on succeeding transferFrom', async function () {
-    await this.helper.doSucceedingTransferFrom();
+    it('reverts on decreaseAllowance', async function () {
+      await shouldFail.reverting(this.helper.doFailingDecreaseAllowance());
+    });
   });
 
-  it('should not throw on succeeding approve', async function () {
-    await this.helper.doSucceedingApprove();
+  describe('with token that returns true on all calls', function () {
+    it('doesn\'t revert on transfer', async function () {
+      await this.helper.doSucceedingTransfer();
+    });
+
+    it('doesn\'t revert on transferFrom', async function () {
+      await this.helper.doSucceedingTransferFrom();
+    });
+
+    describe('approvals', function () {
+      context('with zero allowance', function () {
+        beforeEach(async function () {
+          await this.helper.setAllowance(0);
+        });
+
+        it('doesn\'t revert when approving a non-zero allowance', async function () {
+          await this.helper.doSucceedingApprove(100);
+        });
+
+        it('doesn\'t revert when approving a zero allowance', async function () {
+          await this.helper.doSucceedingApprove(0);
+        });
+
+        it('doesn\'t revert when increasing the allowance', async function () {
+          await this.helper.doSucceedingIncreaseAllowance(10);
+        });
+
+        it('reverts when decreasing the allowance', async function () {
+          await shouldFail.reverting(this.helper.doSucceedingDecreaseAllowance(10));
+        });
+      });
+
+      context('with non-zero allowance', function () {
+        beforeEach(async function () {
+          await this.helper.setAllowance(100);
+        });
+
+        it('reverts when approving a non-zero allowance', async function () {
+          await shouldFail.reverting(this.helper.doSucceedingApprove(20));
+        });
+
+        it('doesn\'t revert when approving a zero allowance', async function () {
+          await this.helper.doSucceedingApprove(0);
+        });
+
+        it('doesn\'t revert when increasing the allowance', async function () {
+          await this.helper.doSucceedingIncreaseAllowance(10);
+        });
+
+        it('doesn\'t revert when decreasing the allowance to a positive value', async function () {
+          await this.helper.doSucceedingDecreaseAllowance(50);
+        });
+
+        it('reverts when decreasing the allowance to a negative value', async function () {
+          await shouldFail.reverting(this.helper.doSucceedingDecreaseAllowance(200));
+        });
+      });
+    });
   });
 });