Add SafeERC20.forceApprove()

.changeset/small-terms-sleep.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': minor
+---
+
+`SafeERC20`: Add a `forceApprove` function to improve compatibility with tokens behaving like USDT.

contracts/mocks/token/ERC20ForceApproveMock.sol

@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: MIT
+
+pragma solidity ^0.8.0;
+
+import "../../token/ERC20/ERC20.sol";
+
+// contract that replicate USDT (0xdac17f958d2ee523a2206206994597c13d831ec7) approval beavior
+contract ERC20ForceApproveMock is ERC20("FakeUSDT", "FUSDT") {
+    function approve(address spender, uint256 amount) public virtual override returns (bool) {
+        require(amount == 0 || allowance(msg.sender, spender) == 0, "USDT approval failure");
+        return super.approve(spender, amount);
+    }
+}

contracts/token/ERC20/utils/SafeERC20.sol

@@ -47,18 +47,28 @@ library SafeERC20 {
 
     function safeIncreaseAllowance(IERC20 token, address spender, uint256 value) internal {
         uint256 newAllowance = token.allowance(address(this), spender) + value;
-        _callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, newAllowance));
+        forceApprove(token, spender, newAllowance);
     }
 
     function safeDecreaseAllowance(IERC20 token, address spender, uint256 value) internal {
         unchecked {
             uint256 oldAllowance = token.allowance(address(this), spender);
             require(oldAllowance >= value, "SafeERC20: decreased allowance below zero");
             uint256 newAllowance = oldAllowance - value;
-            _callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, newAllowance));
+            forceApprove(token, spender, newAllowance);
         }
     }
 
+    function forceApprove(IERC20 token, address spender, uint256 value) internal {
+        // solidity has lazy evaluation
+        require(
+            _callOptionalReturnBool(token, abi.encodeWithSelector(token.approve.selector, spender, value)) ||
+                (_callOptionalReturnBool(token, abi.encodeWithSelector(token.approve.selector, spender, 0)) &&
+                    _callOptionalReturnBool(token, abi.encodeWithSelector(token.approve.selector, spender, value))),
+            "SafeERC20: force approve failed"
+        );
+    }
+
     function safePermit(
         IERC20Permit token,
         address owner,
@@ -87,9 +97,23 @@ library SafeERC20 {
         // the target address contains contract code and also asserts for success in the low-level call.
 
         bytes memory returndata = address(token).functionCall(data, "SafeERC20: low-level call failed");
-        if (returndata.length > 0) {
-            // Return data is optional
-            require(abi.decode(returndata, (bool)), "SafeERC20: ERC20 operation did not succeed");
-        }
+        require(returndata.length == 0 || abi.decode(returndata, (bool)), "SafeERC20: ERC20 operation did not succeed");
+    }
+
+    /**
+     * @dev Imitates a Solidity high-level call (i.e. a regular function call to a contract), relaxing the requirement
+     * on the return value: the return value is optional (but if data is returned, it must not be false).
+     * @param token The token targeted by the call.
+     * @param data The call data (encoded using abi.encode or one of its variants).
+     *
+     * This is a variant of {_callOptionalReturn} that silents catches all reverts and returns a bool instead.
+     */
+    function _callOptionalReturnBool(IERC20 token, bytes memory data) private returns (bool) {
+        // We need to perform a low level call here, to bypass Solidity's return data size checking mechanism, since
+        // we're implementing it ourselves. We cannot use {Address-functionCall} here since this should return false
+        // and not revert is the subcall reverts.
+
+        (bool success, bytes memory returndata) = address(token).call(data);
+        return success && (returndata.length == 0 || abi.decode(returndata, (bool))) && Address.isContract(address(token));
     }
 }

package.json

@@ -72,7 +72,7 @@
     "glob": "^8.0.3",
     "graphlib": "^2.1.8",
     "hardhat": "^2.9.1",
-    "hardhat-exposed": "^0.3.1",
+    "hardhat-exposed": "^0.3.2",
     "hardhat-gas-reporter": "^1.0.4",
     "hardhat-ignore-warnings": "^0.2.0",
     "keccak256": "^1.0.2",

test/token/ERC20/utils/SafeERC20.test.js

@@ -5,6 +5,7 @@ const ERC20ReturnFalseMock = artifacts.require('ERC20ReturnFalseMock');
 const ERC20ReturnTrueMock = artifacts.require('ERC20ReturnTrueMock');
 const ERC20NoReturnMock = artifacts.require('ERC20NoReturnMock');
 const ERC20PermitNoRevertMock = artifacts.require('ERC20PermitNoRevertMock');
+const ERC20ForceApproveMock = artifacts.require('$ERC20ForceApproveMock');
 
 const { getDomain, domainType, Permit } = require('../../../helpers/eip712');
 
@@ -165,6 +166,43 @@ contract('SafeERC20', function (accounts) {
       );
     });
   });
+
+  describe('with usdt approval beaviour', function () {
+    const spender = hasNoCode;
+
+    beforeEach(async function () {
+      this.token = await ERC20ForceApproveMock.new();
+    });
+
+    describe('with initial approval', function () {
+      beforeEach(async function () {
+        await this.token.$_approve(this.mock.address, spender, 100);
+      });
+
+      it('safeApproval fails to update approval to non-zero', async function () {
+        await expectRevert(
+          this.mock.$safeApprove(this.token.address, spender, 200),
+          'SafeERC20: approve from non-zero to non-zero allowance',
+        );
+      });
+
+      it('safeApproval can update approval to zero', async function () {
+        await this.mock.$safeApprove(this.token.address, spender, 0);
+      });
+
+      it('safeApproval can increasse approval', async function () {
+        await this.mock.$safeIncreaseAllowance(this.token.address, spender, 10);
+      });
+
+      it('safeApproval can decreasse approval', async function () {
+        await this.mock.$safeDecreaseAllowance(this.token.address, spender, 10);
+      });
+
+      it('forceApproval works', async function () {
+        await this.mock.$forceApprove(this.token.address, spender, 200);
+      });
+    });
+  });
 });
 
 function shouldRevertOnAllCalls(reason) {
@@ -192,8 +230,17 @@ function shouldRevertOnAllCalls(reason) {
     // [TODO] make sure it's reverting for the right reason
     await expectRevert.unspecified(this.mock.$safeDecreaseAllowance(this.token.address, constants.ZERO_ADDRESS, 0));
   });
+
+  it('reverts on forceApprove', async function () {
+    // [TODO] make sure it's reverting for the right reason
+    await expectRevert.unspecified(this.mock.$forceApprove(this.token.address, constants.ZERO_ADDRESS, 0));
+  });
 }
 
+// This test is designed to work in conjunction with ERC20ReturnTrueMock, ERC20NoReturnMock, which don't include any
+// proper approval logic. This is why the "with non-zero allowance" case can use `token.setAllowance`. This would not
+// work with a proper ERC20 implementation has the allowance setting would have to be done by the caller of the
+// subsequent calls (this.mock).
 function shouldOnlyRevertOnErrors() {
   it("doesn't revert on transfer", async function () {
     await this.mock.$safeTransfer(this.token.address, constants.ZERO_ADDRESS, 0);
@@ -217,6 +264,14 @@ function shouldOnlyRevertOnErrors() {
         await this.mock.$safeApprove(this.token.address, constants.ZERO_ADDRESS, 0);
       });
 
+      it("doesn't revert when force approving a non-zero allowance", async function () {
+        await this.mock.$forceApprove(this.token.address, constants.ZERO_ADDRESS, 100);
+      });
+
+      it("doesn't revert when force approving a zero allowance", async function () {
+        await this.mock.$forceApprove(this.token.address, constants.ZERO_ADDRESS, 0);
+      });
+
       it("doesn't revert when increasing the allowance", async function () {
         await this.mock.$safeIncreaseAllowance(this.token.address, constants.ZERO_ADDRESS, 10);
       });
@@ -245,6 +300,14 @@ function shouldOnlyRevertOnErrors() {
         await this.mock.$safeApprove(this.token.address, constants.ZERO_ADDRESS, 0);
       });
 
+      it("doesn't revert when force approving a non-zero allowance", async function () {
+        await this.mock.$forceApprove(this.token.address, constants.ZERO_ADDRESS, 20);
+      });
+
+      it("doesn't revert when force approving a zero allowance", async function () {
+        await this.mock.$forceApprove(this.token.address, constants.ZERO_ADDRESS, 0);
+      });
+
       it("doesn't revert when increasing the allowance", async function () {
         await this.mock.$safeIncreaseAllowance(this.token.address, constants.ZERO_ADDRESS, 10);
       });