Skip to content

Percentage-split validation for teams and split-releases can be gamed with more than 100 recipients or negative/NaN percentages bypassing sum checks #15

Description

@chonilius

Background

src/teams/team-split.util.ts's validateSplitPercentages and
EscrowService.assertValidSplits both validate that a list of percentages
sums to (approximately) 100 within a small epsilon, and assertValidSplits
additionally rejects percentages that are zero or negative. Read
team-split.util.ts in full as part of this issue — confirm whether it
duplicates or diverges from assertValidSplits's rules (two independent
implementations of "must sum to 100" is itself a maintainability/consistency
risk worth flagging in the PR even if not the primary fix).

Problem Statement

Investigate and fix the following edge cases against both validators:

  • NaN/Infinity inputs: does class-validator's DTO validation (see
    src/teams/dto) actually reject non-finite numbers before they reach
    validateSplitPercentages, or can a crafted JSON body with an
    extremely large exponent (parsing to Infinity), or a value that survives
    JSON parsing as effectively NaN, slip through? A sum involving Infinity
    or NaN may or may not trip the epsilon check depending on JS's NaN
    comparison semantics (any comparison with NaN is false, so a
    NaN-containing sum's epsilon check would actually evaluate false —
    meaning a NaN percentage could pass validation silently!). Confirm this
    experimentally and fix.
  • A huge number of tiny recipients (e.g. 10,000 recipients at 0.01% each) is
    arithmetically valid but will blow up EscrowService.splitRelease's
    per-recipient DB insert loop and the corresponding soroban.invoke call's
    argument array size — is there any upper bound on recipient count? There
    should be, both for abuse-prevention and because Soroban transactions have
    real resource-fee limits on argument/footprint size.
  • Duplicate recipientAddress/userId entries across splits — are they
    rejected, or would the same person silently receive two payments from one
    release?

Requirements

  • Fix the NaN sum-bypass bug in both assertValidSplits and
    validateSplitPercentages (reject any non-finite percentage explicitly,
    don't rely on the sum-epsilon check to catch it).
  • Add an explicit, justified upper bound on recipient count for both team
    creation and split-release (research realistic Soroban transaction
    resource limits to justify the number, don't just pick one arbitrarily).
  • Reject duplicate recipients within a single team/split.
  • Consolidate or clearly cross-reference the two independent validation
    implementations so they can't drift out of sync in the future (a shared
    utility, or at minimum a shared test suite run against both).
  • Add tests for: NaN/Infinity percentages, 100+ recipients, duplicate
    recipients, negative percentages, percentages summing to exactly 100.00
    with floating point representation quirks (e.g. three-way 33.33/33.33/33.34 splits).

Acceptance Criteria

  • A NaN-containing percentage list is rejected with a clear error, not
    silently accepted.
  • Recipient count is bounded with a documented, justified maximum.
  • Duplicate recipients are rejected.
  • Both validators have equivalent, tested behavior (or are unified into one).

Technical Notes

Files: src/teams/team-split.util.ts, src/escrow/escrow.service.ts
(assertValidSplits), src/teams/dto/create-team.dto.ts,
src/escrow/dto (split-release DTO, if present).

Difficulty Justification

Finding the NaN-comparison validation bypass requires genuinely careful
reasoning about JavaScript numeric comparison semantics (an easy thing to
miss even for experienced engineers), plus real research into Soroban
transaction resource limits to justify a non-arbitrary bound — not a
surface-level fix.

Metadata

Metadata

Assignees

No one assigned

    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