dfinity / dfinity/icp-cli

token transfer: use to_token_unit_amount to avoid silent truncation / u128 overflow in amount scaling

Open
#638 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Rust
Stars
115
Forks
12
Avg merge
2d 19h
Merged PRs (30d)
36

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/allowance commands will land. This issue concerns the pre-existing transfer code path regardless of that outcome — the helper to_token_unit_amount already exists in crates/icp/src/parsers.rs and 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

  1. Silent truncation. If the amount has more fractional digits than the token's icrc1_decimals (e.g. 0.000000001 for an 8-decimal token), to_bigint() truncates toward zero — the user transfers a different amount than they typed, with no error.
  2. Overflow. 10u128.pow(decimals) overflows u128 for decimals >= 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 transfer rejects amounts that are not exactly representable at the token's decimals, with a clear error.
  • No u128 overflow 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^decimals pattern while here.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

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.

Assessment

Tech stack
rust
Domain
cli
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
74/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.