Skip to content

Use SignatureChecker inside Permit to allow smart contract wallets to use it #3951

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.

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

Closed

Conversation

k06a
Copy link
Contributor

@k06a k06a commented Jan 12, 2023

I see current implementation of EIP-2612 (aka Permit) is not supporting smart contract wallets and they are required to invent own signature-based operations to execute token approve by smart contract wallet owner signature (pseudocode example: https://ethereum.stackexchange.com/a/142814/3032). Trying to fix it without violating EIP-2612 specification.

First (EIP-2612) defines signature as following:

r, s and v is a valid secp256k1 signature from owner

But if owner is smart contract wallet, can we consider signature to be valid secp256k1 signature but of wallet owner() of ERC20 owner?

I think it's possible and doesn't violate EIP that much. Let's discuss.

@k06a k06a changed the title Use SignatureChecked inside Permit to allow smart contract wallets to use it Use SignatureChecker inside Permit to allow smart contract wallets to use it Jan 12, 2023
@k06a k06a force-pushed the feature/permit-signature-checker branch from c8dc58e to abf1855 Compare January 12, 2023 14:00
@Amxx
Copy link
Collaborator

Amxx commented Jan 12, 2023

Hello @k06a

This was already discussed (can't find the issue though :/) and we decided against doing it for the following reasons:

  • smart wallets are likely to support meta transaction (and batch transactions), therefore the approve-transferFrom is not as bad for them as it is for EOA
  • some smart wallets want to limit daily/monthly transfers (that was raised by argent). If the transfers are done by the smart wallet, directly calling the ERC20.transfer function, then its possible to enforce that onchain. However, if the transfers are the consequence of an permit, then the quota can no longer be enforced.

@k06a k06a closed this Jan 18, 2023
@frangio
Copy link
Contributor

frangio commented Jan 18, 2023

The issue is #2845.

The main reason is that EIP-2612 specifically says v,r,s a sepck256k1 signature by the owner.

@frangio
Copy link
Contributor

frangio commented Jan 18, 2023

But if owner is smart contract wallet, can we consider signature to be valid secp256k1 signature but of wallet owner() of ERC20 owner?

I think it's possible and doesn't violate EIP that much. Let's discuss.

I see this comment now. It's true that it doesn't violate the EIP that much... EIP-1271 goes beyond these signatures though. For example, Gnosis Safe's implementation wouldn't work with this. What wallets would be enabled by this change? There is Argent as mentioned in #2845. Are there others?

@k06a
Copy link
Contributor Author

k06a commented Jan 18, 2023

@frangio I think smart contract wallets will be able to let receiver claim their tokens without performing transaction. In case of permit to other wallet (EOA or SC).

@k06a k06a reopened this Jan 18, 2023
@k06a
Copy link
Contributor Author

k06a commented Jan 18, 2023

@Amxx
Copy link
Collaborator

Amxx commented Jan 19, 2023

Interresting: they also support both 65 and 64 bytes signatures

@k06a
Copy link
Contributor Author

k06a commented Jan 19, 2023

Interresting: they also support both 65 and 64 bytes signatures

I think they support any length and effectively support Gnosis Safes

@elshan-eth
Copy link

I agree with @Amxx's comment.

It seems to me that there is no advantage in using permit for Account Abstraction because the gas efficiency would either be too negligible, but it introduces additional "variables" that need to be considered when working with tokens. For example, if Account Abstraction decides to prohibit the use of permit, as in the case of Argent, it would add extra complexity on the side of AA contracts, but the absence of the permit function wouldn't cause any inconvenience when using AA. Any AA wallet focuses on signature management within its own contracts, and built-in batching eliminates the issues that permit was designed to solve.

@k06a
Copy link
Contributor Author

k06a commented Nov 25, 2024

USDC finally supports ERC-1271 signatures, could be done as new ERC, extending ERC-2612…

@elshan-eth
Copy link

USDC finally supports ERC-1271 signatures, could be done as new ERC, extending ERC-2612…

I don't understand the technical benefits, but if it's becoming standard then it might be worth it

@k06a
Copy link
Contributor Author

k06a commented Nov 26, 2024

@elshan-eth you can’t place limit order from miltisig without manually executing approve.

@frangio
Copy link
Contributor

frangio commented Nov 26, 2024

It's not just about batching, but it is true that in theory AA allows this to be fixed at the account level. That said it won't happen in practice until there's a standard way to do it.

USDC finally supports ERC-1271 signatures

For reference: https://github.com/circlefin/stablecoin-evm/blob/a4ba37d0495b1c979ff0dcfdbad2e5e076828208/contracts/v2/EIP2612.sol#L77

I'm not convinced this is worth doing... It will only work for the simplest accounts, it won't work for Safes, it won't work for passkey accounts, etc.

@elshan-eth
Copy link

@k06a, good example 👍

@Amxx Amxx added this to the 5.3 milestone Nov 26, 2024
@Amxx Amxx added the idea label Nov 26, 2024
@k06a
Copy link
Contributor Author

k06a commented Nov 26, 2024

I think we could have separate permit() method with bytes signature to support ERC-1271. Exactly the same way as Circle team (USDC) did it.

@frangio
Copy link
Contributor

frangio commented Nov 26, 2024

@k06a
Copy link
Contributor Author

k06a commented Nov 26, 2024

Could be calldata, but yes.

@elshan-eth
Copy link

@k06a, I think your example is a pretty strong point for having such a feature. I think it could also be useful for some bridges or other protocols that have some off-chain logic to avoid having to do a physical approve.

Copy link

changeset-bot bot commented Feb 18, 2025

⚠️ No Changeset found

Latest commit: a92b380

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@Amxx
Copy link
Collaborator

Amxx commented Feb 18, 2025

Considering that Permits take 65 bits signatures (v,r,s), this is very restrictive on the account. Accounts may need different signature format, for example to accomodate ERC-7739. Additionally, the development of smart account (in particular through ERC-4337 and ERC-7702) is making this mostly irrelevant IMO.

Closing.

@Amxx Amxx closed this Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants