Add a SafeERC20:safePermit function (#3280)

CHANGELOG.md

@@ -6,6 +6,7 @@
  * `CrossChainEnabledPolygonChild`: replace the `require` statement with the custom error `NotCrossChainCall`. ([#3380](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3380))
  * `ERC20FlashMint`: Add customizable flash fee receiver. ([#3327](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3327))
  * `ERC20TokenizedVault`: add an extension of `ERC20` that implements the ERC4626 Tokenized Vault Standard. ([#3171](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3171))
+ * `SafeERC20`: add `safePermit` as mitigation against phantom permit functions. ([#3280](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3280))
  * `Math`: add a `mulDiv` function that can round the result either up or down. ([#3171](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3171))
  * `Math`: Add a `sqrt` function to compute square roots of integers, rounding either up or down. ([#3242](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3242))
  * `Strings`: add a new overloaded function `toHexString` that converts an `address` with fixed length of 20 bytes to its not checksummed ASCII `string` hexadecimal representation. ([#3403](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3403))

contracts/mocks/SafeERC20Helper.sol

@@ -4,6 +4,7 @@ pragma solidity ^0.8.0;
 
 import "../utils/Context.sol";
 import "../token/ERC20/IERC20.sol";
+import "../token/ERC20/extensions/draft-ERC20Permit.sol";
 import "../token/ERC20/utils/SafeERC20.sol";
 
 contract ERC20ReturnFalseMock is Context {
@@ -105,6 +106,43 @@ contract ERC20NoReturnMock is Context {
     }
 }
 
+contract ERC20PermitNoRevertMock is
+    ERC20("ERC20PermitNoRevertMock", "ERC20PermitNoRevertMock"),
+    ERC20Permit("ERC20PermitNoRevertMock")
+{
+    function getChainId() external view returns (uint256) {
+        return block.chainid;
+    }
+
+    function permitThatMayRevert(
+        address owner,
+        address spender,
+        uint256 value,
+        uint256 deadline,
+        uint8 v,
+        bytes32 r,
+        bytes32 s
+    ) public virtual {
+        super.permit(owner, spender, value, deadline, v, r, s);
+    }
+
+    function permit(
+        address owner,
+        address spender,
+        uint256 value,
+        uint256 deadline,
+        uint8 v,
+        bytes32 r,
+        bytes32 s
+    ) public virtual override {
+        try this.permitThatMayRevert(owner, spender, value, deadline, v, r, s) {
+            // do nothing
+        } catch {
+            // do nothing
+        }
+    }
+}
+
 contract SafeERC20Wrapper is Context {
     using SafeERC20 for IERC20;
 
@@ -134,6 +172,18 @@ contract SafeERC20Wrapper is Context {
         _token.safeDecreaseAllowance(address(0), amount);
     }
 
+    function permit(
+        address owner,
+        address spender,
+        uint256 value,
+        uint256 deadline,
+        uint8 v,
+        bytes32 r,
+        bytes32 s
+    ) public {
+        SafeERC20.safePermit(IERC20Permit(address(_token)), owner, spender, value, deadline, v, r, s);
+    }
+
     function setAllowance(uint256 allowance_) public {
         ERC20ReturnTrueMock(address(_token)).setAllowance(allowance_);
     }

contracts/token/ERC20/utils/SafeERC20.sol

@@ -4,6 +4,7 @@
 pragma solidity ^0.8.0;
 
 import "../IERC20.sol";
+import "../extensions/draft-IERC20Permit.sol";
 import "../../../utils/Address.sol";
 
 /**
@@ -79,6 +80,22 @@ library SafeERC20 {
         }
     }
 
+    function safePermit(
+        IERC20Permit token,
+        address owner,
+        address spender,
+        uint256 value,
+        uint256 deadline,
+        uint8 v,
+        bytes32 r,
+        bytes32 s
+    ) internal {
+        uint256 nonceBefore = token.nonces(owner);
+        token.permit(owner, spender, value, deadline, v, r, s);
+        uint256 nonceAfter = token.nonces(owner);
+        require(nonceAfter == nonceBefore + 1, "SafeERC20: permit 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).

test/helpers/eip712.js

@@ -7,6 +7,14 @@ const EIP712Domain = [
   { name: 'verifyingContract', type: 'address' },
 ];
 
+const Permit = [
+  { name: 'owner', type: 'address' },
+  { name: 'spender', type: 'address' },
+  { name: 'value', type: 'uint256' },
+  { name: 'nonce', type: 'uint256' },
+  { name: 'deadline', type: 'uint256' },
+];
+
 async function domainSeparator (name, version, chainId, verifyingContract) {
   return '0x' + ethSigUtil.TypedDataUtils.hashStruct(
     'EIP712Domain',
@@ -17,5 +25,6 @@ async function domainSeparator (name, version, chainId, verifyingContract) {
 
 module.exports = {
   EIP712Domain,
+  Permit,
   domainSeparator,
 };

test/token/ERC20/extensions/draft-ERC20Permit.test.js

@@ -10,15 +10,7 @@ const Wallet = require('ethereumjs-wallet').default;
 
 const ERC20PermitMock = artifacts.require('ERC20PermitMock');
 
-const { EIP712Domain, domainSeparator } = require('../../../helpers/eip712');
-
-const Permit = [
-  { name: 'owner', type: 'address' },
-  { name: 'spender', type: 'address' },
-  { name: 'value', type: 'uint256' },
-  { name: 'nonce', type: 'uint256' },
-  { name: 'deadline', type: 'uint256' },
-];
+const { EIP712Domain, Permit, domainSeparator } = require('../../../helpers/eip712');
 
 contract('ERC20Permit', function (accounts) {
   const [ initialHolder, spender, recipient, other ] = accounts;

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

@@ -1,10 +1,17 @@
-const { expectRevert } = require('@openzeppelin/test-helpers');
+const { constants, expectRevert } = require('@openzeppelin/test-helpers');
 
 const ERC20ReturnFalseMock = artifacts.require('ERC20ReturnFalseMock');
 const ERC20ReturnTrueMock = artifacts.require('ERC20ReturnTrueMock');
 const ERC20NoReturnMock = artifacts.require('ERC20NoReturnMock');
+const ERC20PermitNoRevertMock = artifacts.require('ERC20PermitNoRevertMock');
 const SafeERC20Wrapper = artifacts.require('SafeERC20Wrapper');
 
+const { EIP712Domain, Permit } = require('../../../helpers/eip712');
+
+const { fromRpcSig } = require('ethereumjs-util');
+const ethSigUtil = require('eth-sig-util');
+const Wallet = require('ethereumjs-wallet').default;
+
 contract('SafeERC20', function (accounts) {
   const [ hasNoCode ] = accounts;
 
@@ -39,6 +46,119 @@ contract('SafeERC20', function (accounts) {
 
     shouldOnlyRevertOnErrors();
   });
+
+  describe('with token that doesn\'t revert on invalid permit', function () {
+    const wallet = Wallet.generate();
+    const owner = wallet.getAddressString();
+    const spender = hasNoCode;
+
+    beforeEach(async function () {
+      this.token = await ERC20PermitNoRevertMock.new();
+      this.wrapper = await SafeERC20Wrapper.new(this.token.address);
+
+      const chainId = await this.token.getChainId();
+
+      this.data = {
+        primaryType: 'Permit',
+        types: { EIP712Domain, Permit },
+        domain: { name: 'ERC20PermitNoRevertMock', version: '1', chainId, verifyingContract: this.token.address },
+        message: { owner, spender, value: '42', nonce: '0', deadline: constants.MAX_UINT256 },
+      };
+      this.signature = fromRpcSig(ethSigUtil.signTypedMessage(wallet.getPrivateKey(), { data: this.data }));
+    });
+
+    it('accepts owner signature', async function () {
+      expect(await this.token.nonces(owner)).to.be.bignumber.equal('0');
+      expect(await this.token.allowance(owner, spender)).to.be.bignumber.equal('0');
+
+      await this.wrapper.permit(
+        this.data.message.owner,
+        this.data.message.spender,
+        this.data.message.value,
+        this.data.message.deadline,
+        this.signature.v,
+        this.signature.r,
+        this.signature.s,
+      );
+
+      expect(await this.token.nonces(owner)).to.be.bignumber.equal('1');
+      expect(await this.token.allowance(owner, spender)).to.be.bignumber.equal(this.data.message.value);
+    });
+
+    it('revert on reused signature', async function () {
+      expect(await this.token.nonces(owner)).to.be.bignumber.equal('0');
+      // use valid signature and consume nounce
+      await this.wrapper.permit(
+        this.data.message.owner,
+        this.data.message.spender,
+        this.data.message.value,
+        this.data.message.deadline,
+        this.signature.v,
+        this.signature.r,
+        this.signature.s,
+      );
+      expect(await this.token.nonces(owner)).to.be.bignumber.equal('1');
+      // invalid call does not revert for this token implementation
+      await this.token.permit(
+        this.data.message.owner,
+        this.data.message.spender,
+        this.data.message.value,
+        this.data.message.deadline,
+        this.signature.v,
+        this.signature.r,
+        this.signature.s,
+      );
+      expect(await this.token.nonces(owner)).to.be.bignumber.equal('1');
+      // invalid call revert when called through the SafeERC20 library
+      await expectRevert(
+        this.wrapper.permit(
+          this.data.message.owner,
+          this.data.message.spender,
+          this.data.message.value,
+          this.data.message.deadline,
+          this.signature.v,
+          this.signature.r,
+          this.signature.s,
+        ),
+        'SafeERC20: permit did not succeed',
+      );
+      expect(await this.token.nonces(owner)).to.be.bignumber.equal('1');
+    });
+
+    it('revert on invalid signature', async function () {
+      // signature that is not valid for owner
+      const invalidSignature = {
+        v: 27,
+        r: '0x71753dc5ecb5b4bfc0e3bc530d79ce5988760ed3f3a234c86a5546491f540775',
+        s: '0x0049cedee5aed990aabed5ad6a9f6e3c565b63379894b5fa8b512eb2b79e485d',
+      };
+
+      // invalid call does not revert for this token implementation
+      await this.token.permit(
+        this.data.message.owner,
+        this.data.message.spender,
+        this.data.message.value,
+        this.data.message.deadline,
+        invalidSignature.v,
+        invalidSignature.r,
+        invalidSignature.s,
+      );
+
+      // invalid call revert when called through the SafeERC20 library
+      await expectRevert(
+        this.wrapper.permit(
+          this.data.message.owner,
+          this.data.message.spender,
+          this.data.message.value,
+          this.data.message.deadline,
+          invalidSignature.v,
+          invalidSignature.r,
+          invalidSignature.s,
+        ),
+        'SafeERC20: permit did not succeed',
+      );
+    });
+  });
 });
 
 function shouldRevertOnAllCalls (reason) {