Skip to content

fix: Use holder's balance for AMMClawback owner-reserve check#7796

Open
ckeshava wants to merge 7 commits into
XRPLF:developfrom
ckeshava:ammclawback-fixes
Open

fix: Use holder's balance for AMMClawback owner-reserve check#7796
ckeshava wants to merge 7 commits into
XRPLF:developfrom
ckeshava:ammclawback-fixes

Conversation

@ckeshava

Copy link
Copy Markdown
Contributor

Summary

AMMClawback reuses AMMWithdraw's helpers to return the paired asset to the holder. When the paired asset's trustline has to be auto-created, the reserve check was fed the issuer's pre-fee balance (sfAccount is the issuer) instead of the holder's. As a result, the issuer's XRP could satisfy a reserve requirement that is meant to protect the holder, forcing an under-reserved holder into a new trustline and an increased owner count — even though the equivalent holder-signed AMMWithdraw correctly fails with tecINSUFFICIENT_RESERVE.

Fix

Gated behind the new fixAMMClawbackReserve1 amendment: the owner-reserve check now uses the holder's own XRP balance. Pre-amendment behavior is unchanged.

Tests

Added a unit test covering both paths:

  • amendment enabled → clawback fails with tecINSUFFICIENT_RESERVE, owner count unchanged (matches the control AMMWithdraw).
  • amendment disabled → legacy behavior preserved.

🤖 Generated with Claude Code

ckeshava and others added 2 commits July 13, 2026 21:08
…w trustline is created; Legacy version of the code reads the Issuer's balance which is different from Holder's balance
@codecov

codecov Bot commented Jul 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.8%. Comparing base (a24e543) to head (6f637d5).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #7796   +/-   ##
=======================================
  Coverage     82.8%   82.8%           
=======================================
  Files         1036    1036           
  Lines        80107   80110    +3     
  Branches      8932    8930    -2     
=======================================
+ Hits         66300   66307    +7     
+ Misses       13798   13794    -4     
  Partials         9       9           
Files with missing lines Coverage Δ
src/libxrpl/tx/transactors/dex/AMMWithdraw.cpp 93.7% <100.0%> (+<0.1%) ⬆️

... and 2 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ckeshava

Copy link
Copy Markdown
Contributor Author

@gregtatcam @yinyiqian1 Please review this PR and advise on the next steps.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@gregtatcam

Copy link
Copy Markdown
Contributor

@gregtatcam @yinyiqian1 Please review this PR and advise on the next steps.

@ckeshava I don't think this should be "fixed" by failing the transaction — the current outcome is correct; it's the intent that reads unclearly.
AMMClawback is a compliance recovery that deliberately works without holder cooperation: it already withdraws with IgnoreFreeze/IgnoreAuth and recreates deleted MPTokens. Gating the paired-asset return on the holder's reserve reintroduces the holder veto the design intentionally strips out — a holder could block clawback indefinitely by deleting the paired trustline and keeping spendable XRP below one incremental reserve. tecINSUFFICIENT_RESERVE here is a regression, not the safe choice. And the trustline holds the holder's own returned asset, so failing doesn't protect them — it denies them the asset and blocks the issuer.
That said, the current code does look accidental: std::max(priorBalance, balance) with priorBalance = the issuer's preFeeBalance_ reads like the wrong account was passed. So rather than change behavior, let's make intent explicit:

Skip sufficientReserve on the clawback path (e.g. when authHandling == IgnoreAuth) so the outcome no longer depends on the issuer incidentally holding enough XRP, and
Add a comment noting clawback intentionally bypasses the recipient reserve check, consistent with IgnoreFreeze/IgnoreAuth.

This keeps the correct outcome, removes the ambiguity, and fixes a real edge case: for MPT the check consults the issuer's balance alone, so a low-XRP issuer can currently be blocked.

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