diff --git a/src/escrow/escrow.service.ts b/src/escrow/escrow.service.ts index 6d2dd28..ac1580b 100644 --- a/src/escrow/escrow.service.ts +++ b/src/escrow/escrow.service.ts @@ -372,6 +372,32 @@ export class EscrowService { ); } + /** + * Validates the preconditions for a pool withdrawal without touching the + * DB balance — called by {@link MaintenancePoolService.assignReward} *before* + * the atomic balance decrement so that cheap, infallible-to-reverse errors + * (bad amount format, recipient/user mismatch, escrow not LOCKED) never + * cause the pool's DB balance to drift below the real on-chain balance. + * + * Deliberately mirrors the first three checks inside {@link poolWithdraw} + * exactly, so a caller that passes these will not encounter the same + * failures a second time inside poolWithdraw itself (barring a race on the + * escrow status between the two calls, which is an acceptable residual risk + * given that the pool already guards against concurrent payouts at the + * balance level). + */ + async assertPoolWithdrawPreconditions( + escrowId: string, + amount: string, + recipientAddress: string, + recipientId?: string, + ): Promise { + const escrow = await this.getOrThrow(escrowId); + this.assertLocked(escrow); + this.assertValidAmount(amount); + await this.assertRecipientsMatchUsers([{ recipientAddress, recipientId }]); + } + /** * Pays a reward out of a maintenance pool's running balance. * @@ -584,20 +610,14 @@ export class EscrowService { recipients: Array<[string, number]>, manager?: EntityManager, ): Promise { - return this.invokeOnLockedEscrow(escrow, operation, () => - this.soroban.invoke( - 'release', - // `release(issue_id: u64, recipients)` — u64-typed on-chain (#301). - [u64(this.onChainKeyFor(escrow)), recipients], - this.contractOpts(escrow), - ), return this.invokeOnLockedEscrow( escrow, operation, () => this.soroban.invoke( 'release', - [this.onChainKeyFor(escrow), recipients], + // `release(issue_id: u64, recipients)` — u64-typed on-chain (#301). + [u64(this.onChainKeyFor(escrow)), recipients], this.contractOpts(escrow), ), manager, diff --git a/src/maintenance-pool/maintenance-pool.service.spec.ts b/src/maintenance-pool/maintenance-pool.service.spec.ts index 88ff0c4..7a9ad12 100644 --- a/src/maintenance-pool/maintenance-pool.service.spec.ts +++ b/src/maintenance-pool/maintenance-pool.service.spec.ts @@ -18,7 +18,7 @@ describe('MaintenancePoolService', () => { decrement: jest.Mock; createQueryBuilder: jest.Mock; }; - let escrowService: { fund: jest.Mock; poolWithdraw: jest.Mock }; + let escrowService: { fund: jest.Mock; poolWithdraw: jest.Mock; assertPoolWithdrawPreconditions: jest.Mock }; let issueRepo: { findOne: jest.Mock }; let paymentRepo: { findOne: jest.Mock; @@ -41,6 +41,7 @@ describe('MaintenancePoolService', () => { escrowService = { fund: jest.fn(), poolWithdraw: jest.fn(), + assertPoolWithdrawPreconditions: jest.fn().mockResolvedValue(undefined), }; issueRepo = { findOne: jest.fn().mockResolvedValue({ @@ -339,6 +340,7 @@ describe('MaintenancePoolService', () => { id: 'pool-1', balance: '100', escrowId: 'escrow-1', + status: MaintenancePoolStatus.ACTIVE, }); // Mock the atomic balance check to succeed const mockQueryBuilder = { @@ -412,6 +414,7 @@ describe('MaintenancePoolService', () => { id: 'pool-1', balance: '100', escrowId: 'escrow-1', + status: MaintenancePoolStatus.ACTIVE, }); // Mock the payment query to return an existing payment const mockPaymentQueryBuilder = { @@ -442,7 +445,7 @@ describe('MaintenancePoolService', () => { escrowId: 'escrow-1', }; poolRepo.findOne.mockImplementation(() => - Promise.resolve({ id: 'pool-1', ...sharedPoolRow }), + Promise.resolve({ id: 'pool-1', status: MaintenancePoolStatus.ACTIVE, ...sharedPoolRow }), ); // Mock the atomic balance check to succeed for both calls const mockQueryBuilder = { @@ -463,5 +466,125 @@ describe('MaintenancePoolService', () => { // Both calls should have succeeded (atomic check passed) expect(mockQueryBuilder.execute).toHaveBeenCalledTimes(2); }); + + // Balance-leak regression tests (#issue): any throw inside poolWithdraw + // after the balance has been decremented must trigger an increment + // restoration so the pool's DB balance never drifts below the real + // on-chain balance. + + describe('balance restoration on poolWithdraw failure', () => { + let mockQueryBuilder: { + update: jest.Mock; + set: jest.Mock; + where: jest.Mock; + setParameter: jest.Mock; + execute: jest.Mock; + }; + + beforeEach(() => { + poolRepo.findOne.mockResolvedValue({ + id: 'pool-1', + balance: '100', + escrowId: 'escrow-1', + status: MaintenancePoolStatus.ACTIVE, + }); + mockQueryBuilder = { + update: jest.fn().mockReturnThis(), + set: jest.fn().mockReturnThis(), + where: jest.fn().mockReturnThis(), + setParameter: jest.fn().mockReturnThis(), + execute: jest.fn().mockResolvedValue({ affected: 1 }), + }; + poolRepo.createQueryBuilder.mockReturnValue(mockQueryBuilder); + }); + + it('restores pool balance when poolWithdraw throws a Soroban invocation error', async () => { + const sorobanError = new Error('Soroban simulate failed: insufficient fee'); + escrowService.poolWithdraw.mockRejectedValue(sorobanError); + + await expect( + service.assignReward('pool-1', 'issue-1', '30', 'GRECIPIENT', 'user-1'), + ).rejects.toThrow('Soroban simulate failed: insufficient fee'); + + // Balance must be restored via increment after the decrement + expect(poolRepo.increment).toHaveBeenCalledWith( + { id: 'pool-1' }, + 'balance', + 30, + ); + }); + + it('restores pool balance when poolWithdraw throws a paymentRepo.save error', async () => { + const dbError = new Error('connection terminated unexpectedly'); + escrowService.poolWithdraw.mockRejectedValue(dbError); + + await expect( + service.assignReward('pool-1', 'issue-1', '50', 'GRECIPIENT'), + ).rejects.toThrow('connection terminated unexpectedly'); + + expect(poolRepo.increment).toHaveBeenCalledWith( + { id: 'pool-1' }, + 'balance', + 50, + ); + }); + + it('restores the exact numeric amount that was decremented', async () => { + escrowService.poolWithdraw.mockRejectedValue(new Error('tx failed')); + + await expect( + service.assignReward('pool-1', 'issue-1', '12.5', 'GRECIPIENT'), + ).rejects.toThrow(); + + // Number('12.5') === 12.5 — must match what the decrement used + expect(poolRepo.increment).toHaveBeenCalledWith( + { id: 'pool-1' }, + 'balance', + 12.5, + ); + }); + + it('does NOT call increment when poolWithdraw succeeds', async () => { + escrowService.poolWithdraw.mockResolvedValue({ id: 'payment-1' }); + + await service.assignReward('pool-1', 'issue-1', '30', 'GRECIPIENT', 'user-1'); + + expect(poolRepo.increment).not.toHaveBeenCalled(); + }); + + it('rethrows the original poolWithdraw error after restoring balance', async () => { + const originalError = new BadRequestException('recipient mismatch'); + escrowService.poolWithdraw.mockRejectedValue(originalError); + + const thrown = await service + .assignReward('pool-1', 'issue-1', '10', 'GRECIPIENT') + .catch((e) => e); + + // The caller sees the original error, not a wrapped one + expect(thrown).toBe(originalError); + // And the balance was still restored + expect(poolRepo.increment).toHaveBeenCalledWith( + { id: 'pool-1' }, + 'balance', + 10, + ); + }); + + it('does NOT decrement at all when assertPoolWithdrawPreconditions rejects before the decrement', async () => { + // Cheap pre-decrement validation fires — no balance should be touched + escrowService.assertPoolWithdrawPreconditions.mockRejectedValue( + new BadRequestException('recipientAddress does not match Stellar address'), + ); + + await expect( + service.assignReward('pool-1', 'issue-1', '10', 'GSTALE_ADDRESS', 'user-1'), + ).rejects.toThrow('recipientAddress does not match Stellar address'); + + // The atomic decrement was never attempted + expect(mockQueryBuilder.execute).not.toHaveBeenCalled(); + // And no restoration increment is needed (nothing was decremented) + expect(poolRepo.increment).not.toHaveBeenCalled(); + }); + }); }); }); diff --git a/src/maintenance-pool/maintenance-pool.service.ts b/src/maintenance-pool/maintenance-pool.service.ts index 13415a6..0dd23d6 100644 --- a/src/maintenance-pool/maintenance-pool.service.ts +++ b/src/maintenance-pool/maintenance-pool.service.ts @@ -164,6 +164,18 @@ export class MaintenancePoolService { ); } + // Run cheap preconditions (escrow LOCKED, valid amount, recipient/user + // match) *before* the balance decrement so that bad-input errors never + // leave the pool's DB balance below the real on-chain balance (#issue). + // assertPoolWithdrawPreconditions mirrors the first three guards inside + // poolWithdraw exactly, so if they pass here they won't fire again there. + await this.escrowService.assertPoolWithdrawPreconditions( + pool.escrowId, + amount, + recipientAddress, + recipientId, + ); + // Atomic balance check and decrement — prevents TOCTOU race where concurrent // calls could overdraw the pool (#274). Uses a conditional UPDATE that only // succeeds if balance >= amount, then checks affected rows. @@ -185,14 +197,25 @@ export class MaintenancePoolService { // not a milestone-style fixed lock that gets partially released and then // closed out — so pay the reward via the pool contract's `withdraw`, // leaving the escrow LOCKED for the next reward (#163). - const payment = await this.escrowService.poolWithdraw( - pool.escrowId, - amount, - recipientAddress, - recipientId, - ); - - return payment; + // + // If poolWithdraw throws for any reason — escrow status race, Soroban + // simulate/send/poll failure, paymentRepo.save failure — the balance + // decrement above has already been applied but no USDC has left the + // contract (the escrow stays LOCKED either way). Restore the reservation + // atomically so the pool's DB balance never permanently drifts below the + // real on-chain balance (#issue). The original error is rethrown so the + // caller observes a clean failure. + try { + return await this.escrowService.poolWithdraw( + pool.escrowId, + amount, + recipientAddress, + recipientId, + ); + } catch (err) { + await this.poolRepo.increment({ id }, 'balance', Number(amount)); + throw err; + } } async list(): Promise {