From bb68a5dde4832480a1171087ce991ae894a83c6f Mon Sep 17 00:00:00 2001 From: highlander Date: Mon, 17 Aug 2026 15:28:55 -0600 Subject: [PATCH] fix(evm): drop Base, Arbitrum and Avalanche from the 0x decoder allowlist 0x deploys the same Exchange Proxy on these three chains, so the decoder finds the contract -- but it cannot describe the trade there, and a screen that asserts understanding it does not have is worse than no screen. TokenType.chain_id and tokenByChainAddress() are uint8_t (ethereum_tokens.h:42,54), so the chain ids truncate: Base 8453 -> 5, Arbitrum 42161 -> 177, Avalanche 43114 -> 106. None matches a table entry, the lookup returns UnknownToken, and ethereumFormatAmount() short-circuits to the literal "Unknown token value" (ethereum.c:347) for BOTH operands. zx_confirmZxTransERC20 then renders Transform ERC20 Input Unknown token value Output Unknown token value while the transformations[] body executes. That matters because it removes the only justification for clear-signing transformERC20 at all. The exemption exists on the argument that the input amount and the minimum output amount shown on screen bound the outcome. On these three chains no amount is shown, so there is no bound and no disclosure: a blind signature wearing a decoder's title. Denying them routes those swaps to the generic raw-calldata path -- AdvancedMode gated, bytes on screen. The owner can still sign; the device simply stops claiming it understood. Ethereum, BNB Chain and Polygon keep decoded signing, where the amounts actually render. This is the near-term half. The underlying defect is the narrow chain id (#455); once the token table carries a full-width one these three can be restored with their amounts rendering, and the test says so. Verified: ARM cross-compile in the pinned builder image; full firmware-unit + board-unit suites green; cppcheck clean under CI's exact invocation (0 findings); clang-format clean. Refs #455 --- docs/release/7.14.2.md | 1 + lib/firmware/ethereum_contracts.c | 35 +++++++++++++++++++++++++------ unittests/firmware/ethereum.cpp | 28 +++++++++++++++++++------ 3 files changed, 52 insertions(+), 12 deletions(-) diff --git a/docs/release/7.14.2.md b/docs/release/7.14.2.md index 6f30acd7f..2f866b12e 100644 --- a/docs/release/7.14.2.md +++ b/docs/release/7.14.2.md @@ -166,6 +166,7 @@ landed, so the table below is the current state, not what #440 originally set. | TRON `SignTx` | AdvancedMode | `raw_data` still cannot be parsed on this line (#405) | | EVM `SignTx` with **no `chain_id`** | **REFUSED** | produced a pre-EIP-155 signature replayable on every EVM chain, and the confirm screen named no network (#445) | | 0x `transformERC20` over 1024 bytes | **REFUSED** by default, AdvancedMode to sign | was clear-signed with an undisclosed `transformations[]` tail; now falls through to the raw-hex path (#444) | +| 0x swaps on **Base / Arbitrum / Avalanche** | **REFUSED** by default, AdvancedMode to sign | the token table's `uint8_t` chain id truncates on these three, so the decoder rendered "Unknown token value" for both amounts — no bound, no disclosure (#455) | **On the 0x change — this affects real swaps, not an edge case.** `EthereumSignTx.data_initial_chunk` holds 1024 bytes and 0x aggregates across liquidity diff --git a/lib/firmware/ethereum_contracts.c b/lib/firmware/ethereum_contracts.c index 02cdda856..304a4b6d0 100644 --- a/lib/firmware/ethereum_contracts.c +++ b/lib/firmware/ethereum_contracts.c @@ -35,13 +35,36 @@ bool zx_isExchangeProxyChain(uint32_t chain_id) { for ZXSWAP_ADDRESS would let the 0x decoder narrate an unrelated contract — exactly the confusion the chain scoping exists to prevent. Verified against 0xProject/protocol packages/contract-addresses/addresses.json. */ + /* Base (8453), Arbitrum (42161) and Avalanche (43114) are deliberately absent + even though 0x deploys the same Exchange Proxy there. The decoder can find + the contract on those chains but cannot describe the trade, so allowing + them would produce a screen that asserts understanding it does not have. + + TokenType.chain_id and tokenByChainAddress() are uint8_t + (include/keepkey/firmware/ethereum_tokens.h:42,54), so those three chain + ids truncate to 5, 177 and 106 -- values matching no table entry. The + lookup returns UnknownToken and ethereumFormatAmount() short-circuits to + the literal "Unknown token value" (lib/firmware/ethereum.c:347) for BOTH + operands, so zx_confirmZxTransERC20() renders + + Transform ERC20 + Input Unknown token value + Output Unknown token value + + while the transformations[] body executes. The whole justification for + clear-signing transformERC20 is that the input amount and minimum output + amount shown on screen bound the outcome; on these chains no amount is + shown, so there is no bound and no disclosure -- a blind signature wearing + a decoder's title. + + Denying them here routes those swaps to the generic raw-calldata path, + which is AdvancedMode-gated and shows the bytes. That is the honest answer + until the token table carries a full-width chain id (#455), at which point + these three can be restored with their amounts actually rendering. */ switch (chain_id) { - case 1: /* Ethereum */ - case 56: /* BNB Chain */ - case 137: /* Polygon */ - case 8453: /* Base */ - case 42161: /* Arbitrum */ - case 43114: /* Avalanche */ + case 1: /* Ethereum */ + case 56: /* BNB Chain */ + case 137: /* Polygon */ return true; default: /* Including chain_id 0 / absent, which callers treat as unknown. */ diff --git a/unittests/firmware/ethereum.cpp b/unittests/firmware/ethereum.cpp index 0b09d7199..4755b0770 100644 --- a/unittests/firmware/ethereum.cpp +++ b/unittests/firmware/ethereum.cpp @@ -135,12 +135,28 @@ extern "C" { // (0xdef1abe32c034e558cdd535791643c58a13acc10), so allowing chain 10 for // ZXSWAP_ADDRESS would narrate an unrelated contract. TEST(Ethereum, ZxExchangeProxyChainAllowlist) { - EXPECT_TRUE(zx_isExchangeProxyChain(1)); // Ethereum - EXPECT_TRUE(zx_isExchangeProxyChain(56)); // BNB Chain - EXPECT_TRUE(zx_isExchangeProxyChain(137)); // Polygon - EXPECT_TRUE(zx_isExchangeProxyChain(8453)); // Base - EXPECT_TRUE(zx_isExchangeProxyChain(42161)); // Arbitrum - EXPECT_TRUE(zx_isExchangeProxyChain(43114)); // Avalanche + // Chains where the token table can actually name the tokens, so the input + // and minimum-output amounts that bound the trade are rendered. + EXPECT_TRUE(zx_isExchangeProxyChain(1)); // Ethereum + EXPECT_TRUE(zx_isExchangeProxyChain(56)); // BNB Chain + EXPECT_TRUE(zx_isExchangeProxyChain(137)); // Polygon + + // 0x deploys the same Exchange Proxy on these three, but TokenType.chain_id + // and tokenByChainAddress() are uint8_t, so the ids truncate to values that + // match no table entry: 8453->5, 42161->177, 43114->106. Both operands then + // render "Unknown token value" and the screen shows no bound at all, which + // is a blind signature with a decoder's title. They must fall through to the + // AdvancedMode raw-calldata path until #455 widens the chain id. + EXPECT_FALSE(zx_isExchangeProxyChain(8453)) + << "Base truncates to 5 and renders no amounts"; + EXPECT_FALSE(zx_isExchangeProxyChain(42161)) + << "Arbitrum truncates to 177 and renders no amounts"; + EXPECT_FALSE(zx_isExchangeProxyChain(43114)) + << "Avalanche truncates to 106 and renders no amounts"; + + // The truncated values themselves must not be a way back in. + EXPECT_FALSE(zx_isExchangeProxyChain(177)); + EXPECT_FALSE(zx_isExchangeProxyChain(106)); EXPECT_FALSE(zx_isExchangeProxyChain(10)) << "Optimism uses a different 0x proxy";