Skip to content

refactor(canister): range-check untrusted amounts without copying them - #242

Open
mbjorkqvist wants to merge 2 commits into
mainfrom
mathias/DEFI-2959-followup-validate-amount-nits
Open

refactor(canister): range-check untrusted amounts without copying them#242
mbjorkqvist wants to merge 2 commits into
mainfrom
mathias/DEFI-2959-followup-validate-amount-nits

Conversation

@mbjorkqvist

@mbjorkqvist mbjorkqvist commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Cleans up the needless copies of untrusted Nat amounts left behind by #237. Behaviour-preserving throughout — no path that reaches the ledger changes.

  • The range check no longer copies the unbounded magnitude it is about to reject. Quantity gains a borrowing conversion, and the deposit/withdraw handlers — plus add_trading_pair's notional bounds — range-check through a reference instead. The owned conversion stays as a delegating impl, since other callers still pass owned values.
  • Where a validated value already existed, it is now the one used: the withdraw zero-check and the deposit ledger transfer no longer read the raw request field back. That also makes the two ledger functions symmetric about what they receive, so the guarantee no longer rests on statement order at the call site.

No new tests: the conversion's boundary is already covered by proptests in canister/src/order/tests.rs, and the owned impl delegating to the borrowing one means they exercise both paths.

The regression tests for #237's ordering guarantees are split out into #243, since they stand on their own and don't depend on anything here.

Copilot AI review requested due to automatic review settings July 31, 2026 11:57

Copilot AI left a comment

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.

Pull request overview

This PR hardens the canister’s handling of untrusted Nat amounts by allowing constant-time range checks to operate on borrowed values (avoiding copies of oversized magnitudes) and by ensuring only validated amounts are used for ledger calls, with new tests pinning the intended validation/authorization ordering.

Changes:

  • Add TryFrom<&Nat> for order::Quantity and refactor callers to range-check through references instead of cloning untrusted Nats.
  • Ensure deposit/withdraw use the already-validated amount for downstream logic (withdraw zero-check; deposit ledger transfer), and apply the same borrowing range-check to add_trading_pair notional bounds.
  • Add tests covering (1) range-check precedence over other request-error branches and (2) restricted-mode allowlist rejection occurring before any expensive work.

Reviewed changes

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

Show a summary per file
File Description
integration_tests/tests/tests.rs Adds restricted-mode integration coverage ensuring non-allowlisted callers are rejected via the allowlist path even with oversized amounts.
canister/src/tests.rs Adds unit tests asserting oversized amounts are rejected before unsupported-token branches (for both deposit and withdraw).
canister/src/order/mod.rs Introduces TryFrom<&Nat> for Quantity to enable constant-time range checks without copying untrusted magnitudes; retains TryFrom<Nat> for compatibility.
canister/src/lib.rs Refactors deposit/withdraw and trading-pair notional validation to use borrowed range checks and to pass validated amounts onward (including to the ledger).
canister/src/ledger/mod.rs Changes ledger::deposit to accept an already-validated amount value, preventing any unvalidated magnitude from reaching the ledger call.

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

@github-actions

github-actions Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

canbench 🏋 (dir: canister) 827e4a5 2026-07-31 12:53:52 UTC

canister/canbench_results.yml is up to date
📦 canbench_results_benchmark.csv available in artifacts

---------------------------------------------------

Summary:
  instructions:
    status:   No significant changes 👍
    counts:   [total 16 | regressed 0 | improved 0 | new 0 | unchanged 16]
    change:   [max +42.54M | p75 +1.06M | median +9.50K | p25 +31 | min -66.75K]
    change %: [max +0.19% | p75 +0.11% | median +0.03% | p25 -0.04% | min -0.39%]

  heap_increase:
    status:   No significant changes 👍
    counts:   [total 16 | regressed 0 | improved 0 | new 0 | unchanged 16]
    change:   [max 0 | p75 0 | median 0 | p25 0 | min 0]
    change %: [max 0.00% | p75 0.00% | median 0.00% | p25 0.00% | min 0.00%]

  stable_memory_increase:
    status:   No significant changes 👍
    counts:   [total 16 | regressed 0 | improved 0 | new 0 | unchanged 16]
    change:   [max 0 | p75 0 | median 0 | p25 0 | min 0]
    change %: [max 0.00% | p75 0.00% | median 0.00% | p25 0.00% | min 0.00%]

---------------------------------------------------

Only significant changes:
| status | name                               | calls |    ins |  ins Δ% | HI |  HI Δ% | SMI |  SMI Δ% |
|--------|------------------------------------|-------|--------|---------|----|--------|-----|---------|
|   -    | bench_write_events::AddTradingPair |     1 | 15.92K |  -3.39% |  0 |  0.00% |   0 |   0.00% |

ins = instructions, HI = heap_increase, SMI = stable_memory_increase, Δ% = percent change

---------------------------------------------------
CSV results saved to canbench_results.csv

mbjorkqvist and others added 2 commits July 31, 2026 12:49
`Quantity::try_from(request.amount.clone())` copied the whole unbounded
magnitude before the O(1) `bits()` check that rejects it, so the handlers
still made one full copy of unvalidated input. Add a borrowing
`TryFrom<&Nat> for Quantity` (the owned impl now delegates to it) and
range-check through the reference instead.

Also use the validated value where the raw request field was still being
read back: `amount.is_zero()` for the withdraw zero-check, and
`ledger::deposit` now takes the range-checked amount rather than the whole
request, mirroring `ledger::withdraw`. The values are equal on every path
that reaches them, so behavior is unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`add_trading_pair` cloned `min_notional` and `max_notional` only to hand them
to `Quantity::try_from`, which now accepts a reference. The error payload still
needs owned copies, so `invalid_notional` keeps its clones — but they are only
built on the failure path.

This endpoint is controller-gated, so there is no DoS exposure here; it is the
same needless copy the borrowing conversion was added to remove.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 31, 2026 12:49
@mbjorkqvist
mbjorkqvist force-pushed the mathias/DEFI-2959-followup-validate-amount-nits branch from 7862493 to 8b6fdb3 Compare July 31, 2026 12:49

Copilot AI left a comment

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.

Pull request overview

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

@mbjorkqvist
mbjorkqvist marked this pull request as ready for review July 31, 2026 13:05
@mbjorkqvist
mbjorkqvist requested a review from a team as a code owner July 31, 2026 13:05
@zeropath-ai

zeropath-ai Bot commented Jul 31, 2026

Copy link
Copy Markdown

No security or compliance issues detected. Reviewed everything up to 8b6fdb3.

Security Overview
Detected Code Changes
Change Type Relevant files
Enhancement ► canister/src/ledger/mod.rs
      Refactor deposit signature to accept token and amount directly and adjust transfers
► canister/src/lib.rs
      Update deposit call to pass token_id and amount as Nat
► canister/src/order/mod.rs
      Update TryFrom implementation for Quantity to support &Nat and Nat
► canister/src/ledger/mod.rs
      Adjust amount handling in deposit function (use direct amount)
Refactor ► canister/src/ledger/mod.rs
      Update type imports and comments around deposit function
Other ► canister/src/lib.rs
      Modify deposit-related logic to use borrowed Nat for amount conversion and adjust checks
► canister/src/order/mod.rs
      Update error and conversion paths to align with new QuantityFrom implementations

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.

2 participants