Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove SafeERC20.safePermit #4582

Merged
merged 14 commits into from
Sep 11, 2023
Merged

Remove SafeERC20.safePermit #4582

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
5f9a8e9
Remove SafeERC20.safePermit
Amxx Sep 7, 2023
c15706f
Document the danger of authentication using permit
Amxx Sep 7, 2023
ddd60b3
add changeset
Amxx Sep 7, 2023
cefc133
Update green-pumpkins-end.md
frangio Sep 8, 2023
2f77daf
remove unused variables
frangio Sep 8, 2023
92abe2c
Update green-pumpkins-end.md
frangio Sep 8, 2023
a6e37ad
use inheritdoc
frangio Sep 8, 2023
9a8b2e8
add IERC20Permit on docs site
frangio Sep 8, 2023
db787f6
move recommendations to IERC20Permit
frangio Sep 8, 2023
35664bf
remove broken link
frangio Sep 8, 2023
b09fb90
use full example
frangio Sep 8, 2023
c7c46ea
expand example
frangio Sep 8, 2023
c1c95aa
wording
frangio Sep 8, 2023
487b3f9
Update IERC20Permit.sol
frangio Sep 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Document the danger of authentication using permit
  • Loading branch information
@Amxx
Amxx committed Sep 7, 2023
commit c15706fdfad12d4319a14c4aad7977afdf0bbaa1
3 changes: 3 additions & 0 deletions contracts/token/ERC20/extensions/IERC20Permit.sol
View file
Open in desktop
Copy link
Contributor

@frangio frangio Sep 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like IERC20Permit is not listed in the docs site. (https://docs.openzeppelin.com/contracts/4.x/api/token/erc20)

I added it in this PR, let's see if it looks nice.

When we have this interface/implementation separation I don't like having duplicate docs in the site. The same applies to I/AccessManager, for example. But if the interface is in the library it should be somewhere in the documentation... so I may have to accept adding it.

Using @inheritdoc leads to more duplication but it might make it nicer to navigate than a link like "See IERC20Permit.permit".

Original file line number Diff line number Diff line change
Expand Up @@ -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].
frangio marked this conversation as resolved.
Show resolved Hide resolved
*/
function permit(
address owner,
Expand Down
12 changes: 12 additions & 0 deletions docs/modules/ROOT/pages/utilities.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down