release() and splitRelease() never check for pre-existing Payment rows, so calling them after releasePartial causes a double payout of the full escrow amount
Maintainer thường phản hồi trong vòng 1 ngày
Đánh giá
Issue này chưa được đánh giá.
Mô tả
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.
- Ngôn ngữ chính
- TypeScript
- Star
- 2
- Fork
- 90
- Merge trung bình
- 31 phút
- Pull request đã merge (30 ngày)
- 20
Chuẩn bị môi trường
- Có Dockerfile hoặc tệp Docker Compose
- Không có mẫu pull request
- Không có hướng dẫn đóng góp
Bắt đầu từ đâu
- Đọc hết issue, rồi đọc hướng dẫn đóng góp của dự án.
- Bình luận trên issue rằng bạn sẽ nhận — tránh hai người làm cùng một việc.
- Fork repository và làm thay đổi trên một nhánh.
- Mở pull request có tham chiếu số hiệu của issue.
Issue khác của MergeFi/backend
-
bug
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 85/100
Maintainer thường phản hồi trong vòng 1 ngày
-
bug security
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 74/100
Maintainer thường phản hồi trong vòng 1 ngày
-
bug
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 82/100
Maintainer thường phản hồi trong vòng 1 ngày
-
bug testing
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 85/100
Maintainer thường phản hồi trong vòng 1 ngày
-
bug security
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 76/100
Maintainer thường phản hồi trong vòng 1 ngày
Tất cả issue của MergeFi/backend
Issue tương tự
-
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 76/100
zapcooking/frontend#946 ·
Maintainer thường phản hồi trong vòng 1 ngày
-
enhancement good first issue
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 88/100
WiliamRosa/cosmos-semantic-platform#40 ·
Maintainer thường phản hồi trong vòng 1 ngày
-
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 77/100
liberation-data/drivine#733 · 1 bình luận ·
Maintainer thường phản hồi trong vòng 1 ngày
-
bug
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 64/100
makecindy/cindy#10062 · 1 bình luận ·
Maintainer thường phản hồi trong vòng 1 ngày
-
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 78/100
Maintainer thường phản hồi trong vòng 1 ngày