Skip to content

No Stellar public-key format or checksum validation exists anywhere in the DTOs — malformed funder/recipient addresses fall through toScVal's heuristic instead of being rejected at the API boundary #60

Closed

Description

@chonilius

Overview

Every DTO field representing a Stellar public key — funderAddress on FundEscrowDto, DepositDto, and the inline FundBountyDto/FundMilestoneDto; recipientAddress on ReleaseEscrowDto, SplitRecipientDto, and AssignRewardDto — is validated with nothing more than @IsString():

// src/escrow/dto/fund-escrow.dto.ts:18-20
@ApiProperty({ description: 'Stellar public key of the funding sponsor' })
@IsString()
funderAddress: string;
// src/escrow/dto/release-escrow.dto.ts:4-7
@ApiProperty({ description: 'Stellar public key of the recipient' })
@IsString()
recipientAddress: string;

A real Stellar public key ("StrKey") is a specific, checkable format: it starts with G, is exactly 56 characters, is valid base32, and encodes a version byte plus a CRC16 checksum over the payload — the @stellar/stellar-sdk package already depended on by this codebase exposes StrKey.isValidEd25519PublicKey(string) to validate exactly this, for free, with no new dependency needed. None of these DTOs use it, or any other format check — @IsString() accepts "", "not-an-address", a 55-character near-miss, or a syntactically-plausible-but-checksum-invalid string with equal enthusiasm.

Trace what happens to a garbage address that gets past validation. It reaches SorobanClientService.toScVal's heuristic:

// src/escrow/soroban-client.service.ts:167-176
private toScVal(value: unknown) {
  if (typeof value === 'string' && value.length >= 32 && /^[A-Z0-9]+$/.test(value)) {
    try { return new Address(value).toScVal(); }
    catch { return nativeToScVal(value, { type: 'string' }); }   // <- silent fallback, not a rejection
  }
  // ...
}

An invalid-but-address-shaped string (56 uppercase alphanumeric characters that fail the SDK's checksum) throws inside new Address(value), which is caught and silently downgraded to a generic string encoding — not surfaced as a validation error anywhere. An invalid-and-not-address-shaped string (wrong length, lowercase, whatever) never even reaches the Address constructor — it goes straight to the generic string fallback. Either way, nothing in this pipeline ever produces a clean 400 Bad Request telling the caller "this isn't a valid Stellar address." Whatever eventually happens next depends entirely on how the deployed Soroban contract's Rust code handles being given a malformed Address argument — likely a confusing simulation failure deep inside soroban.invoke (per the companion "no BytesN<32> encoding" issue, this whole path is untested against a real contract today), or, in the worst case, an amount genuinely getting "released" toward an address that was never checked to be a real, spendable Stellar account, silently consuming the escrow's LOCKED funds with nothing to show for it.

This is explicitly not the same gap as the already-closed "amount fields accept unbounded string input" issue in this repo — that issue hardened IsMoneyAmount/isSupportedEscrowAsset for numeric/asset fields specifically and doesn't mention address fields anywhere. It's also distinct from the already-open "Bounty payout to a contributor with no linked Stellar address silently releases to an empty-string recipient" issue — that issue is about a missing address (the ?? '' fallback when stellarAddress is null); this issue is about an address that was provided but was never checked to be syntactically or cryptographically valid in the first place, whether it arrives via the empty-string path or a directly client-supplied funderAddress/recipientAddress on the raw escrow/pool endpoints.

Requirements

  • Add a reusable @IsStellarAddress() class-validator decorator (mirroring the existing IsMoneyAmount/IsSupportedEscrowAsset pattern in src/common/validators/money.validator.ts, perhaps in a sibling stellar-address.validator.ts) backed by StrKey.isValidEd25519PublicKey from @stellar/stellar-sdk.
  • Apply it to every funderAddress/recipientAddress field currently typed as a bare @IsString(): FundEscrowDto.funderAddress, DepositDto.funderAddress, ReleaseEscrowDto.recipientAddress, SplitRecipientDto.recipientAddress, AssignRewardDto.recipientAddress, and the inline FundBountyDto.funderAddress/FundMilestoneDto.funderAddress in the bounties/milestones controllers.
  • Ensure the empty-string fallback from the companion "no linked Stellar address" issue is also caught by this same validator once that issue's fix routes an address through these DTOs/service methods — an empty string should fail IsStellarAddress() too, giving that issue's fix a second, independent layer of protection rather than relying solely on whatever check that issue's own fix adds.
  • Add tests: a syntactically-invalid string, a checksum-invalid-but-right-length string, and an empty string are all rejected at the DTO layer with a 400, before ever reaching EscrowService/SorobanClientService.

Acceptance Criteria

  • A reusable Stellar-address validator exists and is backed by the SDK's own StrKey checksum validation, not a hand-rolled regex.
  • Every DTO field listed above uses it.
  • A malformed address (wrong length, wrong prefix, invalid checksum, or empty) is rejected with a 400 at the API boundary, proven by a test that feeds each case directly to the HTTP layer (not just unit-testing the validator function in isolation, matching the rigor of the existing amount-validation fix's own acceptance criteria).
  • SorobanClientService.toScVal's silent try/catch fallback for an unparseable address (:172-175) is no longer the only thing standing between a malformed address and a Soroban call — by the time execution reaches this function, the address has already been validated.

Additional Notes

Precise references: src/escrow/dto/fund-escrow.dto.ts:18-20, src/escrow/dto/release-escrow.dto.ts:4-7, src/escrow/dto/split-release.dto.ts:14-17, src/maintenance-pool/dto/create-pool.dto.ts and the inline DepositDto/AssignRewardDto in src/maintenance-pool/maintenance-pool.controller.ts:9-27, the inline FundBountyDto in src/bounties/bounties.controller.ts:10-13, the inline FundMilestoneDto/ResolveIssueDto in src/milestones/milestones.controller.ts:8-20 — every one of these currently uses bare @IsString() for a field that's supposed to be a Stellar public key. src/escrow/soroban-client.service.ts:167-176 (toScVal's silent fallback, the downstream consequence). src/common/validators/money.validator.ts (the existing, already-closed amount-validation fix this issue's requested decorator should sit alongside, following the same IsXyz()/isValidXyz() naming and structure).

Test/reproduction plan:

const cases = ['', 'not-an-address', 'GAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA', /* right length, bad checksum */];
for (const bad of cases) {
  await request(app).post('/escrow/fund')
    .send({ amount: '10.0000000', asset: 'USDC', funderAddress: bad, bountyId })
    .expect(400);
}
const validCases = [Keypair.random().publicKey()];
for (const good of validCases) {
  await request(app).post('/escrow/fund').send({ ...validPayload, funderAddress: good }).expect(201);
}

Cross-references: distinct from, and complementary to, the closed "amount fields accept unbounded string input" issue (same class of gap — DTO-layer validation missing for a financially-critical field type — applied to the address surface that issue didn't cover) and the open "Bounty payout to a contributor with no linked Stellar address silently releases to an empty-string recipient" issue (that issue is about a missing address specifically in the merge-and-release flow; this issue is the general-purpose format/checksum validation that should exist regardless of how an invalid address value arrives, including but not limited to that empty-string path). Also relevant to the companion "no BytesN<32> encoding" issue, since a validated, guaranteed-well-formed address is one less variable to account for once that encoding work is undertaken.

Activity

  1. added
    bugSomething isn't working
    very hardVery difficult task, expert-level effort required
    Maybe RewardedIssue may be eligible for a GrantFox reward
    GrantFox OSSIssue tracked in GrantFox OSS
    on Aug 16, 2026
  2. ghzhost commented on Aug 16, 2026

    @ghzhost

    I have implemented the fix and opened PR #66 with full test coverage and validation across all Stellar address DTOs and EscrowService methods.

  3. kosisochukwu1234 commented on Aug 16, 2026

    @kosisochukwu1234

    Hello, I’ve gone through the requirements and I’m confident I can implement a proper solution that matches the acceptance criteria. Kindly assign this issue to me.

  4. oluwarantimini commented on Aug 16, 2026

    @oluwarantimini
    Contributor

    The API DTOs are not actually validating that Stellar address fields contain valid Stellar public keys. Every funderAddress and recipientAddress field is protected only by @IsString(), which verifies the JavaScript type but says nothing about the address format.

    A Stellar StrKey public key has a specific encoded structure, including the expected prefix, length, base32 representation, version byte, and checksum. The project already depends on @stellar/stellar-sdk, which provides StrKey.isValidEd25519PublicKey() specifically for this validation, so there is no need to introduce another dependency.

    With the current validation, values such as an empty string, arbitrary text, incorrectly sized keys, or checksum-invalid StrKeys pass DTO validation and reach deeper application logic before failing—or potentially being stored or processed.

    I have experience reviewing API validation boundaries where blockchain-specific identifiers need stronger validation than generic string checks. These DTOs should validate public-key fields with the SDK's StrKey validator (via a custom class-validator constraint or equivalent) so malformed addresses are rejected at the API boundary with a clear validation error.

  5. grantfox-oss commented on Aug 16, 2026

    @grantfox-oss

    🦊 GrantFox — @oluwarantimini 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 #60)
    2. Your PR will be reviewed by the MergeFi maintainers

    Good luck! Track your progress on GrantFox.

  6. grantfox-oss commented on Aug 19, 2026

    @grantfox-oss

    🎉 This issue has been marked as completed on GrantFox as part of the Third Campaign campaign!

    @oluwarantimini's PR #84 was approved and merged by @chonilius.

    🏆 @oluwarantimini: You earned 35 FoxPoints for this contribution! Your current tier: Explorer (118 total points). Track your full progress on GrantFox.

    👏 Great work, @oluwarantimini! Keep contributing to MergeFi.

  7. added a commit that references this issue on Aug 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

GrantFox OSSIssue tracked in GrantFox OSSMaybe RewardedIssue may be eligible for a GrantFox rewardOfficial Campaign | FWC26Campaign: Official Campaign | FWC26Third CampaignCampaign: Third CampaignbugSomething isn't workingvery 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