Skip to content

fix(ethereum): stop silently truncating Universal Router U256 amounts to zero#333

Merged
prasanna-anchorage merged 3 commits into
mainfrom
pepefigueira/prs-234-universal-router-u256-to-u128-parse-silently-truncates-large
Jun 1, 2026
Merged

fix(ethereum): stop silently truncating Universal Router U256 amounts to zero#333
prasanna-anchorage merged 3 commits into
mainfrom
pepefigueira/prs-234-universal-router-u256-to-u128-parse-silently-truncates-large

Conversation

@pepe-anchor
Copy link
Copy Markdown
Contributor

@pepe-anchor pepe-anchor commented May 25, 2026

Summary

The Universal Router visualizer was running every on-chain uint256 amount through value.to_string().parse::<u128>().unwrap_or(0) before handing it to format_units. Any value above u128::MAX was therefore rendered as 0 in the signing payload, hiding the real amount the user was about to authorise. A malicious frontend can craft an amountMinimum above 2^128 to make a SWEEP, WRAP_ETH, or V2/V3 swap look like "Sweep >=0 WETH ..." while actually emptying the account.

What changed

  • Added ContractRegistry::format_token_amount_u256, the U256-native counterpart of format_token_amount, which calls alloy_primitives::utils::format_units directly on the U256.
  • Replaced every u128 = value.to_string().parse().unwrap_or(0) site in universal_router.rs (decode_sweep, decode_wrap_eth, decode_unwrap_weth, V2/V3 exact-in and exact-out swaps, decode_pay_portion) with the new U256 path.
  • Replaced the bips unwrap_or(0) with u128::try_from. In range it formats as a percentage like today, out of range it now renders as <raw> bips (raw) instead of "0%". Bips realistically fit in u128, but the on-chain type is uint256, so out-of-range values must surface, not silently zero.
  • Regression tests:
    • registry::tests::test_format_token_amount_u256_above_u128_max_is_not_zero (u128::MAX + 1)
    • registry::tests::test_format_token_amount_u256_max_is_not_zero (U256::MAX)
    • registry::tests::test_format_token_amount_u256_matches_u128_in_range (parity with the u128 path)
    • universal_router::tests::test_decode_sweep_amount_above_u128_max_is_not_zero
    • universal_router::tests::test_decode_wrap_eth_amount_above_u128_max_is_not_zero

Scope is Universal Router only, matching the ticket. permit2.rs and the DECODER_GUIDE.md still teach/use the old unwrap_or(0) pattern and should be cleaned up in follow-ups.

Rollback

Revert this commit. No data or schema changes, no protobuf regeneration, no migrations. The new format_token_amount_u256 method is additive and would simply become unused; the call sites in universal_router.rs revert to their previous string-parse-truncate behaviour.

Test plan

  • cargo fmt --check -p visualsign-ethereum clean
  • cargo clippy -p visualsign-ethereum --all-targets -- -D warnings clean
  • cargo test -p visualsign-ethereum (204 unit + 4 lib + 3 doc tests pass, including the 5 new U256 truncation regressions)
  • Manual: parse a Universal Router calldata that uses an amountMinimum > u128::MAX and confirm the rendered field shows the real amount, not "0".

@pepe-anchor pepe-anchor marked this pull request as ready for review May 25, 2026 17:00
Copilot AI review requested due to automatic review settings May 25, 2026 17:00
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a security-relevant rendering bug in the Ethereum Universal Router visualizer where uint256 token amounts were previously coerced through a u128 parse with unwrap_or(0), causing any value above u128::MAX to display as zero and potentially misleading users about the true authorized amount.

Changes:

  • Added ContractRegistry::format_token_amount_u256 to format token amounts directly from U256 using format_units.
  • Updated Universal Router decoding paths to format amounts via the new U256-aware formatter (and adjusted bips formatting to avoid silent zeroing).
  • Added regression tests covering amounts above u128::MAX (including U256::MAX) and parity with the existing u128 path for in-range values.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/chain_parsers/visualsign-ethereum/src/registry.rs Adds a U256-native token amount formatter and regression tests to ensure large values never render as zero.
src/chain_parsers/visualsign-ethereum/src/protocols/uniswap/contracts/universal_router.rs Switches Universal Router amount formatting to U256-safe logic, hardens bips display, and adds decode regressions for large SWEEP/WRAP_ETH values.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@prasanna-anchorage
Copy link
Copy Markdown
Contributor

Heads up: this repo is public, so internal Linear refs (PRS-xxx) shouldn't leak into PR-visible surface — external readers can't resolve them.

This PR's leaks:

  • Body links linear.app/anchorlabs/issue/PRS-234 — swap for a one-line bug-class summary or a public GH issue reference.
  • 13 code references / 13 inline comments mention PRS-234 — rename to describe the bug class.
  • Branch pepefigueira/prs-234-... also exposes the ID (optional).

Suggested convention: describe the bug in the name (e.g., test_u256_amount_over_u128_max_not_truncated). For persistent traceability, file a public GitHub issue.

@pepe-anchor
Copy link
Copy Markdown
Contributor Author

Scrubbed PRS-234 refs from body and code/comments. Branch left as-is since it'll disappear on merge.

@shahan-khatchadourian-anchorage
Copy link
Copy Markdown
Contributor

Tracking the pay-portion bips domain-validation as a follow-up in #352 (the (raw) branch only guards > u128::MAX; bips > 10_000 still renders a bogus percentage, and the anomaly should surface as a structured diagnostic rather than display text). Out of scope for this PR — the (raw) boundary here is correct for the U256-truncation fix this PR targets.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 👍

pepe-anchor and others added 2 commits June 1, 2026 22:24
… to zero

The Universal Router visualizer was rendering oversized token amounts as "0"
because every U256 was being run through `value.to_string().parse::<u128>().unwrap_or(0)`
before formatting. Any amount above `u128::MAX` therefore showed up as 0 in the
signing UI, including SWEEP/WRAP_ETH/V2/V3 swap minimums and bips, which is the
exact kind of value a malicious frontend can craft to hide a large outbound transfer.

Add `ContractRegistry::format_token_amount_u256` that calls `format_units` directly
on the U256, switch every Universal Router decoder to it, and convert the bips path
to `u128::try_from` so out-of-range bips render as "<raw> bips (raw)" instead of "0%".

Regression tests cover U256::MAX, u128::MAX + 1, and parity with the u128 path in
range, plus end-to-end coverage on `decode_sweep` and `decode_wrap_eth`.

Closes PRS-234

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Removes PRS-NNN references from comments so external readers of this
public repo don't see internal triage IDs.
@prasanna-anchorage prasanna-anchorage force-pushed the pepefigueira/prs-234-universal-router-u256-to-u128-parse-silently-truncates-large branch from 8c1139c to 98840e5 Compare June 1, 2026 22:28
Locks in the oversized-bips fallback added by this PR: a uint256 bips
value above u128::MAX must surface as a raw figure rather than silently
rendering as a near-zero percentage.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@prasanna-anchorage prasanna-anchorage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving after rebasing onto main and adding the missing bips-overflow test.

Rebase: resolved the conflicts in universal_router.rs — both sides were purely additive (main's V3-path tests from #330 + this PR's PRS-234 U256 regression tests), so I kept both sets; the second commit's PRS-NNN comment scrub applied cleanly on top. registry.rs auto-merged.

Follow-up addressed: added test_decode_pay_portion_bips_above_u128_max_is_not_zero, which locks in the oversized-bips fallback this PR introduces (u128::try_from(params.bips) -> "{} bips (raw)"). It asserts the rendered percentage isn't a near-zero figure and surfaces the raw value; it fails on the old to_string().parse::<u128>().unwrap_or(0) path.

Verified locally: 235 + 9 crate tests pass (incl. the merged-in V3-path tests and all PRS-234 U256 tests), cargo clippy --all-targets -- -D warnings clean, cargo fmt --check clean.

Out-of-scope follow-up filed: the same truncation pattern lives in permit2.rs (amount + expiration) — tracked in #353, intentionally not expanded into this PR.

LGTM pending green CI.

@prasanna-anchorage prasanna-anchorage merged commit 5d3b717 into main Jun 1, 2026
9 checks passed
@prasanna-anchorage prasanna-anchorage deleted the pepefigueira/prs-234-universal-router-u256-to-u128-parse-silently-truncates-large branch June 1, 2026 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants