token transfer: use to_token_unit_amount to avoid silent truncation / u128 overflow in amount scaling
Nobody has claimed this yet.
Assessment
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Newbie friendliness
- 74/100
Research direction
Start in crates/icp-cli/src/operations/token/transfer.rs and compare transfer() with to_token_unit_amount in crates/icp/src/parsers.rs, including its use in parse_cycles_str. Then find the existing integration coverage for token transfer and extend it for an over-precise amount. Done means exact representability is enforced, the error is clear, and conversion has no u128 overflow path.
Written by the indexing model from the issue text.
Description
Summary
icp token transfer converts the user-supplied decimal amount into the ledger's smallest unit using a manual 10^decimals multiplication followed by to_bigint(), which can silently truncate non-representable amounts and can overflow u128 for ledgers reporting large decimals.
This was identified during review of #637 (which adds token approve/allowance). The new approve command was fixed to use the validated helper icp::parsers::to_token_unit_amount; transfer was intentionally left untouched to keep that PR focused. This issue tracks bringing transfer in line.
Note: #637 has not been merged and it is not yet decided whether the new
approve/allowancecommands will land. This issue concerns the pre-existingtransfercode path regardless of that outcome — the helperto_token_unit_amountalready exists incrates/icp/src/parsers.rsand is used elsewhere (e.g.parse_cycles_str).
Location
crates/icp-cli/src/operations/token/transfer.rs (in transfer()), currently:
let ledger_amount_decimal = amount.clone() * 10u128.pow(decimals);
let ledger_amount = ledger_amount_decimal
.to_bigint()
.ok_or(TokenTransferError::InvalidAmount)?
.to_biguint()
.ok_or(TokenTransferError::InvalidAmount)
.map(Nat::from)?;
Problems
- Silent truncation. If the amount has more fractional digits than the token's
icrc1_decimals(e.g.0.000000001for an 8-decimal token),to_bigint()truncates toward zero — the user transfers a different amount than they typed, with no error. - Overflow.
10u128.pow(decimals)overflowsu128fordecimals >= 39(panics in debug, wraps in release), producing an incorrect amount for exotic/malicious ledgers.
Suggested fix
Replace the manual scaling with the existing helper, which uses arbitrary-precision math and rejects non-representable amounts:
let ledger_amount = to_token_unit_amount(amount.clone(), decimals)
.map(Nat::from)
.map_err(|message| TokenTransferError::InvalidAmount { message })?;
This likely means changing TokenTransferError::InvalidAmount from a unit variant to InvalidAmount { message: String } so the precision error surfaces to the user (mirroring what #637 did for TokenApproveError).
Acceptance criteria
icp token transferrejects amounts that are not exactly representable at the token's decimals, with a clear error.- No
u128overflow path in amount conversion. - Add/extend integration coverage for the rejection case (an over-precise amount fails cleanly).
- Consider auditing other call sites for the same
10^decimalspattern while here.
- Dominant language
- Rust
- Stars
- 115
- Forks
- 12
- Avg merge
- 2d 19h
- Merged PRs (30d)
- 36
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from dfinity/icp-cli
-
Difficulty 3/5 1-2 days Newbie friendliness 68/100
-
sync plugin OOMs ("memory allocation of N bytes failed") when the target canister already has assets Open
Difficulty 4/5 3-5 days Newbie friendliness 45/100
-
question
Difficulty 5/5 Over a week Newbie friendliness 35/100
-
Difficulty 3/5 1-2 days Newbie friendliness 55/100
Similar issues
-
Difficulty 2/5 1-3 hours Newbie friendliness 84/100
Eynzof/Hermes-CN-Desktop#610 ·
-
bug
Difficulty 2/5 1-3 hours Newbie friendliness 68/100
gitbutlerapp/gitbutler#15998 · 1 comment ·
-
bug triage:deciding
Difficulty 1/5 Under an hour Newbie friendliness 88/100
open-telemetry/otel-arrow#4132 ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 88/100
-
Difficulty 2/5 1-3 hours Newbie friendliness 84/100