Skip to content

[Regression on closed #273] assignReward's double-payout guard is keyed on (pool, recipient) instead of (pool, issue) — the same issue can still be paid twice #458

Description

@chonilius

Problem

#273 ("no guard against being called twice for the same issueId") was closed with this check in MaintenancePoolService.assignReward():

// Guard against double/triple payout for the same issue (#273).
const existingPayment = await this.paymentRepo.findOne({
  where: { recipientId: recipientId ?? null },
});
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();
if (existingPoolPayment) {
  throw new ConflictException(`Issue ${issueId} has already received a reward from pool ${id}`);
}

issueId doesn't appear in either query, and Payment has no issue column at all. The guard checks who was paid from this pool, not which issue was rewarded:

  1. The same issue can still be paid twice. Call assignReward(pool, issue#1, …, recipientA) and then assignReward(pool, issue#1, …, recipientB), and both succeed. This is the double payout MaintenancePoolService.assignReward has no guard against being called twice for the same issueId — double/triple payout of a single completed task #273 asked to prevent.
  2. Anonymous payouts are never guarded. When recipientId is omitted, the query becomes payment.recipientId = NULL, which is never true in SQL. So repeated rewards for the same issue with no recipientId always pass.
  3. Legitimate rewards are rejected. A maintainer who fixes two different maintenance issues gets a 409 on the second one, with a misleading message saying that issue was already rewarded.
  4. existingPayment is dead code. It's an unscoped query across every payment in the system, whose result is never read, and it costs an extra DB round trip per call.

Suggested fix

  • Record the rewarded issue on the payment, for example a nullable Payment.maintenanceIssueId (FK to issues), plus a unique index on (escrowId, maintenanceIssueId).
  • Guard on (pool escrow, issueId), and let the unique index close the concurrent-call race.
  • Remove the unused existingPayment query.
  • Add tests: the same issue with two recipients gets a 409, and two issues for the same recipient both succeed.

Related: #273, #274, #276.

Activity

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

Metadata

Metadata

Labels

Stellar WaveIssues in the Stellar wave programbugSomething isn't workingsecuritySecurity-related issuevery hardVery difficult task, expert-level effort required

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions