From 095c8e120c0cc0d9003db358708740bcbc4276a1 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 11 Sep 2023 17:07:25 +0200 Subject: [PATCH] Remove SafeERC20.safePermit (#4582) Co-authored-by: Francisco --- .changeset/green-pumpkins-end.md | 5 + .../mocks/token/ERC20PermitNoRevertMock.sol | 35 ----- contracts/token/ERC20/README.adoc | 8 +- .../token/ERC20/extensions/ERC20Permit.sol | 6 +- .../token/ERC20/extensions/IERC20Permit.sol | 30 +++++ contracts/token/ERC20/utils/SafeERC20.sol | 22 ---- test/token/ERC20/utils/SafeERC20.test.js | 123 ------------------ 7 files changed, 43 insertions(+), 186 deletions(-) create mode 100644 .changeset/green-pumpkins-end.md delete mode 100644 contracts/mocks/token/ERC20PermitNoRevertMock.sol diff --git a/.changeset/green-pumpkins-end.md b/.changeset/green-pumpkins-end.md new file mode 100644 index 00000000000..03cfe023fc5 --- /dev/null +++ b/.changeset/green-pumpkins-end.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': major +--- + +`SafeERC20`: Removed `safePermit` in favor of documentation-only `permit` recommendations. diff --git a/contracts/mocks/token/ERC20PermitNoRevertMock.sol b/contracts/mocks/token/ERC20PermitNoRevertMock.sol deleted file mode 100644 index 64e82b6f39c..00000000000 --- a/contracts/mocks/token/ERC20PermitNoRevertMock.sol +++ /dev/null @@ -1,35 +0,0 @@ -// SPDX-License-Identifier: MIT - -pragma solidity ^0.8.20; - -import {ERC20Permit} from "../../token/ERC20/extensions/ERC20Permit.sol"; - -abstract contract ERC20PermitNoRevertMock is ERC20Permit { - 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 - } - } -} diff --git a/contracts/token/ERC20/README.adoc b/contracts/token/ERC20/README.adoc index 9482b581b4b..2c508802dad 100644 --- a/contracts/token/ERC20/README.adoc +++ b/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}} diff --git a/contracts/token/ERC20/extensions/ERC20Permit.sol b/contracts/token/ERC20/extensions/ERC20Permit.sol index d6efb477e95..4165fbaca62 100644 --- a/contracts/token/ERC20/extensions/ERC20Permit.sol +++ b/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) { diff --git a/contracts/token/ERC20/extensions/IERC20Permit.sol b/contracts/token/ERC20/extensions/IERC20Permit.sol index 23704100671..b3260f3053b 100644 --- a/contracts/token/ERC20/extensions/IERC20Permit.sol +++ b/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, diff --git a/contracts/token/ERC20/utils/SafeERC20.sol b/contracts/token/ERC20/utils/SafeERC20.sol index fcdbbae76f6..e8b699cb0e6 100644 --- a/contracts/token/ERC20/utils/SafeERC20.sol +++ b/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). diff --git a/test/token/ERC20/utils/SafeERC20.test.js b/test/token/ERC20/utils/SafeERC20.test.js index eb6e267550b..4ff27f14d39 100644 --- a/test/token/ERC20/utils/SafeERC20.test.js +++ b/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;