From 5f9a8e99ff0b717c59b5ec912ed81b28e059316c Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 7 Sep 2023 10:36:47 +0200 Subject: [PATCH 01/14] Remove SafeERC20.safePermit --- .../mocks/token/ERC20PermitNoRevertMock.sol | 35 ----- contracts/token/ERC20/utils/SafeERC20.sol | 22 ---- test/token/ERC20/utils/SafeERC20.test.js | 120 +----------------- 3 files changed, 1 insertion(+), 176 deletions(-) delete mode 100644 contracts/mocks/token/ERC20PermitNoRevertMock.sol 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/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..eef8251f163 100644 --- a/test/token/ERC20/utils/SafeERC20.test.js +++ b/test/token/ERC20/utils/SafeERC20.test.js @@ -4,10 +4,9 @@ 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 { getDomain, domainType } = require('../../../helpers/eip712'); const { expectRevertCustomError } = require('../../../helpers/customError'); const { fromRpcSig } = require('ethereumjs-util'); @@ -122,123 +121,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; From c15706fdfad12d4319a14c4aad7977afdf0bbaa1 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 7 Sep 2023 11:18:46 +0200 Subject: [PATCH 02/14] Document the danger of authentication using permit --- contracts/token/ERC20/extensions/IERC20Permit.sol | 3 +++ docs/modules/ROOT/pages/utilities.adoc | 12 ++++++++++++ 2 files changed, 15 insertions(+) diff --git a/contracts/token/ERC20/extensions/IERC20Permit.sol b/contracts/token/ERC20/extensions/IERC20Permit.sol index 23704100671..b26c1bd1f9a 100644 --- a/contracts/token/ERC20/extensions/IERC20Permit.sol +++ b/contracts/token/ERC20/extensions/IERC20Permit.sol @@ -32,6 +32,9 @@ interface IERC20Permit { * For more information on the signature format, see the * https://eips.ethereum.org/EIPS/eip-2612#specification[relevant EIP * section]. + * + * WARNING: relying on {permit} for user authentication is a bad practice. + * See xref:utilities.adoc#authentication-using-permit[this]. */ function permit( address owner, diff --git a/docs/modules/ROOT/pages/utilities.adoc b/docs/modules/ROOT/pages/utilities.adoc index 487b47a800d..d99f5817762 100644 --- a/docs/modules/ROOT/pages/utilities.adoc +++ b/docs/modules/ROOT/pages/utilities.adoc @@ -25,6 +25,18 @@ function _verify(bytes32 data, bytes memory signature, address account) internal WARNING: Getting signature verification right is not trivial: make sure you fully read and understand xref:api:utils.adoc#MessageHashUtils[`MessageHashUtils`]'s and xref:api:utils.adoc#ECDSA[`ECDSA`]'s documentation. +[[authentication-using-permit]] +==== On the use of ERC-2612 for authentication + +Some people have been tempted to rely on ERC-2612 (also known as `ERC20Permit`) to authenticate users using the `permit` function. This is not practice is not recommended for the following reasons: + +- A valid `permit` signature expresses an allowance. It should not be assumed to convey additional meaning. In particular, it should not be considered as an intention to spend the approval in any specific way. +- Some ERC-2612 implementations do not revert if the provided signature is invalid. They will just not set the allowance (and not increase the nonce). Detecting that a signature is invalid is not obvious to do. Checking that the allowance has the right value might not be enough, as it could be the result of another previous approval. One could check that the nonce was updated, as this would only happen if the provided signature was valid. +- Because of the built-in replay protection, any logic that requires a valid ERC-2616 signature to authenticate a user is subject to DOS. The user-provided signature can be extracted from the transaction in the mempool, and could be submitted by anyone to the ERC-2616 token (front-running). This would set the allowance, and increase the nonce, resulting in the initial transaction failing to perform the permit operation. +- Smart contract wallets (such as Argent or Gnosis Safe) would not be able to authenticate as ERC-2612 often does not come with ERC-1271 support. + +For all these reasons, we strongly recommend NOT relying on signed permits of user authentication. + === Verifying Merkle Proofs xref:api:utils.adoc#MerkleProof[`MerkleProof`] provides: From ddd60b300c9515e9cdc6b7a44d2fe17163800789 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 7 Sep 2023 11:20:14 +0200 Subject: [PATCH 03/14] add changeset --- .changeset/green-pumpkins-end.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/green-pumpkins-end.md diff --git a/.changeset/green-pumpkins-end.md b/.changeset/green-pumpkins-end.md new file mode 100644 index 00000000000..fa1b8081a20 --- /dev/null +++ b/.changeset/green-pumpkins-end.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': major +--- + +`SafeERC20Permit`: remove `safePermit` From cefc133b1a829975ec8506602883ace7e8a6268a Mon Sep 17 00:00:00 2001 From: Francisco Date: Thu, 7 Sep 2023 22:39:08 -0300 Subject: [PATCH 04/14] Update green-pumpkins-end.md --- .changeset/green-pumpkins-end.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/green-pumpkins-end.md b/.changeset/green-pumpkins-end.md index fa1b8081a20..c122e82f4b8 100644 --- a/.changeset/green-pumpkins-end.md +++ b/.changeset/green-pumpkins-end.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': major --- -`SafeERC20Permit`: remove `safePermit` +`SafeERC20`: Removed `safePermit` in favor of documenting the recommended `permit` usage pattern. From 2f77daf8b12742414b3e1bbe931316aa89eef080 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 7 Sep 2023 22:40:52 -0300 Subject: [PATCH 05/14] remove unused variables --- test/token/ERC20/utils/SafeERC20.test.js | 5 ----- 1 file changed, 5 deletions(-) diff --git a/test/token/ERC20/utils/SafeERC20.test.js b/test/token/ERC20/utils/SafeERC20.test.js index eef8251f163..4ff27f14d39 100644 --- a/test/token/ERC20/utils/SafeERC20.test.js +++ b/test/token/ERC20/utils/SafeERC20.test.js @@ -6,13 +6,8 @@ const ERC20ReturnTrueMock = artifacts.require('$ERC20'); // default implementati const ERC20NoReturnMock = artifacts.require('$ERC20NoReturnMock'); const ERC20ForceApproveMock = artifacts.require('$ERC20ForceApproveMock'); -const { getDomain, domainType } = 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'; From 92abe2cd19f41de7f2e05d814e7f221ed9790841 Mon Sep 17 00:00:00 2001 From: Francisco Date: Thu, 7 Sep 2023 22:43:56 -0300 Subject: [PATCH 06/14] Update green-pumpkins-end.md --- .changeset/green-pumpkins-end.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/green-pumpkins-end.md b/.changeset/green-pumpkins-end.md index c122e82f4b8..03cfe023fc5 100644 --- a/.changeset/green-pumpkins-end.md +++ b/.changeset/green-pumpkins-end.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': major --- -`SafeERC20`: Removed `safePermit` in favor of documenting the recommended `permit` usage pattern. +`SafeERC20`: Removed `safePermit` in favor of documentation-only `permit` recommendations. From a6e37ad30f6905f9f60b6f811a34a95e403dc44d Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 7 Sep 2023 22:46:52 -0300 Subject: [PATCH 07/14] use inheritdoc --- contracts/token/ERC20/extensions/ERC20Permit.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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) { From 9a8b2e8cd0d617d03493078eb7fa46f073a4de05 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 7 Sep 2023 22:48:46 -0300 Subject: [PATCH 08/14] add IERC20Permit on docs site --- contracts/token/ERC20/README.adoc | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) 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}} From db787f679223bdbfbe44c58506157c94a321ae7f Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 7 Sep 2023 23:34:25 -0300 Subject: [PATCH 09/14] move recommendations to IERC20Permit --- .../token/ERC20/extensions/IERC20Permit.sol | 20 +++++++++++++++++++ docs/modules/ROOT/pages/utilities.adoc | 12 ----------- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/contracts/token/ERC20/extensions/IERC20Permit.sol b/contracts/token/ERC20/extensions/IERC20Permit.sol index b26c1bd1f9a..5e3de589482 100644 --- a/contracts/token/ERC20/extensions/IERC20Permit.sol +++ b/contracts/token/ERC20/extensions/IERC20Permit.sol @@ -10,6 +10,26 @@ 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 approval 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 good pattern that can + * be generally recommended is: + * + * ``` + * try token.permit(msg.sender, spender, value, deadline, v, r, s) {} catch {} + * ``` + * + * 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. In general, `address(this)` would + * be used as the `spender` parameter. + * + * 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 { /** diff --git a/docs/modules/ROOT/pages/utilities.adoc b/docs/modules/ROOT/pages/utilities.adoc index d99f5817762..487b47a800d 100644 --- a/docs/modules/ROOT/pages/utilities.adoc +++ b/docs/modules/ROOT/pages/utilities.adoc @@ -25,18 +25,6 @@ function _verify(bytes32 data, bytes memory signature, address account) internal WARNING: Getting signature verification right is not trivial: make sure you fully read and understand xref:api:utils.adoc#MessageHashUtils[`MessageHashUtils`]'s and xref:api:utils.adoc#ECDSA[`ECDSA`]'s documentation. -[[authentication-using-permit]] -==== On the use of ERC-2612 for authentication - -Some people have been tempted to rely on ERC-2612 (also known as `ERC20Permit`) to authenticate users using the `permit` function. This is not practice is not recommended for the following reasons: - -- A valid `permit` signature expresses an allowance. It should not be assumed to convey additional meaning. In particular, it should not be considered as an intention to spend the approval in any specific way. -- Some ERC-2612 implementations do not revert if the provided signature is invalid. They will just not set the allowance (and not increase the nonce). Detecting that a signature is invalid is not obvious to do. Checking that the allowance has the right value might not be enough, as it could be the result of another previous approval. One could check that the nonce was updated, as this would only happen if the provided signature was valid. -- Because of the built-in replay protection, any logic that requires a valid ERC-2616 signature to authenticate a user is subject to DOS. The user-provided signature can be extracted from the transaction in the mempool, and could be submitted by anyone to the ERC-2616 token (front-running). This would set the allowance, and increase the nonce, resulting in the initial transaction failing to perform the permit operation. -- Smart contract wallets (such as Argent or Gnosis Safe) would not be able to authenticate as ERC-2612 often does not come with ERC-1271 support. - -For all these reasons, we strongly recommend NOT relying on signed permits of user authentication. - === Verifying Merkle Proofs xref:api:utils.adoc#MerkleProof[`MerkleProof`] provides: From 35664bf23ceced11103114d0b8d99ef93d1d9de5 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Fri, 8 Sep 2023 14:24:22 -0300 Subject: [PATCH 10/14] remove broken link --- contracts/token/ERC20/extensions/IERC20Permit.sol | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/contracts/token/ERC20/extensions/IERC20Permit.sol b/contracts/token/ERC20/extensions/IERC20Permit.sol index 5e3de589482..43779db0782 100644 --- a/contracts/token/ERC20/extensions/IERC20Permit.sol +++ b/contracts/token/ERC20/extensions/IERC20Permit.sol @@ -53,8 +53,7 @@ interface IERC20Permit { * https://eips.ethereum.org/EIPS/eip-2612#specification[relevant EIP * section]. * - * WARNING: relying on {permit} for user authentication is a bad practice. - * See xref:utilities.adoc#authentication-using-permit[this]. + * CAUTION: See Security Considerations above. */ function permit( address owner, From b09fb90ec3e93ae6a3fa847f40cf0cf69331d18d Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Fri, 8 Sep 2023 14:28:46 -0300 Subject: [PATCH 11/14] use full example --- contracts/token/ERC20/extensions/IERC20Permit.sol | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/contracts/token/ERC20/extensions/IERC20Permit.sol b/contracts/token/ERC20/extensions/IERC20Permit.sol index 43779db0782..14547d224a1 100644 --- a/contracts/token/ERC20/extensions/IERC20Permit.sol +++ b/contracts/token/ERC20/extensions/IERC20Permit.sol @@ -20,8 +20,11 @@ pragma solidity ^0.8.20; * take this into consideration and allow a `permit` call to fail. Combining these two aspects, a good pattern that can * be generally recommended is: * - * ``` - * try token.permit(msg.sender, spender, value, deadline, v, r, s) {} catch {} + * ```solidity + * function doThingWithPermit(..., uint256 value, uint256 deadline, uint8 v, bytes32 r, bytes32 s) { + * try token.permit(msg.sender, address(this), value, deadline, v, r, s) {} catch {} + * doThing(..., value); // will use token.transferFrom + * } * ``` * * Observe that: 1) `msg.sender` is used as the owner, leaving no ambiguity as to the signer intent, and 2) the use of From c7c46ea645991335ca38c568eec29b333d5db1d4 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Fri, 8 Sep 2023 14:33:26 -0300 Subject: [PATCH 12/14] expand example --- .../token/ERC20/extensions/IERC20Permit.sol | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/contracts/token/ERC20/extensions/IERC20Permit.sol b/contracts/token/ERC20/extensions/IERC20Permit.sol index 14547d224a1..2de1bd89f9a 100644 --- a/contracts/token/ERC20/extensions/IERC20Permit.sol +++ b/contracts/token/ERC20/extensions/IERC20Permit.sol @@ -17,19 +17,24 @@ pragma solidity ^0.8.20; * 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 approval 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 good pattern that can - * be generally recommended is: + * take this into consideration and allow a `permit` call to fail. Combining these two aspects, a good pattern that may + * be generally recommended is shown below: * * ```solidity - * function doThingWithPermit(..., uint256 value, uint256 deadline, uint8 v, bytes32 r, bytes32 s) { + * 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); // will use token.transferFrom + * 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. In general, `address(this)` would - * be used as the `spender` parameter. + * Observe that: 1) `msg.sender` is used as the owner, leaving no ambiguity as to the signer intent, 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. From c1c95aad4ff5c666b20f19ae79df589e07700eb3 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Fri, 8 Sep 2023 14:34:32 -0300 Subject: [PATCH 13/14] wording --- contracts/token/ERC20/extensions/IERC20Permit.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/token/ERC20/extensions/IERC20Permit.sol b/contracts/token/ERC20/extensions/IERC20Permit.sol index 2de1bd89f9a..7e49fa6d8e8 100644 --- a/contracts/token/ERC20/extensions/IERC20Permit.sol +++ b/contracts/token/ERC20/extensions/IERC20Permit.sol @@ -17,8 +17,8 @@ pragma solidity ^0.8.20; * 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 approval 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 good pattern that may - * be generally recommended is shown below: + * 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 { From 487b3f9cdd0561e151d4965ca18590bba907209a Mon Sep 17 00:00:00 2001 From: Francisco Date: Mon, 11 Sep 2023 11:23:38 -0300 Subject: [PATCH 14/14] Update IERC20Permit.sol --- contracts/token/ERC20/extensions/IERC20Permit.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/token/ERC20/extensions/IERC20Permit.sol b/contracts/token/ERC20/extensions/IERC20Permit.sol index 7e49fa6d8e8..b3260f3053b 100644 --- a/contracts/token/ERC20/extensions/IERC20Permit.sol +++ b/contracts/token/ERC20/extensions/IERC20Permit.sol @@ -15,7 +15,7 @@ pragma solidity ^0.8.20; * * 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 approval in any specific way. The second is that because permits have + * 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: @@ -32,7 +32,7 @@ pragma solidity ^0.8.20; * } * ``` * - * Observe that: 1) `msg.sender` is used as the owner, leaving no ambiguity as to the signer intent, 2) the use of + * 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}). *