release() and splitRelease() never check for pre-existing Payment rows, so calling them after releasePartial causes a double payout of the full escrow amount

[chonilius]

Overview

EscrowService has two disjoint families of release methods that both operate on a LOCKED escrow, and neither is aware the other exists:

• releasePartial (src/escrow/escrow.service.ts:180-230) — releases a portion of the escrow, checking cumulative released-so-far against escrow.amount before proceeding.
• release (:94-127) and splitRelease (:133-171) — release the entire escrow.amount to one recipient or split across recipients, with no check for any pre-existing Payment rows against this escrow at all.

// src/escrow/escrow.service.ts:94-101
async release(escrowId: string, recipientAddress: string, recipientId?: string): Promise<Escrow> {
  const escrow = await this.getOrThrow(escrowId);
  this.assertLocked(escrow);   // <- only checks escrow.status === LOCKED, nothing about existing payments

  const result = await this.soroban.invoke('release', [...]);
  // ... creates a Payment for the FULL escrow.amount, unconditionally

Compare to releasePartial, which does do this check, but only against its own prior calls:

// src/escrow/escrow.service.ts:190-202
const existingPayments = await this.paymentRepo.find({ where: { escrowId: escrow.id } });
const releasedSoFar = existingPayments.reduce((sum, p) => sum + Number(p.amount), 0);
const requested = Number(amount);
if (releasedSoFar + requested > Number(escrow.amount) + 1e-7) {
  throw new BadRequestException(`Partial release of ${amount} would exceed remaining escrow balance`);
}

The critical detail: releasePartial only sets escrow.status = EscrowStatus.RELEASED once the cumulative total reaches the full amount (:222-227) — until then, the escrow stays LOCKED. That means assertLocked (the only guard release/splitRelease perform) still passes on an escrow that has already had some fraction of its funds distributed via releasePartial. Nothing stops calling release next: it will happily release the entire escrow.amount again, on top of whatever releasePartial already paid out — a straightforward double payout of the already-distributed portion.

This is directly reachable in the current codebase, not just via the raw EscrowController HTTP surface (see the companion "no auth" issue for how trivially reachable that is): MilestonesService.resolveIssue and MaintenancePoolService.assignReward both call releasePartial on an escrow shared across a milestone's issues or a pool's multiple reward assignments — any of those same escrows is also a valid target for the generic POST /escrow/:id/release or /split-release endpoints, which have no idea a partial-release history exists. Even without malicious intent: an operator or admin tool that calls release on an escrow ID pulled from a list (not realizing it's a milestone escrow that's only 60% distributed via releasePartial) would pay out the full original amount a second time.

Requirements

• release and splitRelease must check for pre-existing Payment rows against the target escrow before proceeding, exactly as releasePartial already does, and reject (or explicitly release only the remaining balance, if that's the intended semantic — pick one and document it) if any prior payments exist.
• Conversely, releasePartial should reject being called on an escrow that has already had a full release/splitRelease recorded against it (currently guarded only by escrow.status !== LOCKED, which does correctly block this direction since release/splitRelease do set RELEASED — confirm this direction is safe and add a regression test rather than assuming).
• Since this check-then-act sequence has the same TOCTOU shape as the escrow double-release issue already tracked in this repo (concurrent calls both reading the same "existing payments" snapshot before either writes), make sure whatever locking mechanism that issue introduces is applied consistently across all four release-family methods (release, splitRelease, releasePartial, and now this cross-method check) rather than only the releasePartial-vs-releasePartial case it was originally scoped for.

Acceptance Criteria

• Calling release or splitRelease on an escrow that already has one or more Payment rows from a prior releasePartial call is rejected, not silently double-paid.
• Calling releasePartial on an escrow that already has a Payment row from a prior full release/splitRelease is rejected (test this explicitly rather than assuming the existing status !== LOCKED guard covers it).
• A test reproduces the exact sequence described above (partial release via releasePartial, then a full release call on the same still-LOCKED escrow) and asserts the second call is rejected, not executed.
• The fix is coordinated with (not duplicative of) whatever locking mechanism lands for the existing escrow double-release TOCTOU issue.

Additional Notes

Precise references: src/escrow/escrow.service.ts:94-127 (release, no payment-history check), :133-171 (splitRelease, same), :180-230 (releasePartial, has the check but only reasons about its own prior partial calls), :260-266 (assertLocked, the only guard shared across all of them, insufficient on its own since releasePartial deliberately keeps status LOCKED mid-distribution).

Test/reproduction plan:

// Escrow locked at 100 USDC, funds a milestone with 2 open issues.
await milestonesService.resolveIssue(milestoneId, issue1Id, recipientA, ...); // releasePartial: pays 50, escrow stays LOCKED
// Now call the raw escrow endpoint directly (as MaintenancePool/generic admin tooling could):
await escrowService.release(escrowId, attackerAddress); // pre-fix: pays the FULL 100 again -> 150 paid out of a 100 escrow

Assert pre-fix reproduces the overpay (documents the bug), post-fix the second call throws BadRequestException and no second soroban.invoke('release', ...) call is made.

Cross-references: shares its root TOCTOU shape with the existing open "Escrow double-release: releasePartial has a read-then-write TOCTOU gap" issue, but that issue's scope (per its own text) is releasePartial-vs-releasePartial concurrency — this issue is releasePartial-vs-release/splitRelease sequencing, which that issue's stated fix (locking within releasePartial) does not cover unless explicitly extended to the other two methods, which this issue's requirements call for. Also relevant to MilestonesService.resolveIssue and MaintenancePoolService.assignReward, both of which create exactly the "escrow still LOCKED with partial history" state this bug depends on.

[sebas11042]

@chonilius hi sr!
I was wondering if I could create the issue?

[Kappa16]

please maintainer ,ill love to work on this issue , expect PR ASAP WITHOUT ERRORS

[grantfox-oss]

🦊 GrantFox — @Kappa16 has been assigned to this issue as part of the Third Campaign campaign!

Next steps:

1. Open a Pull Request referencing this issue (e.g., Closes #44)
2. Your PR will be reviewed by the MergeFi maintainers

Good luck! Track your progress on GrantFox.