fix: Check trustline limits for share-denominated vault withdrawals by Tapanito · Pull Request #6645 · XRPLF/rippled

Tapanito · 2026-03-25T12:18:24Z

VaultWithdraw preclaim skipped the canWithdraw trustline limit check when the withdrawal amount was specified in shares (MPT) rather than assets. Convert shares to the equivalent asset amount before calling canWithdraw so that the destination's trustline limit is enforced regardless of denomination.

Gated behind fixAssortedFixes for ledger replay compatibility.

High Level Overview of Change

Context of Change

API Impact

Public API: New feature (new methods and/or new fields)
Public API: Breaking change (in general, breaking changes should only impact the next api_version)
libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
Peer protocol change (must be backward compatible or bump the peer protocol version)

VaultWithdraw preclaim skipped the canWithdraw trustline limit check when the withdrawal amount was specified in shares (MPT) rather than assets. Convert shares to the equivalent asset amount before calling canWithdraw so that the destination's trustline limit is enforced regardless of denomination. Gated behind fixAssortedFixes for ledger replay compatibility.

Move sleIssuance ledger read inside the fixAssortedFixes branch where it is actually needed, avoiding an unnecessary read on the pre-amendment and asset-denominated paths. Reuse existing account and dstAcct variables instead of recomputing from and to. Add pre-amendment test to verify the old behavior (share-denominated withdrawal bypasses trustline limit check) alongside the post-fix behavior. Fix stale comments in the test.

github-actions · 2026-03-26T00:44:35Z

This PR has conflicts, please resolve them in order for the PR to be reviewed.

github-actions · 2026-03-26T09:42:11Z

All conflicts have been resolved. Assigned reviewers can now start or resume their review.

Tapanito · 2026-03-26T09:48:30Z

/ai-review

xrplf-ai-reviewer

Rounding safety concern and two code-quality issues flagged inline — see comments at lines 70, 76, and 90.

Review by ReviewBot 🤖

Review by Claude Opus 4.6 · Prompt: V12

src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp

codecov · 2026-03-26T10:16:16Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.4%. Comparing base (2c765f6) to head (cdecd5d).

Additional details and impacted files

@@           Coverage Diff           @@
##           develop   #6645   +/-   ##
=======================================
  Coverage     81.4%   81.4%           
=======================================
  Files          998     998           
  Lines        74443   74452    +9     
  Branches      7563    7560    -3     
=======================================
+ Hits         60632   60640    +8     
- Misses       13811   13812    +1

Files with missing lines	Coverage Δ
src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp	`99.1% <100.0%> (+0.1%)`	⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

xrplf-ai-reviewer

Gave this a review

Two issues flagged inline: a rounding-direction concern in the share→asset conversion that could allow boundary bypass, and a tec/tef inconsistency on the unreachable error path.

Review by ReviewBot 🤖

Review by Claude Opus 4.6 · Prompt: V12

xrplf-ai-reviewer · 2026-03-27T14:04:41Z

src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp


-    if (auto const ret = canWithdraw(ctx.view, ctx.tx))
-        return ret;
+    if (ctx.view.rules().enabled(fixSecurity3_1_3) && amount.asset() == vaultShare)


Verify sharesToAssetsWithdraw rounds up (conservative) when passed to canWithdraw — rounding down could allow a withdrawal that slightly exceeds the trust-line limit.

If the conversion rounds down, shares worth exactly the limit could pass the check while delivering more assets than permitted. Confirm the rounding direction is conservative for the amount passed to canWithdraw.

xrplf-ai-reviewer · 2026-03-27T14:04:41Z

src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp

+
+        auto const maybeAssets = sharesToAssetsWithdraw(vault, sleIssuance, amount);
+        if (!maybeAssets)
+            return tecINTERNAL;  // LCOV_EXCL_LINE


tecINTERNAL charges the account a fee; use tefINTERNAL for unreachable internal errors, consistent with line 82:

Suggested change

return tecINTERNAL; // LCOV_EXCL_LINE

return tefINTERNAL; // LCOV_EXCL_LINE

Copilot

Pull request overview

Fixes a VaultWithdraw preclaim gap where withdrawals specified in vault shares (MPT) could bypass the destination IOU trustline limit check by converting share-denominated amounts into the underlying asset amount before applying canWithdraw (gated by an amendment for replay compatibility).

Changes:

Update VaultWithdraw::preclaim to convert share-denominated withdrawals into asset-denominated amounts (when the fix amendment is enabled) before calling canWithdraw.
Add a regression test reproducing the trustline-limit bypass when withdrawing in shares vs assets.

Reviewed changes

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

File	Description
`src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp`	Converts share-denominated withdrawal amounts to asset amounts (behind `fixSecurity3_1_3`) so trustline limits are enforced consistently.
`src/test/app/Vault_test.cpp`	Adds a unit test demonstrating the pre-fix bypass and asserting rejection post-fix.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-27T14:07:35Z

src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp

+    if (ctx.view.rules().enabled(fixSecurity3_1_3) && amount.asset() == vaultShare)
+    {
+        // Post-fixSecurity3_1_3: if the user specified shares, convert
+        // to the equivalent asset amount before checking withdrawal
+        // limits. Pre-amendment the limit check was skipped for
+        // share-denominated withdrawals.
+        auto const mptIssuanceID = vault->at(sfShareMPTID);
+        auto const sleIssuance = ctx.view.read(keylet::mptIssuance(mptIssuanceID));
+        if (!sleIssuance)
+        {
+            // LCOV_EXCL_START
+            JLOG(ctx.j.error()) << "VaultWithdraw: missing issuance of vault shares.";
+            return tefINTERNAL;
+            // LCOV_EXCL_STOP
+        }
+
+        auto const maybeAssets = sharesToAssetsWithdraw(vault, sleIssuance, amount);
+        if (!maybeAssets)
+            return tecINTERNAL;  // LCOV_EXCL_LINE
+
+        if (auto const ret = canWithdraw(
+                ctx.view, account, dstAcct, *maybeAssets, ctx.tx.isFieldPresent(sfDestinationTag)))
+            return ret;


The new share-denominated path calls sharesToAssetsWithdraw() in preclaim without guarding against std::overflow_error. The same conversion is wrapped in a try/catch in doApply(), so an overflow here could propagate as an exception during preclaim instead of returning a TER. Consider wrapping the conversion (and/or the whole share-conversion block) in a try/catch and mapping overflow to the same TER used in doApply (e.g. tecPATH_DRY) or another appropriate code.

Copilot · 2026-03-27T14:07:36Z

src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp

+    if (ctx.view.rules().enabled(fixSecurity3_1_3) && amount.asset() == vaultShare)
+    {
+        // Post-fixSecurity3_1_3: if the user specified shares, convert
+        // to the equivalent asset amount before checking withdrawal
+        // limits. Pre-amendment the limit check was skipped for
+        // share-denominated withdrawals.


PR description says this behavior is gated behind fixAssortedFixes, but the implementation (and the new test) gates it behind fixSecurity3_1_3. There doesn't appear to be a fixAssortedFixes amendment in the codebase, so either the PR description should be updated to match fixSecurity3_1_3, or the code should use the intended amendment name for replay compatibility.

Tapanito added 2 commits March 25, 2026 12:31

Tapanito added Amendment AI Triage Bugs and fixes that have been triaged via AI initiatives labels Mar 25, 2026

github-actions bot added the PR: has conflicts label Mar 26, 2026

Merge origin/develop; replace fixAssortedFixes with fixSecurity3_1_3

cdecd5d

github-actions bot removed the PR: has conflicts label Mar 26, 2026

Tapanito requested review from pratikmankawde and shawnxie999 March 26, 2026 09:48

Tapanito marked this pull request as ready for review March 26, 2026 09:48

xrplf-ai-reviewer bot reviewed Mar 26, 2026

View reviewed changes

src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp Show resolved Hide resolved

src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp Show resolved Hide resolved

src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp Show resolved Hide resolved

kuznetsss requested a review from Copilot March 27, 2026 14:01

Copilot started reviewing on behalf of kuznetsss March 27, 2026 14:02 View session

xrplf-ai-reviewer bot reviewed Mar 27, 2026

View reviewed changes

Copilot AI reviewed Mar 27, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Check trustline limits for share-denominated vault withdrawals#6645

fix: Check trustline limits for share-denominated vault withdrawals#6645
Tapanito wants to merge 3 commits intodevelopfrom
tapanito/fix-vault-withdraw-shares

Tapanito commented Mar 25, 2026

Uh oh!

github-actions bot commented Mar 26, 2026

Uh oh!

github-actions bot commented Mar 26, 2026

Uh oh!

Tapanito commented Mar 26, 2026

Uh oh!

xrplf-ai-reviewer bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Mar 26, 2026

Uh oh!

xrplf-ai-reviewer bot left a comment

Uh oh!

xrplf-ai-reviewer bot Mar 27, 2026

Uh oh!

xrplf-ai-reviewer bot Mar 27, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	return tecINTERNAL; // LCOV_EXCL_LINE
	return tefINTERNAL; // LCOV_EXCL_LINE

Conversation

Tapanito commented Mar 25, 2026

High Level Overview of Change

Context of Change

API Impact

Uh oh!

github-actions bot commented Mar 26, 2026

Uh oh!

github-actions bot commented Mar 26, 2026

Uh oh!

Tapanito commented Mar 26, 2026

Uh oh!

xrplf-ai-reviewer bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Mar 26, 2026

Codecov Report

Uh oh!

xrplf-ai-reviewer bot left a comment

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer bot Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer bot Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants