Skip to content

Commit

Permalink
fix(core,legacy): Fix domain-only ethTypedData
Browse files Browse the repository at this point in the history
When doing Ethereum signTypedData, and the primaryType="EIP712Domain",
we 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.

Test case generated by @MetaMask/eth-sig-util's library.

See: https://ethereum-magicians.org/t/eip-712-standards-clarification-primarytype-as-domaintype/3286
  • Loading branch information
aloisklink committed Dec 23, 2021
1 parent c0510fc commit 12c0677
Show file tree
Hide file tree
Showing 4 changed files with 84 additions and 28 deletions.
22 changes: 22 additions & 0 deletions common/tests/fixtures/ethereum/sign_typed_data.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,28 @@
"passphrase": ""
},
"tests": [
{
"name": "empty_message",
"comment": "Bare minimum valid EIP-712 signature",
"parameters": {
"path": "m/44'/60'/0'/0/0",
"metamask_v4_compat": true,
"data": {
"types": {
"EIP712Domain": []
},
"primaryType": "EIP712Domain",
"message": {},
"domain": {}
},
"message_hash": "0x",
"domain_separator_hash": "0x6192106f129ce05c9075d319c1fa6ea9b3ae37cbd0c1ef92e2be7137bb07baa1"
},
"result": {
"address": "0x73d0385F4d8E00C5e6504C6030F47BF6212736A8",
"sig": "0x18aaea9abed7cd88d3763a9a420e2e7b71a9f991685fbc62d74da86326cffa680644862d459d1973e422777a3933bc74190b1cae9a5418ddaea645a7d7630dd91c"
}
},
{
"name": "basic_data",
"parameters": {
Expand Down
54 changes: 34 additions & 20 deletions core/src/apps/ethereum/sign_typed_data.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,32 +71,46 @@ async def generate_typed_data_hash(
)
await typed_data_envelope.collect_types()

# EIP-712 prefix
parts = [b"\x19\x01"]

name, version = await get_name_and_version_for_domain(ctx, typed_data_envelope)
show_domain = await should_show_domain(ctx, name, version)
domain_separator = await typed_data_envelope.hash_struct(
primary_type="EIP712Domain",
member_path=[0],
show_data=show_domain,
parent_objects=["EIP712Domain"],
# domain_seperator
parts.append(
await typed_data_envelope.hash_struct(
primary_type="EIP712Domain",
member_path=[0],
show_data=show_domain,
parent_objects=["EIP712Domain"],
)
)

show_message = await should_show_struct(
ctx,
description=primary_type,
data_members=typed_data_envelope.types[primary_type].members,
title="Confirm message",
button_text="Show full message",
)
message_hash = await typed_data_envelope.hash_struct(
primary_type=primary_type,
member_path=[1],
show_data=show_message,
parent_objects=[primary_type],
)
# Setting the primary_type to "EIP712Domain" is technically in spec
# In this case, we ignore the "message" part and only use the "domain" part
# https://ethereum-magicians.org/t/eip-712-standards-clarification-primarytype-as-domaintype/3286
if primary_type != "EIP712Domain":
show_message = await should_show_struct(
ctx,
description=primary_type,
data_members=typed_data_envelope.types[primary_type].members,
title="Confirm message",
button_text="Show full message",
)
parts.append(
await typed_data_envelope.hash_struct(
primary_type=primary_type,
member_path=[1],
show_data=show_message,
parent_objects=[primary_type],
)
)

full_bytes_to_hash = b"".join(parts)

await confirm_hash(ctx, message_hash)
await confirm_hash(ctx, full_bytes_to_hash)

return keccak256(b"\x19" + b"\x01" + domain_separator + message_hash)
return keccak256(full_bytes_to_hash)


def get_hash_writer() -> HashWriter:
Expand Down
19 changes: 17 additions & 2 deletions legacy/firmware/ethereum.c
Original file line number Diff line number Diff line change
Expand Up @@ -987,12 +987,27 @@ static void ethereum_typed_hash(const uint8_t domain_separator_hash[32],
keccak_Final(&ctx, hash);
}

// When primaryType="EIP712Domain", we ignore message_hash completely
static void ethereum_domain_only_typed_hash(
const uint8_t domain_separator_hash[32], uint8_t hash[32]) {
struct SHA3_CTX ctx = {0};
sha3_256_Init(&ctx);
sha3_Update(&ctx, (const uint8_t *)"\x19\x01", 2);
sha3_Update(&ctx, domain_separator_hash, 32);
keccak_Final(&ctx, hash);
}

void ethereum_typed_hash_sign(const EthereumSignTypedHash *msg,
const HDNode *node,
EthereumTypedDataSignature *resp) {
uint8_t hash[32] = {0};
ethereum_typed_hash(msg->domain_separator_hash.bytes, msg->message_hash.bytes,
hash);

if (msg->message_hash.size) {
ethereum_typed_hash(msg->domain_separator_hash.bytes,
msg->message_hash.bytes, hash);
} else {
ethereum_domain_only_typed_hash(msg->domain_separator_hash.bytes, hash);
}

uint8_t v = 0;
if (ecdsa_sign_digest(&secp256k1, node->private_key, hash,
Expand Down
17 changes: 11 additions & 6 deletions legacy/firmware/fsm_msg_ethereum.h
Original file line number Diff line number Diff line change
Expand Up @@ -262,12 +262,17 @@ void fsm_msgEthereumSignTypedHash(const EthereumSignTypedHash *msg) {
layoutHome();
return;
}
layoutConfirmHash(&bmp_icon_warning, _("EIP-712 message hash"),
msg->message_hash.bytes, 32);
if (!protectButton(ButtonRequestType_ButtonRequest_Other, false)) {
fsm_sendFailure(FailureType_Failure_ActionCancelled, NULL);
layoutHome();
return;

// No message hash when setting primaryType="EIP712Domain"
// https://ethereum-magicians.org/t/eip-712-standards-clarification-primarytype-as-domaintype/3286
if (msg->message_hash.size) {
layoutConfirmHash(&bmp_icon_warning, _("EIP-712 message hash"),
msg->message_hash.bytes, 32);
if (!protectButton(ButtonRequestType_ButtonRequest_Other, false)) {
fsm_sendFailure(FailureType_Failure_ActionCancelled, NULL);
layoutHome();
return;
}
}

ethereum_typed_hash_sign(msg, node, resp);
Expand Down

0 comments on commit 12c0677

Please sign in to comment.