fix(osmosis): fence MsgSend to uosmo — the firmware signs a denom it does not display

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.

Author: BitHighlander

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'],

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

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