Skip to content

fix(escrow): assert caller authorization on refund and release (#299) - #480

Open
PINYOPATTANAWASANPORN wants to merge 4 commits into
MergeFi:mainfrom
PINYOPATTANAWASANPORN:fix/escrow-caller-authorization-guard-299
Open

PINYOPATTANAWASANPORN wants to merge 4 commits into
MergeFi:mainfrom
PINYOPATTANAWASANPORN:fix/escrow-caller-authorization-guard-299

Conversation

@PINYOPATTANAWASANPORN

Copy link
Copy Markdown

Summary of Changes

Fixed an Authorization / Access Control vulnerability in EscrowController and EscrowService (#299). Previously, refund(), release(), and splitRelease() checked caller roles via RolesGuard (@Roles(UserRole.MAINTAINER, UserRole.SPONSOR)), but did not thread the authenticated caller's identity into EscrowService to verify resource ownership. This allowed any user with a SPONSOR role to call POST /escrow/:id/refund or release against another sponsor's locked escrow.

This change threads the authenticated caller's userId into EscrowService and introduces an explicit caller authorization check (assertCallerAuthorized) ensuring that non-maintainer sponsors can only refund or release escrows attributed to themselves (escrow.sponsorId === callingUserId or matching fundedByAddress).

Root Cause / Technical Context

  1. RolesGuard only asserts that the request user possesses a required role (UserRole.SPONSOR / UserRole.MAINTAINER). It operates purely at the HTTP metadata level without entity context.
  2. EscrowService.refund(), release(), and splitRelease() accepted only escrowId without caller identity context. As a result, a valid SPONSOR account could trigger an unauthorized refund on any LOCKED escrow, forcing funds back to the funder and cancelling rival bounties or milestones in flight.
  3. Fix details:
    • Passed req.user.userId from EscrowController to EscrowService.refund(), release(), and splitRelease().
    • Added assertCallerAuthorized(escrow, callingUserId, action) in EscrowService. Maintainers retain system-wide authority, while sponsors are restricted to their own attributed escrows.
    • Added unit test coverage in escrow.service.spec.ts validating authorized vs. unauthorized callers.

Verification & Testing

Ran full NestJS Jest unit test suite for src/escrow (5 test suites, 119 unit tests):

$ npx jest src/escrow

PASS src/escrow/escrow-response.mapper.spec.ts
PASS src/escrow/split-math.util.spec.ts
PASS src/escrow/soroban-client.service.spec.ts
PASS src/escrow/escrow.service.spec.ts
PASS src/escrow/escrow.controller.spec.ts

Test Suites: 5 passed, 5 total
Tests:       119 passed, 119 total
Snapshots:   0 total
Time:        3.384 s

…Fi#299)

Ensure non-maintainer sponsors can only refund or release escrows attributed to themselves, preventing unauthorized cross-sponsor escrow cancellation or release.
…Fi#299)

Ensure non-maintainer sponsors can only refund or release escrows attributed to themselves, preventing unauthorized cross-sponsor escrow cancellation or release.
…Fi#299)

Ensure non-maintainer sponsors can only refund or release escrows attributed to themselves, preventing unauthorized cross-sponsor escrow cancellation or release.
…Fi#299)

Ensure non-maintainer sponsors can only refund or release escrows attributed to themselves, preventing unauthorized cross-sponsor escrow cancellation or release.
@vercel

vercel Bot commented Oct 4, 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

Development

Successfully merging this pull request may close these issues.

1 participant