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

Abierto
  • ¿Algo no está bien?
#722 0 comentarios 0 reacciones 0 asignados Ver en GitHub

Los mantenedores suelen responder en 1 día

Nadie ha tomado este issue todavía.

Evaluación

Dificultad
4/5
Tiempo estimado
3-5 días
Aptitud para principiantes
55/100
Tipo de issue
Error
Claridad
Bien especificado
Estado de actividad
Activo
Stack tecnológico
typescript
Área
backend, databases

Línea de trabajo

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.

Escrito por el modelo de indexación a partir del texto del issue.

Descripción

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).
Lenguaje dominante
TypeScript
Estrellas
2
Forks
90
Merge medio
31 min
PR fusionados (30 d)
20

Preparar el entorno

  • Incluye un Dockerfile o un archivo de Docker Compose
  • Sin plantilla de pull request
  • Sin guía de contribución

Primeros pasos

  1. Lee el issue completo y luego la guía de contribución del proyecto.
  2. Comenta en el issue que vas a ocuparte — evita que dos personas hagan lo mismo.
  3. Haz un fork del repositorio y trabaja en una rama.
  4. Abre un pull request que haga referencia al número del issue.

Más de MergeFi/backend

Todos los issues de MergeFi/backend

Issues similares

Más issues de TypeScript

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.