POST /milestones/:id/close refunds the whole escrow on a partially-distributed milestone, double-counting budget already paid to contributors

Open
  • Is something wrong?
#722 0 comments 0 reactions 0 assignees View on GitHub

Maintainers usually reply within 1 day

Nobody has claimed this yet.

Assessment

Difficulty
4/5
Estimated time
3-5 days
Newbie friendliness
55/100
Issue type
Bug
Clarity
Clearly specified
Activity status
Active
Tech stack
typescript
Domain
backend, databases

Research direction

Read src/milestones/milestones.service.ts around MilestonesService.close and src/escrow/escrow.service.ts around releasePartial and refund to trace how payments and refunds are recorded. Start by checking the escrow contract's refund and release entry points, then review src/milestones/milestones.service.spec.ts. Done means closing a partially distributed milestone returns only its unspent budget, records consistent ledger state, and has coverage for the listed close scenarios.

Written by the indexing model from the issue text.

Description

bug

Location

src/milestones/milestones.service.ts:262 — MilestonesService.close

async close(id: string, userId: string): Promise<Milestone> {
  const milestone = await this.findOne(id);
  ...
  assertTransition(milestone.status, MilestoneStatus.CLOSED);

  if (milestone.escrowId) {
    try {
      await this.escrowService.refund(milestone.escrowId);
    } catch (err) {
      // Escrow may already be refunded or partially released
    }
  }

  milestone.status = MilestoneStatus.CLOSED;
  return this.milestoneRepo.save(milestone);
}

Problem

The route is documented as "Close a milestone and refund unspent budget" (src/milestones/milestones.controller.ts:148), but EscrowService.refund has no concept of "unspent". It refunds the full original locked amount:

src/escrow/escrow.service.ts:440-457

async refund(escrowId: string): Promise<Escrow> {
  const escrow = await this.getOrThrow(escrowId);
  this.assertLocked(escrow);
  const result = await this.invokeOnLockedEscrow(escrow, 'refund', () =>
    this.soroban.invoke('refund', [u64(this.onChainKeyFor(escrow))], ...));
  escrow.status = EscrowStatus.REFUNDED;
  ...
}

There is no amount argument, no reading of milestone.distributed, and no subtraction of the Payment rows already written against that escrow by releasePartial.

Crucially, a partially-released milestone escrow is still LOCKED: releasePartial only flips it to RELEASED once the cumulative payouts reach escrow.amount (src/escrow/escrow.service.ts:375-380). So assertLocked passes and the refund proceeds on exactly the escrows where it is wrong.

The milestone state machine permits this path explicitly: IN_PROGRESS -> CLOSED (src/milestones/milestone-state-machine.ts:16-19). IN_PROGRESS is the status resolveIssue writes after every partial payout (milestones.service.ts:223-230), so "partially distributed" is the normal state a milestone is closed from.

Why it matters

For a milestone funded at 1000 with 600 already released to contributors across three resolved issues:

  • The ledger has milestone.distributed = 600 and Payment rows summing to 600.
  • close() asks the contract to refund the escrow keyed by onChainKeyFor(escrow) — the full 1000, not the unspent 400.
  • The escrow row flips to REFUNDED.

Consequences:

  • Accounting drift. The system now records 600 paid out and a full-amount refund against the same escrow. Escrow.amount is 1000 while sum(payments) + refunded = 1600. Every sponsor-dashboard and analytics aggregate that reads either side is now wrong, and the two sides no longer reconcile.
  • On a contract that tracks its own balance, the refund either over-pays the sponsor by 600 (value created from nothing) or fails outright on insufficient balance — and the failure is swallowed by the empty catch on line 273-275, so the milestone is still marked CLOSED and the remaining 400 is stranded on-chain with the escrow row claiming REFUNDED.
  • In dry-run mode (SorobanClientService succeeding unconditionally), the bad state is written to Postgres with no on-chain pushback at all, which is how this reaches staging unnoticed.

Reproduction / failure path

  1. POST /milestones with { "repositoryId": "...", "budget": "1000.0000000", "asset": "USDC" } → milestone M, status OPEN.
  2. POST /milestones/M/issues/<i1>, .../<i2>, .../<i3> — attach three issues.
  3. POST /milestones/M/fund with { "funderAddress": "G...SPONSOR" } → escrow E, amount = 1000, status LOCKED, milestone FUNDED.
  4. POST /milestones/M/issues/<i1>/resolve with { "recipientAddress": "G...C1" } → releases ~333.3333333, milestone IN_PROGRESS, distributed ≈ 333.
  5. Repeat for <i2> → distributed ≈ 666. Escrow E is still LOCKED (666 < 1000).
  6. POST /milestones/M/close.
  7. escrowService.refund(E) is invoked with no amount: the contract is asked to return the entire 1000 to G...SPONSOR, and E.status becomes REFUNDED with refundTxHash set — despite 666 having already left the escrow.

Suggested fix

close() must refund only escrow.amount - sum(payments for that escrow), and must not use the full-amount refund entrypoint on a partially-released escrow:

  1. Add a partial-refund path to EscrowService — e.g. refundRemaining(escrowId, manager?) — that runs under withLockedEscrow (as releasePartial already does), computes the remaining stroops from the Payment rows exactly as releasePartial does (escrow.service.ts:324-339), and invokes the contract's refund with that amount. If the deployed escrow contract has no amount-taking refund entrypoint, the remainder should be returned to the funder via the existing release/release_partial path with the funder as recipient, so the ledger records a real Payment row for it.
  2. Have close() call that instead of refund(), inside a single transaction with the status = CLOSED write, so the two cannot diverge.
  3. Short-circuit when the remaining amount is zero (every issue resolved, distributed === budget): close the milestone without any on-chain call.
  4. Add specs to src/milestones/milestones.service.spec.ts — which currently has no close() tests at all — covering: close from OPEN with no escrow, close from FUNDED with nothing distributed (full refund is correct here), and close from IN_PROGRESS with partial distribution (asserting the refunded amount equals budget - distributed).
Dominant language
TypeScript
Stars
2
Forks
90
Avg merge
31m
Merged PRs (30d)
20

Getting set up

  • Ships a Dockerfile or Docker Compose file
  • No pull request template
  • No contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from MergeFi/backend

All issues in MergeFi/backend

Similar issues

More TypeScript issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.