Skip to content

[medium] Harden all caller-supplied input: unbounded collections and panicking index access #260

Description

@Cybermaxi7

🚨 ALL CI CHECKS MUST PASS

Your PR will not be reviewed or merged until every CI job is green. No exceptions.

Run all four locally before you push:

cargo fmt --all -- --check                   # Formatting
cargo clippy --all-targets -- -D warnings    # Clippy — warnings are errors
cargo test                                   # Tests
./scripts/build_wasm.sh                      # Build Optimized WASM

A red build is the single most common reason work stalls on this repo. If CI fails and you are stuck, say so in the PR — do not push a failing build and go quiet.

Also required: put Closes #<this issue number> in your PR description. Without it, GrantFox cannot link your PR to this issue.


What needs to be done

Two related classes of unvalidated input. First, functions that accept caller-controlled collections with no size limit. Second, index-based lookups that call .unwrap() on a get(), panicking instead of returning a typed error when the index is out of range.

Why it matters

Uncapped inputs let a caller push the contract past its resource budget, turning a normal call into a guaranteed failure and wasting fees. Panicking on a bad index gives callers an unrecoverable trap instead of a catchable error — clients cannot distinguish "you passed a bad index" from a genuine contract fault, and the panic surfaces as an opaque failure on-chain. The codebase already establishes the right pattern in both cases; these are the spots that were missed.

Technical context

Uncapped collections

  • batch_collect_fees(escrow_ids: Vec<u64>)lib.rs:3656
  • get_escrows(limit: u32)lib.rs:805

Panicking index access on caller-supplied indices

  • lib.rs:1518item_index
  • lib.rs:3826milestone_index
  • lib.rs:4158 and lib.rs:4248 — group-buy index

Remaining unwrap()/expect() sites to audit and convert where reachable

  • lib.rs:213, 303, 784, 3365, 3786, 4087, 4402

Precedent to follow

  • MAX_ITEMS_PER_ESCROW (types.rs:145) paired with the TooManyItems error
  • ItemNotFound and MilestoneNotFound already exist for the index cases

Acceptance criteria

  • A MAX_ESCROWS_PER_BATCH constant exists in types.rs and batch_collect_fees rejects oversized input with a typed error
  • get_escrows enforces a maximum page size
  • Every caller-supplied index returns a typed error instead of panicking
  • Each converted site has a test passing an out-of-range index and asserting the specific error
  • Each new cap has a test at the limit and one over it
  • Any remaining unwrap()/expect() carries a comment explaining why it is unreachable
  • All four CI jobs pass

Out of scope

  • Changing pagination semantics of get_escrows beyond adding the cap
  • The fee calculation logic — that is covered by the batch_collect_fees issue

Getting started

cargo test    # 120 tests currently pass
grep -n "unwrap()\|expect(" contracts/marketx/src/lib.rs

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

GrantFox OSSIssue tracked in GrantFox OSSMaybe RewardedIssue may be eligible for a GrantFox rewardThird CampaignCampaign: Third CampaignbugSomething isn't working

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions