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

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

Maintainers usually reply within 1 day

Nobody has claimed this yet.

Assessment

Difficulty
3/5
Estimated time
1-2 days
Newbie friendliness
75/100
Issue type
Bug
Clarity
Clearly specified
Activity status
Active
Tech stack
node.js, typescript
Domain
backend-api-design, payments, security, testing

Research direction

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.

Written by the indexing model from the issue text.

Description

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.

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.