Skip to content

Bounty/escrow/payment amount fields accept unbounded string input from DTOs with no upper bound, min precision, or asset-currency cross-check #29

Description

@chonilius

Background

CreateBountyDto, FundEscrowInput, CreateMilestoneDto,
CreatePoolDto (under src/bounties/dto, src/escrow/dto,
src/milestones/dto, src/maintenance-pool/dto) accept amount as a
string. Read each DTO's class-validator decorators to confirm exactly
what's enforced today (likely just basic string/non-empty checks — confirm
rather than assume).

Problem Statement

Investigate and close the following input-validation gaps around monetary
amounts, all of which have real consequences given the arithmetic in
EscrowService:

  • Is there any upper bound on amount? A sponsor (or a compromised sponsor
    account) submitting an absurdly large amount string would pass through
    Number(input.amount) in EscrowService.toStroops — at this scale,
    Number silently loses precision or overflows Number.MAX_SAFE_INTEGER,
    and the resulting BigInt conversion could produce a wildly incorrect
    stroop amount passed to the Soroban contract call, either erroring
    unpredictably deep inside the SDK or (worse) submitting a transaction with
    a nonsensical amount.
  • Is there any validation that amount is a syntactically valid decimal
    numeric string at all (reject non-numeric strings, scientific notation,
    negative numbers, comma-separated numbers, and amounts with more than
    Stellar's 7-decimal precision) before it reaches any arithmetic?
    Number of a non-numeric string is NaN, and NaN propagating through
    toStroops/roundAmount produces further NaNs that may or may not be
    caught before reaching a BigInt conversion of NaN (which throws — confirm
    whether that throw is handled gracefully or crashes the request with a
    500 and an unhelpful message).
  • Is a negative amount rejected anywhere before it reaches the escrow layer?
    assertValidSplits checks that split percentages are positive, but does
    the base fund amount get an equivalent positivity check in
    CreateBountyDto/FundEscrowInput?
  • Does the asset field get cross-validated against the actual configured
    USDC asset code/issuer (or native XLM) before a fund call, or could an
    unsupported/misconfigured asset silently reach SorobanClientService?

Requirements

  • Add a robust, precisely-specified custom class-validator decorator
    enforcing: syntactically valid non-negative decimal string, at most 7
    fractional digits, within a documented sane maximum (justify the number —
    e.g. bounded by realistic total XLM/USDC supply considerations or a
    simple configurable platform maximum), applied to every DTO/interface
    accepting a monetary amount across bounties, escrow, milestones, and
    maintenance-pool modules.
  • Ensure any amount failing validation is rejected with a clear 400 at the
    DTO layer, never reaching Number/BigInt arithmetic inside
    EscrowService/SorobanClientService.
  • Add asset validation: reject fund/deposit requests specifying an asset
    type that doesn't correspond to a currently configured asset.
  • Tests: absurdly large amounts, NaN-producing strings, negative amounts,
    over-precision amounts (8+ decimals), and unsupported assets are all
    rejected at the API boundary with clear errors, never reaching the
    Soroban client.

Acceptance Criteria

  • A reusable amount-validation decorator/utility is applied consistently
    across all DTOs accepting monetary amounts.
  • No malformed amount string can reach Number/BigInt arithmetic
    in EscrowService/SorobanClientService — proven by tests feeding
    adversarial strings directly to the HTTP layer (not just unit-testing
    the validator in isolation).
  • Asset/currency validation rejects unsupported assets before any escrow
    call.

Technical Notes

Files: src/bounties/dto/create-bounty.dto.ts, src/escrow/dto,
src/milestones/dto/create-milestone.dto.ts,
src/maintenance-pool/dto/create-pool.dto.ts, src/escrow/escrow.service.ts,
src/common/enums (AssetType).

Difficulty Justification

Requires writing a correct custom class-validator decorator with real
numeric-string edge-case handling (not just a regex that looks plausible),
tracing every DTO in the codebase that accepts a monetary value to apply it
consistently, and reasoning about the actual precision/overflow behavior of
mixed Number/BigInt arithmetic under adversarial input.

Metadata

Metadata

Labels

GrantFox OSSIssue tracked in GrantFox OSSMaybe RewardedIssue may be eligible for a GrantFox rewardOfficial Campaign | FWC26Campaign: Official Campaign | FWC26bugSomething isn't workingsecuritySecurity-related issuetestingTesting/QA infrastructurevery hardVery difficult task, expert-level effort required

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions