More unit tests for terNO_RIPPLE in VaultWithdraw

include/xrpl/ledger/View.h

@@ -5,6 +5,7 @@
 #include <xrpl/ledger/ApplyView.h>
 #include <xrpl/ledger/OpenView.h>
 #include <xrpl/ledger/ReadView.h>
+#include <xrpl/protocol/Asset.h>
 #include <xrpl/protocol/Indexes.h>
 #include <xrpl/protocol/MPTIssue.h>
 #include <xrpl/protocol/Protocol.h>
@@ -1082,6 +1083,26 @@ canTransfer(
     AccountID const& from,
     AccountID const& to);
 
+[[nodiscard]] TER
+canTransfer(
+    ReadView const& view,
+    Issue const& issue,
+    AccountID const& from,
+    AccountID const& to);
+
+[[nodiscard]] TER inline canTransfer(
+    ReadView const& view,
+    Asset const& asset,
+    AccountID const& from,
+    AccountID const& to)
+{
+    return std::visit(
+        [&]<ValidIssueType TIss>(TIss const& issue) -> TER {
+            return canTransfer(view, issue, from, to);
+        },
+        asset.value());
+}
+
 /** Deleter function prototype. Returns the status of the entry deletion
  * (if should not be skipped) and if the entry should be skipped. The status
  * is always tesSUCCESS if the entry should be skipped.

src/libxrpl/ledger/View.cpp

@@ -3277,6 +3277,42 @@ canTransfer(
     return tesSUCCESS;
 }
 
+[[nodiscard]] TER
+canTransfer(
+    ReadView const& view,
+    Issue const& issue,
+    AccountID const& from,
+    AccountID const& to)
+{
+    if (issue.native())
+        return tesSUCCESS;
+
+    auto const issuerId = issue.getIssuer();
+    auto const sleIssuer = view.read(keylet::account(issuerId));
+    if (sleIssuer == nullptr)
+        return tefINTERNAL;  // LCOV_EXCL_LINE
+    if (issuerId == from || issuerId == to)
+        return tesSUCCESS;
+
+    auto const isRippleDisabled = [&](AccountID account) -> bool {
+        // Line might not exist, but some transfers can create it. If this
+        // is the case, just check the default ripple on the issuer account.
+        auto const line = view.read(keylet::line(account, issue));
+        if (line)
+        {
+            bool const issuerHigh = issuerId > account;
+            return line->isFlag(issuerHigh ? lsfHighNoRipple : lsfLowNoRipple);
+        }
+        return sleIssuer->isFlag(lsfDefaultRipple) == false;
+    };
+
+    // Fail if rippling disabled on either trust line
+    if (isRippleDisabled(from) || isRippleDisabled(to))
+        return terNO_RIPPLE;
+
+    return tesSUCCESS;
+}
+
 TER
 cleanupOnAccountDelete(
     ApplyView& view,

src/test/app/Loan_test.cpp

@@ -5591,11 +5591,11 @@ protected:
         });
     }
 
-#if LOANTODO
     void
-    testCoverDepositAllowsNonTransferableMPT()
+    testCoverDepositWithdrawNonTransferableMPT()
     {
-        testcase("CoverDeposit accepts MPT without CanTransfer");
+        testcase(
+            "CoverDeposit and CoverWithdraw reject MPT without CanTransfer");
         using namespace jtx;
         using namespace loanBroker;
 
@@ -5615,7 +5615,7 @@ protected:
 
         env.close();
 
-        PrettyAsset const asset = mpt["BUG"];
+        PrettyAsset const asset = mpt["MPT"];
         mpt.authorize({.account = alice});
         env.close();
 
@@ -5649,21 +5649,58 @@ protected:
         env(pay(alice, pseudoAccount, asset(1)), ter(tecNO_AUTH));
         env.close();
 
+        // Cover cannot be transferred to broker account
         auto const depositAmount = asset(1);
-        env(coverDeposit(alice, brokerKeylet.key, depositAmount));
-        BEAST_EXPECT(env.ter() == tesSUCCESS);
+        env(coverDeposit(alice, brokerKeylet.key, depositAmount),
+            ter{tecNO_AUTH});
         env.close();
 
         if (auto const refreshed = env.le(brokerKeylet);
             BEAST_EXPECT(refreshed))
         {
-            // with an MPT that cannot be transferred the covrAvailable should
-            // remain zero
             BEAST_EXPECT(refreshed->at(sfCoverAvailable) == 0);
+            env.require(balance(pseudoAccount, asset(0)));
+        }
+
+        // Set CanTransfer again and transfer some deposit
+        mpt.set({.mutableFlags = tmfMPTSetCanTransfer});
+        env.close();
+
+        env(coverDeposit(alice, brokerKeylet.key, depositAmount));
+        env.close();
+
+        if (auto const refreshed = env.le(brokerKeylet);
+            BEAST_EXPECT(refreshed))
+        {
+            BEAST_EXPECT(refreshed->at(sfCoverAvailable) == 1);
             env.require(balance(pseudoAccount, depositAmount));
         }
+
+        // Remove CanTransfer after the deposit
+        mpt.set({.mutableFlags = tmfMPTClearCanTransfer});
+        env.close();
+
+        // Cover cannot be transferred from broker account
+        env(coverWithdraw(alice, brokerKeylet.key, depositAmount),
+            ter{tecNO_AUTH});
+        env.close();
+
+        // Set CanTransfer again and withdraw
+        mpt.set({.mutableFlags = tmfMPTSetCanTransfer});
+        env.close();
+
+        env(coverWithdraw(alice, brokerKeylet.key, depositAmount));
+        env.close();
+
+        if (auto const refreshed = env.le(brokerKeylet);
+            BEAST_EXPECT(refreshed))
+        {
+            BEAST_EXPECT(refreshed->at(sfCoverAvailable) == 0);
+            env.require(balance(pseudoAccount, asset(0)));
+        }
     }
 
+#if LOANTODO
     void
     testLoanPayLateFullPaymentBypassesPenalties()
     {
@@ -6858,10 +6895,10 @@ public:
     run() override
     {
 #if LOANTODO
-        testCoverDepositAllowsNonTransferableMPT();
         testLoanPayLateFullPaymentBypassesPenalties();
         testLoanCoverMinimumRoundingExploit();
 #endif
+        testCoverDepositWithdrawNonTransferableMPT();
         testPoC_UnsignedUnderflowOnFullPayAfterEarlyPeriodic();
 
         testDisabled();

src/test/app/Vault_test.cpp

@@ -2,6 +2,7 @@
 #include <test/jtx/AMMTest.h>
 #include <test/jtx/Env.h>
 #include <test/jtx/amount.h>
+#include <test/jtx/mpt.h>
 
 #include <xrpl/basics/base_uint.h>
 #include <xrpl/beast/unit_test/suite.h>
@@ -587,6 +588,7 @@ class Vault_test : public beast::unit_test::suite
             Vault vault{env};
             env.fund(XRP(1000), issuer, owner, depositor, charlie, dave);
             env.close();
+            env(fset(issuer, asfDefaultRipple));
             env(fset(issuer, asfAllowTrustLineClawback));
             env(fset(issuer, asfRequireAuth));
             env(fset(dave, asfRequireDest));
@@ -657,6 +659,7 @@ class Vault_test : public beast::unit_test::suite
             env.fund(XRP(1000), issuer, owner);
             env.close();
 
+            env(fset(issuer, asfDefaultRipple));
             env(fset(issuer, asfAllowTrustLineClawback));
             env(fset(issuer, asfRequireAuth));
             env.close();
@@ -1765,7 +1768,8 @@ class Vault_test : public beast::unit_test::suite
             mptt.create(
                 {.flags = tfMPTCanTransfer | tfMPTCanLock |
                      (args.enableClawback ? tfMPTCanClawback : none) |
-                     (args.requireAuth ? tfMPTRequireAuth : none)});
+                     (args.requireAuth ? tfMPTRequireAuth : none),
+                 .mutableFlags = tmfMPTCanMutateCanTransfer});
             PrettyAsset asset = mptt.issuanceID();
             mptt.authorize({.account = owner});
             mptt.authorize({.account = depositor});
@@ -2479,6 +2483,53 @@ class Vault_test : public beast::unit_test::suite
             env(tx2, ter{tecWRONG_ASSET});
             env.close();
         }
+
+        testCase([this](
+                     Env& env,
+                     Account const&,
+                     Account const& owner,
+                     Account const& depositor,
+                     PrettyAsset const& asset,
+                     Vault& vault,
+                     MPTTester& mptt) {
+            testcase("MPT non-transferable");
+
+            auto [tx, keylet] = vault.create({.owner = owner, .asset = asset});
+            env(tx);
+            env.close();
+
+            tx = vault.deposit(
+                {.depositor = depositor,
+                 .id = keylet.key,
+                 .amount = asset(100)});
+            env(tx);
+            env.close();
+
+            // Remove CanTransfer
+            mptt.set({.mutableFlags = tmfMPTClearCanTransfer});
+            env.close();
+
+            env(tx, ter{tecNO_AUTH});
+            env.close();
+
+            tx = vault.withdraw(
+                {.depositor = depositor,
+                 .id = keylet.key,
+                 .amount = asset(100)});
+
+            env(tx, ter{tecNO_AUTH});
+            env.close();
+
+            // Restore CanTransfer
+            mptt.set({.mutableFlags = tmfMPTSetCanTransfer});
+            env.close();
+
+            env(tx);
+            env.close();
+
+            // Delete vault with zero balance
+            env(vault.del({.owner = owner, .id = keylet.key}));
+        });
     }
 
     void
@@ -2491,6 +2542,7 @@ class Vault_test : public beast::unit_test::suite
             int initialXRP = 1000;
             Number initialIOU = 200;
             double transferRate = 1.0;
+            bool charlieRipple = true;
         };
 
         auto testCase =
@@ -2511,13 +2563,27 @@ class Vault_test : public beast::unit_test::suite
                 Account const charlie{"charlie"};
                 Vault vault{env};
                 env.fund(XRP(args.initialXRP), issuer, owner, charlie);
+                env(fset(issuer, asfDefaultRipple));
                 env(fset(issuer, asfAllowTrustLineClawback));
                 env.close();
 
                 PrettyAsset const asset = issuer["IOU"];
                 env.trust(asset(1000), owner);
-                env.trust(asset(1000), charlie);
                 env(pay(issuer, owner, asset(args.initialIOU)));
+                env.close();
+                if (!args.charlieRipple)
+                {
+                    env(fset(issuer, 0, asfDefaultRipple));
+                    env.close();
+                    env.trust(asset(1000), charlie);
+                    env.close();
+                    env(pay(issuer, charlie, asset(args.initialIOU)));
+                    env.close();
+                    env(fset(issuer, asfDefaultRipple));
+                }
+                else
+                    env.trust(asset(1000), charlie);
+                env.close();
                 env(rate(issuer, args.transferRate));
                 env.close();
 
@@ -2895,6 +2961,89 @@ class Vault_test : public beast::unit_test::suite
             env(tx1);
         });
 
+        testCase(
+            [&, this](
+                Env& env,
+                Account const& owner,
+                Account const& issuer,
+                Account const& charlie,
+                auto,
+                Vault& vault,
+                PrettyAsset const& asset,
+                std::function<MPTID(ripple::Keylet)> issuanceId) {
+                testcase("IOU non-transferable");
+
+                auto [tx, keylet] =
+                    vault.create({.owner = owner, .asset = asset});
+                tx[sfScale] = 0;
+                env(tx);
+                env.close();
+
+                {
+                    // Charlie cannot deposit
+                    auto tx = vault.deposit(
+                        {.depositor = charlie,
+                         .id = keylet.key,
+                         .amount = asset(100)});
+                    env(tx, ter{terNO_RIPPLE});
+                    env.close();
+                }
+
+                {
+                    PrettyAsset shares = issuanceId(keylet);
+                    auto tx1 = vault.deposit(
+                        {.depositor = owner,
+                         .id = keylet.key,
+                         .amount = asset(100)});
+                    env(tx1);
+                    env.close();
+
+                    // Charlie cannot receive funds
+                    auto tx2 = vault.withdraw(
+                        {.depositor = owner,
+                         .id = keylet.key,
+                         .amount = shares(100)});
+                    tx2[sfDestination] = charlie.human();
+                    env(tx2, ter{terNO_RIPPLE});
+                    env.close();
+
+                    {
+                        // Create MPToken for shares held by Charlie
+                        Json::Value tx{Json::objectValue};
+                        tx[sfAccount] = charlie.human();
+                        tx[sfMPTokenIssuanceID] =
+                            to_string(shares.raw().get<MPTIssue>().getMptID());
+                        tx[sfTransactionType] = jss::MPTokenAuthorize;
+                        env(tx);
+                        env.close();
+                    }
+                    env(pay(owner, charlie, shares(100)));
+                    env.close();
+
+                    // Charlie cannot withdraw
+                    auto tx3 = vault.withdraw(
+                        {.depositor = charlie,
+                         .id = keylet.key,
+                         .amount = shares(100)});
+                    env(tx3, ter{terNO_RIPPLE});
+                    env.close();
+
+                    env(pay(charlie, owner, shares(100)));
+                    env.close();
+                }
+
+                tx = vault.withdraw(
+                    {.depositor = owner,
+                     .id = keylet.key,
+                     .amount = asset(100)});
+                env(tx);
+                env.close();
+
+                // Delete vault with zero balance
+                env(vault.del({.owner = owner, .id = keylet.key}));
+            },
+            {.charlieRipple = false});
+
         testCase(
             [&, this](
                 Env& env,
@@ -3211,6 +3360,7 @@ class Vault_test : public beast::unit_test::suite
             credIssuer1,
             credIssuer2);
         env.close();
+        env(fset(issuer, asfDefaultRipple));
         env(fset(issuer, asfAllowTrustLineClawback));
         env.close();
         env.require(flags(issuer, asfAllowTrustLineClawback));
@@ -3640,6 +3790,7 @@ class Vault_test : public beast::unit_test::suite
             Account const depositor{"depositor"};
             Vault vault{env};
             env.fund(XRP(1000), issuer, owner, depositor);
+            env(fset(issuer, asfDefaultRipple));
             env(fset(issuer, asfAllowTrustLineClawback));
             env.close();
 

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

@@ -52,13 +52,16 @@ LoanBrokerCoverDeposit::preclaim(PreclaimContext const& ctx)
         JLOG(ctx.j.fatal()) << "Vault is missing for Broker " << brokerID;
         return tefBAD_LEDGER;
     }
-    auto const vaultAsset = vault->at(sfAsset);
 
+    auto const vaultAsset = vault->at(sfAsset);
     if (amount.asset() != vaultAsset)
         return tecWRONG_ASSET;
 
     auto const pseudoAccountID = sleBroker->at(sfAccount);
-
+    // Cannot transfer a non-transferable Asset
+    if (auto const ret =
+            canTransfer(ctx.view, vaultAsset, account, pseudoAccountID))
+        return ret;
     // Cannot transfer a frozen Asset
     if (auto const ret = checkFrozen(ctx.view, account, vaultAsset))
         return ret;

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

@@ -60,14 +60,21 @@ LoanBrokerCoverWithdraw::preclaim(PreclaimContext const& ctx)
     auto const vault = ctx.view.read(keylet::vault(sleBroker->at(sfVaultID)));
     if (!vault)
         return tefBAD_LEDGER;  // LCOV_EXCL_LINE
-    auto const vaultAsset = vault->at(sfAsset);
 
+    auto const vaultAsset = vault->at(sfAsset);
     if (amount.asset() != vaultAsset)
         return tecWRONG_ASSET;
 
+    // The broker's pseudo-account is the source of funds.
+    auto const pseudoAccountID = sleBroker->at(sfAccount);
+    // Cannot transfer a non-transferable Asset
+    if (auto const ret =
+            canTransfer(ctx.view, vaultAsset, pseudoAccountID, dstAcct))
+        return ret;
+
     // Withdrawal to a 3rd party destination account is essentially a transfer.
     // Enforce all the usual asset transfer checks.
-    AuthType authType = AuthType::Legacy;
+    AuthType authType = AuthType::WeakAuth;
     if (account != dstAcct)
     {
         if (auto const ret = canWithdraw(ctx.view, tx))
@@ -82,9 +89,6 @@ LoanBrokerCoverWithdraw::preclaim(PreclaimContext const& ctx)
     if (auto const ter = requireAuth(ctx.view, vaultAsset, dstAcct, authType))
         return ter;
 
-    // The broker's pseudo-account is the source of funds.
-    auto const pseudoAccountID = sleBroker->at(sfAccount);
-
     // Check for freezes, unless sending directly to the issuer
     if (dstAcct != vaultAsset.getIssuer())
     {
@@ -146,8 +150,16 @@ LoanBrokerCoverWithdraw::doApply()
     broker->at(sfCoverAvailable) -= amount;
     view().update(broker);
 
-    // Move the funds from the broker's pseudo-account to the dstAcct
+    // Create trust line or MPToken for the receiving account
+    if (dstAcct == account_)
+    {
+        if (auto const ter = addEmptyHolding(
+                view(), account_, mPriorBalance, amount.asset(), j_);
+            !isTesSuccess(ter) && ter != tecDUPLICATE)
+            return ter;
+    }
 
+    // Move the funds from the broker's pseudo-account to the dstAcct
     if (dstAcct == account_ || amount.native())
     {
         // Transfer assets directly from pseudo-account to depositor.

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

@@ -36,41 +36,19 @@ VaultDeposit::preclaim(PreclaimContext const& ctx)
     if (!vault)
         return tecNO_ENTRY;
 
-    auto const account = ctx.tx[sfAccount];
+    auto const& account = ctx.tx[sfAccount];
     auto const assets = ctx.tx[sfAmount];
     auto const vaultAsset = vault->at(sfAsset);
     if (assets.asset() != vaultAsset)
         return tecWRONG_ASSET;
 
-    if (vaultAsset.native())
-        ;  // No special checks for XRP
-    else if (vaultAsset.holds<MPTIssue>())
+    auto const& vaultAccount = vault->at(sfAccount);
+    if (auto ter = canTransfer(ctx.view, vaultAsset, account, vaultAccount);
+        !isTesSuccess(ter))
     {
-        auto mptID = vaultAsset.get<MPTIssue>().getMptID();
-        auto issuance = ctx.view.read(keylet::mptIssuance(mptID));
-        if (!issuance)
-            return tecOBJECT_NOT_FOUND;
-        if (!issuance->isFlag(lsfMPTCanTransfer))
-        {
-            // LCOV_EXCL_START
-            JLOG(ctx.j.error())
-                << "VaultDeposit: vault assets are non-transferable.";
-            return tecNO_AUTH;
-            // LCOV_EXCL_STOP
-        }
-    }
-    else if (vaultAsset.holds<Issue>())
-    {
-        auto const issuer =
-            ctx.view.read(keylet::account(vaultAsset.getIssuer()));
-        if (!issuer)
-        {
-            // LCOV_EXCL_START
-            JLOG(ctx.j.error())
-                << "VaultDeposit: missing issuer of vault assets.";
-            return tefINTERNAL;
-            // LCOV_EXCL_STOP
-        }
+        JLOG(ctx.j.debug())
+            << "VaultDeposit: vault assets are non-transferable.";
+        return ter;
     }
 
     auto const mptIssuanceID = vault->at(sfShareMPTID);

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

@@ -47,35 +47,15 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx)
     if (assets.asset() != vaultAsset && assets.asset() != vaultShare)
         return tecWRONG_ASSET;
 
-    if (vaultAsset.native())
-        ;  // No special checks for XRP
-    else if (vaultAsset.holds<MPTIssue>())
+    auto const& vaultAccount = vault->at(sfAccount);
+    auto const& account = ctx.tx[sfAccount];
+    auto const& dstAcct = ctx.tx[~sfDestination].value_or(account);
+    if (auto ter = canTransfer(ctx.view, vaultAsset, vaultAccount, dstAcct);
+        !isTesSuccess(ter))
     {
-        auto mptID = vaultAsset.get<MPTIssue>().getMptID();
-        auto issuance = ctx.view.read(keylet::mptIssuance(mptID));
-        if (!issuance)
-            return tecOBJECT_NOT_FOUND;
-        if (!issuance->isFlag(lsfMPTCanTransfer))
-        {
-            // LCOV_EXCL_START
-            JLOG(ctx.j.error())
-                << "VaultWithdraw: vault assets are non-transferable.";
-            return tecNO_AUTH;
-            // LCOV_EXCL_STOP
-        }
-    }
-    else if (vaultAsset.holds<Issue>())
-    {
-        auto const issuer =
-            ctx.view.read(keylet::account(vaultAsset.getIssuer()));
-        if (!issuer)
-        {
-            // LCOV_EXCL_START
-            JLOG(ctx.j.error())
-                << "VaultWithdraw: missing issuer of vault assets.";
-            return tefINTERNAL;
-            // LCOV_EXCL_STOP
-        }
+        JLOG(ctx.j.debug())
+            << "VaultWithdraw: vault assets are non-transferable.";
+        return ter;
     }
 
     // Enforce valid withdrawal policy
@@ -87,9 +67,6 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx)
         // LCOV_EXCL_STOP
     }
 
-    auto const account = ctx.tx[sfAccount];
-    auto const dstAcct = ctx.tx[~sfDestination].value_or(account);
-
     if (auto const ret = canWithdraw(ctx.view, ctx.tx))
         return ret;
 
@@ -259,9 +236,9 @@ VaultWithdraw::doApply()
     }
 
     auto const dstAcct = ctx_.tx[~sfDestination].value_or(account_);
-    if (!vaultAsset.native() &&               //
-        dstAcct != vaultAsset.getIssuer() &&  //
-        dstAcct == account_)
+
+    // Create trust line or MPToken for the receiving account
+    if (dstAcct == account_)
     {
         if (auto const ter = addEmptyHolding(
                 view(), account_, mPriorBalance, vaultAsset, j_);