You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Follow-up from the #333 deep review. #333 closes `.to_string().parse::().unwrap_or(0)` truncation in Universal Router amount paths (six call sites in `universal_router.rs`), explicitly scoped out of `permit2.rs`. The deep review found three remaining sites with the same anti-pattern in Permit2, one of which is materially worse than the bug #333 fixed.
Sites in src/chain_parsers/visualsign-ethereum/src/protocols/uniswap/contracts/permit2.rs
decode_approve at line 125: `call.amount.to_string().parse().unwrap_or(0)` on `call.amount: uint160`.
decode_permit at lines 165 + 211: `call.permitSingle.details.amount.to_string().parse().unwrap_or(0)` on `uint160`, then a sentinel check `if amount == U160::MAX { "unlimited" } else { amount_str }` — but `amount_str` was already computed from the truncated u128, so the sentinel is bypassed (see below).
decode_transfer_from at line 323: `call.amount.to_string().parse().unwrap_or(0)` on `uint160`.
`u160::MAX = 2^160 − 1 ≈ 1.46×10^48`. `u128::MAX = 2^128 − 1 ≈ 3.4×10^38`. The exploitable gap is 2^32 ≈ 4 billion× of u128's range — comfortable for an attacker.
The "unlimited approval" sentinel check fires AFTER the truncation:
```rust
let amount_u128: u128 = call.permitSingle.details.amount.to_string().parse().unwrap_or(0);
// ... format amount_str from amount_u128 ...
let amount_str = if amount == U160::MAX { "unlimited" } else { amount_str.clone() };
```
For any `amount` strictly between `u128::MAX` and `U160::MAX - 1`:
The truncation forces `amount_u128 = 0` (overflow on parse). Displayed value: `"0"`.
The sentinel branch is bypassed (the original `amount` isn't `U160::MAX`).
A real, huge, persistent approval is granted.
Realistic attack: `amount = 2^159` grants ≈ 1.46×10^47 units of an ERC20 to the spender while the signer authorizes a `"0"` display. Permit2 approvals are persistent — custody users sign them at least as often as Universal Router swaps, and they unlock spender access for the entire allowance window.
The Uniswap V2/V3 internal "uint128 cap" does NOT mitigate this: Permit2's allowance layer operates before any pool math, and Permit2-mediated transfers to non-Uniswap contracts have no cap at all.
Use the same `format_token_amount_u256` helper introduced by #333 in `registry.rs:349-361`. The U160 type fits in U256 trivially; the existing helper handles overflow gracefully via `alloy_primitives::utils::format_units` directly on the U256, no string round-trip.
```rust
// Before
let amount_u128: u128 = call.amount.to_string().parse().unwrap_or(0);
let (amount_str, _) = format_token_amount(chain_id, call.token, amount_u128, decimals)
.unwrap_or_else(|| (call.amount.to_string(), token_symbol.clone()));
// After
let (amount_str, _) = format_token_amount_u256(chain_id, call.token, call.amount.into(), decimals)
.unwrap_or_else(|| (call.amount.to_string(), token_symbol.clone()));
```
For `decode_permit`, also reorder the sentinel check: compare `call.permitSingle.details.amount == U160::MAX` against the original U160 value, not against the post-narrowing u128:
```rust
let amount_str = if call.permitSingle.details.amount == U160::MAX {
"unlimited".to_string()
} else {
// U256-native format as above
};
```
Acceptance criteria
All three Permit2 sites use `format_token_amount_u256` (or equivalent U256-native formatting) — no `.to_string().parse::()` left in `permit2.rs`.
`decode_permit` sentinel check fires against the original U160, not the narrowed u128.
Regression test: `approve(token, spender, 2^159, expiration)` renders the actual amount, not `"0"`.
Regression test: `permit(...)` with `amount = 2^159` does not show `"unlimited"` and does not show `"0"` — shows the actual large value.
Regression test: `transferFrom(from, to, 2^159)` renders the actual amount, not `"0"`.
Regression test: `permit` with `amount = U160::MAX` still renders `"unlimited"` (sentinel preserved).
decode_pay_portion bips overflow path has no direct test in fix(ethereum): stop silently truncating Universal Router U256 amounts to zero #333. `decode_pay_portion` correctly handles `bips > u128::MAX` via `u128::try_from(bips)` with an `Err` branch rendering the raw value, but the `Err` branch is unexercised. A regression test asserting `pay_portion` with `bips = 2^200` renders `" bips (raw)"` would lock it in.
`DECODER_GUIDE.md` lines 38, 114, 229, 236 still teach the buggy `.to_string().parse::().unwrap_or(0)` pattern. Once this PR lands, update those examples to point at `format_token_amount_u256`.
Severity
High. Same class of UI-spoofing as #333's bug (signer sees `"0"` while real value is huge), with worse blast radius on `decode_permit` because the sentinel bypass produces a string that looks legitimate ("0" rather than "unlimited"). Permit2's allowance persistence and cross-contract reach make this worth tracking ahead of #333's merge or immediately after.
Follow-up from the #333 deep review. #333 closes `.to_string().parse::().unwrap_or(0)` truncation in Universal Router amount paths (six call sites in `universal_router.rs`), explicitly scoped out of `permit2.rs`. The deep review found three remaining sites with the same anti-pattern in Permit2, one of which is materially worse than the bug #333 fixed.
Sites in
src/chain_parsers/visualsign-ethereum/src/protocols/uniswap/contracts/permit2.rsdecode_approveat line 125: `call.amount.to_string().parse().unwrap_or(0)` on `call.amount: uint160`.decode_permitat lines 165 + 211: `call.permitSingle.details.amount.to_string().parse().unwrap_or(0)` on `uint160`, then a sentinel check `if amount == U160::MAX { "unlimited" } else { amount_str }` — but `amount_str` was already computed from the truncated u128, so the sentinel is bypassed (see below).decode_transfer_fromat line 323: `call.amount.to_string().parse().unwrap_or(0)` on `uint160`.`u160::MAX = 2^160 − 1 ≈ 1.46×10^48`. `u128::MAX = 2^128 − 1 ≈ 3.4×10^38`. The exploitable gap is 2^32 ≈ 4 billion× of u128's range — comfortable for an attacker.
Why
decode_permitis worse than the #333 bugThe "unlimited approval" sentinel check fires AFTER the truncation:
```rust
let amount_u128: u128 = call.permitSingle.details.amount.to_string().parse().unwrap_or(0);
// ... format amount_str from amount_u128 ...
let amount_str = if amount == U160::MAX { "unlimited" } else { amount_str.clone() };
```
For any `amount` strictly between `u128::MAX` and `U160::MAX - 1`:
Realistic attack: `amount = 2^159` grants ≈ 1.46×10^47 units of an ERC20 to the spender while the signer authorizes a `"0"` display. Permit2 approvals are persistent — custody users sign them at least as often as Universal Router swaps, and they unlock spender access for the entire allowance window.
The Uniswap V2/V3 internal "uint128 cap" does NOT mitigate this: Permit2's allowance layer operates before any pool math, and Permit2-mediated transfers to non-Uniswap contracts have no cap at all.
Fix shape (mechanical once #333 has landed)
Use the same `format_token_amount_u256` helper introduced by #333 in `registry.rs:349-361`. The U160 type fits in U256 trivially; the existing helper handles overflow gracefully via `alloy_primitives::utils::format_units` directly on the U256, no string round-trip.
```rust
// Before
let amount_u128: u128 = call.amount.to_string().parse().unwrap_or(0);
let (amount_str, _) = format_token_amount(chain_id, call.token, amount_u128, decimals)
.unwrap_or_else(|| (call.amount.to_string(), token_symbol.clone()));
// After
let (amount_str, _) = format_token_amount_u256(chain_id, call.token, call.amount.into(), decimals)
.unwrap_or_else(|| (call.amount.to_string(), token_symbol.clone()));
```
For `decode_permit`, also reorder the sentinel check: compare `call.permitSingle.details.amount == U160::MAX` against the original U160 value, not against the post-narrowing u128:
```rust
let amount_str = if call.permitSingle.details.amount == U160::MAX {
"unlimited".to_string()
} else {
// U256-native format as above
};
```
Acceptance criteria
Also worth folding in (from the #333 deep review)
decode_pay_portionbips overflow path has no direct test in fix(ethereum): stop silently truncating Universal Router U256 amounts to zero #333. `decode_pay_portion` correctly handles `bips > u128::MAX` via `u128::try_from(bips)` with an `Err` branch rendering the raw value, but the `Err` branch is unexercised. A regression test asserting `pay_portion` with `bips = 2^200` renders `" bips (raw)"` would lock it in.Severity
High. Same class of UI-spoofing as #333's bug (signer sees `"0"` while real value is huge), with worse blast radius on `decode_permit` because the sentinel bypass produces a string that looks legitimate ("0" rather than "unlimited"). Permit2's allowance persistence and cross-contract reach make this worth tracking ahead of #333's merge or immediately after.