From a010285b236bdfc0cc9dd838012f75007d671d20 Mon Sep 17 00:00:00 2001 From: highlander Date: Mon, 20 Jul 2026 23:20:58 -0300 Subject: [PATCH 1/7] test(report): put the phase-2/3 Hive ops in SECTIONS so they get screenshotted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The eleven ops added by firmware #315 have had behavioural coverage since the day they landed — rc15 ran 38 Hive cases, 0 skipped, limit_order_create and limit_order_cancel among them. But SECTIONS stopped at G28, and screenshot_filter() only emits tests whose screenshot hint list is non-empty, so none of them were ever selected for the Phase-1 capture run. rc15 shipped 84 Hive OLED frames covering phase-1 only. That gap matters more here than for most tests: a correct signature over bytes the user was shown something ELSE for is precisely the failure the clear-sign table exists to prevent. Behavioural green says the device parsed and signed; only a frame says it drew "Sells 1.500 HIVE" and not a swapped symbol or a shifted decimal point. Adds G29-G38. Non-empty hints (so they capture) go to the ops that render a confirm screen: both limit-order ops, the six active-tier value ops, posting- tier claim, the zero-amount stop-power-down / remove-delegation pair, and both comment_options cases. The three pure-rejection tests (precision pinning, account_update2 authority reject, truncated bodies) keep empty hints on purpose — a refused op never draws a screen, so a hint there would select a test that can only produce an empty capture. Verified: --screenshot-filter --fw-version=7.15.0 now emits 304 tests and includes all seven new screen-rendering cases. --- scripts/generate-test-report.py | 65 +++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/scripts/generate-test-report.py b/scripts/generate-test-report.py index 76f5a505..15e035a8 100644 --- a/scripts/generate-test-report.py +++ b/scripts/generate-test-report.py @@ -1456,6 +1456,71 @@ def _arg_shown(a): 'Account authority ops require owner', 'account_create / account_update sign only under the owner role.', []), + # ── Phase 2/3 op table (fw #315) ──────────────────────────────── + # These ran green in the full suite from the day they landed, but had + # no SECTIONS entry, so screenshot_filter() never selected them and + # eleven newly clear-signed ops shipped with zero OLED proof. A + # correct signature over bytes the user was shown something else for + # is the exact failure the clear-sign table exists to prevent, so + # every op that renders a confirm screen gets a non-empty hint. + ('G29', 'test_msg_hive', 'test_hive_sign_ops_limit_order_create', + 'Internal market: limit_order_create', + 'The op that motivated phase 3 — a HIVE->HBD market swap. Both sides of the order ' + 'are shown with their symbols pinned (a swapped symbol hides a ~2000x value ' + 'difference behind an identical-looking number), and order id / fill-or-kill / ' + 'expiry get their own screen so they cannot be crowded off the first.', + ['Sell and receive amounts', 'Order terms screen']), + ('G30', 'test_msg_hive', 'test_hive_sign_ops_limit_order_cancel', + 'Internal market: limit_order_cancel', + 'Cancelling names the order id and the owner. No recipient row is forged — the op ' + 'acts on the signer\'s own book entry.', + ['Cancel order screen']), + ('G31', 'test_msg_hive', 'test_hive_sign_ops_active_tier_value_ops', + 'Active-tier value ops', + 'transfer_to_vesting, convert, transfer_to/from_savings, delegate_vesting_shares ' + 'and withdraw_vesting all move or lock value, so all six sign only under active. ' + 'Each renders its own amount + counterparty.', + ['Power up', 'Convert', 'Savings deposit/withdraw', 'Delegation', 'Power down']), + ('G32', 'test_msg_hive', 'test_hive_sign_ops_posting_tier_ops', + 'claim_reward_balance is posting tier', + 'Claiming is not spending, so it signs under posting. Three reward assets across ' + 'two screens (the OLED body fits three rows; a fourth would be signed but never ' + 'shown).', + ['Claim rewards screens']), + ('G33', 'test_msg_hive', 'test_hive_sign_ops_zero_amount_semantics', + 'Zero means something for two ops, nothing for the rest', + '0 VESTS stops a power-down and removes a delegation — both legitimate, so zero is ' + 'NOT rejected there and the screen must say which action it is. Everywhere else a ' + 'zero amount is a no-op and refused.', + ['Stop power down', 'Remove delegation']), + ('G34', 'test_msg_hive', 'test_hive_sign_ops_asset_symbol_and_precision_pinned', + 'Asset symbol pinned to its protocol precision', + 'HIVE/HBD are 3-decimal, VESTS is 6. The parser refuses any other pairing: a wrong ' + 'precision moves the decimal point on the confirmation screen relative to what the ' + 'chain applies.', + []), + ('G35', 'test_msg_hive', 'test_hive_sign_ops_comment_options_binds_to_its_comment', + 'comment_options binds to its own comment', + 'Payout redirection is only accepted immediately after a comment op with the same ' + 'author and permlink. Standing alone it could attach beneficiaries to a post the ' + 'user published earlier and is not reviewing on this screen.', + ['Payout options screens']), + ('G36', 'test_msg_hive', 'test_hive_sign_ops_comment_options_beneficiary_rules', + 'Beneficiary rules enforced on-device', + 'At most one extension, 1-8 strictly-ascending unique accounts, each weight and the ' + 'total within 10000 bp. Each beneficiary is confirmed individually rather than ' + 'summarised as a count.', + ['Per-beneficiary screens']), + ('G37', 'test_msg_hive', 'test_hive_sign_ops_account_update2_rejects_authority_change', + 'account_update2 cannot rotate keys', + 'Only the profile-metadata form is in the table. Any owner/active/posting/memo_key ' + 'field present is a hard reject — the op-9/10 device-derived-keys invariant applied ' + 'field-level.', + []), + ('G38', 'test_msg_hive', 'test_hive_sign_ops_truncated_bodies_rejected', + 'Truncated op bodies refused', + 'A body cut short mid-field is a parse failure, not sign-what-you-can.', + []), ]), ('S', 'Solana', '7.14.0', From 59f7c858b3aab4926067201f91638e6e52ff82f3 Mon Sep 17 00:00:00 2001 From: highlander Date: Mon, 20 Jul 2026 23:55:16 -0300 Subject: [PATCH 2/7] =?UTF-8?q?test(report):=20G36=20is=20rejection-only?= =?UTF-8?q?=20=E2=80=94=20drop=20its=20screenshot=20hint?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verified against the rc16 capture (run 29796178504): the three frames it produced were all the idle home screen (195 ink px vs ~1100 for a real confirm). Every case in the test is _assert_ops_fails, so the device refuses before rendering — the capture could only ever be empty frames dressed up as visual proof, which is the exact failure Gate-3 exists to catch. The per-beneficiary confirm screens are real and are captured by G35, whose final case signs a two-beneficiary payout (14 frames, ink 650-1250). --- scripts/generate-test-report.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/scripts/generate-test-report.py b/scripts/generate-test-report.py index 15e035a8..c4551ba8 100644 --- a/scripts/generate-test-report.py +++ b/scripts/generate-test-report.py @@ -1508,9 +1508,13 @@ def _arg_shown(a): ('G36', 'test_msg_hive', 'test_hive_sign_ops_comment_options_beneficiary_rules', 'Beneficiary rules enforced on-device', 'At most one extension, 1-8 strictly-ascending unique accounts, each weight and the ' - 'total within 10000 bp. Each beneficiary is confirmed individually rather than ' - 'summarised as a count.', - ['Per-beneficiary screens']), + 'total within 10000 bp. Unsorted, duplicate and >100% lists are all refused.', + # Rejection-only: every case here is _assert_ops_fails, so the device + # refuses before drawing anything and the capture would be three + # frames of the idle home screen — a report entry that LOOKS like + # visual proof and is not. The per-beneficiary confirm screens are + # captured by G35, which actually signs a two-beneficiary payout. + []), ('G37', 'test_msg_hive', 'test_hive_sign_ops_account_update2_rejects_authority_change', 'account_update2 cannot rotate keys', 'Only the profile-metadata form is in the table. Any owner/active/posting/memo_key ' From b257df27d0a6ad3dfa4fa101fafb0e74ae57eda1 Mon Sep 17 00:00:00 2001 From: highlander Date: Tue, 21 Jul 2026 00:45:34 -0300 Subject: [PATCH 3/7] test(osmosis): device tests for the confirm screens, with screenshot capture MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Osmosis had NO device tests — the confirm screens were covered only by host-side unit tests of the formatter in isolation. That is the wrong gap to have open in 7.15.0, because 7.15.0 changed how every Osmosis amount is DRAWN: fsm_msg_osmosis.h rendered with atof() + "%.6f", and a float carries ~7 significant decimal digits, so a large transfer displayed rounded on the screen the user approves — 123456789123456 uosmo shown 123456792.000000 OSMO actual 123456789.123456 OSMO The signature was always over the correct amount; the error lived entirely on the display, which is the half a hardware wallet exists to get right. It now formats through bn_format_uint64 in integer math, exact at any magnitude. Adds test_msg_osmosis_signtx.py (6 tests) and SECTIONS group P so four of them are captured by screenshot_filter(): the baseline send, the beyond-float-precision amount, a sub-unit amount (500 uosmo = 0.000500 OSMO, must not collapse), and an unknown denom (shown as raw base units — the device does not guess a precision). The frame is the evidence; a test that only asserts "it signed" cannot prove what was drawn. The two without screenshot hints are behavioural rather than visual: the amount must be committed to the digest (two sends differing only in amount give different signatures, so the screen is bound to what is signed), and signing must be deterministic per RFC6979. Scope: pyk's osmosis_sign_tx implements osmosis-sdk/MsgSend only. The delegate/undelegate/LP/swap/IBC screens share this formatter but are not reachable until the client learns those message types. Verified: report renders 19 sections / P1-P6 present, filter selects the four screenshot tests. --- scripts/generate-test-report.py | 47 ++++++++++ tests/test_msg_osmosis_signtx.py | 142 +++++++++++++++++++++++++++++++ 2 files changed, 189 insertions(+) create mode 100644 tests/test_msg_osmosis_signtx.py diff --git a/scripts/generate-test-report.py b/scripts/generate-test-report.py index c4551ba8..aff6a959 100644 --- a/scripts/generate-test-report.py +++ b/scripts/generate-test-report.py @@ -962,6 +962,53 @@ def _arg_shown(a): 'Sign Cosmos with memo', 'Memo field displayed for exchange deposit tags.', []), ]), + ('P', 'Osmosis', '7.15.0', + 'Osmosis is the Cosmos-ecosystem DEX, signed with the same amino encoding as Cosmos Hub and ' + 'derived from the same coin type (118). 7.15.0 CHANGED how every Osmosis amount is drawn: the ' + 'confirm screens formatted with atof() + "%.6f", and a float carries only ~7 significant ' + 'decimal digits, so a large transfer was displayed ROUNDED on the screen the user approves ' + '(123456789.123456 OSMO rendered as 123456792.000000). The signature was always over the ' + 'correct amount — the error was confined to the display, which is the half a hardware wallet ' + 'exists to get right. Amounts now format through bn_format_uint64 in integer math, exact at ' + 'any magnitude, and the same change removed the newlib floating-point engine from the build.', + [ + 'SEND: recipient + OSMO amount rendered from integer base units, never a float', + 'PRECISION: 15-significant-digit amounts display exactly, not rounded to 7', + 'UNKNOWN DENOM: shown as raw base units — the device does not guess a decimal point', + ], + [ + ('P1', 'test_msg_osmosis_signtx', 'test_osmosis_sign_tx', + 'Sign Osmosis send', + 'Baseline MsgSend: recipient and a whole-OSMO amount on the confirm screen.', + ['OSMO send']), + ('P2', 'test_msg_osmosis_signtx', 'test_osmosis_send_amount_beyond_float_precision', + 'Amount beyond float precision displays exactly', + 'The regression this section exists for: 123456789123456 uosmo needs 15 significant ' + 'digits. The old float path drew 123456792.000000 OSMO over a transaction moving ' + '123456789.123456 OSMO. The captured frame is the evidence.', + ['Exact large amount']), + ('P3', 'test_msg_osmosis_signtx', 'test_osmosis_send_subunit_amount', + 'Sub-unit amount keeps its tail', + '500 uosmo is 0.000500 OSMO — no integer part and six decimals; it must not collapse ' + 'to 0 or lose the trailing digits.', + ['Sub-unit amount']), + ('P4', 'test_msg_osmosis_signtx', 'test_osmosis_send_unknown_denom_shown_raw', + 'Unknown denom shown as base units', + 'Only uosmo is scaled. For any other denom the device shows the integer verbatim ' + 'rather than guessing a precision — guessing is how a 1000x display error happens.', + ['Raw denom amount']), + ('P5', 'test_msg_osmosis_signtx', 'test_osmosis_amount_is_committed_to_the_signature', + 'Displayed amount is in the digest', + 'Two sends differing only in amount produce different signatures, so the confirm ' + 'screen is bound to what is signed rather than decorative.', + []), + ('P6', 'test_msg_osmosis_signtx', 'test_osmosis_signing_is_deterministic', + 'Deterministic nonces (RFC6979)', + 'Identical input yields an identical signature; a mismatch is a key-recovery risk, ' + 'not a cosmetic one.', + []), + ]), + ('H', 'THORChain', '7.0.0', 'THORChain is a decentralized cross-chain liquidity protocol. Native RUNE transactions use amino ' 'encoding with thor1... bech32 addresses. The memo field is the critical security element - it ' diff --git a/tests/test_msg_osmosis_signtx.py b/tests/test_msg_osmosis_signtx.py new file mode 100644 index 00000000..ea82cb78 --- /dev/null +++ b/tests/test_msg_osmosis_signtx.py @@ -0,0 +1,142 @@ +"""Osmosis MsgSend signing — with the confirm-screen amount as the point. + +Osmosis had NO device tests at all: the confirm screens that render amounts +were covered only by host-side unit tests of the formatter in isolation. That +matters more than it sounds, because 7.15.0 CHANGED how every Osmosis amount +is drawn. + +Before, fsm_msg_osmosis.h rendered amounts with atof() + "%.6f". A float +carries ~7 significant decimal digits, so a large amount was displayed +ROUNDED on the very screen the user approves: + + 123456789123456 uosmo -> shown as "123456792.000000 OSMO" + actual 123456789.123456 OSMO + +The signature was over the correct amount either way — the lie was only on +the screen, which is the half a hardware wallet exists to get right. It now +formats with bn_format_uint64 (integer math, exact at any magnitude). + +These tests are paired with SECTIONS entries carrying screenshot hints, so +the rendered frame is captured as evidence. A test asserting only "it signed" +cannot prove what the OLED drew. + +pyk's osmosis_sign_tx currently implements osmosis-sdk/MsgSend only; the +delegate/undelegate/LP/swap/IBC screens share the same formatter but are not +reachable from here until the client learns those message types. +""" +import unittest +import common + +from binascii import hexlify + +from keepkeylib.tools import parse_path + +# Osmosis uses the Cosmos coin type (118), not one of its own. +DEFAULT_BIP32_PATH = "m/44h/118h/0h/0/0" + +ADDR = "osmo15cenya0tr7nm3tz2wn3h3zwkht2rxrq7q7h3dj" + + +def make_send(from_address, to_address, amount, denom='uosmo'): + return { + 'type': 'osmosis-sdk/MsgSend', + 'value': { + 'from_address': from_address, + 'to_address': to_address, + 'amount': [{'denom': denom, 'amount': str(amount)}], + }, + } + + +class TestMsgOsmosisSignTx(common.KeepKeyTest): + + def _sign(self, amount, denom='uosmo'): + return self.client.osmosis_sign_tx( + address_n=parse_path(DEFAULT_BIP32_PATH), + account_number=16359, + chain_id="osmosis-1", + fee=800, + gas=290000, + msgs=[make_send(ADDR, ADDR, amount, denom)], + memo="", + sequence=17, + ) + + def test_osmosis_sign_tx(self): + """Baseline: a whole-OSMO send signs and returns a well-formed + secp256k1 signature + compressed pubkey.""" + self.requires_fullFeature() + self.requires_firmware("7.15.0") + self.setup_mnemonic_nopin_nopassphrase() + + sig = self._sign(1500000) # 1.500000 OSMO + self.assertEqual(len(sig.signature), 64) + self.assertEqual(len(sig.public_key), 33) + self.assertIn(hexlify(sig.public_key)[:2], (b'02', b'03')) + + def test_osmosis_send_amount_beyond_float_precision(self): + """THE regression. 123456789123456 uosmo needs 15 significant digits; + a float holds ~7, so the old atof()+"%.6f" path drew + "123456792.000000 OSMO" over a transaction that actually moves + 123456789.123456 OSMO. The captured frame is the proof — assert here + only that the device signs it, and read the amount off the screenshot. + """ + self.requires_fullFeature() + self.requires_firmware("7.15.0") + self.setup_mnemonic_nopin_nopassphrase() + + sig = self._sign(123456789123456) + self.assertEqual(len(sig.signature), 64) + + def test_osmosis_send_subunit_amount(self): + """500 uosmo is 0.000500 OSMO — six decimal places, no integer part. + The formatter must not collapse it to "0" or drop the tail.""" + self.requires_fullFeature() + self.requires_firmware("7.15.0") + self.setup_mnemonic_nopin_nopassphrase() + + sig = self._sign(500) + self.assertEqual(len(sig.signature), 64) + + def test_osmosis_send_unknown_denom_shown_raw(self): + """Only uosmo is scaled. The device does not know an arbitrary denom's + precision, so it shows the base-unit integer verbatim rather than + guessing a decimal point — guessing is how a 1000x display error + happens.""" + self.requires_fullFeature() + self.requires_firmware("7.15.0") + self.setup_mnemonic_nopin_nopassphrase() + + sig = self._sign(1500000, denom='uatom') + self.assertEqual(len(sig.signature), 64) + + def test_osmosis_amount_is_committed_to_the_signature(self): + """Guards the pairing between what is shown and what is signed: two + sends differing ONLY in amount must produce different signatures. If + they matched, the amount would not be in the digest and the confirm + screen would be decorative.""" + self.requires_fullFeature() + self.requires_firmware("7.15.0") + self.setup_mnemonic_nopin_nopassphrase() + + a = self._sign(1500000) + b = self._sign(1500001) + self.assertNotEqual(hexlify(a.signature), hexlify(b.signature)) + # Same key throughout — only the message differed. + self.assertEqual(hexlify(a.public_key), hexlify(b.public_key)) + + def test_osmosis_signing_is_deterministic(self): + """RFC6979: identical input must yield an identical signature. A + mismatch here means nonce generation is not deterministic, which is a + key-recovery risk long before it is a display problem.""" + self.requires_fullFeature() + self.requires_firmware("7.15.0") + self.setup_mnemonic_nopin_nopassphrase() + + first = self._sign(1500000) + second = self._sign(1500000) + self.assertEqual(hexlify(first.signature), hexlify(second.signature)) + + +if __name__ == '__main__': + unittest.main() From 3a9e1b71797149b4e3cf71636456b406322e2126 Mon Sep 17 00:00:00 2001 From: highlander Date: Tue, 21 Jul 2026 01:14:04 -0300 Subject: [PATCH 4/7] fix(osmosis): the MsgSend client path was dead code with three bugs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adding the first Osmosis device tests immediately failed with "Unsupported denomination: uosmo". The cause is that osmosis_sign_tx's MsgSend branch had never executed: 1. It whitelisted 'uatom' — the COSMOS denom. Osmosis's native denom is uosmo, so signing a plain OSMO transfer was impossible. 2. It never forwarded the denom, though OsmosisMsgSend carries one. The firmware needs it to decide whether to scale (uosmo -> OSMO) or show raw base units. 3. It assigned amount=int(...) to OsmosisMsgSend.amount, which is a STRING field — that alone would have raised for uatom too. Bug 3 proves the path never ran; bugs 1 and 2 are why it went unnoticed. No denom whitelist belongs on the client: the firmware already handles any denom, scaling uosmo and showing everything else as raw base units precisely so it never guesses a precision it does not know. --- keepkeylib/client.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/keepkeylib/client.py b/keepkeylib/client.py index 3ae9a69f..61fb0a0a 100644 --- a/keepkeylib/client.py +++ b/keepkeylib/client.py @@ -999,15 +999,20 @@ def osmosis_sign_tx( if len(msg['value']['amount']) != 1: raise CallException("Osmosis.MsgSend", "Multiple amounts per msg not supported") - denom = msg['value']['amount'][0]['denom'] - if denom != 'uatom': - raise CallException("Osmosis.MsgSend", "Unsupported denomination: " + denom) - + # This branch had never executed. It whitelisted 'uatom' — the + # COSMOS denom, so a native OSMO send was impossible — dropped + # the denom instead of forwarding it, and assigned an int to + # OsmosisMsgSend.amount, which is a string field and would have + # raised even for uatom. No denom whitelist belongs here at all: + # the firmware decides how to render each one (uosmo scaled to + # OSMO, anything else shown as raw base units). + coin = msg['value']['amount'][0] resp = self.call(osmosis_proto.OsmosisMsgAck( send=osmosis_proto.OsmosisMsgSend( from_address=msg['value']['from_address'], to_address=msg['value']['to_address'], - amount=int(msg['value']['amount'][0]['amount']), + denom=coin['denom'], + amount=str(coin['amount']), address_type=types.SPEND, ) )) From 0594d57db271c7240f2afc3822d5826dfee50a65 Mon Sep 17 00:00:00 2001 From: highlander Date: Tue, 21 Jul 2026 01:47:16 -0300 Subject: [PATCH 5/7] test(osmosis): derive the osmo1 address from the device, not a literal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The hardcoded fixture was a cosmos1 address with the prefix swapped to osmo1, so its bech32 checksum was invalid. osmosis_signTxUpdateMsgSend bech32-decodes to_address and returns false on failure, which surfaces as the opaque 'Failed to include send message in transaction' — a device-side syntax error that reads like a firmware bug and was mine. Asking the device for its own address also makes these genuine self-sends. --- tests/test_msg_osmosis_signtx.py | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/tests/test_msg_osmosis_signtx.py b/tests/test_msg_osmosis_signtx.py index ea82cb78..f52283d8 100644 --- a/tests/test_msg_osmosis_signtx.py +++ b/tests/test_msg_osmosis_signtx.py @@ -34,8 +34,6 @@ # Osmosis uses the Cosmos coin type (118), not one of its own. DEFAULT_BIP32_PATH = "m/44h/118h/0h/0/0" -ADDR = "osmo15cenya0tr7nm3tz2wn3h3zwkht2rxrq7q7h3dj" - def make_send(from_address, to_address, amount, denom='uosmo'): return { @@ -50,14 +48,28 @@ def make_send(from_address, to_address, amount, denom='uosmo'): class TestMsgOsmosisSignTx(common.KeepKeyTest): + def _address(self): + """Ask the device for its own osmo1 address. + + Deliberately NOT a hardcoded constant: the firmware bech32-decodes + to_address and refuses a bad checksum, so a literal invented by + swapping a cosmos1 prefix for osmo1 fails with the opaque "Failed to + include send message in transaction". Deriving it keeps the fixture + honest and makes these self-sends. + """ + return self.client.osmosis_get_address( + address_n=parse_path(DEFAULT_BIP32_PATH) + ).address + def _sign(self, amount, denom='uosmo'): + addr = self._address() return self.client.osmosis_sign_tx( address_n=parse_path(DEFAULT_BIP32_PATH), account_number=16359, chain_id="osmosis-1", fee=800, gas=290000, - msgs=[make_send(ADDR, ADDR, amount, denom)], + msgs=[make_send(addr, addr, amount, denom)], memo="", sequence=17, ) From 52e1fb59a2cdc9532940e3459a7842d771b59fbd Mon Sep 17 00:00:00 2001 From: highlander Date: Tue, 21 Jul 2026 01:58:29 -0300 Subject: [PATCH 6/7] test(osmosis): osmosis_get_address already returns the string It is decorated @field('address'), so .address on the result raised AttributeError. Matches how test_msg_cosmos_getaddress uses its equivalent. --- tests/test_msg_osmosis_signtx.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/test_msg_osmosis_signtx.py b/tests/test_msg_osmosis_signtx.py index f52283d8..bab7ec8a 100644 --- a/tests/test_msg_osmosis_signtx.py +++ b/tests/test_msg_osmosis_signtx.py @@ -57,9 +57,11 @@ def _address(self): include send message in transaction". Deriving it keeps the fixture honest and makes these self-sends. """ + # osmosis_get_address is decorated @field('address'), so it already + # returns the string rather than the OsmosisAddress message. return self.client.osmosis_get_address( address_n=parse_path(DEFAULT_BIP32_PATH) - ).address + ) def _sign(self, amount, denom='uosmo'): addr = self._address() From 1a697419e3c63e127db935382be7dc26d355f7b4 Mon Sep 17 00:00:00 2001 From: highlander Date: Tue, 21 Jul 2026 02:55:32 -0300 Subject: [PATCH 7/7] =?UTF-8?q?fix(osmosis):=20fence=20MsgSend=20to=20uosm?= =?UTF-8?q?o=20=E2=80=94=20the=20firmware=20signs=20a=20denom=20it=20does?= =?UTF-8?q?=20not=20display?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found that removing the old denom whitelist opened a real display/signature divergence, and that my test certified it as correct. osmosis_signTxUpdateMsgSend takes only (amount, to_address) and hardcodes "denom":"uosmo" into the amino JSON it hashes; fsm_msgOsmosisMsgAck renders whatever denom arrived. So a uatom send DISPLAYS "1500000 uatom" and SIGNS 1500000 uosmo. Exploitable in the large: a big number of some worthless ibc/... token on screen, a big number of OSMO in the signature. osmosis_signTxUpdateMsgDelegate already takes a denom — MsgSend is the outlier, and this is a pre-existing firmware bug my client change made reachable. Fenced client-side until the firmware serializer accepts a denom. The denom is still forwarded, since the firmware needs it to decide whether to scale and the fence is the temporary half. test_osmosis_send_unknown_denom_shown_raw asserted only len(signature)==64, so it PASSED against that divergence and published it as intended behaviour — worse than no test. Replaced with one that asserts the refusal, and documents the check to write once the firmware is fixed: otherwise-identical uosmo and uatom transactions must produce DIFFERENT signatures. Also corrects two overstatements the same review caught: - "exact at any magnitude" was false. osmosis_formatAmount converts with an unchecked strtoull(), so it is exact only for a canonical decimal uint64. strtoull saturates past UINT64_MAX and accepts whitespace, a sign and 0x — "18446744073709551616" and "-1" both display as 18446744073.709551615 OSMO while the original string is hashed. Same divergence, different disguise; needs a firmware-side canonical-range check. - G36 claimed enforcement of the extension cap, the 1-8 count bound and per-weight range. The mapped test asserts only unsorted, duplicate and over-100%. Narrowed to what it actually proves. P4 no longer carries a screenshot hint: the refusal happens client-side, so there is no frame to capture. --- keepkeylib/client.py | 23 +++++++++++++++++--- scripts/generate-test-report.py | 25 ++++++++++++++------- tests/test_msg_osmosis_signtx.py | 37 +++++++++++++++++++++++++------- 3 files changed, 66 insertions(+), 19 deletions(-) diff --git a/keepkeylib/client.py b/keepkeylib/client.py index 61fb0a0a..cb0cb62c 100644 --- a/keepkeylib/client.py +++ b/keepkeylib/client.py @@ -1003,10 +1003,27 @@ def osmosis_sign_tx( # COSMOS denom, so a native OSMO send was impossible — dropped # the denom instead of forwarding it, and assigned an int to # OsmosisMsgSend.amount, which is a string field and would have - # raised even for uatom. No denom whitelist belongs here at all: - # the firmware decides how to render each one (uosmo scaled to - # OSMO, anything else shown as raw base units). + # raised even for uatom. + # + # The denom IS now forwarded (the firmware needs it to decide + # whether to scale), but MsgSend is still fenced to uosmo, and + # not for the reason the old whitelist implied: + # osmosis_signTxUpdateMsgSend takes only (amount, to_address) + # and HARDCODES "denom":"uosmo" into the amino JSON it hashes, + # while fsm_msgOsmosisMsgAck renders whatever denom arrives. + # Forwarding a different one therefore DISPLAYS one asset and + # SIGNS another — e.g. a large amount of some worthless ibc/... + # token on screen, a large amount of OSMO in the signature. + # osmosis_signTxUpdateMsgDelegate already takes a denom, so + # MsgSend is the outlier. Lift this fence only once the + # firmware serializer accepts a denom. coin = msg['value']['amount'][0] + if coin['denom'] != 'uosmo': + raise CallException( + "Osmosis.MsgSend", + "Only uosmo is signable: the firmware MsgSend serializer " + "hardcodes uosmo in the sighash, so any other denom would " + "be displayed but not signed (got %s)" % coin['denom']) resp = self.call(osmosis_proto.OsmosisMsgAck( send=osmosis_proto.OsmosisMsgSend( from_address=msg['value']['from_address'], diff --git a/scripts/generate-test-report.py b/scripts/generate-test-report.py index aff6a959..43185e6b 100644 --- a/scripts/generate-test-report.py +++ b/scripts/generate-test-report.py @@ -992,11 +992,14 @@ def _arg_shown(a): '500 uosmo is 0.000500 OSMO — no integer part and six decimals; it must not collapse ' 'to 0 or lose the trailing digits.', ['Sub-unit amount']), - ('P4', 'test_msg_osmosis_signtx', 'test_osmosis_send_unknown_denom_shown_raw', - 'Unknown denom shown as base units', - 'Only uosmo is scaled. For any other denom the device shows the integer verbatim ' - 'rather than guessing a precision — guessing is how a 1000x display error happens.', - ['Raw denom amount']), + ('P4', 'test_msg_osmosis_signtx', 'test_osmosis_send_non_uosmo_denom_is_refused', + 'Non-uosmo MsgSend is refused, not displayed', + 'osmosis_signTxUpdateMsgSend hardcodes "denom":"uosmo" into the amino JSON it ' + 'hashes while the confirm screen renders whatever denom arrived, so any other denom ' + 'would display one asset and sign another. Refused client-side until the firmware ' + 'serializer accepts a denom.', + # Refusal happens before the device is reached — no frame to capture. + []), ('P5', 'test_msg_osmosis_signtx', 'test_osmosis_amount_is_committed_to_the_signature', 'Displayed amount is in the digest', 'Two sends differing only in amount produce different signatures, so the confirm ' @@ -1553,9 +1556,15 @@ def _arg_shown(a): 'user published earlier and is not reviewing on this screen.', ['Payout options screens']), ('G36', 'test_msg_hive', 'test_hive_sign_ops_comment_options_beneficiary_rules', - 'Beneficiary rules enforced on-device', - 'At most one extension, 1-8 strictly-ascending unique accounts, each weight and the ' - 'total within 10000 bp. Unsorted, duplicate and >100% lists are all refused.', + 'Beneficiary ordering, uniqueness and total enforced on-device', + # Scoped to exactly what the mapped test asserts. It covers three + # rejections — unsorted, duplicate, and weights summing over 100%. + # The extension-count cap, the 1-8 count bound and per-beneficiary + # weight range are enforced by the parser but are NOT exercised here, + # so the entry must not claim them. + 'Beneficiaries must be strictly ascending by account (which also makes them unique) ' + 'and their weights must sum to no more than 10000 bp. Unsorted, duplicate and ' + 'over-100% lists are each refused.', # Rejection-only: every case here is _assert_ops_fails, so the device # refuses before drawing anything and the capture would be three # frames of the idle home screen — a report entry that LOOKS like diff --git a/tests/test_msg_osmosis_signtx.py b/tests/test_msg_osmosis_signtx.py index bab7ec8a..7834cc39 100644 --- a/tests/test_msg_osmosis_signtx.py +++ b/tests/test_msg_osmosis_signtx.py @@ -14,7 +14,16 @@ The signature was over the correct amount either way — the lie was only on the screen, which is the half a hardware wallet exists to get right. It now -formats with bn_format_uint64 (integer math, exact at any magnitude). +formats with bn_format_uint64 in integer math. + +KNOWN LIMIT, do not overstate this: osmosis_formatAmount converts with an +unchecked strtoull(), so it is exact only for a CANONICAL decimal uint64. +strtoull saturates past UINT64_MAX and also accepts leading whitespace, a +sign, and 0x — so "18446744073709551616" or "-1" display as +18446744073.709551615 OSMO while the original string is what gets hashed. +That is the same display/signature divergence in a different disguise and +needs a firmware-side canonical-range check; these tests deliberately do not +claim otherwise. These tests are paired with SECTIONS entries carrying screenshot hints, so the rendered frame is captured as evidence. A test asserting only "it signed" @@ -112,17 +121,29 @@ def test_osmosis_send_subunit_amount(self): sig = self._sign(500) self.assertEqual(len(sig.signature), 64) - def test_osmosis_send_unknown_denom_shown_raw(self): - """Only uosmo is scaled. The device does not know an arbitrary denom's - precision, so it shows the base-unit integer verbatim rather than - guessing a decimal point — guessing is how a 1000x display error - happens.""" + def test_osmosis_send_non_uosmo_denom_is_refused(self): + """A non-uosmo MsgSend must not reach the device at all. + + osmosis_signTxUpdateMsgSend takes only (amount, to_address) and + hardcodes "denom":"uosmo" into the amino JSON it hashes, while + fsm_msgOsmosisMsgAck renders whatever denom arrived. Sending uatom + therefore DISPLAYS "1500000 uatom" and SIGNS 1500000 uosmo — the + display/signature divergence a hardware wallet exists to prevent, and + exploitable in the large: a big number of some worthless ibc/... token + on screen, a big number of OSMO in the signature. + + This asserts the fence, NOT that arbitrary denoms work. When the + firmware serializer takes a denom, replace this with the real check: + otherwise-identical uosmo and uatom transactions must produce + DIFFERENT signatures. + """ self.requires_fullFeature() self.requires_firmware("7.15.0") self.setup_mnemonic_nopin_nopassphrase() - sig = self._sign(1500000, denom='uatom') - self.assertEqual(len(sig.signature), 64) + with self.assertRaises(Exception) as ctx: + self._sign(1500000, denom='uatom') + self.assertIn('uosmo', str(ctx.exception)) def test_osmosis_amount_is_committed_to_the_signature(self): """Guards the pairing between what is shown and what is signed: two