fix(evm): clear-sign 0x only when both traded assets resolve - #472
Merged
BitHighlander merged 2 commits intoAug 17, 2026
Merged
Conversation
The 0x decoders show four values and hide the rest of the calldata. That is only defensible because the input amount and the minimum output amount bound the outcome -- and a bound the user cannot read is not a bound. ethereumFormatAmount() renders the literal "Unknown token value" whenever tokenByChainAddress() misses, so an unresolved token produces a screen stating no amount at all while the calldata executes: a blind signature wearing a decoder's title. So the decoders now claim a transaction only when BOTH lookups resolve. If either misses, the predicate returns false and ethereum.c falls through to the generic raw-calldata path -- AdvancedMode-gated, bytes on screen. The owner can still sign; the device stops claiming it understood. The refusal has to live in the zx_is*() predicate, not the confirm: a false from ethereum_contractConfirmed() is treated as a user cancel and aborts signing, where a false from ethereum_contractHandled() is the fall-through this needs. Both decoders now share one resolver between predicate and confirm, so the two cannot disagree about what is displayable. This replaces the chain-allowlist approach that was on its way as #470. The allowlist was treating a symptom, and measuring the tables showed why it could not work: the generated table carries 1924 entries for chain 1, THREE each for BSC and Polygon, and none at all for Base, Arbitrum or Avalanche. Denying three chains would still have clear-signed unnameable tokens on the other three, and would have started refusing them again once tables were added. Gating on the lookup is correct whether the tables are incomplete, expanded or stale. It also corrects the diagnosis behind #455. The uint8_t chain_id truncation is real (8453->5, 42161->177, 43114->106) but it is NOT why those screens are blank: there are zero token entries for those chains, so a widened type would still miss. The type remains a prerequisite for ever ADDING those tables -- tracked there, not here. Note this does not touch #414: a payload that fits one chunk still carries transformations[] bytes the decoder never reads. Resolving both assets bounds what the trade can cost, it does not disclose the route. Verified: ARM cross-compile in the pinned builder image; firmware-unit Ethereum.* 9/9 and the full xunit suite green; cppcheck clean under CI's exact invocation (0 findings); clang-format clean across CI's entire lint scope. Refs #455 #414
This was referenced Aug 17, 2026
Owner
Author
|
Correction to my comment above: I named the wrong owner for the residual. I wrote that the in-buffer residual — up to 860 bytes of The residual here is non-disclosure, not misdirection: bytes that are never read or shown at all. Different failure mode, and it had no owning issue. Now tracked as #473. Nothing about this issue's own closure changes — #444 was scoped to the streamed tail and #468 fixed exactly that. |
An earlier revision of this document handed the transformERC20 in-buffer residual to #414. Wrong owner, and #414 is about to close. #414 is the unread ABI offset word: a caller placing its real arguments elsewhere so the decoder's hardcoded positions display plausible-but-wrong values. That is misdirection, and it is fixed at head -- zxswap.c:69-74 and thortx.c:70-74 pin the head offset word to canonical values before any dynamic read. The residual is non-disclosure: up to 860 bytes of transformations[] that the decoder never reads or shows at all. Different failure mode, no overlap in remedy. Filed as #473 so it does not vanish when #444 and #414 close.
This was referenced Aug 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces #470. Same goal, correct axis.
What #470 got wrong
#470 dropped Base, Arbitrum and Avalanche from the 0x chain allowlist because their chain ids truncate through
uint8_t. Measuring the generated token tables showed that is not the real shape of the problem:Two consequences:
chain_idwould not restore a single amount. There are no tokens for those three chains, so the lookup misses regardless of type. The truncation is a real latent bug, but it is not what makes the screens blank."Unknown token value"exactly as Base would. Even Ethereum fails for tokens outside its 1924.Chain identity was never the right question. The right question is whether the lookup resolved.
What this does
zx_isZxTransformERC20()andzx_isZxSwap()now claim a transaction only when both token lookups resolve. If either misses, the predicate returns false andethereum.cfalls through to the generic raw-calldata path — AdvancedMode-gated, bytes on screen. The owner can still sign; the device stops claiming it understood.The refusal must live in the
zx_is*()predicate, not the confirm: a false fromethereum_contractConfirmed()is treated as a user cancel and aborts signing (ethereum.c:740-745), whereas a false fromethereum_contractHandled()is the fall-through we want. Both decoders now share one resolver between predicate and confirm, so the two cannot disagree about what is displayable.Properties this has and the allowlist did not:
Tests
Ethereum.TransformErc20RequiresBothTokensResolvablecovers both directions: two real chain-1 table entries (TUSD, TGBP) resolve and the transaction is claimed; an unknown input, an unknown output, both unknown, and each of the three zero-coverage chains all refuse.Ethereum.TransformErc20RequiresCompleteCalldataForClearSigning(from #468) needed updating — it previously used zeroed token words, which under this change correctly no longer resolve. It now uses real addresses, so it still tests the streamed-tail boundary rather than passing for the wrong reason.Note the Base/Arbitrum/Avalanche assertions now pass via the resolution gate while the allowlist still contains all six chains — the allowlist is untouched by this PR.
Scope
Does not touch #414: a payload that fits one chunk still carries
transformations[]bytes the decoder never reads. Resolving both assets bounds what the trade can cost; it does not disclose the route.Corrects the diagnosis on #455, which stays open as the type-correctness bug — and is a prerequisite for ever adding Base/Arbitrum/Avalanche tables, since
uint8_tcannot distinguish 8453, 42161 or 43114.Verified
ARM cross-compile in the pinned builder image;
firmware-unit Ethereum.*9/9 and the fullxunitsuite green; cppcheck clean under CI's exact invocation (0 findings); clang-format clean across CI's entire lint scope, not just the changed files.