release() and splitRelease() never check for pre-existing Payment rows, so calling them after releasePartial causes a double payout of the full escrow amount
Maintainers usually reply within 1 day
Assessment
This issue has not been assessed yet.
Description
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 againstescrow.amountbefore proceeding.release(:94-127) andsplitRelease(:133-171) — release the entireescrow.amountto one recipient or split across recipients, with no check for any pre-existingPaymentrows 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
releaseandsplitReleasemust check for pre-existingPaymentrows against the target escrow before proceeding, exactly asreleasePartialalready 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,
releasePartialshould reject being called on an escrow that has already had a fullrelease/splitReleaserecorded against it (currently guarded only byescrow.status !== LOCKED, which does correctly block this direction sincerelease/splitReleasedo setRELEASED— 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 thereleasePartial-vs-releasePartialcase it was originally scoped for.
Acceptance Criteria
- Calling
releaseorsplitReleaseon an escrow that already has one or morePaymentrows from a priorreleasePartialcall is rejected, not silently double-paid. - Calling
releasePartialon an escrow that already has aPaymentrow from a prior fullrelease/splitReleaseis rejected (test this explicitly rather than assuming the existingstatus !== LOCKEDguard covers it). - A test reproduces the exact sequence described above (partial release via
releasePartial, then a fullreleasecall on the same still-LOCKEDescrow) 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.
- Dominant language
- TypeScript
- Stars
- 2
- Forks
- 90
- Avg merge
- 31m
- Merged PRs (30d)
- 20
Getting set up
- Ships a Dockerfile or Docker Compose file
- No pull request template
- No contributing guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from MergeFi/backend
-
bug
Difficulty 2/5 1-3 hours Newbie friendliness 85/100
Maintainers usually reply within 1 day
-
bug security
Difficulty 2/5 1-3 hours Newbie friendliness 74/100
Maintainers usually reply within 1 day
-
bug
Difficulty 2/5 1-3 hours Newbie friendliness 82/100
Maintainers usually reply within 1 day
-
bug testing
Difficulty 2/5 1-3 hours Newbie friendliness 85/100
Maintainers usually reply within 1 day
-
bug security
Difficulty 2/5 1-3 hours Newbie friendliness 76/100
Maintainers usually reply within 1 day
Similar issues
-
Difficulty 2/5 1-3 hours Newbie friendliness 76/100
zapcooking/frontend#946 ·
Maintainers usually reply within 1 day
-
enhancement good first issue
Difficulty 2/5 1-3 hours Newbie friendliness 88/100
WiliamRosa/cosmos-semantic-platform#40 ·
Maintainers usually reply within 1 day
-
Difficulty 2/5 1-3 hours Newbie friendliness 77/100
liberation-data/drivine#733 · 1 comment ·
Maintainers usually reply within 1 day
-
bug
Difficulty 2/5 1-3 hours Newbie friendliness 64/100
makecindy/cindy#10062 · 1 comment ·
Maintainers usually reply within 1 day
-
Difficulty 2/5 1-3 hours Newbie friendliness 78/100
Maintainers usually reply within 1 day