Skip to content

fix(soroban): harden registry/client attestation security#65

Open
alex-predicate wants to merge 2 commits into
mainfrom
alex-predicate/stellar-adversarial-review
Open

fix(soroban): harden registry/client attestation security#65
alex-predicate wants to merge 2 commits into
mainfrom
alex-predicate/stellar-adversarial-review

Conversation

@alex-predicate

Copy link
Copy Markdown
Contributor

Summary

Adversarial review of the Stellar (Soroban) PredicateRegistry, predicate-client, and the example-compliant-token reference integration surfaced several correctness/robustness issues in the attestation path. This PR fixes the actionable ones.

Findings fixed

HIGH — attestation did not bind the transfer recipient (example-compliant-token)

encoded_sig_and_args was set to sha256("transfer(address,address,i128)") — a constant that only covers the function selector. The recipient to was not covered by the attestation anywhere. Because from.require_auth() gates the call but not the destination, a user could obtain an attestation approved for one recipient and redirect it to any other address at submission time — bypassing the counterparty compliance the attester approved.

Fix: added encode_transfer_call(from, to, amount) which encodes the concrete call (selector + all args) into the signed digest, plus a regression test (test_transfer_redirected_recipient_rejected) proving a redirected recipient is rejected.

HIGH — replay-marker TTL decoupled from attestation lifetime (validation.rs)

Spent UUIDs were marked with a fixed ~30-day TTL. For any attestation whose lifetime exceeds that window, the replay-protection entry can be archived/evicted while the attestation is still valid, re-opening replay.

Fix: extend the UUID marker to the network max TTL (storage().max_ttl()), so the guard lives as long as the ledger allows. Added test_uuid_marker_ttl_extended_to_max.

MEDIUM — attester/policy registration could be silently archived (attesters.rs, policy.rs)

Registration/policy entries used a fixed ~30-day TTL that was never refreshed on reads. A long-lived attester could have its registration archived → validation starts failing (AttesterNotRegistered) — a registry liveness failure.

Fix: register attester/policy entries at max TTL, and refresh the attester's TTL on every successful validation so actively-used attesters are never archived.

LOW — defensive hardening

  • token.transfer now checks the bool returned by authorize_transaction and returns NotAuthorized instead of proceeding.
  • deregister guards against len() - 1 underflow if the registration flag and the attester list ever diverge.

Not changed (surfaced but out of scope — flagged for follow-up)

  • Policy is not enforced on-chain. set_policy_id/get_policy_id are never read by validate; statement.policy is only bound via the attester signature. This may be intentional (off-chain enforcement) — worth confirming vs. the EVM ServiceManager model.
  • ed25519_verify panics rather than returning the typed InvalidSignature error (SDK behavior); the error variant is effectively dead.
  • Attester storage has two sources of truth (instance Vec + persistent flags). The TTL fixes make divergence far less likely; a full single-source refactor is deferred.

Testing

  • cargo test — all 41 tests pass (2 new).
  • cargo build --release --target wasm32-unknown-unknown — registry, client, and token build clean.

🤖 Generated with Claude Code

Adversarial review of the Stellar (Soroban) registry and client surfaced
several correctness/robustness issues. This fixes the actionable ones:

- example-compliant-token: bind the transfer recipient (and amount) into the
  signed call data. Previously `encoded_sig_and_args` was only the hash of the
  function selector string, so `to` was never covered by the attestation — an
  attestation approved for one recipient could be redirected to any other at
  submission time, bypassing counterparty compliance. Added
  `encode_transfer_call()` and a regression test.
- registry validation: extend the UUID replay marker to the network max TTL
  instead of a fixed ~30-day window. A short marker could be archived while a
  longer-lived attestation was still valid, re-opening replay. Added a TTL test.
- registry attesters/policy: extend registration and policy entries to max TTL
  and refresh attester TTL on every successful validation, so long-lived
  attesters/policies are never silently archived (registry liveness).
- token: check the bool returned by authorize_transaction and return
  NotAuthorized rather than proceeding (defensive).
- attesters: guard deregister against `len() - 1` underflow on storage
  divergence.

All 41 tests pass; contracts build for wasm32-unknown-unknown.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comment on lines +120 to +133
@@ -131,6 +128,9 @@ impl CompliantTokenContract {
&policy,
&network,
);
if !authorized {
return Err(TokenError::NotAuthorized);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this is handled from a trait you have to implement in a specific fashion but this is anti-idiomatic Rust. The better option here is to have the enum receive a type and have the authorize_transaction() function return the actual error back (unless the error is sensitive and should not be displayed)

This eats the error otherwise and swaps it with a lesser detailed one.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like enums are not possible due to how Soroban contract errors are structured.
Soroban #[contracterror] enums are #[repr(u32)] and can’t carry a payload

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but that's a good call out

@patrickdappollonio patrickdappollonio Jul 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I don't mean in that line, I am talking about the authorize_transaction() function, you use the !authorized here but you could just be using the error instead

Address review feedback: the token was catching the compliance-check result
and remapping it to a generic TokenError::NotAuthorized, which eats the
detailed error.

In Soroban, `invoke_contract::<bool>` already propagates the registry's exact
error code as a trap when validation fails (never returns Ok(false)), so the
remap branch was dead code that only obscured error detail. Removed the
interception and the NotAuthorized variant; the registry's real error code now
surfaces directly.

Note: `#[contracterror]` enums are `#[repr(u32)]` and cannot carry an
associated payload, so wrapping the underlying error in a variant is not an
option — propagating the real error is the idiomatic Soroban approach.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.

2 participants