Money-moving DTOs trust client-supplied contributorId/recipientId/funderAddress instead of binding to the authenticated caller

[chonilius]

Overview

Even after JWT guards are added to the mutating routes (see the companion "no auth at all" issue), a whole class of these endpoints will still be exploitable, because they derive the identity of who benefits from the action from request-body fields the client controls, not from the authenticated caller (req.user). Adding @UseGuards(JwtAuthGuard) proves someone with a valid token is calling — it does nothing to stop that someone from naming a different person (or a different wallet) as the beneficiary.

Concrete instances:

// src/bounties/dto/claim-bounty.dto.ts
export class ClaimBountyDto {
  @IsUUID()
  contributorId: string;
}
// src/bounties/bounties.controller.ts:41-45
@Idempotent('bounty.claim')
@Post(':id/claim')
claim(@Param('id') id: string, @Body() dto: ClaimBountyDto) {
  return this.bountiesService.claim(id, dto.contributorId);
}

contributorId is whatever UUID the client puts in the body — BountiesService.claim (src/bounties/bounties.service.ts:58-66) sets bounty.claimedById = contributorId with no check that it equals the caller. Any authenticated user can claim a bounty as another user's ID — griefing (blocking the real intended contributor, since CLAIMED is a one-way gate per the state machine) or, combined with the setStellarAddress IDOR in the companion issue, redirecting the eventual payout.

// src/escrow/dto/release-escrow.dto.ts / split-release.dto.ts
recipientAddress: string;   // free-form string, not tied to any user record
recipientId?: string;       // optional, only used for Payment attribution — never cross-checked against recipientAddress

EscrowController.release/splitRelease (escrow.controller.ts:26-42) pass both straight through to EscrowService. There is no check anywhere that recipientAddress actually belongs to recipientId, or to the bounty's claimedById, or to anyone authenticated at all. FundEscrowDto.funderAddress and DepositDto.funderAddress (maintenance pools) have the same shape: a free string, unconnected to the caller.

The point isn't "add auth" (that's the companion issue) — it's that these specific fields need to be removed from the client-writable surface and derived from req.user server-side, because a guard alone leaves exactly this gap: user A, correctly authenticated as themselves, calling POST /bounties/:id/claim with {"contributorId": "<user B's id>"}.

Requirements

• ClaimBountyDto.contributorId should be dropped from the request body; BountiesService.claim should take the ID from req.user.userId once auth lands, not from client input. If there's a legitimate "claim on behalf of" flow (e.g. a maintainer assigning a bounty to a contributor), make it an explicit, separately-authorized action, not the default unauthenticated path.
• For recipientAddress/recipientId on the escrow release endpoints: at minimum, when recipientId is present, validate recipientAddress === user(recipientId).stellarAddress server-side rather than trusting the client's pairing of the two. The service-layer callers (BountiesService.markMergedAndRelease) already do this correctly by deriving the address from the user record — the raw EscrowController HTTP surface should not allow bypassing that by accepting an arbitrary pair directly.
• funderAddress fields (FundEscrowDto, DepositDto, FundBountyDto, FundMilestoneDto) should similarly be validated against the authenticated caller's own linked stellarAddress where the caller is expected to be the funder, or explicitly documented/justified as an intentionally open field (e.g. if funding is meant to be permissionless by design — but that should be a stated decision, not an accident of "the DTO happened to accept a string").
• Audit every DTO under src/**/dto for this pattern (any field named *Id or *Address that represents "who benefits" rather than "what resource") and produce a short table in the PR description of what was found and how each was resolved.

Acceptance Criteria

• bounty.claim no longer accepts a client-supplied contributorId — it's derived from the authenticated caller.
• escrow.release/escrow.splitRelease reject a recipientId/recipientAddress pair that doesn't match the user record on file (or the fields are removed in favor of a fully server-derived recipient for the bounty/milestone/pool-mediated paths).
• A test proves: authenticated user A cannot cause bounty.claimedById to be set to user B's ID via the claim endpoint.
• A test proves: a recipientId/recipientAddress mismatch on a release call is rejected before any Soroban call is made.

Additional Notes

Precise references:

• src/bounties/dto/claim-bounty.dto.ts:1-8 and src/bounties/bounties.controller.ts:41-45 — the claim-spoofing path.
• src/bounties/bounties.service.ts:58-66 — claim() trusts contributorId unconditionally.
• src/escrow/dto/release-escrow.dto.ts:1-13 and src/escrow/dto/split-release.dto.ts:1-37 — recipientAddress/recipientId decoupled, no cross-check.
• src/escrow/escrow.controller.ts:26-42 — passes both straight through.
• Contrast with src/bounties/bounties.service.ts:121-129 — the correct pattern already exists in this codebase (deriving recipientAddress from contributor.stellarAddress server-side for the normal merge-and-release flow); the raw EscrowController HTTP surface just doesn't enforce the same discipline, so it's a shortcut around the safe path that already exists one layer up.

Why this survives the companion "add JWT guards" fix on its own: that issue's acceptance criteria stop at "every mutating route requires a valid JWT" and "at least a minimal ownership/role check exists for the highest-value actions" — this issue is the detailed follow-through on what those ownership checks actually need to look like for the specific fields listed here, since "add a guard" and "validate this specific field against this specific user record" are different amounts of work and this issue's checklist should not be considered satisfied by the other one's.

Test/reproduction plan:

1. Authenticate as user A. Fund a bounty. POST /bounties/:id/claim with {"contributorId": "<B's UUID>"}. Pre-fix: 200, bounty.claimedById === B.id. Post-fix: either the field is rejected/ignored (400 or the ID is silently forced to A) — pick one behavior and test it explicitly.
2. Authenticate as user A (who has recipientId pointing at user B in their own claim). POST /escrow/:id/release with {"recipientAddress": "<A's own address>", "recipientId": "<B's id>"}. Assert this is rejected once the cross-check lands, rather than silently paying A while attributing the Payment row to B.

Cross-references: direct follow-on to the "no auth at all" issue and the setStellarAddress IDOR issue — all three exploit the same root pattern (trusting client-supplied identity over req.user) at three different layers: no guard at all, a guard with no ownership check on the URL param, and a guard (once added) with no ownership check on body fields.

[davieslennox0]

I can help resolve the issue, assign and let me work on it.

[grantfox-oss]

🦊 GrantFox — @davieslennox0 has been assigned to this issue as part of the Third Campaign campaign!

Next steps:

1. Open a Pull Request referencing this issue (e.g., Closes #40)
2. Your PR will be reviewed by the MergeFi maintainers

Good luck! Track your progress on GrantFox.