Skip to content

EscrowForm has no guard against beneficiary === depositor or arbiter === beneficiary (self-escrow / no-op arbiter) #23

Description

@abayomicornelius

Problem

src/components/Escrow/EscrowForm.tsx's zod schema validates that beneficiaryPublicKey and arbiterPublicKey are each individually well-formed Stellar addresses, but never compares them to each other or to the connected wallet's own public key:

const escrowSchema = z
  .object({
    beneficiaryPublicKey: z
      .string()
      .min(1, 'Beneficiary address is required')
      .refine(isValidStellarAddress, 'Invalid Stellar address'),
    arbiterPublicKey: z
      .string()
      .optional()
      .default('')
      .refine((v) => v === '' || isValidStellarAddress(v), 'Invalid Stellar address'),
    assetCode: z.string().min(1),
    amount: z /* ... */,
    unlockDate: z.string().min(1, 'Unlock time is required'),
  })
  .refine((v) => new Date(v.unlockDate).getTime() > Date.now(), {
    message: 'Unlock time must be in the future',
    path: ['unlockDate'],
  })

EscrowForm doesn't receive the depositor's own publicKey as a prop at all, so it has no way to flag beneficiaryPublicKey === <own address> even if it wanted to, and there's also no cross-field check that arbiterPublicKey !== beneficiaryPublicKey.

Why it matters

Per getEscrowPermissions in src/types/index.ts, canRelease = isBeneficiary || isArbiter — beneficiary or arbiter can release funds to the beneficiary at any time, before the unlock time, with no counterparty confirmation step. Two concrete problems this enables:

  1. Self-escrow (beneficiaryPublicKey === depositorPublicKey): the depositor becomes their own beneficiary and can release the "escrow" to themselves immediately, defeating the entire point of locking funds (e.g. a self-imposed savings/vesting use case might want this, but as currently built there's no such intentional product framing — it's simply an unvalidated hole that produces a pointless on-chain round-trip with no actual locking guarantee, and could confuse users who mistype their own address into the beneficiary field expecting an error).
  2. arbiterPublicKey === beneficiaryPublicKey: an arbiter that's identical to the beneficiary provides no dispute-resolution value at all (the beneficiary already has unilateral release rights) — this is very plausibly a copy-paste mistake (the two address fields sit right next to each other in the form) that silently produces an escrow with no actual arbitration.

Reproduction

  1. Connect a wallet and open the Escrow creation form.
  2. Paste your own connected public key into "Beneficiary Stellar Address."
  3. Submit — the form validates successfully and proceeds to ConfirmEscrowModal, with no warning that you are your own beneficiary.

Suggested fix

  • Thread the connected wallet's publicKey into EscrowForm as a prop and add a superRefine that flags (at minimum, warns; arguably blocks) beneficiaryPublicKey === publicKey.
  • Add a superRefine check for arbiterPublicKey !== '' && arbiterPublicKey === beneficiaryPublicKey, attaching the error to the arbiterPublicKey field.
  • Consider whether self-escrow should be a hard block or an allowed-with-warning case — some legitimate "locked savings" use cases do want depositor === beneficiary, so a silent block could be product-incorrect; a confirmation checkbox may be the safer middle ground.

Edge cases

  • Muxed accounts / M... addresses that resolve to the same underlying G... account under a different muxed ID — isValidStellarAddress (see Auth token stored in localStorage is vulnerable to XSS exfiltration #7 for the two divergent implementations of this) may treat these as "different" addresses even though they're economically the same account; any equality check here should be aware of that limitation and documented as a known gap rather than assumed airtight.

Testing strategy

  • Add EscrowForm.test.tsx cases: beneficiary === own key produces a validation error/warning; arbiter === beneficiary produces a validation error; distinct depositor/beneficiary/arbiter passes cleanly (existing happy path).

Related issues in this batch

Cross-references the getEscrowPermissions release-before-unlock design that makes point 1 above possible, and #7's duplicate isValidStellarAddress implementations, which any equality-based validation added here will depend on.

Metadata

Metadata

Assignees

Labels

GrantFox OSSIssue tracked in GrantFox OSSMaybe RewardedIssue may be eligible for a GrantFox rewardOfficial Campaign | FWC26Campaign: Official Campaign | FWC26bugSomething isn't workingvery hardVery difficult / senior-level bounty issue

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions