token transfer: use to_token_unit_amount to avoid silent truncation / u128 overflow in amount scaling
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/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.
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.
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