MetaMask / metamask-extension

:globe_with_meridians: :electric_plug: The MetaMask browser extension enables browsing Ethereum blockchain enabled websites
https://metamask.io
Other
11.83k stars 4.82k forks source link

[Bug]: Signatures - Verify 3rd party details disappears if an empty domain is passed on a Typed Signature 3 & 4 #21011

Closed seaona closed 6 days ago

seaona commented 11 months ago

Describe the bug

Problem: whenever we are triggering a Typed Signature from a dapp, if the dapp passes an empty domain, we can see how the Verify third party details section disappears completely. Furthermore, it is displayed as a Typed 4 signature even though this message does not follow the specs (see below)

Expected behavior

the type of eip712Domain is a struct named EIP712Domain with one or more of the below fields. Protocol designers only need to include the fields that make sense for their signing domain. Unused fields are left out of the struct type.
- string name the user readable name of signing domain, i.e. the name of the DApp or the protocol.
- string version the current major version of the signing domain. Signatures from different versions are not compatible.
- uint256 chainId the [EIP-155](https://github.com/ethereum/EIPs/blob/master/EIPS/eip-155.md) chain id. The user-agent should refuse signing if it does not match the currently active chain.
- address verifyingContract the address of the contract that will verify the signature. The user-agent may do contract specific phishing prevention.
- bytes32 salt an disambiguating salt for the protocol. This can be used as a domain separator of last resort.

Screenshots

https://github.com/MetaMask/metamask-extension/assets/54408225/47a70e53-15d5-4c96-bb8b-908812b9fd48

Steps to reproduce

  1. Checkout test dapp project
  2. Remove the domain data from the Typed Signature 3 oand 4 section
  3. Start test dapp
  4. Click Sign Typed 4
  5. See Verify 3rd party details is not there anymore

Error messages or log output

No response

Version

11.1.0 (possibly earlier)

Build type

None

Browser

Chrome

Operating system

Linux

Hardware wallet

No response

Additional context

No response

Severity

No response

bschorchit commented 11 months ago

Should we be validating if the requests meets the spec on the API level?

shanejonas commented 8 months ago

the json-schema in the spec is pretty lenient on this

it has domain as required but only that domain is an object

required: ["domain",
domain: {type: 'object'},

but since we are actually using domain we should probably specify this deeper.

It should be:

"domain": {
  "type": "object",
  "properties": {
    "chainId": {
      "$ref": "#/components/schemas/ChainId"
    },
    "name": {
       "type": "string"
    },
    "verifyingContract": {
       "$ref": "#/components/schemas/Address"
    },
    "version": {
       "type": "string",
       "const": "1"
    },
  }
}

Should we be validating if the requests meets the spec on the API level?

this is definitely something we want to do/should do. I know for this particular case we actually have just copied in the schema right into eth-sig-utils and use a json schema validator, but ideally it should get it from the api-specs and do it at the api level.

vandan commented 7 months ago

We will add a small note to the https://docs.metamask.io/wallet/reference/eth_signtypeddata_v4/ documentation to clarify how our implementation works.

github-actions[bot] commented 1 month ago

This issue has been automatically marked as stale because it has not had recent activity in the last 90 days. It will be closed in 45 days if there is no further activity. The MetaMask team intends on reviewing this issue before close, and removing the stale label if it is still a bug. We welcome new comments on this issue. We do not intend on closing issues if they report bugs that are still reproducible. Thank you for your contributions.

bschorchit commented 6 days ago

No longer an issue with the redesigned signatures (testing through the empty domain button on the test dapp)