fix(evm): bind transformERC20 clear-signing to full calldata

[BitHighlander]

Summary

Fail closed when an Ethereum contract decoder does not have the complete calldata in data_initial_chunk.

transformERC20 was checked before the existing completeness gate. A host could therefore supply a valid 132-byte 0x prefix, stream an additional calldata tail through EthereumTxAck, and have that tail included in the signature without the raw-data disclosure path.

This moves the completeness gate ahead of every decoder, including transformERC20.

Behavior

• Complete, one-chunk transformERC20 calldata remains clear-signable.
• Any transaction with streamed calldata falls through to the generic raw-data disclosure path and its AdvancedMode policy gate.
• Unknown or oversized inputs fail closed; an incomplete decoder list can reduce display quality, but cannot suppress disclosure.

Regression coverage

The new test asserts both sides of the boundary:

• data_total == data_initial_chunk.size is handled.
• data_total > data_initial_chunk.size is not handled.

The red-team negative control demonstrated that the pre-fix build signed a transaction carrying an appended 2 KiB tail with the same two confirmations as the honest call. The fixed build refuses that transaction under the default raw-data policy.

Verification

• Pinned emulator build: 77 firmware tests, 9 board tests, and 4 crypto tests passed.
• Pinned ARM release build completed successfully.
• Focused cppcheck passed.
• git diff --check and source formatting passed.

Closes #444.

[BitHighlander]

Reviewed, and pushed one commit to this branch: abfe07366.

The gate itself is correct

Hoisting data_total != msg->data_initial_chunk.size above the transformERC20 exemption is the right shape. The other handlers were already behind it; transformERC20 was the sole exemption, and it was the one signing an undisclosed tail.

The added test negative-controls: rebuilt against the pre-fix ethereum_contracts.c, it fails with Actual: true / Expected: false. That is real coverage, not decoration. The positive half exercises the detector properly — chain_id=1 passes zx_isExchangeProxyChain, to is the Exchange Proxy, selector 0x415565b0, size 4 + 4*32 clears the < 4 + 4*32 guard in zx_confirmZxTransERC20.

What was missing: the warning

Falling through is only half the policy. Never block someone from their own funds, but never let them approve bytes the device did not interpret without saying so. The AdvancedMode gate does the first half. The second half did not exist on this path.

Every sibling chain that cannot interpret its payload says so in a screen of its own — TRON (fsm_msg_tron.h:123), opaque Solana (fsm_msg_solana.h:427), EIP-712 blind-sign (fsm_msg_ethereum.h:277). ETH contract data had none, and its screen is the one most likely to be misread as disclosure: layoutEthereumData() prints the first 24 bytes as hex followed by "...N bytes", which looks like the calldata is being shown when the device has not interpreted any of it.

abfe07366 adds an explicit "Blind Sign" screen first, naming data_total — the bytes the signature will cover, not the bytes that happened to arrive first.

That warning is a formatted body whose length grows with the byte count, and data_total is bounded only by the 16000000 limit in ethereum_signing_init(). If its longest form overflowed, confirm_helper() would put a "Cut Off" screen in front of it and the warning would cost two holds. BodyFits.BlindSignWarningFitsAtEveryByteCount pins 132 / 1025 / 65536 / 16000000 — all fit in three rows.

The disclosure gap this PR had

The BREAKING table was not updated. This is a user-facing behaviour change: data_initial_chunk is 1024 bytes and 0x aggregates across liquidity sources, so multi-transformation routes routinely exceed it. Those swaps previously showed a clear-signed "Transform ERC20" screen while the tail streamed in unshown; they now need AdvancedMode and show the blind-sign warning. Added to docs/release/7.14.2.md.

Worth stating plainly for reviewers: the old exemption was reasoned, not an oversight. The four displayed values are static head words and were always correct, and the argument was that they bound the outcome. That argument rests on the 0x Exchange Proxy enforcing it — and the Exchange Proxy is upgradeable, so it is not a guarantee this firmware can make.

Verified on this branch head

ARM cross-compile in the pinned builder image; full firmware-unit + board-unit suites green (make xunit exit 0); cppcheck clean under CI's exact invocation (0 findings); clang-format clean.

One procedural note

This PR branched at 7e07b2d92; release/7.14.2 is now at a34d0532a (the #443 merge plus a docs commit). Per #469, PRs targeting release/** get no pull_request CI event at all — checks come from the head-branch push run, so they never test the merge result. The content here is orthogonal to both intervening commits, and I verified the merge locally, but the checks on this PR should not be read as "green on what will be on the branch".

[BitHighlander]

Correcting my earlier review: this does not close #444, and the Fixes #444. trailer should be Refs #444.

The gate proves the calldata was RECEIVED, not DISPLAYED

transformERC20's ABI head is 164 bytes — selector(4) + inputToken(32) + outputToken(32) + inputTokenAmount(32) + minOutputAmount(32) + offset-to-transformations[](32). zx_confirmZxTransERC20() displays exactly four values, all inside that head (zxtransERC20.c:74,75,80,81).

So for any calldata that fits one chunk:

data_total == data_initial_chunk.size  ->  gate passes
zx_isZxTransformERC20()                ->  true, clear-signs
displayed                              ->  bytes 0..163
signed but never shown                 ->  bytes 164..data_total

At the 1024-byte chunk limit that is up to 860 bytes of attacker-controlled transformations[], clear-signed and undisclosed. That is the #444 defect, unchanged. The fix narrows it from "unbounded tail" to "≤860-byte tail" — a real improvement, but chunk-spanning is an arbitrary boundary that has nothing to do with disclosure.

And the cost is real

The repo's own recorded mainnet fixture (deps/python-keepkey/tests/test_msg_ethereum_erc20_0x_signtx.py, etherscan 0xcf94f79d…) is 1564 bytes of calldata. So real 0x routes do exceed one chunk, and they now require AdvancedMode and get the raw-hex screen.

That means this PR pays most of the user-facing cost of a full fix while leaving the hole open below the chunk boundary.

(One thing I got wrong and want on the record: I initially flagged that this breaks test__sign_transformERC20. It does not — that test calls apply_policy("AdvancedMode", 1). The comment block above that call still says "No AdvancedMode policy is set here on purpose", which is stale and misleading; worth deleting.)

The actual choice

The four displayed values hide the tail at every size, not just above 1024 bytes. So:

1. The bounded-outcome argument holds — input amount and minimum output bound the user's loss, so the tail doesn't need disclosure. Then chunk-spanning is irrelevant: revert this, keep clear-signing at all sizes, and close transformERC20 is exempt from the data_total binding and signs an undisclosed calldata tail #444 as accepted-with-reasoning. The catch: that argument rests on the 0x Exchange Proxy enforcing it, and the Proxy is upgradeable.

2. It doesn't hold — then transformERC20 cannot be clear-signed at any size, because its variable tail is never shown. Drop the decoder; all of it goes to raw hex behind AdvancedMode. That fully closes transformERC20 is exempt from the data_total binding and signs an undisclosed calldata tail #444 and matches the stated policy: always sign, require the advanced flag, and the raw hex is the blind-sign disclosure.

This PR is between the two and gets both costs. My recommendation is (2) if the bounded-outcome argument is not trusted, (1) if it is — but not the midpoint.

If you merge as-is, change the trailer to Refs #444 and leave #444 open with the ≤860-byte residual recorded.

[BitHighlander]

Two corrections to my own numbers above, both found by re-checking rather than by anyone disputing them.

The fixture is 1480 bytes, not 1564. My first count swept up the signature hex literals alongside the data= argument. Decoded properly: 1480 bytes, selector 415565b0, transformations[] count 2 — entry 0 FillQuoteTransformer (deploymentNonce 19) spanning 260..1060, entry 1 PayTakerTransformer (deploymentNonce 7) spanning 1060..1412, plus a 68-byte 0x affiliate suffix. The second transformation starts at byte 1060 — past the 1024 boundary. Dropping it would strand the funds in the Exchange Proxy, so no host emits a shorter form. The conclusion is unchanged and better supported: production 0x traffic crosses the chunk limit.

"It is not a refusal" was wrong, and I had written that into the release notes. AdvancedMode is off by default (include/keepkey/firmware/policy.h:34 → {true, "AdvancedMode", true, false}), so on a default-configuration device lib/firmware/ethereum.c:808 answers a >1024-byte transformERC20 with fsm_sendFailure(ActionCancelled, "Arbitrary contract data signing disabled by policy") — a hard refusal, not a degraded screen. Enabling AdvancedMode signs it and shows the raw hex.

That is still the intended policy — never block someone from their own funds, never present uninterpreted bytes as though the device understood them — but the default answer for a 0x swap becomes "turn on AdvancedMode", and the notes have to say that rather than implying the signing path is unchanged.

d50f4f9b8 corrects the BREAKING entry with the measured figures, the refusal framing, and the ≤860-byte residual that keeps #444 open.

The substantive question from my previous comment stands: the four displayed values hide the tail at every size, so this gate is not a disclosure boundary. Either the bounded-outcome argument is trusted (revert, close #444 as accepted-with-reasoning) or it is not (drop the decoder, raw hex for all transformERC20). Merging as-is is defensible as a strict narrowing, but the trailer must become Refs #444.

[BitHighlander]

Correcting myself: my refutation above was wrong.

I wrote that this PR does not break test__sign_transformERC20 because "that test calls apply_policy("AdvancedMode", 1)". That is true of python-keepkey master. It is not true of the commit this repository actually pins.

deps/python-keepkey is pinned at 81e581f, and in that tree the test is:

def test__sign_transformERC20(self):
    self.requires_fullFeature()
    self.requires_firmware("7.1.5")
    self.setup_mnemonic_nopin_nopassphrase()

    sig_v, sig_r, sig_s = self.client.ethereum_sign_tx(...)   # no apply_policy

No apply_policy anywhere. I read the working tree instead of the pinned submodule and drew the opposite conclusion. The agent finding I dismissed was closer to correct than I credited.

Verified by controlled experiment

Same pinned test file (81e581f), only the firmware varies:

firmware	result
1af2ffe7de (pre-release base)	1 passed in 0.26s
release/7.14.2 head	1 failed — CallException: (4, 'Arbitrary contract data signing disabled by policy')

The mechanism is exactly this PR's: the recorded mainnet payload is 1480 bytes, so it no longer clear-signs and falls through to the generic contract-data path, which is AdvancedMode-gated and refuses on a default device.

What this does and does not change

It does not change whether the fix is right — it is, and master's copy of the test already accounts for it by enabling AdvancedMode. It changes the accounting: this is one of the 22 failures now visible via #477, and it belongs in the "intentional change, stale pinned expectation" bucket that the deps/python-keepkey repin resolves.

Recording it because "I checked and it doesn't break anything" was stated with more confidence than the evidence supported, and the distinction between the pinned submodule and the working tree is exactly the sort of thing that should not be discovered later.