fix(canister): validate deposit/withdraw amount before rendering it

[gregorydemay]

The deposit and withdraw update endpoints rendered the entire request to a diagnostic string before running any business logic. That string interpolates the request's amount, an attacker-controlled unbounded candid nat, formatting it to decimal — a super-linear (quadratic in limb count) operation. Because Candid decodes nat in linear time with no magnitude cap, a cheap multi-hundred-KB magnitude reached the handler and its decimal conversion alone could exhaust a single message's instruction budget, trapping the call.

Crucially this happened before caller authorization (restricted-mode allowlist), the known-token check, and the amount range-check, so it worked even against a non-allowlisted caller in restricted mode, and the rendered string was discarded on the rejection path anyway.

This validates the untrusted numeric input before doing any super-linear work on it. The request is handed to the handler by reference (no pre-auth copy of the unbounded amount) and rendered only inside the logging paths. The handler now runs the constant-time amount range-check ahead of every branch that returns a request error, so no out-of-range amount can reach code that might render it — a stronger, order-independent property than relying on which errors happen to be loggable. This mirrors how the limit-order endpoint already orders validation before formatting.

• Deposit and withdraw no longer copy or format the request before validation/authorization has had the chance to reject it.
• The amount range-check runs first, in constant time, before any request-error branch or ledger interaction, so an oversized amount is rejected as a request error rather than trapping.
• An integration test drives the real endpoints with a genuinely oversized nat: it traps on the pre-fix handler and passes here, guarding against regression.

[Copilot]

Pull request overview

This PR mitigates an instruction-exhaustion/DoS vector in the canister’s deposit/withdraw update endpoints by ensuring attacker-controlled, unbounded Nat amounts are validated before any potentially expensive request rendering/logging occurs.

Changes:

• Reorders deposit/withdraw endpoint logic to avoid formatting the request prior to calling the validated handler path.
• Adds regression tests ensuring oversized amounts are rejected before any ledger interaction (deposit/withdraw).
• Adds unit tests for the “should log?” predicates to ensure request errors (including out-of-range amounts) are not logged/rendered.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
canister/src/main.rs	Moves request formatting into post-validation logging paths (and adds should-log unit-test module).
canister/src/tests.rs	Adds endpoint tests asserting oversized amounts are rejected early (before ledger/event effects).
canister/src/should_log_tests.rs	Adds unit tests to ensure request-error branches are not logged/rendered.

Comments suppressed due to low confidence (1)

canister/src/main.rs:132

• request.clone() duplicates the attacker-controlled Nat amount before any authorization/validation in oisy_trade_canister::withdraw. Even though logging is now gated, this O(n) clone still happens for every call (including those that will be rejected) and can be avoided by moving request into the handler and logging only cheap/sanitized fields.

#[ic_cdk::update]
async fn withdraw(request: WithdrawRequest) -> Result<WithdrawResponse, WithdrawError> {
    let result =
        oisy_trade_canister::withdraw(request.clone(), &oisy_trade_canister::IC_RUNTIME).await;

---

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

[github-actions]

canbench 🏋 (dir: canister) 996d49f 2026-07-30 20:02:33 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 +38 | p75 0 | median 0 | p25 -5.60K | min -42.02K]
    change %: [max 0.00% | p75 0.00% | median 0.00% | p25 -0.07% | 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%]

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

[gregorydemay]

🧐 VERDICT: READY — 0 blockers, 0 mediums, 1 nit; CI green.

Review details

Finding (CWE-407) — closed. The unconditional format!("{request}") at the top of deposit/withdraw is gone. Display of the request now renders only inside the canlog::log! calls, which run after oisy_trade_canister::{deposit,withdraw} returns. Traced every render site (main.rs:113,120,136,143 — the only {request} interpolations in the deposit/withdraw path):

• Ok branch: reachable only after the constant-time range-check (order::Quantity::try_from, lib.rs:313/393) accepted the amount, so amount is ≤256 bits and rendering is bounded.
• Err branch: gated by should_log_{deposit,withdraw}_error, which returns false for ErrorKind::RequestError(_). AmountExceedsMaximum is a RequestError, so an out-of-range amount is never rendered. Any error surfaced after the range-check carries an already-bounded amount.

So no path renders an unvalidated magnitude. assert_caller_is_allowed → is_known_token → range-check ordering inside the handler is preserved and now precedes all payload-proportional work. The added request.clone() is O(limbs) (linear, same order as the decode), not the super-linear to_str_radix that was the DoS — acceptable.

Maintainability

• duplication: the two should_log_* tests and the two oversized-amount endpoint tests mirror each other along the deposit/withdraw axis — see nit below; not gating.
• unused derives: none (no new types).
• primitive-obsession params: none (no new parameters).
• divergent invariant handling: none — deposit and withdraw move formatting after validation identically.
• silent fallbacks: none — the should_log_* suppression is intentional (don't render attacker input) and is not an invariant-breach-masked-as-success.
• test-only code in production: none — should_log_* are productive fns called at main.rs:117,140; the new tests live in #[cfg(test)] mod should_log_tests.

Tests — unit level, correctly placed. should_log_tests.rs (new binary-crate test module, named to avoid colliding with the lib's tests.rs) is table-driven and pins the security-relevant mapping AmountExceedsMaximum → false. Endpoint tests in tests.rs assert AmountExceedsMaximum with no ledger interaction, on both deposit and withdraw. The two facets (no render / no ledger work) together demonstrate the ordering. unit-tests and integration-tests green.

[Copilot]

Pull request overview

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

[gregorydemay]

🧐 VERDICT: READY (re-review of 938b505) — 0 blockers, 0 mediums, 0 new nits; CI green, mergeable: MERGEABLE.

Re-reviewed the follow-up commit addressing Copilot's point that request.clone() did O(n) work on the unbounded amount before authorization. Verified against the actual diff:

Review details

Pre-auth payload work — eliminated. main.rs handlers now pass &request (borrow, no clone) into oisy_trade_canister::{deposit,withdraw}, which now take &DepositRequest/&WithdrawRequest. The handler keeps ownership only for the post-validation {request} log. Tracing both paths from the borrow inward:

• deposit (lib.rs:305): assert_caller_is_allowed is the first statement; the only O(n) op on the amount is request.amount.clone() feeding Quantity::try_from at lib.rs:313, which is after auth + is_known_token, is linear (not the quadratic to_str_radix), and is immediately range-checked.
• withdraw (lib.rs:374): same — auth first; the request.amount == 0u64 compare and the request.amount.clone() into Quantity::try_from (lib.rs:393) all run post-auth.
  So no payload-proportional work (clone or format) precedes authorization or the constant-time range-check on either path.

ledger::withdraw amount change — no behavioral change. It now receives amount.to_nat() derived from the already-range-checked order::Quantity instead of moving request.amount. Quantity::to_nat (order/mod.rs:474) reconstructs the exact value from the ≤2^256 representation, so for any amount that passed try_from it equals the original request.amount. Semantically identical.

No hidden clone / weakening. ledger::deposit now borrows the request and clones the amount only for the TransferFromArgs (ledger/mod.rs:46), which runs after the range-check gate in lib.rs — so it operates on a bounded (≤2^256) value. No other clone of unvalidated input remains. Format of the request is still confined to the post-validation log paths (unchanged), still gated so AmountExceedsMaximum (a RequestError) is never rendered.

Tests. All call sites updated to pass &; the oversized-amount ordering tests and the should_log_* request-error tests are intact. unit-tests + integration-tests green, so the borrow refactor compiles and the ordering assertions still hold. benchmark green (no canbench delta). Nit from the prior review (deposit/withdraw test mirroring across distinct error enums) stands as-is, still non-gating.

[gregorydemay]

🤖 This PR is ready for your review.

The automated implement → review loop is complete: the finding is closed (the two update handlers no longer render the unbounded, attacker-controlled amount to decimal before validation/authorization — and after addressing review feedback they no longer do any payload-proportional work, not even an O(n) copy, before the caller check and constant-time range check), all CI checks are green, review threads are resolved, and the PR is MERGEABLE.

Left as a draft — final approval, marking ready-for-review, and merge are yours.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

canister/src/lib.rs:375

• withdraw clones the unbounded Nat (request.amount.clone()) before determining whether it exceeds the 256-bit Quantity maximum. An oversized value will be rejected, but only after paying an O(n) clone cost. You can avoid this by checking request.amount.0.bits() first, and only cloning when it’s within bounds.

    request: &WithdrawRequest,
    runtime: &impl Runtime,
) -> Result<WithdrawResponse, WithdrawError> {
    state::with_state(|s| s.assert_caller_is_allowed(runtime));
    let token_id = request.token_id.clone();

canister/src/lib.rs:304

• deposit still clones the attacker-controlled, unbounded Nat (request.amount.clone()) before the overflow check. For an oversized Candid nat, this performs an O(n) copy even though the request will be rejected, which undermines the goal of doing amount validation before any potentially expensive work. Consider checking the bit-length on the borrowed Nat first and only cloning once it’s known to be <= 256 bits.

pub async fn deposit(
    request: &DepositRequest,
    runtime: &impl Runtime,
) -> Result<DepositResponse, DepositError> {

[gregorydemay]

🧐 VERDICT: READY (re-review of 2c8699f) — 0 blockers, 0 mediums, 0 nits; CI green, mergeable: MERGEABLE.

Confirmed on the actual diff:

• (a) Only canister/src/should_log_tests.rs was deleted and its #[cfg(test)] mod should_log_tests; wiring removed from canister/src/main.rs. No other change.
• (b) The borrow-based security fix and the meaningful oversized-amount endpoint tests in canister/src/tests.rs are untouched.
• (c) The productive predicates should_log_deposit_error / should_log_withdraw_error remain (main.rs:150/164) and are still called by the handlers (main.rs:116/138).

The removed module only re-encoded the predicates' own branch logic against hard-coded expectations — a tautological/parallel-oracle test that the endpoint tests already subsume for the security-relevant ordering. Dropping it is the right call. My prior nit (deposit/withdraw test mirroring) is now moot. Verdict stays READY.

[gregorydemay]

🤖 Ready for your review again after the latest round.

Addressed the review feedback: removed the tautological should_log_tests.rs (the meaningful coverage — oversized amount rejected with no ledger interaction — remains in the endpoint tests). Reviewer re-confirmed the fix and tests are intact, CI is green on 2c8699f, all review threads are resolved, and the PR is MERGEABLE.

Still a draft — approval and merge are yours.

[zeropath-ai]

✅ No security or compliance issues detected. Reviewed everything up to 094ac12.

Security Overview

• 🔎 Scanned files: 7 changed file(s)
• 🔗 Scan Link: https://zeropath.com/app/repositories/a2241de7-cf71-4864-8711-25a8fbfe66c5?scanId=cd133bb3-c0bd-4dd2-8028-88a3cf339630&codeScanTypes=PrScan&tab=issues

Detected Code Changes

Change Type	Relevant files
Enhancement	► canister/src/ledger/mod.rs       Deposit request now taken by reference and token/amount handling adjusted to use references
Enhancement	► canister/src/lib.rs       Deposit and withdraw handlers updated to accept references and perform amount validation before ledger calls; amount conversion logic adjusted; ledger call uses amount.to_nat() where applicable
Enhancement	► canister/src/main.rs       Update and log messages adjusted to reference the request object instead of a formatted request string
Enhancement	► canister/src/tests.rs       Tests updated to pass references to deposit/withdraw calls; several test cases adjusted to new function signatures and expectations
Enhancement	► integration_tests/Cargo.toml       Add num-bigint as dev dependency for integration tests
Enhancement	► integration_tests/tests/tests.rs       Update imports and test references to align with new request types and response types; added oversized amount test case in integration tests
Enhancement	► integration_tests/tests/tests.rs       New test: oversized_amount_is_rejected_without_trapping to verify maximum amount handling for deposit and withdraw without ledger interaction; includes candid encoding/decoding and assertions
Enhancement	► integration_tests/tests/tests.rs       Additional test scaffolding around DepositRequest/WithdrawRequest and related errors
Enhancement	► integration_tests/tests/tests.rs       Adjust tests for Withdraw and Deposit error type paths (WithdrawRequestError, DepositRequestError)
Enhancement	► Cargo.lock updated with new dependency "num-bigint"
Enhancement	► integration_tests/tests/tests.rs (multiple occurrences)       Replace direct struct initializations with reference-compatible forms in withdraw/deposit tests

[mbjorkqvist]

Thanks @gregorydemay!

In the PR description, "covered by new logging-predicate tests" is now inaccurate (deleted in 2c8699f).

[gregorydemay]

🤖 Fixed the PR description — dropped the stale "covered by new logging-predicate tests" line (those were removed in 2c8699f) and replaced the coverage bullets to reflect the range-check reorder (1f90d23) and the endpoint integration guard (daedfff).

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

canister/src/lib.rs:377

• Same issue as deposit: Quantity::try_from(request.amount.clone()) performs an O(n) clone of an unbounded Nat before the overflow check. With very large inputs this can still burn significant instructions/memory even though the overflow test itself could be O(1).

Prefer a by-reference conversion (TryFrom<&Nat> / try_from_nat_ref) so oversized amounts are rejected without cloning the full bigint.

    state::with_state(|s| s.assert_caller_is_allowed(runtime));
    let amount = order::Quantity::try_from(request.amount.clone()).map_err(|_| {
        WithdrawError::request(oisy_trade_types::WithdrawRequestError::AmountExceedsMaximum)
    })?;

canister/src/lib.rs:308

• Quantity::try_from(request.amount.clone()) clones the attacker-controlled Nat before the overflow check. For oversized values this is an O(n) copy of unbounded input, which undermines the goal of making the “amount exceeds maximum” rejection essentially constant-time and can still waste instructions/memory on rejected calls.

Consider adding a TryFrom<&Nat> for Quantity (or a Quantity::try_from_nat_ref(&Nat)) that checks bits() first and only allocates for in-range values, then use it here to avoid cloning oversized amounts.

This issue also appears on line 374 of the same file.

    state::with_state(|s| s.assert_caller_is_allowed(runtime));
    let amount = order::Quantity::try_from(request.amount.clone()).map_err(|_| {
        DepositError::request(oisy_trade_types::DepositRequestError::AmountExceedsMaximum)
    })?;

[gregorydemay]

🧐 VERDICT: CHANGES_REQUESTED — 0 blockers, 1 medium, 0 nits; CI green.

Re-review of the two follow-up commits only (1f90d23, daedfff). Both of mbjorkqvist's points are genuinely closed; one test-duplication finding gates the verdict.

Review details

mbjorkqvist point 1 — robustness reorder: CLOSED. In both deposit and withdraw the order::Quantity::try_from(request.amount) range check now runs first, immediately after assert_caller_is_allowed (which takes only runtime and never renders the request), ahead of is_known_token and every other request-error branch. BigUint::bits() > 256 short-circuits in O(1), so an oversized amount is rejected before any code path that could render {request}. The fix no longer depends on should_log_*_error returning false for RequestError(_) — it is now order-independent. Single-fault paths keep their variants (unknown-token → UnsupportedToken, zero → AmountTooSmall, over-balance → InsufficientBalance); the oversized-AND-unknown-token case now returns AmountExceedsMaximum, which is inherent to the reorder and documented in the PR body. Authorization still precedes the range check and does not render the amount, so ordering is sane.

mbjorkqvist point 2 — real regression guard: CLOSED. should_reject_oversized_{deposit,withdraw}_amount_without_trapping drive the real bin handler via env().update_call (not the unwrapping client helper — the previously-uncovered path). The .expect(...) on the call result is the trap-vs-reject assertion, followed by RequestError(Some(AmountExceedsMaximum)). OVERSIZED_LIMBS = 200_000 u32 limbs (~6.4M bits / ~1.9M decimal digits) is large enough to trip the pre-fix quadratic to_str_radix. CI integration-tests is green, confirming it passes post-fix (implementer reported it traps pre-fix).

No regression: post-validation-only formatting and by-reference handlers intact; no .did change.

Maintainability rundown:

• duplication: FOUND (🟠) — the two new integration tests are one-axis near-duplicates (~24 near-identical lines each); extract one generic helper (see inline comment).
• structural duplication vs codebase: none — no new type mirrors an existing sibling.
• unused derives: none — no new types.
• primitive-obsession params: none — amount is the typed order::Quantity; OVERSIZED_LIMBS is a local test const.
• divergent invariant handling: none — the range check is now applied identically (first) in both deposit and withdraw.
• silent fallbacks: none — every failure path returns a typed RequestError variant; no unwrap_or_default/NaN/discarded Result.
• test-only code in productive module: none — oversized_amount()/OVERSIZED_LIMBS live in the test file.
• redundant/derivable param: none.

Test pyramid: single-fault variants are covered by the existing unit tests in canister/src/tests.rs; the new integration tests are justified because the DoS is only observable through a raw update call (the client helper unwraps the trap away). num-bigint added as a workspace dev-dep; one-line Cargo.lock delta — sound.

CI: all checks pass (unit-tests, integration-tests, benchmark, reproducibility-verify, reproducible-build, candid-backward-compat, lint). mergeable: MERGEABLE (BLOCKED reflects the draft/approval gate, not a conflict).

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

canister/src/lib.rs:377

• Same as deposit: order::Quantity::try_from(request.amount.clone()) clones an unbounded, attacker-controlled Nat before rejecting values > 256 bits. Pre-check request.amount.0.bits() and return the request error before any clone/allocation for oversized values.

    state::with_state(|s| s.assert_caller_is_allowed(runtime));
    let amount = order::Quantity::try_from(request.amount.clone()).map_err(|_| {
        WithdrawError::request(oisy_trade_types::WithdrawRequestError::AmountExceedsMaximum)
    })?;

canister/src/lib.rs:308

• order::Quantity::try_from(request.amount.clone()) clones the attacker-controlled Nat before the overflow check inside Quantity::try_from runs. For oversized Nats this still does an O(n) copy/allocation in the canister, which can consume significant instructions/memory even though the amount is immediately rejected. Consider checking bits() on the borrowed value first and only cloning when the value is within the 256-bit bound.

This issue also appears on line 374 of the same file.

    state::with_state(|s| s.assert_caller_is_allowed(runtime));
    let amount = order::Quantity::try_from(request.amount.clone()).map_err(|_| {
        DepositError::request(oisy_trade_types::DepositRequestError::AmountExceedsMaximum)
    })?;

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

canister/src/lib.rs:377

• Same as deposit: Quantity::try_from(request.amount.clone()) clones the full untrusted Nat before Quantity::try_from can reject it via its bits() > 256 check. Adding a fast-path request.amount.0.bits() guard avoids the O(n) clone for oversized inputs and makes the rejection cost effectively constant for huge magnitudes.

    state::with_state(|s| s.assert_caller_is_allowed(runtime));
    let amount = order::Quantity::try_from(request.amount.clone()).map_err(|_| {
        WithdrawError::request(oisy_trade_types::WithdrawRequestError::AmountExceedsMaximum)
    })?;

canister/src/lib.rs:308

• Quantity::try_from(request.amount.clone()) clones the attacker-controlled Nat before the overflow check inside Quantity::try_from runs. For very large Nat values this adds an avoidable O(n) copy (even though Quantity::try_from would immediately reject based on bits() > 256). You can short-circuit on request.amount.0.bits() first (O(1)) and only clone/convert when the value is within range.

This issue also appears on line 374 of the same file.

    state::with_state(|s| s.assert_caller_is_allowed(runtime));
    let amount = order::Quantity::try_from(request.amount.clone()).map_err(|_| {
        DepositError::request(oisy_trade_types::DepositRequestError::AmountExceedsMaximum)
    })?;

[mbjorkqvist]

Thanks @gregorydemay! I'll go ahead and (approve and) merge this PR, and create a follow-up PR that proposes to address a few nits.

The description's headline scenario - "worked even against a non-allowlisted caller in restricted mode" - is only covered indirectly. Post-fix that caller still traps, via the panic! in assert_caller_is_allowed (state/mod.rs:167). The fix removes the real harm (a cheap panic instead of burning a full message's instruction budget), but no test distinguished the two at the time. #243 adds one: a restricted-mode fixture via SetupBuilder::with_init_arg plus Mode::restricted_to(...), asserting on the reject message to separate the authorization panic from an instruction-limit trap.