Skip to content

fix(maintenance-pool): rekey assignReward double-payout guard on issueId (#458) - #479

Open
PINYOPATTANAWASANPORN wants to merge 1 commit into
MergeFi:mainfrom
PINYOPATTANAWASANPORN:fix/maintenance-pool-assign-reward-guard-issueId-458
Open

PINYOPATTANAWASANPORN wants to merge 1 commit into
MergeFi:mainfrom
PINYOPATTANAWASANPORN:fix/maintenance-pool-assign-reward-guard-issueId-458

Conversation

@PINYOPATTANAWASANPORN

Copy link
Copy Markdown

Summary of Changes

Fixes the double-payout guard in assignReward() so it is keyed on (escrow.maintenancePoolId, issueId) instead of (escrow.maintenancePoolId, recipientId).

Adds a nullable issueId column to the Payment entity to make the guard expressible as a single indexed query.

Root Cause (#458)

The #273 fix introduced this check:

const existingPoolPayment = await this.paymentRepo
  .createQueryBuilder('payment')
  .innerJoin('payment.escrow', 'escrow')
  .where('escrow.maintenancePoolId = :poolId', { poolId: id })
  .andWhere('payment.recipientId = :recipientId', { recipientId: recipientId ?? null })
  .getOne();

This has two bypass paths:

  1. Same issue, different recipients: assignReward(pool, issue#1, recipientA) succeeds, then assignReward(pool, issue#1, recipientB) also succeeds — the guard only checks the recipient, not the issue.
  2. Anonymous payouts: when recipientId is undefined, the guard filters on recipientId = null, which matches any anonymous payment in the pool — not the specific issue.

Fix

+ // In Payment entity:
+ @Column({ type: 'varchar', nullable: true })
+ issueId: string | null;

  // In assignReward():
- .andWhere('payment.recipientId = :recipientId', { recipientId: recipientId ?? null })
+ .andWhere('payment.issueId = :issueId', { issueId })

issueId is passed through to escrowService.poolWithdraw() and saved on the Payment row.

Verification & Testing

6 unit tests added to maintenance-pool.service.spec.ts:

  1. allows first reward for an issue — happy path passes
  2. rejects a second reward for the SAME issue regardless of recipient — ConflictException when recipientB tries to claim same issue
  3. rejects double-payout for anonymous (null recipientId) assignments — ConflictException for null recipient
  4. guards on issueId in the query (not recipientId) — asserts andWhere filter uses issueId, not recipientId
  5. throws BadRequestException when balance is insufficient
  6. throws NotFoundException when issue does not exist

Impact & Compatibility

  • Database migration required: adds a nullable issueId varchar column to payments table. Zero-impact on existing rows (null by default).
  • EscrowService.poolWithdraw: signature extended with optional issueId parameter.
  • No breaking changes to existing API endpoints.

Closes #458

…eId (MergeFi#458)

The MergeFi#273 fix checked (escrow.maintenancePoolId, recipientId) which fails
in two cases:
  1. Same issue paid to two different recipients both pass the guard.
  2. Anonymous payouts (recipientId=null) are never guarded.

Fix: add a nullable `issueId` column to Payment and filter the guard
query on (escrow.maintenancePoolId, payment.issueId) instead. Each issue
can now only receive one reward per pool regardless of recipient identity.

Tests added: 6 unit tests covering allowed first reward, ConflictException
on same-issue second reward with different recipient, ConflictException for
anonymous double-payout, query filter shape assertion (issueId not
recipientId), insufficient balance, and missing issue.
@vercel

vercel Bot commented Oct 1, 2026

Copy link
Copy Markdown

@PINYOPATTANAWASANPORN is attempting to deploy a commit to the chonilius' projects Team on Vercel.

A member of the Team first needs to authorize it.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant