From 09833abd46072972cdd15a5b59256d8bd80bc208 Mon Sep 17 00:00:00 2001
From: Bronek Kozicki <brok@incorrekt.com>
Date: Wed, 2 Apr 2025 12:10:22 +0100
Subject: [PATCH] Improve checks in VaultWithdraw, more tests

---
 src/test/app/Vault_test.cpp               | 74 ++++++++++++++++++++++-
 src/xrpld/app/tx/detail/VaultWithdraw.cpp | 16 ++---
 2 files changed, 79 insertions(+), 11 deletions(-)

diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp
index ef639f9509..352d07f9e5 100644
--- a/src/test/app/Vault_test.cpp
+++ b/src/test/app/Vault_test.cpp
@@ -899,9 +899,14 @@ class Vault_test : public beast::unit_test::suite
             Vault vault{env};
 
             MPTTester mptt{env, issuer, mptInitNoFund};
-            mptt.create({.flags = tfMPTCanTransfer | tfMPTCanLock});
+            mptt.create(
+                {.flags = tfMPTCanTransfer | tfMPTCanLock | lsfMPTCanClawback |
+                     tfMPTRequireAuth});
             PrettyAsset asset = mptt.issuanceID();
+            mptt.authorize({.account = owner});
+            mptt.authorize({.account = issuer, .holder = owner});
             mptt.authorize({.account = depositor});
+            mptt.authorize({.account = issuer, .holder = depositor});
             env(pay(issuer, depositor, asset(1000)));
             env.close();
 
@@ -978,6 +983,54 @@ class Vault_test : public beast::unit_test::suite
             tx[sfDestination] = issuer.human();
             env(tx, ter(tecFROZEN));
         });
+
+        testCase([this](
+                     Env& env,
+                     Account const& issuer,
+                     Account const& owner,
+                     Account const& depositor,
+                     Asset const& asset,
+                     Vault& vault,
+                     MPTTester& mptt) {
+            testcase("MPT un-authorization blocks withdrawal");
+            auto [tx, keylet] = vault.create({.owner = owner, .asset = asset});
+            env(tx);
+            env.close();
+            tx = vault.deposit(
+                {.depositor = depositor,
+                 .id = keylet.key,
+                 .amount = asset(1000)});
+            env(tx);
+            env.close();
+
+            mptt.authorize(
+                {.account = issuer,
+                 .holder = depositor,
+                 .flags = tfMPTUnauthorize});
+            tx = vault.withdraw(
+                {.depositor = depositor,
+                 .id = keylet.key,
+                 .amount = asset(100)});
+            env(tx, ter(tecNO_AUTH));
+
+            // Withdrawal to other (authorized) accounts works
+            tx[sfDestination] = issuer.human();
+            env(tx);
+            tx[sfDestination] = owner.human();
+            env(tx);
+
+            // Clawback works
+            tx = vault.clawback(
+                {.issuer = issuer,
+                 .id = keylet.key,
+                 .holder = depositor,
+                 .amount = asset(800)});
+            env(tx);
+
+            // Can delete empty vault
+            tx = vault.del({.owner = owner, .id = keylet.key});
+            env(tx);
+        });
     }
 
     void
@@ -1235,10 +1288,25 @@ class Vault_test : public beast::unit_test::suite
         }
 
         {
-            testcase("IOU freeze");
+            testcase("IOU deleted trust line, withdraw to 3rd party");
+            env(trust(issuer, asset(0), owner, tfSetFreeze));
+
+            auto tx = vault.withdraw(
+                {.depositor = owner, .id = keylet.key, .amount = asset(10)});
+            env(tx, ter{tecFROZEN});
+
+            tx[sfDestination] = charlie.human();
+            env(tx);
+            env.close();
+
+            BEAST_EXPECT(env.balance(charlie, issue) == asset(30));
+        }
+
+        {
+            testcase("IOU global freeze");
             env(fset(issuer, asfGlobalFreeze));
             auto tx = vault.withdraw(
-                {.depositor = owner, .id = keylet.key, .amount = asset(20)});
+                {.depositor = owner, .id = keylet.key, .amount = asset(10)});
             env(tx, ter{tecFROZEN});
 
             tx[sfDestination] = issuer.human();
diff --git a/src/xrpld/app/tx/detail/VaultWithdraw.cpp b/src/xrpld/app/tx/detail/VaultWithdraw.cpp
index f3fd798af9..5f8f6086e5 100644
--- a/src/xrpld/app/tx/detail/VaultWithdraw.cpp
+++ b/src/xrpld/app/tx/detail/VaultWithdraw.cpp
@@ -99,14 +99,6 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx)
                 return tecNO_PERMISSION;
         }
 
-        if (auto const ter = std::visit(
-                [&]<ValidIssueType TIss>(TIss const& issue) -> TER {
-                    return requireAuth(ctx.view, issue, dstAcct);
-                },
-                asset.value());
-            !isTesSuccess(ter))
-            return ter;
-
         if (assets.holds<MPTIssue>())
         {
             if (auto const ter = canTransfer(
@@ -116,6 +108,14 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx)
         }
     }
 
+    if (auto const ter = std::visit(
+            [&]<ValidIssueType TIss>(TIss const& issue) -> TER {
+                return requireAuth(ctx.view, issue, dstAcct);
+            },
+            asset.value());
+        !isTesSuccess(ter))
+        return ter;
+
     // Cannot withdraw from a Vault an Asset frozen for the destination account
     if (isFrozen(ctx.view, dstAcct, asset))
         return tecFROZEN;