fix(core,legacy): Fix domain-only ethTypedData

[aloisklink]

When doing Ethereum signTypedData, and the primary type = "EIP712Domain", we should completely ignore the "message" part and only sign the domain.

According to the community, this is technically allowed by the spec, and may be used by ETH smart contracts to save on gas (Ethereum smart contract computational cost).

It's also been supported by MetaMask since MetaMask/metamask-extension#6896 (or July 2019).

Test case generated by @MetaMask/eth-sig-util's library, using this gist by @alisinabh.

See https://ethereum-magicians.org/t/eip-712-standards-clarification-primarytype-as-domaintype/3286 for community discussion.

Todo

• legacy tests: I couldn't figure out how to run legacy tests on an emulator, so I'll need to wait for CI to test it for me.
• create towncrier changelog entries once I know the number of this PR (rebased)

[grdddj]

Some small notes/suggestions on the TT code. As for CI, I will transfer your branch into our repo so that we can run it.

EDIT: CI could be seen here - https://gitlab.com/satoshilabs/trezor/trezor-firmware/-/pipelines/440076528

[matejcik]

We might want to do a UX tweak for TT. I am not sure what exactly, but the user should be notified that there is only the domain and if they want to inspect the data, that's what they should look into.

OTOH there is the final confirmation screen so the user always has a chance to back out.

What do you think @aloisklink ?

[matejcik]

maybe should_show_domain would use title "confirm message" instead of "confirm domain" in that case?

[aloisklink]

We might want to do a UX tweak for TT. I am not sure what exactly, but the user should be notified that there is only the domain and if they want to inspect the data, that's what they should look into.

OTOH there is the final confirmation screen so the user always has a chance to back out.

My gut feeling is that with EIP712 typed signing being so common nowadays in the Ethereum ecosystem, it's pretty likely people would just skip reading the title of transactions, and just do a "tap, tap, tap" while only looking at the contents.

Instead I've changed the "Confirm message" screen so it just says "No message field" in 23be403 (see screenshot below). That way there is the same amount of screens to do an EIP712 typed signing.

I'd admit that UI/UX isn't something I'm skilled at, so I'd be happy to change it, or let you guys change it.

[grdddj]

Good job! CI could be seen at https://gitlab.com/satoshilabs/trezor/trezor-firmware/-/pipelines/442570184

Everything is fine there, except for the legacy test stage - the T1 tests did not even run because the testing code failed when parsing the "message_hash": null in test vectors in

trezor-firmware/tests/device_tests/ethereum/test_sign_typed_data.py

Line 53 in 33c243b

ethereum.decode_hex(parameters["message_hash"]),

(It does not account for possibility of empty message_hash)
We could do

ethereum.decode_hex(parameters["message_hash"]) if parameters["message_hash"] else None

---

The UI/UX for this special case seems fine for me - https://satoshilabs.gitlab.io/-/trezor/trezor-firmware/-/jobs/1947225717/artifacts/master_diff/added/ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data[full_domain_empty_message].html

There is also a UI change in all the "normal" EIP712 test-cases where the message_hash is nonzero. It causes the final hash to be longer and it currently does not fit on one screen. Now it shows the whole hash, but at the expense of user having to swipe the screen once to confirm it. I am fine with it, even though regular users would maybe find it troublesome (no possibility of "tap tap tap", they would now need to "tap tap swipe tap" :))
Example: https://satoshilabs.gitlab.io/-/trezor/trezor-firmware/-/jobs/1947225717/artifacts/master_diff/diff/ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data[basic_data].html

---

With regards to running the T1 tests locally, I also do not know how to do it, but will try to find out :)

[matejcik]

Instead I've changed the "Confirm message" screen so it just says "No message field"

this is perfectly reasonable, thanks.

[matejcik]

(It does not account for possibility of empty message_hash)

should we perhaps modify decode_hex to also accept None ?

[matejcik]

ACK on the UI changes.

please modify the test show_more_button so that it scrolls through the final hash -- you'll need to insert a client.debug.swipe_up() in the appropriate place
(https://satoshilabs.gitlab.io/-/trezor/trezor-firmware/-/jobs/1947225717/artifacts/master_diff/diff/ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data_show_more_button.html)

[matejcik]

you can run T1 tests locally the normal way :)

1. build the T1 emulator:

export EMULATOR=1 DEBUG_LINK=1
cd legacy
./script/setup
./script/cibuild

2. run the T1 emulator: ./firmware/trezor.elf
3. run the tests: from repository root, pytest tests/device_tests

[grdddj]

should we perhaps modify decode_hex to also accept None ?

That could be done, but currently putting None into it is so rare that I think it is not worth complicating the decode_hex by taking and returning Optional. (It could also mean having to handle some type-checking issues)

[grdddj]

you can run T1 tests locally the normal way :)

1. build the T1 emulator:

export EMULATOR=1 DEBUG_LINK=1
cd legacy
./script/setup
./script/cibuild

2. run the T1 emulator: `./firmware/trezor.elf`

3. run the tests: from repository root, `pytest tests/device_tests`

Thanks :) After getting rid of the None-error, all the T1 tests are fine locally.

One needed change is to modifiy trezorlib::ethereum::sign_typed_data_hash's signature to include message_hash: Optional[bytes] for type-checking purposes (even though the tests are not being type-checked).

Also the CLI function trezorlib::cli::ethereum::sign_typed_data_hash needs to account for the possibly-empty-message-hash inputted from user (possibly with the same ethereum.decode_hex(message_hash_hex) if message_hash_hex else None)

[aloisklink]

trezorlib::ethereum::sign_typed_data_hash and trezorlib::cli::ethereum::sign_typed_data_hash have been updated in 8085331

I managed to get the T1 emulator working! Thanks for those instructions @matejcik (btw, it's probably worth adding it into the docs: https://docs.trezor.io/trezor-firmware/index.html)

I've also tested the CLI for T1 and it also works:

poetry run python3 -m python.src.trezorlib.cli.trezorctl ethereum sign-typed-data-hash 0x6192106f129ce05c9075d319c1fa6ea9b3ae37cbd0c1ef92e2be7137bb07baa1 '' -n "m/44'/60'/0'/0/0"

[aloisklink]

There is also a UI change in all the "normal" EIP712 test-cases where the message_hash is nonzero. It causes the final hash to be longer and it currently does not fit on one screen. Now it shows the whole hash, but at the expense of user having to swipe the screen once to confirm it. I am fine with it, even though regular users would maybe find it troublesome (no possibility of "tap tap tap", they would now need to "tap tap swipe tap" :))
Example: https://satoshilabs.gitlab.io/-/trezor/trezor-firmware/-/jobs/1947225717/artifacts/master_diff/diff/ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data[basic_data].html

Good catch, I didn't notice it. I've slightly changed this screen by paging it. Since the hash is created from the domain/message screens, I though it'd be quite unlikely that somebody would want to view the full hash, so using pagination seems to be better than swiping (plus it keeps it as a "tap tap tap" :) )

I've also updated the show_more_button test so it swipes through the new show more.

Changes made in 0cec5c9

[matejcik]

The final screen should ​be hold-to-confirm.
(We use normal confirms in the Bitcoin and Ethereum SignMessage, and arguably we should have a hold-to-confirm there as well. Given how important EIP-712 seems to be for smart contracts, I'm guessing there is potential for coin loss if you sign a message that you didn't intend, so there should be at least this.

I don't think the "show more" dialog supports this at the moment. However, UX-wise, the hash doesn't seem to be great content for the hold-to-confirm anyway.

I realize that I originally said that we want to keep the hash, but in the end it does seem pointless -- nobody is ever going to be verifying the hash against anything, except perhaps as a debugging tool (which should not be in the face of normal user's workflow).

I'll confirm this with the team, but right now it seems better to not show it at all, and instead put something like "really sign EIP-712 message?" at end

[aloisklink]

I don't think the "show more" dialog supports this at the moment. However, UX-wise, the hash doesn't seem to be great content for the hold-to-confirm anyway.

I read your mind and spotted the same issue and added it in 7c23e86 😄

Edit: Yep, nowadays EIP-712 can be used for people to access your NFTs/ERC-20s. I believe Opensea (major NFT site) and Cowswap (major ERC-20 exchange) allow trading without making a transaction, just by signing an EIP-712.

Showing the final hash would work as a "Really sign EIP-712 message", but I could understand replacing it to avoid confusing a user with too much information.

[matejcik]

I read your mind

well done :)

avoid confusing a user with too much information.

Exactly, we don't want to train users to click through meaningless dialogs.

For now let's keep it as it is now, I'll confirm what we want to do here and get back to you.

[matejcik]

@aloisklink final decision: please replace the hash screen with a simple "Really sign EIP-712 message?" and a hold-to-confirm.

the wording is not awesome, so if you have a better idea, feel free.

[aloisklink]

@aloisklink final decision: please replace the hash screen with a simple "Really sign EIP-712 message?" and a hold-to-confirm.

Would you like me to add a commit reverting 7c23e86?
That commit adds a new parameter called hold to should_show_more(), which won't be needed with the shorter "Really sign EIP-712 message?".

the wording is not awesome, so if you have a better idea, feel free.

Maybe the title could be "Confirm typed data" with the message as "Confirm signing EIP-712 typed data?"

Some places call it typed data, and some places call it EIP-712, so it's worth putting both names in the message.

[matejcik]

Would you like me to add a commit reverting 7c23e86?

yes please. even if we wanted to keep the code, we would want it in a separate commit

Maybe the title could be "Confirm typed data" with the message as "Confirm signing EIP-712 typed data?"

sure, that makes sense.

[aloisklink]

I've reverted 7c23e86 and 0cec5c9 since the changes in them don't matter anymore. I'll manually remove them and their revert commits in the final git rebase -i --autosquash before merging.

The new final confirm screen in 30c84dc looks like the following:

I added a hold parameter to confirm_text() to make it happen, see

trezor-firmware/core/src/trezor/ui/layouts/tt/__init__.py

Line 697 in 30c84dc

hold: bool = False, # TODO Only supported if NOT paginating

[matejcik]

I added a hold parameter to confirm_text() to make it happen, see

confirm_action should be used here, which already has hold-to-confirm

[matejcik]

i have no more comments (unless something weird shows up on CI), so please squash & rebase afterwards

[aloisklink]

Sorry, forgot to add a towncrier changelog entry when we added changes to the python folder and that seems to have prevented CI tests from running.

Added in 1cb29d3

Any chance you can restart the CI job? 😄

[aloisklink]

It seems like CI failed at connect test core in the tests "TrezorConnect methods › TrezorConnect.signTransaction › Testnet (P2WPKH): with proof" and "TrezorConnect methods › TrezorConnect.signTransaction › Testnet (P2TR): with proof"

I'm guessing this is a false positive since I didn't touch any Bitcoin code?
https://gitlab.com/satoshilabs/trezor/trezor-firmware/-/jobs/1972352609

[matejcik]

I squashed and rebased the code, with one more tweak: the "hold to confirm" button should read "Hold to confirm".

after rebase, the Connect test will very likely be OK (or at least cause an expected failure)

[matejcik]

and we're green!
thanks for your contribution 💯

[aloisklink]

and we're green! thanks for your contribution

And thank you for being patient and giving great code reviews 🚀