Skip to content

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

Description

@marc0olo

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions