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 3db0397
Show file tree
Hide file tree
Showing 6 changed files with 86 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": "domain_only_message",
"comment": "Bare minimum EIP-712 message",
"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
1 change: 1 addition & 0 deletions core/.changelog.d/2036.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix domain-only EIP-712 hashes (i.e. when `primaryType`=`EIP712Domain`)
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
1 change: 1 addition & 0 deletions legacy/firmware/.changelog.d/2036.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix domain-only EIP-712 hashes (i.e. when `primaryType`=`EIP712Domain`)
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 3db0397

Please sign in to comment.