EscrowService.refund and poolWithdraw bypass withLockedEscrow, so a concurrent refund and release both pass the LOCKED check and both move funds

Read src/escrow/escrow.service.ts, especially refund, poolWithdraw, and withLockedEscrow, then inspect the related transaction patterns in release and releasePartial. Check the callers mentioned in src/milestones/milestones.service.ts and src/escrow/escrow.service.spec.ts. Done means both fund moving methods serialize on the escrow row and save their ledger changes through the transaction manager, with tests covering those behaviors.

Location

• src/escrow/escrow.service.ts:440 — EscrowService.refund
• src/escrow/escrow.service.ts:407 — EscrowService.poolWithdraw
• compare src/escrow/escrow.service.ts:484 — withLockedEscrow

Problem

The pessimistic row lock added for #303 was applied to three of the five fund-moving methods. release, splitRelease and releasePartial all route through withLockedEscrow, which takes SELECT ... FOR UPDATE on the escrow row and asserts the LOCKED precondition under that lock:

const escrow = await mgr.findOne(Escrow, {
  where: { id: escrowId },
  lock: { mode: 'pessimistic_write' },
});
if (!escrow) throw new NotFoundException(...);
this.assertLocked(escrow);
return fn(escrow, mgr);

refund and poolWithdraw do not. Both still use the unlocked read-then-check-then-write pattern the lock was introduced to eliminate:

async refund(escrowId: string): Promise<Escrow> {
  const escrow = await this.getOrThrow(escrowId);   // plain findOne, no lock
  this.assertLocked(escrow);                        // check on an unlocked read
  const result = await this.invokeOnLockedEscrow(escrow, 'refund', () => ...);
  escrow.status = EscrowStatus.REFUNDED;
  ...
  return this.escrowRepo.save(escrow);              // save() on a possibly-stale entity
}

getOrThrow (line 463) is a bare escrowRepo.findOne({ where: { id } }) with no lock option and no transaction.

Why it matters

The lock in withLockedEscrow only serialises callers that take it. A refund holds no lock, so it is invisible to a concurrent release and vice versa:

• release(E) opens its transaction, locks row E, reads status = LOCKED, and begins its on-chain release invocation. The row lock is held until that transaction commits — and it spans the whole Soroban round trip, which IdempotencyInterceptor's 90s stale window (#90) tells us can take tens of seconds.
• Meanwhile refund(E) reads row E without the lock. SELECT without FOR UPDATE is not blocked by another transaction's row lock in Postgres under the default READ COMMITTED isolation — it reads the last committed version, which still says LOCKED.
• assertLocked passes. refund invokes the contract's refund for the same escrow key.
• Both on-chain calls are now in flight for one escrow: the full amount to the recipient and the full amount back to the funder.

Then the ledger diverges regardless of which on-chain call wins:

• refund finishes with this.escrowRepo.save(escrow) on the entity it read before release committed. TypeORM's save issues a full-column UPDATE, so it overwrites release's status = RELEASED, releaseTxHash and releasedAt with the stale pre-release values plus REFUNDED. The Payment row written by release survives, so the escrow reads REFUNDED while a confirmed full-amount Payment exists against it — the exact double-accounting #154 and #303 were meant to close.

On a correctly-implemented contract one of the two on-chain calls would fail, which bounds the loss — but the local ledger is corrupted either way, and in dry-run mode (SorobanClientService.invoke succeeding unconditionally) both "succeed" and the double payout is recorded as real. The same unlocked-read problem applies to poolWithdraw, which additionally runs concurrently with itself by design (a pool stays LOCKED across many rewards) and which MaintenancePoolService.assignReward is the sole caller of.

refund is reachable from three places, so this is not a narrow window: POST /escrow/:id/refund, BountiesService.refund (src/bounties/bounties.service.ts:270), and MilestonesService.close (src/milestones/milestones.service.ts:272). A merged-PR payout racing a sponsor's refund of the same bounty is a realistic sequence, not a contrived one — the bounty state machine permits MERGED -> REFUNDED (src/bounties/bounty-state-machine.ts:32-36).

Reproduction / failure path

1. Fund a bounty: POST /bounties → B, POST /bounties/B/fund → escrow E, status = LOCKED.
2. Claim it and merge the PR so B reaches MERGED.
3. Fire two requests concurrently (or add an artificial delay inside soroban.invoke to widen the window):
   • POST /escrow/E/release with { "recipientAddress": "G...C" }
   • POST /escrow/E/refund
4. Both return 200. Payment has a confirmed row for the full amount to G...C, and E.status is REFUNDED with releaseTxHash/releasedAt cleared by refund's stale save().
5. Reversing the order gives the mirror image: E.status = RELEASED with a refundTxHash written and then clobbered.

Serialising the two requests produces the correct 400 Escrow E is not in LOCKED state on the second, which confirms the only thing missing is the lock.

Suggested fix

Route both methods through the existing helper, exactly as release does:

async refund(escrowId: string): Promise<Escrow> {
  return this.withLockedEscrow(escrowId, async (escrow, manager) => {
    const result = await this.invokeOnLockedEscrow(
      escrow, 'refund',
      () => this.soroban.invoke('refund', [u64(this.onChainKeyFor(escrow))], this.contractOpts(escrow)),
      manager,
    );
    escrow.status = EscrowStatus.REFUNDED;
    escrow.refundTxHash = result.txHash;
    escrow.refundedAt = new Date();
    return manager.save(Escrow, escrow);
  });
}

and for poolWithdraw, take the lock and write the Payment through the transaction manager (manager.save(Payment, ...)) instead of this.paymentRepo.save(...), so the withdraw and its ledger row are one atomic span.

Add an optional manager?: EntityManager passthrough on refund so MilestonesService.close can bring it into its own transaction, mirroring releasePartial's signature.

Tests to add in src/escrow/escrow.service.spec.ts: assert that refund and poolWithdraw call findOne with lock: { mode: 'pessimistic_write' } (the release specs already assert this shape), and that both write through the transaction manager rather than the bare repository.