Skip to content

Wallet address validation is grammar-only: no Stellar CRC16 checksum check, so checksum-invalid keys are accepted #364

Description

@usmanimamu17-create

Problem

wallet_address.normalize (app/utils/wallet_address.py) validates length, prefix, and alphabet:

if not _STELLAR_PUBLIC_KEY_RE.match(upper):
    raise WalletAddressError(...)

Stellar public keys embed a CRC16-CCITT checksum over (version byte + payload) in the last two characters (base32-encoded). The regex accepts any G + 55 base32 chars, including keys whose checksum is wrong.

Consequences:

  • Typos and corrupted keys pass validation: a single transposed character that keeps the alphabet valid produces a "valid" address that is not a real Stellar key; downstream operations (wallet linking, governance proposals, funding) target a nonexistent account.
  • The platform's own validator is the weakest gate: get_current_user_or_service, wallet linking, and governance all rely (or should rely) on this module; a checksum check is the standard way Stellar tooling rejects bad keys, and its absence lets bad keys flow into the DB.
  • The error message claims more than the check delivers: the regex error text mentions "base-32 alphabet" — correct, but it implies the key is valid, which the checksum would refute.

Root cause

The validator was implemented from the grammar only; the checksum (part of SEP-23/Stellar address encoding) was never added.

Why this is architecturally hard

  1. Implementing CRC16-CCITT-XModem is ~30 lines and well-specified, but the value object (NormalizedAddress) and every caller must agree that checksum validation is part of normalize; relaxing it later for test fixtures would be a security regression.
  2. WalletRegistry._build_public_key generates keys that do not satisfy even the current grammar (tracked separately), so a stricter validator will break internal key generation — the two fixes must land together or generation must be fixed first.
  3. A checksum test needs known-good and known-bad vectors; tests/test_wallet_persistence.py and the Stellar integration docs provide some, but a canonical vector set should be added.

Proposed design

Add CRC16-CCITT-XModem verification to normalize (reject invalid checksums with a descriptive WalletAddressError), keep the grammar checks, and add known-vector tests (valid key, transposed-char key, wrong-checksum key).

Acceptance criteria

Service

  • Checksum-invalid addresses are rejected by normalize/is_valid.
  • Valid addresses pass unchanged.

Tests

  • Known-vector tests cover valid, transposed, and wrong-checksum keys.
  • Existing wallet tests pass.

Out of scope

The internal key generator (tracked separately) and governance address validation wiring.

Getting started

pytest tests/test_wallet_persistence.py tests/test_check_stellar_networks.py -q
make typecheck

Good first files to read: app/utils/wallet_address.py, app/models/orm/wallet.py.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    GrantFox OSSIssue tracked in GrantFox OSSMaybe RewardedIssue may be eligible for a GrantFox rewardThird CampaignCampaign: Third Campaignarea/securitySecurity hardening, vulnerability fixes, threat surface reductionarea/walletsImported campaign issue labelpriority/highImportant; address in current quarter

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions