Skip to content

[ANCHOR-1311]: One non-SAC contract payment permanently halts customer-fund crediting - #2020

Draft
amandagonsalves wants to merge 4 commits into
developfrom
fix/anchor-1311
Draft

amandagonsalves wants to merge 4 commits into
developfrom
fix/anchor-1311

Conversation

@amandagonsalves

Copy link
Copy Markdown
Collaborator

Description

The payment observer resolved an invoke-host-function payment's asset from the top-level invoked
contract instead of the actual balance change. For any non-SAC contract that lookup returned null,
and reading it back was fatal, taking the whole observer offline and advancing the stored cursor
past the uncredited payment anyway.

Three classes derived or consumed that same wrong value (DefaultPaymentListener,
HorizonPaymentObserver, StellarRpcPaymentObserver), plus a set of reliability bugs turned one
bad payment into a permanent, silent loss. Everything that used to throw now fails closed instead:
skip the operation, keep the observer running, keep the cursor where it was.

For RPC mode, the credited amount also came from the top-level call's own declared argument, not
the emitting contract's verified event, so a forwarding contract could declare any amount while
moving almost nothing. Amount is now taken from the same verified source as the asset.

Changes

  • SacToAssetMapper.java: getAssetFromSac fails closed on RPC errors instead of throwing.
  • AbstractPaymentObserver.java: handleEvent isolates one listener's exception per listener.
  • HorizonPaymentObserver.java: asset comes from any matching SAC balance change, not [0]/entrypoint name; cursor advances only on success; fetchStreamingCursor resumes from the stored cursor; check() maps PUBLISHER_ERROR to red.
  • DefaultPaymentListener.java: validate() takes the asset from the event; SacToAssetMapper dependency removed.
  • StellarRpcPaymentObserver.java: asset and amount come from the event-emitting contract, not the top-level invoked contract; skips instead of throwing on a non-SAC contract.
  • PaymentObserverBeans.java: paymentListener bean no longer wires SacToAssetMapper.
  • Test files updated for new signatures, plus new regression tests and SacToAssetMapperTest.kt.

Acceptance Criteria

  • An invoke-host-function payment is credited from the asset/amount the genuine SAC actually moved, regardless of the top-level contract, entrypoint name, or declared amount.
  • A non-SAC contract call is skipped, not thrown, and doesn't take the observer offline or skip the cursor.
  • A misconfigured RPC endpoint fails the affected payment, not the whole observer.
  • check() reports red while PUBLISHER_ERROR is active.

Context

HackerOne #4001848

Testing

  • Unit: ./gradlew :platform:test --tests "org.stellar.anchor.platform.observer.stellar.*" --rerun
  • Full module: ./gradlew :platform:test --rerun (all green)

Documentation

N/A

Known limitations

from/to for RPC-mode invoke-host-function payments still come from the top-level args, not the
verified event. No new misattribution risk beyond the existing attacker-chosen memo, so left as is.

* update invoke host function payment validation
* remove sac to asset mapper from default payment listener
* add listener error isolation in payment observers
* update stellar rpc observer to use verified event data
* fix horizon payment observer cursor persistence
* update health check status for publisher errors

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens Stellar payment observers against incorrect SAC resolution and malformed payment events.

Changes:

  • Derives invoke-host-function assets and amounts from verified SAC activity.
  • Improves cursor handling, listener isolation, and observer health reporting.
  • Updates wiring and adds regression tests.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
platform/src/test/kotlin/org/stellar/anchor/platform/observer/stellar/StellarRpcPaymentObserverTest.kt Updated as part of this pull request.
platform/src/test/kotlin/org/stellar/anchor/platform/observer/stellar/StellarPaymentObserverTest.kt Updated as part of this pull request.
platform/src/test/kotlin/org/stellar/anchor/platform/observer/stellar/SacToAssetMapperTest.kt Updated as part of this pull request.
platform/src/test/kotlin/org/stellar/anchor/platform/observer/stellar/HorizonPaymentObserverTest.kt Updated as part of this pull request.
platform/src/test/kotlin/org/stellar/anchor/platform/observer/stellar/DefaultPaymentListenerTest.kt Updated as part of this pull request.
platform/src/main/java/org/stellar/anchor/platform/observer/stellar/StellarRpcPaymentObserver.java Updated as part of this pull request.
platform/src/main/java/org/stellar/anchor/platform/observer/stellar/SacToAssetMapper.java Updated as part of this pull request.
platform/src/main/java/org/stellar/anchor/platform/observer/stellar/HorizonPaymentObserver.java Updated as part of this pull request.
platform/src/main/java/org/stellar/anchor/platform/observer/stellar/DefaultPaymentListener.java Updated as part of this pull request.
platform/src/main/java/org/stellar/anchor/platform/observer/stellar/AbstractPaymentObserver.java Updated as part of this pull request.
platform/src/main/java/org/stellar/anchor/platform/component/observer/PaymentObserverBeans.java Updated as part of this pull request.
Suppressed comments (2)

platform/src/main/java/org/stellar/anchor/platform/observer/stellar/AbstractPaymentObserver.java:152

  • PaymentListener.onReceived declares AnchorException and IOException, so a listener can fail without throwing RuntimeException. Those exceptions bypass this isolation, abort the remaining listeners, and send the observer down its publisher-error path. Catch Exception here (or otherwise isolate the declared checked failures) to match the stated behavior.
      } catch (RuntimeException rex) {

platform/src/main/java/org/stellar/anchor/platform/observer/stellar/StellarRpcPaymentObserver.java:427

  • Using the verified event amount here still causes the forwarding-contract case to be dropped: DefaultPaymentListener.processAndDispatchLedgerPayment() compares this value with ledgerPayment.getAmount() (the top-level declared argument) and returns on mismatch. Thus a genuine SAC transfer is not credited at all when the router declares a different amount, contrary to the acceptance criterion. Invoke-host-function dispatch should use the verified event amount downstream instead of rejecting it against the attacker-controlled declaration.
                .amount(BigInteger.valueOf(emittingAmount))

Note

Copilot is running an experiment and ran this review at Lite.


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@amandagonsalves
amandagonsalves marked this pull request as ready for review September 14, 2026 15:35
@amandagonsalves
amandagonsalves marked this pull request as draft September 14, 2026 15:36
* add dynamic soroban invoke host function name parsing

* add strict argument type and count validation for soroban invoke host functions

* update payment amounts to biginteger

* refactor payment listener error propagation

* fix invoke host function payment processing with verified event amounts
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.

3 participants