VaultWithdraw destination account bugfix #5572

Bronek · 2025-07-16T17:51:01Z

High Level Overview of Change

Remove automatic creation of trust line to Destination account in VaultWithdraw.

Context of Change

#5224 added (among other things) a VaultWithdraw transaction which allows setting the recipient of the withdrawn funds in the Destination transaction field. This technically turns this transaction into a payment, and in some respect the implementation does follow payment rules e.g. enforcement of lsfRequireDestTag or lsfDepositAuth or that MPT transfer has destination MPToken. However for IOU, it missed verification that the destination account has trust line to the asset issuer. Since the default behaviour of accountSendIOU is to create this trust line (if missing), this is what VaultWithdraw currently does. This is incorrect, since the Destination might not be interested in holding the asset in question; this basically enables spammy transfers.

Thanks to @ximinez who found the issue and implemented the fix.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Performance (increase or change in throughput and/or latency)
Tests (you added tests for code that already exists, or your new feature included in this PR)
Documentation update
Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
Release

- Unfortunately, to not change behavior, a new "Legacy" authorization type was created. It acts like "StrongAuth" for MPTs and "WeakAuth" for IOUs.

codecov · 2025-07-17T13:57:32Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.8%. Comparing base (5c2a3a2) to head (f4c3c2e).
Report is 1 commits behind head on develop.

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #5572     +/-   ##
=========================================
- Coverage     78.8%   78.8%   -0.0%     
=========================================
  Files          814     814             
  Lines        71205   71222     +17     
  Branches      8330    8362     +32     
=========================================
- Hits         56111   56089     -22     
- Misses       15094   15133     +39

Files with missing lines	Coverage Δ
include/xrpl/protocol/detail/transactions.macro	`100.0% <ø> (ø)`
src/xrpld/app/tx/detail/Escrow.cpp	`93.6% <100.0%> (ø)`
src/xrpld/app/tx/detail/VaultWithdraw.cpp	`94.0% <100.0%> (+0.4%)`	⬆️
src/xrpld/ledger/View.h	`100.0% <100.0%> (ø)`
src/xrpld/ledger/detail/View.cpp	`90.7% <100.0%> (-0.3%)`	⬇️

... and 10 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

src/xrpld/ledger/View.h

ximinez

Tiny comment change

src/xrpld/app/tx/detail/VaultWithdraw.cpp

src/xrpld/ledger/View.h

shawnxie999

LGTM

Co-authored-by: Ed Hennis <ed@ripple.com>

gregtatcam · 2025-07-21T13:47:07Z

src/xrpld/ledger/detail/View.cpp

+    auto const trustLine =
+        view.read(keylet::line(account, issue.account, issue.currency));
+    // If account has no line, and this is a strong check, fail
+    if (!trustLine && authType == AuthType::StrongAuth)


Shouldn't this be gated by an amendment?

No, because before this change there was no way to pass AuthType::StrongAuth to this function (i.e. the old behaviour was always as-if AuthType::WeakAuth was used). So the new behaviour will only be enabled in the code changed in this PR, where we change the parameters passed to requireAuth.

gregtatcam

LGTM 👍

ximinez

Not a super-thorough review, but since there are two others, I figure that's ok. I confirmed that my earlier requested changes have been addressed, and gave the rest of the changes a quick read-through.

Bronek · 2025-07-25T11:35:33Z

Not a super-thorough review, but since there are two others, I figure that's ok. I confirmed that my earlier requested changes have been addressed, and gave the rest of the changes a quick read-through.

thanks @ximinez !

TheTyg · 2025-07-28T19:14:18Z

src/xrpld/app/tx/detail/Escrow.cpp

@@ -315,14 +315,14 @@ escrowCreatePreclaimHelper<MPTIssue>(
    // authorized
    auto const& mptIssue = amount.get<MPTIssue>();
    if (auto const ter =
-            requireAuth(ctx.view, mptIssue, account, MPTAuthType::WeakAuth);
+            requireAuth(ctx.view, mptIssue, account, AuthType::WeakAuth);


Require proof of work to withdrawl

TheTyg · 2025-07-28T19:14:57Z

src/xrpld/app/tx/detail/Escrow.cpp

        ter != tesSUCCESS)
        return ter;

    // If the issuer has requireAuth set, check if the destination is
    // authorized
    if (auto const ter =
-            requireAuth(ctx.view, mptIssue, dest, MPTAuthType::WeakAuth);
+            requireAuth(ctx.view, mptIssue, dest, AuthType::WeakAuth);


- See also #5572 / e7a7bb8

…to ximinez/lending-XLS-66 * XRPLF/ximinez/lending-refactoring-4: Build options cleanup (#5581) Updates Conan dependencies: Boost 1.86 (#5264) VaultWithdraw destination account bugfix (#5572)

Bronek and others added 2 commits July 16, 2025 18:20

Add unit test for MPTs

0676700

Make requireAuth work more similarly between IOUs and MPTs

36949df

- Unfortunately, to not change behavior, a new "Legacy" authorization type was created. It acts like "StrongAuth" for MPTs and "WeakAuth" for IOUs.

Bronek force-pushed the Bronek/vault_destination_bugfix branch from 932dd8b to dec0bf7 Compare July 17, 2025 09:13

Bronek added 2 commits July 17, 2025 10:47

Enforce that trustline for Destination exists

3b7e9df

Merge branch 'develop' into Bronek/vault_destination_bugfix

14cb2e1

Bronek force-pushed the Bronek/vault_destination_bugfix branch from 250cea2 to 14cb2e1 Compare July 17, 2025 09:48

Bronek requested review from mvadari, gregtatcam and ximinez and removed request for gregtatcam and mvadari July 17, 2025 11:31

Bronek marked this pull request as ready for review July 17, 2025 13:13

Bronek requested review from gregtatcam and shawnxie999 July 17, 2025 13:34

Update comment

465b69e

shawnxie999 reviewed Jul 17, 2025

View reviewed changes

src/xrpld/ledger/View.h Outdated Show resolved Hide resolved

Update comment for AuthType

2868359

shawnxie999 reviewed Jul 17, 2025

View reviewed changes

src/xrpld/ledger/View.h Show resolved Hide resolved

Bronek requested a review from a team as a code owner July 18, 2025 14:40

Add sfDestinationTag to VaultWithdraw

88d712e

Bronek force-pushed the Bronek/vault_destination_bugfix branch from cb4a73c to 88d712e Compare July 18, 2025 15:04

Bronek added the Bug label Jul 18, 2025

ximinez requested changes Jul 18, 2025

View reviewed changes

src/xrpld/app/tx/detail/VaultWithdraw.cpp Outdated Show resolved Hide resolved

src/xrpld/ledger/View.h Show resolved Hide resolved

shawnxie999 approved these changes Jul 18, 2025

View reviewed changes

Bronek and others added 2 commits July 21, 2025 13:44

Update comment

f0883a4

Co-authored-by: Ed Hennis <ed@ripple.com>

Merge branch 'develop' into Bronek/vault_destination_bugfix

1cd46bc

gregtatcam reviewed Jul 21, 2025

View reviewed changes

Merge branch 'develop' into Bronek/vault_destination_bugfix

8fa3629

gregtatcam approved these changes Jul 21, 2025

View reviewed changes

More unit tests

1847fdc

Bronek requested a review from ximinez July 22, 2025 14:14

Bronek added 2 commits July 23, 2025 11:53

Merge branch 'develop' into Bronek/vault_destination_bugfix

654bfef

Merge branch 'develop' into Bronek/vault_destination_bugfix

1d88d97

ximinez approved these changes Jul 24, 2025

View reviewed changes

Bronek added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Jul 25, 2025

Bronek removed the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Jul 25, 2025

Merge branch 'develop' into Bronek/vault_destination_bugfix

be4cef2

bthomee enabled auto-merge (squash) July 25, 2025 12:28

Add preflight check for orphaned sfDestinationTag

f4c3c2e

bthomee merged commit e7a7bb8 into develop Jul 25, 2025
27 checks passed

bthomee deleted the Bronek/vault_destination_bugfix branch July 25, 2025 13:53

TheTyg reviewed Jul 28, 2025

View reviewed changes

ximinez added a commit that referenced this pull request Jul 28, 2025

Check LoanBrokerCoverWithdraw Destination and DestinationTag fields

a2be55f

- See also #5572 / e7a7bb8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

VaultWithdraw destination account bugfix #5572

VaultWithdraw destination account bugfix #5572

Uh oh!

Bronek commented Jul 16, 2025 •

edited

Loading

Uh oh!

codecov bot commented Jul 17, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

ximinez left a comment

Uh oh!

Uh oh!

Uh oh!

shawnxie999 left a comment

Uh oh!

gregtatcam Jul 21, 2025

Uh oh!

Bronek Jul 21, 2025

Uh oh!

gregtatcam left a comment

Uh oh!

ximinez left a comment •

edited

Loading

Uh oh!

Bronek commented Jul 25, 2025

Uh oh!

Uh oh!

TheTyg Jul 28, 2025

Uh oh!

TheTyg Jul 28, 2025

Uh oh!

Uh oh!

VaultWithdraw destination account bugfix #5572

VaultWithdraw destination account bugfix #5572

Uh oh!

Conversation

Bronek commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

High Level Overview of Change

Context of Change

Type of Change

Uh oh!

codecov bot commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

ximinez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shawnxie999 left a comment

Choose a reason for hiding this comment

Uh oh!

gregtatcam Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

Bronek Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

gregtatcam left a comment

Choose a reason for hiding this comment

Uh oh!

ximinez left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bronek commented Jul 25, 2025

Uh oh!

Uh oh!

TheTyg Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

TheTyg Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Bronek commented Jul 16, 2025 •

edited

Loading

codecov bot commented Jul 17, 2025 •

edited

Loading

ximinez left a comment •

edited

Loading