Skip to content

claim() leaves past-due packages in Created state: locked funds and aggregates stay stale until manual refund #423

Description

@kilodesodiq-arch

Problem

When a claim arrives after a package's expires_at, both claim and claim_with_proof return an error but leave the package in PackageStatus::Created:

// app/onchain/contracts/aid_escrow/src/lib.rs (claim)
if package.expires_at > 0 && now > package.expires_at {
    return Err(Error::PackageExpired);   // ← returns early, no state transition
}

No decrement_locked, no add_to_status_totals, and no status write happen on this path. The funds stay counted in KEY_TOTAL_LOCKED and in the Created (KEY_TOTAL_COMMITTED) aggregate until an admin manually calls refund(id).

Consequences, distinctly:

  • Solvency pool leak. create_package rejects new packages when contract_balance < current_locked + amount. Because expired-but-unclaimed packages never decrement KEY_TOTAL_LOCKED, their escrowed funds are invisible as available pool, and the pool shrinks over time as packages expire. This is a liveness bug: the admin must remember to sweep every expired package by hand to reclaim capacity.
  • Aggregates lie. get_aggregates(token) returns total_committed that overstates genuinely claimable commitments and total_expired_cancelled that undercounts expiry. Dashboards and indexers built on get_aggregates report funds as committed that are actually unclaimable.
  • The documentation contradicts itself. BOUNDARY_VALIDATION_BEHAVIOR.md section "Late Claim Behavior" says the package "remains Created", but the same file's "Test Coverage" and "Conclusion" describe package_status_auto_expires_on_first_late_claim_attempt and "late claim attempts automatically expire the package status on-chain". The shipped tests do not match the shipped code, so a contributor cannot tell which behavior is intended.

Root cause

The expiry check was added as a guard that returns early, but the corresponding state transition (mark Expired, release locked funds, move aggregates Created → Expired) was only implemented in the admin refund path, not on the claim path.

Why this is architecturally hard

  1. The transition must be atomic with the check. Marking Expired on a late claim is not just a status write: it must also decrement_locked and move KEY_TOTAL_COMMITTED → KEY_TOTAL_EXPIRED_CANCELLED for the token, or the invariant Σ locked + Σ claimed + Σ surplus == total_funded (see the closed issue Invariant test: Σ locked_tokens + Σ claimed_tokens + Σ surplus == total_funded #223) drifts. A naive "set status = Expired" makes the problem worse.
  2. Aggregates vs. the storage model. add_to_status_totals uses PackageStatus::Refunded as its own bucket mapping; the design must decide whether Expired is written by the claim path (and what refund then sees) or only by refund, and how refund's existing was_committed logic stays correct.
  3. The check is duplicated. claim and claim_with_proof each inline the now > expires_at guard, and refund/cancel_package/extend_expiry each have their own slightly different reading of expiry. The fix must decide whether to introduce a shared assert_not_expired/expire_if_past_due helper rather than a fifth copy.
  4. Backend parity. BOUNDARY_VALIDATION_BEHAVIOR.md also documents a backend ClaimsService.cleanupExpiredClaims() cron that archives and revokes/refunds. If the contract starts auto-expiring on claim, the backend's "Created → archived" flow and its onchain revoke/refund step must be reconciled so a package is not both auto-expired and then refund-attempted.

Proposed design

Prefer an idempotent transition helper invoked on every claim/claim_with_proof when expires_at > 0 && now > expires_at: set status = Expired, decrement_locked, move the amount from Created to the expired/cancelled bucket, then return Error::PackageExpired. refund must remain able to convert an Expired package to Refunded and transfer the funds. Keep view_package_status cheap (still a single read).

Acceptance criteria

Contract

  • A late claim/claim_with_proof on a Created package transitions it to Expired and returns Error::PackageExpired.
  • After auto-expiry, get_total_locked(token) no longer includes that package's amount and get_aggregates(token) shows the amount in total_expired_cancelled, not total_committed.
  • refund(id) on an auto-expired package still transfers funds to the admin and ends in Refunded without double-decrementing locked totals.

Tests

  • Tests in app/onchain/contracts/aid_escrow/tests/ assert the locked/aggregate deltas after a late claim, and that a second late claim does not double-move the amount.
  • A test pins the refund after auto-expiry path so no invariant drift is introduced.

Documentation

  • BOUNDARY_VALIDATION_BEHAVIOR.md is rewritten so "Late Claim Behavior", "Test Coverage", and "Conclusion" describe one consistent behavior, and the contradictory auto-expiry statements are removed.

Out of scope

Adding a recipient-index or pagination to package reads, and any change to expires_at == 0 (never-expire) semantics, are separate issues.

Getting started

Files: app/onchain/contracts/aid_escrow/src/lib.rs (claim, claim_with_proof, refund, add_to_status_totals, decrement_locked), app/onchain/contracts/aid_escrow/BOUNDARY_VALIDATION_BEHAVIOR.md, app/onchain/contracts/aid_escrow/tests/boundary_validation_tests.rs.

cd app/onchain
make test        # cargo test -- --nocapture

Good first files to read: src/lib.rs refund (the existing Created→Expired→Refunded logic) and tests/aggregates.rs to see how totals are asserted today.

Activity

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

Metadata

Metadata

Assignees

Labels

GrantFox OSSIssue tracked in GrantFox OSSMaybe RewardedIssue may be eligible for a GrantFox rewardThird CampaignCampaign: Third Campaignarea:onchainOn-chain (Soroban) areabugSomething isn't workinghighHigh severity issues

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions