No duplicate-recipient protection across multiple POST /airdrops/:id/recipients calls — double-payment risk

[prodbycorne]

Overview

validateAirdropCreate and the POST /airdrops/:id/recipients handler in src/routes/airdrops.js both de-duplicate recipient addresses within a single request body using a local Set:

const recipientSet = new Set();
let sum = 0;
for (let i = 0; i < recipients.length; i++) {
  const r = recipients[i];
  if (!r.address || !isValidStellarAddress(r.address)) { ... }
  if (recipientSet.has(r.address)) {
    return `recipient ${i}: duplicate address ${r.address}`; // only catches dupes inside THIS request
  }
  recipientSet.add(r.address);
  ...
}

But airdropsService.addRecipients() simply appends to a Redis list with no de-duplication against what is already stored:

async function addRecipients(airdropId, recipients) {
  const redis = cache.getClient();
  await redis.rpush(recipientsKey(airdropId), ...recipients.map((r) => JSON.stringify(r)));
}

Nothing checks whether an address being added in a new POST /airdrops/:id/recipients call already exists in the recipients list previously stored for that airdrop (from the initial POST /airdrops body, or from an earlier call to this same endpoint). An operator (or a retried client request, e.g. a network timeout causing a client to resubmit the same CSV upload) can add the same Stellar address twice with two different amounts, both of which will later be paid out once this pipeline drives real on-chain distribution — a direct double-payment / fund-loss risk with no server-side safeguard, despite the API clearly modeling recipients as a set of unique addresses (the single-request dedup logic already proves that intent).

Requirements

• Before appending new recipients, check the existing recipients list for the airdrop (airdropsService.listRecipients or an equivalent internal read) for address collisions with the incoming batch.
• Reject the entire batch (matching the existing all-or-nothing validation style already used for in-request duplicates) if any incoming address already exists among the airdrop's stored recipients, returning a VALIDATION_ERROR naming the conflicting address(es).
• This check must be race-safe against two concurrent POST /airdrops/:id/recipients calls for the same airdrop that each contain non-overlapping-with-each-other-but-overlapping-with-storage addresses (i.e. don't just check-then-append with a TOCTOU gap — use an atomic set-membership check as part of the same Redis operation, or a per-airdrop lock).
• Recompute and re-validate that the airdrop's stored total_amount still reconciles against the full (old + new) recipient sum, using the stroop-safe summation from the companion floating-point precision issue.

Acceptance Criteria

• Adding a recipient address a second time (via a second POST /airdrops/:id/recipients call) for the same airdrop is rejected with a VALIDATION_ERROR naming the duplicate address.
• Two concurrent requests each adding a non-overlapping (with each other) but individually-already-stored address are both correctly rejected — no TOCTOU race allows a duplicate through.
• Legitimate distinct-address batches across multiple calls continue to succeed.
• GET /airdrops/:id/recipients never returns the same address twice for a single airdrop after the fix.
• New tests in test/airdrops.test.js / test/airdrops-service.test.js cover cross-request duplicate detection and the concurrency case.

Additional Notes

Additional edge cases / failure modes

• The current storage structure is a plain Redis list (recipientsKey, populated via LPUSH at create() time and RPUSH at addRecipients() time — note the inconsistency: create() uses lpush, addRecipients() uses rpush, meaning the initial recipients from POST /airdrops end up in reverse order relative to later POST /airdrops/:id/recipients batches; worth fixing for consistency while touching this code even though it's not the core dedup bug). A list has no native set-membership operation — an atomic "is this address already present" check either requires maintaining a parallel Redis Set of addresses per airdrop (airdrop:{id}:recipient_addresses, checked via SISMEMBER/added via SADD, which does support atomic check-and-add via a Lua script or SADD's natural dedup return value) or scanning the full list on every write, which doesn't scale for large recipient counts (up to the existing 10,000 cap) and still isn't atomic against concurrent writers.
• Race-safety per the Requirements: SADD key addr1 addr2 ... is itself atomic and returns the count of newly added members — if that count is less than the number of addresses attempted, some were already present, which gives an atomic conflict-detection primitive for free without a separate Lua script, as long as the whole batch's addresses are added in a single SADD call and the result count is checked before deciding to commit the RPUSH of the actual recipient records. But this introduces an ordering problem: if SADD succeeds (claims the addresses) but the subsequent RPUSH of full recipient records fails (e.g. Redis blip mid-write), the address-set and the recipient-list are now inconsistent (addresses claimed but records not stored) — needs either a Lua script combining both operations atomically, or a clearly defined reconciliation/rollback step.
• The "recompute and re-validate total_amount" requirement means this fix has a hard dependency on Airdrop recipient-sum validation uses strict floating-point equality against total_amount #72's stroop-safe summation helper — re-summing the entire stored recipient list (potentially thousands of entries via LRANGE) on every incremental POST /airdrops/:id/recipients call could become a meaningful latency cost at the 10,000-recipient ceiling; consider whether the airdrop record should maintain a running stroop-sum counter (updated atomically alongside each accepted batch) rather than re-summing from scratch every time — flag this as a follow-up if not addressed in the initial fix.
• What happens when total_amount reconciliation fails (old + new sum exceeds or falls short of stored total_amount) after the duplicate check has already passed? The issue doesn't fully specify: should the whole incoming batch be rejected (matching the existing all-or-nothing style), and should partial application even be structurally possible given the atomicity requirement above — the two checks (duplicate-address and total-amount-reconciliation) should probably be combined into a single atomic accept/reject decision, not two sequential checks where the first could pass and the second fail after some state was already mutated.

Implementation sketch

// airdrops.js service
async function addRecipients(airdropId, recipients) {
  const redis = cache.getClient();
  const addrSetKey = `airdrop:${airdropId}:recipient_addresses`;
  const addresses = recipients.map((r) => r.address);

  // Atomic claim: SADD returns the count of members that were newly added.
  const added = await redis.sadd(addrSetKey, ...addresses);
  if (added < addresses.length) {
    // Some addresses were already present — find which, roll back the claim, reject.
    const alreadyPresent = [];
    for (const addr of addresses) {
      if (!(await redis.sismember(addrSetKey, addr))) continue; // crude; prefer a single pipelined check
    }
    // Roll back any newly-added members from this failed batch to avoid polluting the set with a rejected batch.
    await redis.srem(addrSetKey, ...addresses);
    throw new DuplicateRecipientError(alreadyPresent);
  }

  await redis.rpush(recipientsKey(airdropId), ...recipients.map((r) => JSON.stringify(r)));
}

(A single Lua script performing the SADD-check-RPUSH sequence atomically, rolling back cleanly on conflict, is the more robust version of the above — worth the extra complexity given the fund-loss stakes described in the Overview.)

Test / reproduction plan

1. Create an airdrop with 2 recipients; call POST /airdrops/:id/recipients again with one new address and one address already in the stored list; assert VALIDATION_ERROR naming the duplicate, and assert the new (non-duplicate) address was not partially added (all-or-nothing).
2. Fire two concurrent POST /airdrops/:id/recipients calls, each with a different address that doesn't overlap with the other request but does overlap with an address already stored; assert both are rejected, and the underlying recipient list count is unchanged.
3. Fire two concurrent calls with genuinely disjoint new addresses (no overlap with storage or each other); assert both succeed and the final list contains all of them exactly once.
4. Add recipients that reconcile total_amount exactly using stroop-safe summation (from Airdrop recipient-sum validation uses strict floating-point equality against total_amount #72); add a further batch that would push the cumulative sum over total_amount; assert rejection.
5. Confirm GET /airdrops/:id/recipients never surfaces a duplicate address across pages after a mix of successful and rejected batches.

Related issues in this batch

• Airdrop recipient-sum validation uses strict floating-point equality against total_amount #72 (floating-point recipient-sum equality) — this issue's cumulative total-amount reconciliation directly depends on Airdrop recipient-sum validation uses strict floating-point equality against total_amount #72's stroop-safe summation helper; implement or land that helper first.
• No upper-bound / int64 stroop overflow validation on airdrop total_amount and recipient amounts #73 (int64/stroop overflow) — the cumulative (old + new) sum check here must also respect the Int64 ceiling from No upper-bound / int64 stroop overflow validation on airdrop total_amount and recipient amounts #73, not just re-derive Airdrop recipient-sum validation uses strict floating-point equality against total_amount #72's exact-equality check.
• Redis SMEMBERS-backed pagination is unstable across airdrops, alerts, and API-key listing endpoints #82 (SMEMBERS-based pagination instability) — this issue proposes introducing a new Redis Set (airdrop:{id}:recipient_addresses) purely for atomic membership checks, not for pagination/ordering — make sure it isn't confused with or accidentally reused as the pagination-ordering structure Redis SMEMBERS-backed pagination is unstable across airdrops, alerts, and API-key listing endpoints #82 is separately fixing for the airdrop listing itself (different key, different purpose).