MilestonesService.close ignores its userId argument — any SPONSOR or MAINTAINER can close and refund another sponsor's funded milestone

Abierto
  • ¿Algo no está bien?
#725 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
3/5
Tiempo estimado
1-2 días
Aptitud para principiantes
75/100
Tipo de issue
Error
Claridad
Bien especificado
Estado de actividad
Activo
Stack tecnológico
node.js, typescript
Área
backend-api-design, payments, security, testing

Línea de trabajo

Start with src/milestones/milestones.service.ts, especially close() and findOne(), then compare authorization in fund() and src/bounties/bounties.service.ts. Check how the milestone repository relation exposes its maintainer and review src/milestones/milestones.service.spec.ts for test setup. Done when unauthorized callers receive 403 without a refund, permitted callers can close, and the unreachable guard is removed.

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

Descripción

bug security

Location

src/milestones/milestones.service.ts:262 — MilestonesService.close(id, userId)

Problem

The controller correctly extracts the authenticated caller and passes it in (src/milestones/milestones.controller.ts:155-161):

@UseGuards(JwtAuthGuard, RolesGuard)
@Roles(UserRole.SPONSOR, UserRole.MAINTAINER)
@Post(':id/close')
close(@Param('id', new ParseUUIDPipe()) id: string, @Req() req: Request) {
  const userId = (req.user as any)?.userId;
  return this.milestonesService.close(id, userId);
}

But the service never uses the parameter:

async close(id: string, userId: string): Promise<Milestone> {
  const milestone = await this.findOne(id);
  if (!milestone) { throw new NotFoundException(`Milestone ${id} not found`); }
  assertTransition(milestone.status, MilestoneStatus.CLOSED);
  if (milestone.escrowId) { ... await this.escrowService.refund(milestone.escrowId); ... }
  milestone.status = MilestoneStatus.CLOSED;
  return this.milestoneRepo.save(milestone);
}

userId is dead. The milestone is bound entirely from the URL :id, so authorization stops at "the caller holds the SPONSOR or MAINTAINER role somewhere on the platform".

The sibling method in the same file does the binding properly — MilestonesService.fund (line 73):

if (milestone.sponsorId && milestone.sponsorId !== callerUserId) {
  throw new ForbiddenException('Only the milestone sponsor can fund this milestone');
}

as does BountiesService.refund (src/bounties/bounties.service.ts:257-265) for the equivalent bounty operation. close() is the one money-moving milestone route with no such check.

Two further dead-code smells in the same method confirm it was never finished: the if (!milestone) check on line 264 is unreachable because findOne (line 55-62) already throws NotFoundException, and the catch (err) on line 273 binds err and does nothing with it.

Why it matters

close() triggers an on-chain refund of the escrow to the original funder and then drives the milestone to the terminal CLOSED status (MILESTONE_TRANSITIONS[CLOSED] = [], src/milestones/milestone-state-machine.ts:21). There is no reopen path.

So any account holding SPONSOR or MAINTAINER can, for any milestone belonging to anyone:

  • force an on-chain refund of its escrow, and
  • permanently terminate it — contributors mid-work on its attached issues can no longer be paid, because resolveIssue requires a transition to IN_PROGRESS, which CLOSED forbids.

This is not fund theft (the refund goes to the original funder, not the caller) but it is an unauthenticated-by-ownership denial of service against other sponsors' funded work, and it is irreversible. Combined with the IN_PROGRESS -> CLOSED transition being legal, it can be fired mid-milestone.

Worth noting that SPONSOR/MAINTAINER are platform-wide roles, not per-repository ones, so the role gate narrows the attacker set only to "any account that has ever been granted either role".

Reproduction / failure path

  1. Sponsor A: POST /milestones with { "repositoryId": "R", "budget": "1000.0000000", "asset": "USDC" } → milestone M with sponsorId = A.
  2. Sponsor A: POST /milestones/M/fund with { "funderAddress": "G...A" } → escrow E locked, M.status = FUNDED.
  3. Sponsor B (unrelated account, holds SPONSOR, no connection to M, R, or E), using their own JWT:
POST /milestones/M/close
(no body)
  1. Expected: 403 Forbidden. Actual: 200 OK. E is refunded on-chain and M.status = CLOSED, permanently.

Suggested fix

Use the userId the controller already supplies, and reject a caller who is neither the milestone's sponsor nor the maintainer of its repository:

async close(id: string, callerUserId: string): Promise<Milestone> {
  const milestone = await this.milestoneRepo.findOne({
    where: { id },
    relations: { repository: true },
  });
  if (!milestone) throw new NotFoundException(`Milestone ${id} not found`);

  const isSponsor = milestone.sponsorId === callerUserId;
  const isRepoMaintainer =
    !!milestone.repository?.maintainerId &&
    milestone.repository.maintainerId === callerUserId;
  if (!isSponsor && !isRepoMaintainer) {
    throw new ForbiddenException(
      "Only the milestone sponsor or the repository's maintainer can close this milestone",
    );
  }
  ...
}

Note findOne currently loads relations: { issues: true } only, so repository needs to be loaded for the maintainer branch.

Also remove the unreachable if (!milestone) guard on line 264, and add service specs for close() — the method has no test coverage at all in src/milestones/milestones.service.spec.ts — including a case asserting an unrelated SPONSOR gets 403 and the escrow is not refunded.

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.