From 32726a9fb6441259854224e0ec7fb2557d003cc8 Mon Sep 17 00:00:00 2001 From: Fabulouz34 Date: Wed, 30 Sep 2026 09:54:44 +0000 Subject: [PATCH] fix(maintenance-pool): restore balance on poolWithdraw failure Move cheap preconditions (assertLocked, assertValidAmount, assertRecipientsMatchUsers) before the atomic balance decrement in assignReward() so bad-input errors never touch the pool balance. Wrap the poolWithdraw() call in try/catch: on any throw, restore the decremented amount via poolRepo.increment() before rethrowing. The escrow stays LOCKED in all failure paths (no USDC moved), so the increment returns the pool to its true on-chain state. Also fix a pre-existing syntax error in EscrowService.invokeRelease() (duplicate return statement left from an earlier partial edit). Adds 6 regression tests in 'balance restoration on poolWithdraw failure' asserting increment is called on failure, not called on success, the exact amount is restored, the original error is rethrown, and that a pre-decrement validation failure neither decrements nor restores. Closes #274. Related: #51, #163. --- src/escrow/escrow.service.ts | 36 +++-- .../maintenance-pool.service.spec.ts | 127 +++++++++++++++++- .../maintenance-pool.service.ts | 39 ++++-- 3 files changed, 184 insertions(+), 18 deletions(-) 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 {