fix(evm): drop Base, Arbitrum and Avalanche from the 0x decoder allowlist - #470
fix(evm): drop Base, Arbitrum and Avalanche from the 0x decoder allowlist#470BitHighlander wants to merge 1 commit into
Conversation
…list
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
|
Blast radius, checked and intentional: this gates two decoders, not one.
Both share the defect, so both should be gated. So the change routes both 0x decoders to the raw-calldata path on those three chains, which is the intended outcome — the remedy tracks the defect, not one symptom of it.
|
|
No existing test breaks — and that is the point. Every 0x test in the pinned suite ( That absence is also the reason the defect shipped. The decoder was extended to six chains, and the test suite only ever exercised the one where the token table happens to work. On the other five the screen was never looked at — and on three of them it renders Worth adding as part of #455: when the chain id is widened and these chains are restored, the restoring change should carry a test that asserts the amounts actually render on each allowlisted chain, not merely that the chain is accepted. CI note for this PR: the first run's |
|
Superseded by #472, which fixes the same problem on the correct axis. Measuring the generated token tables showed the chain allowlist was treating a symptom:
Denying three chains would have left the identical hole on the three it kept — BSC and Polygon carry three tokens each, so any 0x swap there involving a fourth renders #472 gates on whether both token lookups actually resolve, which is chain-agnostic, data-agnostic, costs no flash, and self-corrects as tables change. Closing unmerged. Nothing here is lost: the allowlist stays as it was (all six chains), and the Base/Arbitrum/Avalanche behaviour this PR wanted is now enforced by the resolution gate instead, with tests covering it. |
1 participant
Follow-up to #468, as agreed: keep the 0x decoder where it works, gate it where it doesn't.
The problem
TokenType.chain_idandtokenByChainAddress()areuint8_t(include/keepkey/firmware/ethereum_tokens.h:42,54). Three of the six chains inzx_isExchangeProxyChain()don't fit:uint8_tThe truncated values match no table entry, so the lookup returns
UnknownTokenandethereumFormatAmount()short-circuits to the literal"Unknown token value"(lib/firmware/ethereum.c:347) — for both operands.zx_confirmZxTransERC20()renders:…while the
transformations[]body executes.Why that is worse than it looks
It removes the only justification for clear-signing
transformERC20at all. The exemption exists on the argument that the input amount and 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.0x does deploy the same Exchange Proxy on all six, so this is not an address-confusion problem like Optimism (which uses a different proxy and was already excluded). The decoder correctly identifies the contract and then cannot say anything true about the trade.
What this does
Denies those three, so their swaps take the generic raw-calldata path: AdvancedMode-gated, bytes on screen. The owner can still sign — the device just stops claiming it understood. Ethereum, BNB Chain and Polygon keep decoded signing, where the amounts render.
The test now pins both directions, including that the truncated values (177, 106) are not themselves a way back in.
Scope
This is the near-term half. The underlying defect is the narrow chain id — #455. Once the token table carries a full-width chain id, these three can be restored with their amounts actually rendering; the test comment says so, so the restoration is a deliberate act rather than an accident.
Verified
ARM cross-compile in the pinned builder image; full
firmware-unit+board-unitsuites green (make xunitexit 0); cppcheck clean under CI's exact invocation (0 findings); clang-format clean.Branched from
release/7.14.2at2d2637747(the #468 merge). Note #469: PRs targetingrelease/**get nopull_requestCI event, so this PR's checks come from the head-branch push run and do not test the merge result.