search

MetaMask / metamask-extension

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

`signTypedData_v4` encoding problems on extension #12124

Open Gudahtt opened 3 years ago

Gudahtt commented 3 years ago

Describe the bug Our signTypedData_v4 implementation has various problems that make it easy to produce non-standard and/or non-portable encodings unintentionally. There are three main problems: our array encoding is not spec-compliant, we don't perform enough input validation, and we don't clearly document how each Solidity type should be encoded in the input data.

Steps to reproduce (REQUIRED) See the eth-sig-util test suite for examples of nonsense inputs that we still accept or interpret in silly ways, and see this issue for an explanation of the array encoding problem.

Expected behavior We should ensure our eth_signTypedData implementation is spec-compliant, and that it does not produce non-standard/non-portable encodings, and that there is no remaining ambiguity in how data is interpreted. We can't accomplish these goals without breaking compatibility, so this will have to be released as signTypedData_v5.

See the eth-sig-util `signTypedData_v5 milestone for a list of related issues.

Gudahtt commented 2 years ago

One example of an invalid message that we still support signing today: https://github.com/MetaMask/test-dapp/pull/133

github-actions[bot] commented 1 year 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.

github-actions[bot] commented 9 months 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.

github-actions[bot] commented 2 months 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.