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

Ensure signTypedData is restricted to a domain #10576

Open
danfinlay opened this issue Mar 3, 2021 · 9 comments
Open

Ensure signTypedData is restricted to a domain #10576

danfinlay opened this issue Mar 3, 2021 · 9 comments

Comments

@danfinlay
Copy link
Contributor

EIP712 has a domain field that is meant to be used as a security measure, to restrict the range of possible signatures.

We do not currently enforce domain in any way that could prove to a contract that a signature was suggested to the wallet from a given web app.

This has been requested by an L2 team using signTypedData.

@danfinlay danfinlay changed the title Ensure signTypedData restricted to a domain Ensure signTypedData is restricted to a domain Jun 7, 2021
@danfinlay
Copy link
Contributor Author

But since we haven’t added this behavior yet, this could break existing Ðapps.

Proposal: We add a metrics event for that method, which just records when a domain requests a signature from a domain that does not match itself, and record those two values. If after a sprint this is an acceptable number of reports, we can roll out this security improvement.

@danfinlay
Copy link
Contributor Author

Another way to avoid breaking as badly: make this an optional param. Contracts can then require it if they like. This allows contracts to only work with some sites, which damages interoperability and permissionless innovation but can provide a type of security in an opt in way.

@Gudahtt
Copy link
Member

Gudahtt commented Feb 21, 2022

The EIP-712 spec includes a domainSeparator component with various optional fields, but there is no origin field currently defined (or any similar alternative). In effect, the domain is not present for us to verify.

There has been much discussion on including an origin field, but none has been accepted yet in the current draft of EIP-712.

There is a verifyingContract field, but I don't understand how we're supposed to use this field. The description states:

the address of the contract that will verify the signature. The user-agent may do contract specific phishing prevention.

@bentobox19
Copy link

Some good literature on Self Authenticated Domains:

(referenced by @holantonela, btw)

@adonesky1
Copy link
Contributor

Proposal: We add a metrics event for that method, which just records when a domain requests a signature from a domain that does not match itself, and record those two values. If after a sprint this is an acceptable number of reports, we can roll out this security improvement.

Should we implement this metric event and get a sense of how frequently this mismatch occurs and what domain fields dapps are most often including in signTypedData calls?

Since as @Gudahtt points out there is currently no full "origin" field on the domain object, do we feel ok matching the name field against the URL hostname for some base verification (showing a non-blocking warning to the user when there is a mismatch)? This was the approach I was taking here

cc @Gudahtt @danfinlay

@holantonela
Copy link

We may be able to reuse domain binding check coming with SIWE implementation. Ref: https://github.com/spruceid/metamask-extension-siwe/tree/feat/siwe-update

@bschorchit bschorchit added area-transactions team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead labels Sep 28, 2022
@holantonela
Copy link

#16616

@danfinlay
Copy link
Contributor Author

my impression is that it is very rare to find a 712 signature bound to a website domain. Usually the domain separator is just used to specify the verifying contract and chain ID, a version, and the name of the contract. Might be interesting to even have a metric for if people ever use domain binding.

@holantonela
Copy link

holantonela commented Feb 14, 2023

my impression is that it is very rare to find a 712 signature bound to a website domain.

I agree, especially when login front-ends are dissociated from the main domain. We will quickly learn about dapp implementations when this gets released. We can even be more flexible by doing something like wildcards for subdomains.

While discussing this enforcement with stakeholders, most people in the room agreed that this is a great way to mitigate phishing attacks. Also, if we warn users about this mismatch, there is not available recovery flow. More details here #16295

Might be interesting to even have a metric for if people ever use domain binding.

Yes. We have metrics in place to learn about SIWE usage.

@hilvmason hilvmason added team-confirmations-planning (only for internal use within Confirmations team) and removed team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead labels Jun 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants