From 47953d41eaf7e832f907d88f647a81887b6660ac Mon Sep 17 00:00:00 2001 From: Kappa16 Date: Fri, 21 Aug 2026 01:17:48 +0100 Subject: [PATCH 1/2] #44 release() and splitRelease() never check for pre-existing Payment rows, so calling them after releasePartial causes a double payout of the full escrow amount FIXED --- src/escrow/escrow.service.spec.ts | 71 ++++++- src/escrow/escrow.service.ts | 302 +++++++++++++++++------------- 2 files changed, 244 insertions(+), 129 deletions(-) diff --git a/src/escrow/escrow.service.spec.ts b/src/escrow/escrow.service.spec.ts index dd9dd9e..e63d886 100644 --- a/src/escrow/escrow.service.spec.ts +++ b/src/escrow/escrow.service.spec.ts @@ -9,7 +9,7 @@ import { AssetType, EscrowStatus } from '../common/enums'; describe('EscrowService', () => { let service: EscrowService; let escrowRepo: { create: jest.Mock; save: jest.Mock; findOne: jest.Mock }; - let paymentRepo: { create: jest.Mock; save: jest.Mock }; + let paymentRepo: { create: jest.Mock; save: jest.Mock; find: jest.Mock }; let soroban: { invoke: jest.Mock }; beforeEach(async () => { @@ -19,6 +19,7 @@ describe('EscrowService', () => { findOne: jest.fn(), }; paymentRepo = { + find: jest.fn().mockResolvedValue([]), create: jest.fn((data: Partial) => ({ id: 'payment-1', ...data, @@ -218,6 +219,74 @@ describe('EscrowService', () => { }); }); + it('rejects a full release after a partial release on the same still-LOCKED escrow', async () => { + const escrow = { + id: 'escrow-partial-then-full', + status: EscrowStatus.LOCKED, + amount: '100', + asset: AssetType.USDC, + milestoneId: 'milestone-1', + } as Escrow; + const payments: Partial[] = []; + escrowRepo.findOne.mockResolvedValue(escrow); + paymentRepo.find.mockImplementation(() => Promise.resolve(payments)); + paymentRepo.save.mockImplementation(async (payment: Partial) => { + payments.push(payment); + return payment; + }); + + await service.releasePartial( + 'escrow-partial-then-full', + '50', + 'GRECIPIENT', + ); + expect(escrow.status).toBe(EscrowStatus.LOCKED); + + await expect( + service.release('escrow-partial-then-full', 'GATTACKER'), + ).rejects.toThrow(BadRequestException); + expect(soroban.invoke).toHaveBeenCalledTimes(1); + expect(soroban.invoke).toHaveBeenCalledWith('release', [ + 'milestone-1', + 'GRECIPIENT', + 500000000n, + ]); + }); + + it('rejects splitRelease when an escrow has prior payment history', async () => { + escrowRepo.findOne.mockResolvedValue({ + id: 'escrow-split-after-partial', + status: EscrowStatus.LOCKED, + amount: '100', + asset: AssetType.USDC, + milestoneId: 'milestone-2', + }); + paymentRepo.find.mockResolvedValue([{ amount: '25' }]); + + await expect( + service.splitRelease('escrow-split-after-partial', [ + { recipientAddress: 'GA', percentage: 100 }, + ]), + ).rejects.toThrow(BadRequestException); + expect(soroban.invoke).not.toHaveBeenCalled(); + }); + + it('rejects releasePartial after a full release is recorded', async () => { + escrowRepo.findOne.mockResolvedValue({ + id: 'escrow-full', + status: EscrowStatus.RELEASED, + amount: '100', + asset: AssetType.USDC, + bountyId: 'bounty-full', + }); + paymentRepo.find.mockResolvedValue([{ amount: '100' }]); + + await expect( + service.releasePartial('escrow-full', '1', 'GRECIPIENT'), + ).rejects.toThrow(BadRequestException); + expect(soroban.invoke).not.toHaveBeenCalled(); + }); + describe('assertValidSplits / splitRelease', () => { it('throws when percentages do not sum to 100', () => { expect(() => diff --git a/src/escrow/escrow.service.ts b/src/escrow/escrow.service.ts index 9986857..04eade1 100644 --- a/src/escrow/escrow.service.ts +++ b/src/escrow/escrow.service.ts @@ -5,7 +5,7 @@ import { NotFoundException, } from '@nestjs/common'; import { InjectRepository } from '@nestjs/typeorm'; -import { Repository } from 'typeorm'; +import { EntityManager, Repository } from 'typeorm'; import { Escrow, Payment } from '../common/entities'; import { AssetType, EscrowStatus, PaymentStatus } from '../common/enums'; import { @@ -98,150 +98,153 @@ export class EscrowService { recipientAddress: string, recipientId?: string, ): Promise { - const escrow = await this.getOrThrow(escrowId); - this.assertLocked(escrow); - - const result = await this.soroban.invoke('release', [ - escrow.bountyId ?? - escrow.milestoneId ?? - escrow.maintenancePoolId ?? - escrow.id, - recipientAddress, - ]); - - escrow.status = EscrowStatus.RELEASED; - escrow.releaseTxHash = result.txHash; - escrow.releasedAt = new Date(); - await this.escrowRepo.save(escrow); - - const payment = this.paymentRepo.create({ - escrowId: escrow.id, - recipientId: recipientId ?? null, - recipientAddress, - amount: escrow.amount, - asset: escrow.asset, - status: PaymentStatus.CONFIRMED, - txHash: result.txHash, - }); - await this.paymentRepo.save(payment); - - return escrow; + return this.withReleaseLock( + escrowId, + async (escrowRepo, paymentRepo, escrow) => { + this.assertNoExistingPayments( + escrow, + await paymentRepo.find({ where: { escrowId: escrow.id } }), + ); + + const result = await this.soroban.invoke('release', [ + escrow.bountyId ?? + escrow.milestoneId ?? + escrow.maintenancePoolId ?? + escrow.id, + recipientAddress, + ]); + + escrow.status = EscrowStatus.RELEASED; + escrow.releaseTxHash = result.txHash; + escrow.releasedAt = new Date(); + await escrowRepo.save(escrow); + + const payment = paymentRepo.create({ + escrowId: escrow.id, + recipientId: recipientId ?? null, + recipientAddress, + amount: escrow.amount, + asset: escrow.asset, + status: PaymentStatus.CONFIRMED, + txHash: result.txHash, + }); + await paymentRepo.save(payment); + return escrow; + }, + ); } /** - * Splits the escrowed amount across multiple recipients by percentage - * (team bounties). Percentages must sum to exactly 100. - * - * The recorded `Payment.amount` values are derived from the same - * basis-point integers sent on-chain — not recomputed independently from the - * raw percentages — so the local ledger can never drift from what was - * instructed to the contract. Shares are allocated in whole stroops via a - * largest-remainder method, guaranteeing `sum(payments.amount) === - * escrow.amount` exactly (#43). + * Splits the escrowed amount across multiple recipients by percentage. + * A release is intentionally all-or-nothing: an escrow with any prior + * payment (including a partial release) cannot be released again. */ async splitRelease( escrowId: string, recipients: SplitRecipient[], ): Promise { - const escrow = await this.getOrThrow(escrowId); - this.assertLocked(escrow); - this.assertValidSplits(recipients); - - const totalStroops = amountToStroops(escrow.amount); - // Single source of truth for the split: integer basis points summing to - // exactly 10,000 (100.00%), used both on-chain and to derive the ledger. - const bps = apportionBasisPoints(recipients.map((r) => r.percentage)); - - const result = await this.soroban.invoke('split_release', [ - escrow.bountyId ?? escrow.milestoneId ?? escrow.id, - recipients.map((r) => r.recipientAddress), - bps, - ]); - - const shares = splitStroops(totalStroops, bps); - this.reconcileSplitResult(escrow.id, totalStroops, result.returnValue); - - escrow.status = EscrowStatus.RELEASED; - escrow.releaseTxHash = result.txHash; - escrow.releasedAt = new Date(); - escrow.metadata = { ...(escrow.metadata ?? {}), splitRelease: result }; - await this.escrowRepo.save(escrow); - - const payments: Payment[] = []; - for (let i = 0; i < recipients.length; i++) { - const recipient = recipients[i]; - const payment = this.paymentRepo.create({ - escrowId: escrow.id, - recipientId: recipient.recipientId ?? null, - recipientAddress: recipient.recipientAddress, - amount: stroopsToAmount(shares[i]), - asset: escrow.asset, - splitPercentage: (bps[i] / 100).toFixed(2), - status: PaymentStatus.CONFIRMED, - txHash: result.txHash, - }); - payments.push(await this.paymentRepo.save(payment)); - } - return payments; + return this.withReleaseLock( + escrowId, + async (escrowRepo, paymentRepo, escrow) => { + this.assertNoExistingPayments( + escrow, + await paymentRepo.find({ where: { escrowId: escrow.id } }), + ); + this.assertValidSplits(recipients); + + const totalStroops = amountToStroops(escrow.amount); + const bps = apportionBasisPoints(recipients.map((r) => r.percentage)); + const result = await this.soroban.invoke('split_release', [ + escrow.bountyId ?? escrow.milestoneId ?? escrow.id, + recipients.map((r) => r.recipientAddress), + bps, + ]); + + const shares = splitStroops(totalStroops, bps); + this.reconcileSplitResult(escrow.id, totalStroops, result.returnValue); + escrow.status = EscrowStatus.RELEASED; + escrow.releaseTxHash = result.txHash; + escrow.releasedAt = new Date(); + escrow.metadata = { ...(escrow.metadata ?? {}), splitRelease: result }; + await escrowRepo.save(escrow); + + const payments: Payment[] = []; + for (let i = 0; i < recipients.length; i++) { + const recipient = recipients[i]; + payments.push( + await paymentRepo.save( + paymentRepo.create({ + escrowId: escrow.id, + recipientId: recipient.recipientId ?? null, + recipientAddress: recipient.recipientAddress, + amount: stroopsToAmount(shares[i]), + asset: escrow.asset, + splitPercentage: (bps[i] / 100).toFixed(2), + status: PaymentStatus.CONFIRMED, + txHash: result.txHash, + }), + ), + ); + } + return payments; + }, + ); } - /** - * Releases a portion of a LOCKED escrow to a single recipient without - * closing it out — used by milestone funding, where the total budget is - * distributed incrementally as individual issues resolve. The escrow - * moves to RELEASED once the cumulative released amount reaches the - * total locked amount. - */ + /** Releases one leg while leaving the escrow LOCKED until fully distributed. */ async releasePartial( escrowId: string, amount: string, recipientAddress: string, recipientId?: string, ): Promise { - const escrow = await this.getOrThrow(escrowId); - this.assertLocked(escrow); - this.assertValidAmount(amount); - - const existingPayments = await this.paymentRepo.find({ - where: { escrowId: escrow.id }, - }); - const releasedSoFar = existingPayments.reduce( - (sum, p) => sum + Number(p.amount), - 0, + return this.withReleaseLock( + escrowId, + async (escrowRepo, paymentRepo, escrow) => { + this.assertValidAmount(amount); + // assertLocked deliberately remains before the history calculation: a + // full release marks RELEASED, so partial-after-full is rejected too. + const existingPayments = await paymentRepo.find({ + where: { escrowId: escrow.id }, + }); + this.assertLocked(escrow); + 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`, + ); + } + + const result = await this.soroban.invoke('release', [ + escrow.milestoneId ?? escrow.bountyId ?? escrow.id, + recipientAddress, + this.toStroops(amount), + ]); + const payment = await paymentRepo.save( + paymentRepo.create({ + escrowId: escrow.id, + recipientId: recipientId ?? null, + recipientAddress, + amount, + asset: escrow.asset, + status: PaymentStatus.CONFIRMED, + txHash: result.txHash, + }), + ); + + if (releasedSoFar + requested >= Number(escrow.amount) - 1e-7) { + escrow.status = EscrowStatus.RELEASED; + escrow.releaseTxHash = result.txHash; + escrow.releasedAt = new Date(); + await escrowRepo.save(escrow); + } + return payment; + }, ); - const requested = Number(amount); - if (releasedSoFar + requested > Number(escrow.amount) + 1e-7) { - throw new BadRequestException( - `Partial release of ${amount} would exceed remaining escrow balance`, - ); - } - - const result = await this.soroban.invoke('release', [ - escrow.milestoneId ?? escrow.bountyId ?? escrow.id, - recipientAddress, - this.toStroops(amount), - ]); - - const payment = await this.paymentRepo.save( - this.paymentRepo.create({ - escrowId: escrow.id, - recipientId: recipientId ?? null, - recipientAddress, - amount, - asset: escrow.asset, - status: PaymentStatus.CONFIRMED, - txHash: result.txHash, - }), - ); - - if (releasedSoFar + requested >= Number(escrow.amount) - 1e-7) { - escrow.status = EscrowStatus.RELEASED; - escrow.releaseTxHash = result.txHash; - escrow.releasedAt = new Date(); - await this.escrowRepo.save(escrow); - } - - return payment; } /** Refunds the full escrowed amount back to the original funder. */ @@ -266,6 +269,49 @@ export class EscrowService { return this.getOrThrow(id); } + /** + * Serializes every release-family operation on the escrow row. The database + * lock is held through the Soroban call and ledger writes, preventing two + * concurrent requests from both passing the payment-history check. + * The fallback exists only for lightweight unit-test repository doubles; + * real TypeORM repositories always have a transaction manager. + */ + private async withReleaseLock( + escrowId: string, + operation: ( + escrowRepo: Repository, + paymentRepo: Repository, + escrow: Escrow, + ) => Promise, + ): Promise { + const manager = this.escrowRepo.manager; + if (!manager?.transaction) { + const escrow = await this.getOrThrow(escrowId); + this.assertLocked(escrow); + return operation(this.escrowRepo, this.paymentRepo, escrow); + } + + return manager.transaction(async (transactionManager: EntityManager) => { + const escrowRepo = transactionManager.getRepository(Escrow); + const paymentRepo = transactionManager.getRepository(Payment); + const escrow = await escrowRepo.findOne({ + where: { id: escrowId }, + lock: { mode: 'pessimistic_write' }, + }); + if (!escrow) throw new NotFoundException(`Escrow ${escrowId} not found`); + this.assertLocked(escrow); + return operation(escrowRepo, paymentRepo, escrow); + }); + } + + private assertNoExistingPayments(escrow: Escrow, payments: Payment[]): void { + if (payments.length > 0) { + throw new BadRequestException( + `Escrow ${escrow.id} already has payment history and cannot be fully released`, + ); + } + } + private async getOrThrow(id: string): Promise { const escrow = await this.escrowRepo.findOne({ where: { id } }); if (!escrow) throw new NotFoundException(`Escrow ${id} not found`); From a14110d83c8435e5ceed49dcc3e75b7d9c99b1ce Mon Sep 17 00:00:00 2001 From: Kappa16 Date: Fri, 21 Aug 2026 01:41:16 +0100 Subject: [PATCH 2/2] fix escrow cross-release double payout --- src/escrow/escrow.service.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/escrow/escrow.service.spec.ts b/src/escrow/escrow.service.spec.ts index e63d886..0f3864d 100644 --- a/src/escrow/escrow.service.spec.ts +++ b/src/escrow/escrow.service.spec.ts @@ -230,7 +230,7 @@ describe('EscrowService', () => { const payments: Partial[] = []; escrowRepo.findOne.mockResolvedValue(escrow); paymentRepo.find.mockImplementation(() => Promise.resolve(payments)); - paymentRepo.save.mockImplementation(async (payment: Partial) => { + paymentRepo.save.mockImplementation((payment: Partial) => { payments.push(payment); return payment; });