fix(legacy): fix Polkadot signing issues #172
Conversation
WalkthroughThe changes modify Polkadot network handling by adjusting address type initialization to 42, removing strict buffer validation checks, clearing memory before use, and establishing defaults for unrecognized networks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
legacy/firmware/polkadot.c (1)
136-152: Reject signing for unknown Polkadot networks (critical)get_signer_address now defaults addressType = 42 and always returns true. That lets polkadot_sign_tx proceed and sign transactions for unrecognized networks using SS58 prefix 42 (westend). This can misencode recipient addresses and risk irreversible fund loss. Restore explicit validation: reject unknown networks instead of defaulting.
Location: legacy/firmware/polkadot.c (get_signer_address, ~lines 136–152).
Suggested fix (minimal):
} else if (!strncmp(msg->network, "manta", 5)) { addressType = 77; + } else { + // Unknown network - reject signing + return false; } polkadot_get_address_from_public_key(node->public_key + 1, address, addressType); return true;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
legacy/firmware/polkadot.c(2 hunks)legacy/firmware/polkadot/parser_impl.c(0 hunks)legacy/firmware/polkadot/parser_impl_common.c(2 hunks)
💤 Files with no reviewable changes (1)
- legacy/firmware/polkadot/parser_impl.c
🧰 Additional context used
🪛 Clang (14.0.6)
legacy/firmware/polkadot/parser_impl_common.c
[warning] 346-346: the result from calling 'memcpy' is not null-terminated
(bugprone-not-null-terminated-result)
[warning] 346-346: 5 is a magic number; consider replacing it with a named constant
(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
[warning] 373-373: 11 is a magic number; consider replacing it with a named constant
(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Style check
- GitHub Check: Defs check
- GitHub Check: Gen check
🔇 Additional comments (2)
legacy/firmware/polkadot.c (1)
163-164: Good practice: clearing the buffer before use.Adding
memzeroprevents stale data inpolkadot_networkfrom previous operations. This is correct.legacy/firmware/polkadot/parser_impl_common.c (1)
373-374: Address type 42 is safe, but silent fallback for unknown networks needs clearer handling.The code does intentionally degrade to SS58 prefix 42 (the generic Substrate address format—a valid, standard prefix) for unknown networks. However, the real concern is justified: unknown networks proceed silently with minimal user warning.
Here's what actually happens:
- Unknown network → ticker shows " UNKN", decimal set to 0, returns type 42
- Westend (legitimate) → ticker shows "WND", decimal 12, also returns type 42
- Users only see " UNKN" ticker as the signal something is wrong (ticker appears alongside amounts in UI)
The fallback itself isn't technically unsafe—prefix 42 is standard and valid. The risk is that users can sign transactions for unrecognized networks without explicit rejection or prominent warnings. The ticker alone may not catch user attention.
Consider:
- Should unknown networks be rejected outright instead?
- Should a user confirmation dialog appear before signing?
- Is the " UNKN" ticker sufficient, or does it need a more visible warning?
| memcpy(__polkadot_ticker, " UNKN", 5); | ||
| __polkadot_decimal = 0; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the leading space in " UNKN" and decimal = 0.
Two concerns:
- The ticker
" UNKN"has a leading space. Is this intentional? - Setting
__polkadot_decimal = 0means amounts for unknown networks display without decimal places, which could mislead users about the actual value being transferred.
Apply this diff if the leading space is unintentional:
- memcpy(__polkadot_ticker, " UNKN", 5);
+ memcpy(__polkadot_ticker, "UNKN", 4);Consider setting a non-zero default decimal or warning the user that the network is unrecognized.
🏁 Script executed:
#!/bin/bash
# View the function context and surrounding lines
sed -n '340,380p' legacy/firmware/polkadot/parser_impl_common.cLength of output: 1639
🏁 Script executed:
#!/bin/bash
# Find all assignments to __polkadot_ticker to see the pattern
rg -n '__polkadot_ticker' legacy/firmware/polkadot/parser_impl_common.c -A 1 -B 1Length of output: 1298
🏁 Script executed:
#!/bin/bash
# Find all assignments to __polkadot_decimal to see the pattern
rg -n '__polkadot_decimal' legacy/firmware/polkadot/parser_impl_common.c -A 1 -B 1Length of output: 1518
🏁 Script executed:
#!/bin/bash
# Find where these variables are declared/used in display/printf contexts
rg -n '__polkadot_ticker|__polkadot_decimal' legacy/firmware/polkadot/ --type c -C 3Length of output: 6342
Fix " UNKN" leading space and decimal = 0 for unknown networks.
Two real issues found:
-
Leading space is inconsistent. Every known network ticker (Polkadot, Kusama, Astar, Westend, Joystream, Manta) copies without a leading space. The " UNKN" breaks this pattern and should match.
-
Decimal = 0 breaks display formatting. All known networks get proper decimal values (COIN_AMOUNT_DECIMAL_PLACES, etc.). Setting it to 0 for unknown networks means amounts display as whole numbers with no decimal places—misleading users about actual values.
Apply the suggested diff to remove the space and fix the buffer size:
- memcpy(__polkadot_ticker, " UNKN", 5);
+ memcpy(__polkadot_ticker, "UNKN", 4);Also set a meaningful decimal value for unknown networks instead of 0—consider using a safe default like COIN_AMOUNT_DECIMAL_PLACES or a smaller fallback.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Clang (14.0.6)
[warning] 346-346: the result from calling 'memcpy' is not null-terminated
(bugprone-not-null-terminated-result)
[warning] 346-346: 5 is a magic number; consider replacing it with a named constant
(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
🤖 Prompt for AI Agents
In legacy/firmware/polkadot/parser_impl_common.c around lines 346-347, the code
copies " UNKN" (with leading space) and sets __polkadot_decimal = 0 for unknown
networks; remove the leading space and copy the correct number of bytes for
"UNKN" (use 4 or sizeof("UNKN")-1) and ensure the ticker buffer is
null-terminated, and set __polkadot_decimal to a sensible default (e.g.
COIN_AMOUNT_DECIMAL_PLACES or a smaller fallback constant) instead of 0.
Summary by CodeRabbit
Release Notes