Remove SafeERC20.safePermit (#4582)

Co-authored-by: Francisco <fg@frang.io>

.changeset/green-pumpkins-end.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': major
+---
+
+`SafeERC20`: Removed `safePermit` in favor of documentation-only `permit` recommendations.

contracts/token/ERC20/README.adoc

@@ -15,10 +15,10 @@ There are a few core contracts that implement the behavior specified in the EIP:
 
 Additionally there are multiple custom extensions, including:
 
+* {ERC20Permit}: gasless approval of tokens (standardized as ERC2612).
 * {ERC20Burnable}: destruction of own tokens.
 * {ERC20Capped}: enforcement of a cap to the total supply when minting tokens.
 * {ERC20Pausable}: ability to pause token transfers.
-* {ERC20Permit}: gasless approval of tokens (standardized as ERC2612).
 * {ERC20FlashMint}: token level support for flash loans through the minting and burning of ephemeral tokens (standardized as ERC3156).
 * {ERC20Votes}: support for voting and vote delegation.
 * {ERC20Wrapper}: wrapper to create an ERC20 backed by another ERC20, with deposit and withdraw methods. Useful in conjunction with {ERC20Votes}.
@@ -44,14 +44,16 @@ NOTE: This core set of contracts is designed to be unopinionated, allowing devel
 
 == Extensions
 
+{{IERC20Permit}}
+
+{{ERC20Permit}}
+
 {{ERC20Burnable}}
 
 {{ERC20Capped}}
 
 {{ERC20Pausable}}
 
-{{ERC20Permit}}
-
 {{ERC20Votes}}
 
 {{ERC20Wrapper}}

contracts/token/ERC20/extensions/ERC20Permit.sol

@@ -39,7 +39,7 @@ abstract contract ERC20Permit is ERC20, IERC20Permit, EIP712, Nonces {
     constructor(string memory name) EIP712(name, "1") {}
 
     /**
-     * @dev See {IERC20Permit-permit}.
+     * @inheritdoc IERC20Permit
      */
     function permit(
         address owner,
@@ -67,14 +67,14 @@ abstract contract ERC20Permit is ERC20, IERC20Permit, EIP712, Nonces {
     }
 
     /**
-     * @dev See {IERC20Permit-nonces}.
+     * @inheritdoc IERC20Permit
      */
     function nonces(address owner) public view virtual override(IERC20Permit, Nonces) returns (uint256) {
         return super.nonces(owner);
     }
 
     /**
-     * @dev See {IERC20Permit-DOMAIN_SEPARATOR}.
+     * @inheritdoc IERC20Permit
      */
     // solhint-disable-next-line func-name-mixedcase
     function DOMAIN_SEPARATOR() external view virtual returns (bytes32) {

contracts/token/ERC20/extensions/IERC20Permit.sol

@@ -10,6 +10,34 @@ pragma solidity ^0.8.20;
  * Adds the {permit} method, which can be used to change an account's ERC20 allowance (see {IERC20-allowance}) by
  * presenting a message signed by the account. By not relying on {IERC20-approve}, the token holder account doesn't
  * need to send a transaction, and thus is not required to hold Ether at all.
+ *
+ * ==== Security Considerations
+ *
+ * There are two important considerations concerning the use of `permit`. The first is that a valid permit signature
+ * expresses an allowance, and it should not be assumed to convey additional meaning. In particular, it should not be
+ * considered as an intention to spend the allowance in any specific way. The second is that because permits have
+ * built-in replay protection and can be submitted by anyone, they can be frontrun. A protocol that uses permits should
+ * take this into consideration and allow a `permit` call to fail. Combining these two aspects, a pattern that may be
+ * generally recommended is:
+ *
+ * ```solidity
+ * function doThingWithPermit(..., uint256 value, uint256 deadline, uint8 v, bytes32 r, bytes32 s) public {
+ *     try token.permit(msg.sender, address(this), value, deadline, v, r, s) {} catch {}
+ *     doThing(..., value);
+ * }
+ *
+ * function doThing(..., uint256 value) public {
+ *     token.safeTransferFrom(msg.sender, address(this), value);
+ *     ...
+ * }
+ * ```
+ *
+ * Observe that: 1) `msg.sender` is used as the owner, leaving no ambiguity as to the signer intent, and 2) the use of
+ * `try/catch` allows the permit to fail and makes the code tolerant to frontrunning. (See also
+ * {SafeERC20-safeTransferFrom}).
+ *
+ * Additionally, note that smart contract wallets (such as Argent or Safe) are not able to produce permit signatures, so
+ * contracts should have entry points that don't rely on permit.
  */
 interface IERC20Permit {
     /**
@@ -32,6 +60,8 @@ interface IERC20Permit {
      * For more information on the signature format, see the
      * https://eips.ethereum.org/EIPS/eip-2612#specification[relevant EIP
      * section].
+     *
+     * CAUTION: See Security Considerations above.
      */
     function permit(
         address owner,

contracts/token/ERC20/utils/SafeERC20.sol

@@ -82,28 +82,6 @@ library SafeERC20 {
         }
     }
 
-    /**
-     * @dev Use a ERC-2612 signature to set the `owner` approval toward `spender` on `token`.
-     * Revert on invalid signature.
-     */
-    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);
-        if (nonceAfter != nonceBefore + 1) {
-            revert SafeERC20FailedOperation(address(token));
-        }
-    }
-
     /**
      * @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/token/ERC20/utils/SafeERC20.test.js

@@ -4,16 +4,10 @@ const SafeERC20 = artifacts.require('$SafeERC20');
 const ERC20ReturnFalseMock = artifacts.require('$ERC20ReturnFalseMock');
 const ERC20ReturnTrueMock = artifacts.require('$ERC20'); // default implementation returns true
 const ERC20NoReturnMock = artifacts.require('$ERC20NoReturnMock');
-const ERC20PermitNoRevertMock = artifacts.require('$ERC20PermitNoRevertMock');
 const ERC20ForceApproveMock = artifacts.require('$ERC20ForceApproveMock');
 
-const { getDomain, domainType, Permit } = require('../../../helpers/eip712');
 const { expectRevertCustomError } = require('../../../helpers/customError');
 
-const { fromRpcSig } = require('ethereumjs-util');
-const ethSigUtil = require('eth-sig-util');
-const Wallet = require('ethereumjs-wallet').default;
-
 const name = 'ERC20Mock';
 const symbol = 'ERC20Mock';
 
@@ -122,123 +116,6 @@ contract('SafeERC20', function (accounts) {
     shouldOnlyRevertOnErrors(accounts);
   });
 
-  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(name, symbol, name);
-
-      this.data = await getDomain(this.token).then(domain => ({
-        primaryType: 'Permit',
-        types: { EIP712Domain: domainType(domain), Permit },
-        domain,
-        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.mock.$safePermit(
-        this.token.address,
-        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.mock.$safePermit(
-        this.token.address,
-        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 expectRevertCustomError(
-        this.mock.$safePermit(
-          this.token.address,
-          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,
-        ),
-        'SafeERC20FailedOperation',
-        [this.token.address],
-      );
-      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 expectRevertCustomError(
-        this.mock.$safePermit(
-          this.token.address,
-          this.data.message.owner,
-          this.data.message.spender,
-          this.data.message.value,
-          this.data.message.deadline,
-          invalidSignature.v,
-          invalidSignature.r,
-          invalidSignature.s,
-        ),
-        'SafeERC20FailedOperation',
-        [this.token.address],
-      );
-    });
-  });
-
   describe('with usdt approval beaviour', function () {
     const spender = hasNoCode;