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.

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

Fix EIP712 for delegatecalls #2852

Merged
merged 4 commits into from Oct 6, 2021

Conversation

k06a
Copy link
Contributor

@k06a k06a commented Sep 9, 2021

Some tokens do delegate their implementations, this fix would potential double spends.

Let's discuss.

@Amxx
Copy link
Collaborator

Amxx commented Sep 9, 2021

Hello @k06a and thank you for your PR.

The code you are pointing to is indeed not designed to be used in the context of delegate calls. If you are building an "implementation" that proxy contract will delegate to, you should use @openzeppelin/contracts-upgradeable and NOT @openzeppelin/contracts

If you have a look at the equivalent upgradeable contract you will see that this caching system is not present.

Thus I don't think this is critical, but it might still be a great idea to apply your PR for extra security.

@frangio
Copy link
Contributor

frangio commented Sep 9, 2021

@k06a Have you seen tokens that do delegation outside the context of upgradeability?

It is never a good idea to use this package with upgradeable contracts and I don't think we should include this fix because it would be validating that use.

@k06a
Copy link
Contributor Author

k06a commented Sep 9, 2021

USDC token have permit and is proxy contract.

@k06a
Copy link
Contributor Author

k06a commented Sep 9, 2021

I would prefer to have this check as extra security to avoid misusage. Misusage could lead to a double spend.

@frangio
Copy link
Contributor

frangio commented Oct 6, 2021

After some thought I think we should go ahead with this. What convinced me is that we currently have a discrepancy in the "this" value that is used in each branch of the if statement (cached vs recreated).

Note that this slightly changes the behavior and could be considered a breaking change, but the cases that would be affected would have had the domain separator change anyway.

As always, note that upgradeable contracts should be using the Upgradeable version of this contract.


A changelog entry is needed before merging.

Amxx
Amxx previously approved these changes Oct 6, 2021
@Amxx Amxx enabled auto-merge (squash) October 6, 2021 07:19
@frangio
Copy link
Contributor

frangio commented Oct 6, 2021

There is something wrong with Codecov.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants