fix(tron): bind transfer fields before signing - #15
Conversation
📝 WalkthroughWalkthroughTron transaction specifications now include transfer details. Verification checks native amounts and TRC20 parameters, and callers pass the transfer descriptor through signing and service flows. Tests update fixtures and cover valid and invalid transfer payloads. ChangesTron transfer verification
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TransactionSpec
participant TronClient
participant TronVerifier
TransactionSpec->>TronClient: transfer descriptor
TronClient->>TronVerifier: raw data, recipient, txid, transfer
TronVerifier->>TronVerifier: validate native amount or TRC20 parameter
TronVerifier-->>TronClient: verification result
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tx/tron.rs`:
- Around line 84-97: Replace the raw byte-substring validation in the transfer
verification flow with Tron transaction decoding: parse the transaction
contract, require the contract type matching the requested TronTransfer variant,
and compare the native amount or TRC20 ABI parameters from that contract payload
against the requested values. Ensure expected bytes in unrelated fields are
rejected, and add fixtures/tests covering those cases.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8d2313d0-5573-41b7-904b-4633112791d4
📒 Files selected for processing (8)
crates/tinywallet-module/src/service/mod.rscrates/tinywallet-module/src/service/test.rscrates/tinywallet-module/tests/module_e2e.rssrc/client/test.rssrc/client/tron.rssrc/tx/tron.rssrc/wire/mod.rssrc/wire/test.rs
| let raw = decode_hex(raw_data_hex)?; | ||
| let expected = match transfer { | ||
| TronTransfer::Native { amount_sun } => encode_varint(*amount_sun), | ||
| TronTransfer::Trc20 { parameter_hex } => decode_hex(parameter_hex)?, | ||
| }; | ||
| if expected.is_empty() || !raw.windows(expected.len()).any(|window| window == expected) { | ||
| let field = match transfer { | ||
| TronTransfer::Native { .. } => "amount", | ||
| TronTransfer::Trc20 { .. } => "TRC20 transfer parameter", | ||
| }; | ||
| return Err(Error::UntrustedResponse { | ||
| reason: format!("the node's transaction does not contain the requested {field}"), | ||
| }); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- tron implementation and tests ---'
sed -n '1,125p' src/tx/tron.rs
sed -n '230,325p' src/tx/tron.rs
printf '%s\n' '--- Tron verification call sites ---'
rg -n -C 5 'verify_transfer|raw_data_hex|expected_txid|build_unsigned|attach_signature' \
src crates/tinywallet-module/src/service/mod.rs
printf '%s\n' '--- exact predicate probe ---'
python3 - <<'PY'
def accepts(raw: bytes, expected: bytes) -> bool:
return bool(expected) and any(
raw[i:i + len(expected)] == expected
for i in range(len(raw) - len(expected) + 1)
)
cases = [
("native bytes occur in unrelated field", bytes.fromhex("12010f220101"), bytes.fromhex("0f")),
("trc20 parameter occurs in unrelated field", bytes.fromhex("1204deadbeef2204aabbccdd"), bytes.fromhex("aabbccdd")),
("expected bytes absent", bytes.fromhex("1201aa"), bytes.fromhex("0f")),
]
for name, raw, expected in cases:
print(f"{name}: {accepts(raw, expected)}")
PYRepository: tinyhumansai/tinywallet
Length of output: 46718
Other (CWE-345)
Reachability: External
Parse the transfer contract before accepting the requested fields.
raw.windows(...) only proves that the requested bytes occur somewhere in raw_data. A compromised node can place the expected native varint or TRC20 parameter in an unrelated field while using different transfer values. The transaction-ID check does not bind these bytes to the requested transfer fields.
Decode the Tron transaction structure, require the expected contract type, and compare the native amount or TRC20 ABI parameters within that contract payload. Add fixtures that place the expected bytes in an unrelated field and assert rejection.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tx/tron.rs` around lines 84 - 97, Replace the raw byte-substring
validation in the transfer verification flow with Tron transaction decoding:
parse the transaction contract, require the contract type matching the requested
TronTransfer variant, and compare the native amount or TRC20 ABI parameters from
that contract payload against the requested values. Ensure expected bytes in
unrelated fields are rejected, and add fixtures/tests covering those cases.
1 participant
Summary
Bind every host-requested Tron transfer field to the node-built transaction before signing. Native transfers verify the protobuf-varint amount, while TRC20 transfers verify the full ABI parameter that carries both recipient and amount; both paths continue to recompute and compare the transaction ID.
Related issue
None. Required by review feedback on tinyhumansai/openhuman#5495.
API or behavior changes
TransactionSpec::Tronnow includes a requiredTronTransferverification descriptor. This is a breaking wire-contract change for callers constructing that variant and is paired with the OpenHuman host update.Validation
Commands actually run, with their outcome:
cargo fmt --all -- --checkcargo clippy --all-targets --all-features -- -D warningscargo build --all-targets --all-featurescargo test --all-featuresTests
Added rejection coverage for a substituted native amount and an altered TRC20 parameter. Updated client and module fixtures for the expanded wire contract. All 276 library tests, 7 public API tests, 9 module tests, and 18 doctests pass.
Documentation
Updated public wire-type and verification documentation in code; no separate guide is needed for this focused contract extension.
Checklist
#[allow(...)],#[ignore], or relaxed lints.envcontents in the diff or the descriptionSummary by CodeRabbit
New Features
Bug Fixes
Tests