Fix authorization issues

src/test/app/Vault_test.cpp

@@ -1385,6 +1385,108 @@ class Vault_test : public beast::unit_test::suite
         }
     }
 
+    void
+    testWithDomainCheckXRP()
+    {
+        testcase("private XRP vault");
+
+        Env env{*this, supported_amendments() | featureSingleAssetVault};
+        Account owner{"owner"};
+        Account depositor{"depositor"};
+        Account alice{"charlie"};
+        std::string const credType = "credential";
+        Vault vault{env};
+        env.fund(XRP(100000), owner, depositor, alice);
+        env.close();
+
+        PrettyAsset asset = xrpIssue();
+        auto [tx, keylet] = vault.create(
+            {.owner = owner, .asset = asset, .flags = tfVaultPrivate});
+        env(tx);
+        env.close();
+
+        auto const [vaultAccount, issuanceId] =
+            [&env, keylet = keylet, this]() -> std::tuple<AccountID, uint192> {
+            auto const vault = env.le(keylet);
+            BEAST_EXPECT(vault != nullptr);
+            return {vault->at(sfAccount), vault->at(sfShareMPTID)};
+        }();
+        BEAST_EXPECT(env.le(keylet::account(vaultAccount)));
+        BEAST_EXPECT(env.le(keylet::mptIssuance(issuanceId)));
+        PrettyAsset shares{issuanceId};
+
+        {
+            testcase("private XRP vault owner can deposit");
+            auto tx = vault.deposit(
+                {.depositor = owner, .id = keylet.key, .amount = asset(50)});
+            env(tx);
+        }
+
+        {
+            testcase("private XRP vault cannot pay shares to depositor yet");
+            env(pay(owner, depositor, shares(1)), ter{tecNO_AUTH});
+        }
+
+        {
+            testcase("private XRP vault depositor not authorized yet");
+            auto tx = vault.deposit(
+                {.depositor = depositor,
+                 .id = keylet.key,
+                 .amount = asset(50)});
+            env(tx, ter{tecNO_AUTH});
+        }
+
+        {
+            testcase("private XRP vault set DomainID");
+            pdomain::Credentials const credentials{
+                {.issuer = owner, .credType = credType}};
+
+            env(pdomain::setTx(owner, credentials));
+            auto const domainId = [&]() {
+                auto tx = env.tx()->getJson(JsonOptions::none);
+                return pdomain::getNewDomain(env.meta());
+            }();
+
+            auto tx = vault.set({.owner = owner, .id = keylet.key});
+            tx[sfDomainID] = to_string(domainId);
+            env(tx);
+            env.close();
+        }
+
+        auto const credKeylet = credentials::keylet(depositor, owner, credType);
+        {
+            testcase("private XRP vault depositor now authorized");
+            env(credentials::create(depositor, owner, credType));
+            env(credentials::accept(depositor, owner, credType));
+            env.close();
+
+            BEAST_EXPECT(env.le(credKeylet));
+            auto tx = vault.deposit(
+                {.depositor = depositor,
+                 .id = keylet.key,
+                 .amount = asset(50)});
+            env(tx);
+            env.close();
+        }
+
+        {
+            testcase("private XRP vault can pay shares to depositor");
+            env(pay(owner, depositor, shares(1)));
+        }
+
+        {
+            testcase("private XRP vault cannot pay shares to 3rd party");
+            Json::Value jv;
+            jv[sfAccount] = alice.human();
+            jv[sfTransactionType] = jss::MPTokenAuthorize;
+            jv[sfMPTokenIssuanceID] = to_string(issuanceId);
+            env(jv);
+            env.close();
+
+            env(pay(owner, alice, shares(1)), ter{tecNO_AUTH});
+        }
+    }
+
     void
     testWithIOU()
     {
@@ -1409,10 +1511,11 @@ class Vault_test : public beast::unit_test::suite
         env(tx);
         env.close();
 
-        auto const vaultAccount = [&env, keylet = keylet, this]() -> AccountID {
+        auto const [vaultAccount, issuanceId] =
+            [&env, keylet = keylet, this]() -> std::tuple<AccountID, uint192> {
             auto const vault = env.le(keylet);
             BEAST_EXPECT(vault != nullptr);
-            return vault->at(sfAccount);
+            return {vault->at(sfAccount), vault->at(sfShareMPTID)};
         }();
 
         auto const share = [&env, keylet = keylet, this]() -> Asset {
@@ -1468,35 +1571,72 @@ class Vault_test : public beast::unit_test::suite
 
         {
             testcase("IOU froze trust line, cannot withdraw to 3rd party");
+            auto tx1 = test::jtx::pay(owner, charlie, STAmount{share, 10});
+            env(tx1, ter{tecNO_AUTH});
+            auto tx2 = test::jtx::pay(charlie, owner, STAmount{share, 10});
+            env(tx2, ter{tecNO_AUTH});
+            env.close();
+
             env(trust(issuer, asset(0), owner, tfSetFreeze));
             env.close();
 
+            // Since the vault is public, Charlie can simply create MPToken
+            // to gain authorization to receive its shares.
+            Json::Value jv;
+            jv[sfAccount] = charlie.human();
+            jv[sfTransactionType] = jss::MPTokenAuthorize;
+            jv[sfMPTokenIssuanceID] = to_string(issuanceId);
+            env(jv);
+            env.close();
+
             auto tx = vault.withdraw(
                 {.depositor = owner, .id = keylet.key, .amount = asset(10)});
             env(tx, ter{tecFROZEN});
 
             tx[sfDestination] = charlie.human();
             env(tx, ter{tecLOCKED});  // owner transitively locked via MPToken
-
-            auto tx1 = test::jtx::pay(owner, charlie, STAmount{share, 10});
             env(tx1, ter{tecLOCKED});
-
-            auto tx2 = test::jtx::pay(charlie, owner, STAmount{share, 10});
             env(tx2, ter{tecLOCKED});
             env.close();
 
             BEAST_EXPECT(env.balance(charlie, issue) == asset(20));
         }
 
+        {
+            testcase("IOU unfroze trust line, can withdraw or pay");
+            env(trust(issuer, asset(500), owner, tfClearFreeze));
+            env.close();
+
+            auto tx = vault.withdraw(
+                {.depositor = owner, .id = keylet.key, .amount = asset(1)});
+            env(tx);
+
+            tx[sfDestination] = charlie.human();
+            env(tx);
+
+            auto tx1 = test::jtx::pay(owner, charlie, STAmount{share, 1});
+            env(tx1);
+
+            auto tx2 = test::jtx::pay(charlie, owner, STAmount{share, 1});
+            env(tx2);
+        }
+
         {
             testcase("IOU global freeze");
             env(fset(issuer, asfGlobalFreeze));
+            env.close();
+
             auto tx = vault.withdraw(
                 {.depositor = owner, .id = keylet.key, .amount = asset(10)});
             env(tx, ter{tecFROZEN});
 
             tx[sfDestination] = issuer.human();
             env(tx, ter{tecFROZEN});
+
+            auto tx1 = test::jtx::pay(owner, charlie, STAmount{share, 10});
+            env(tx1, ter{tecLOCKED});
+            auto tx2 = test::jtx::pay(charlie, owner, STAmount{share, 10});
+            env(tx2, ter{tecLOCKED});
             env.close();
         }
     }
@@ -1916,6 +2056,7 @@ public:
         testWithMPT();
         testWithIOU();
         testWithDomainCheck();
+        testWithDomainCheckXRP();
         testNonTransferableShares();
         testFailedPseudoAccount();
         testRPC();

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

@@ -66,10 +66,15 @@ VaultDeposit::preclaim(PreclaimContext const& ctx)
     if (assets.asset() != vaultAsset)
         return tecWRONG_ASSET;
 
-    auto const share = MPTIssue(vault->at(sfShareMPTID));
+    auto const mptIssuanceID = vault->at(sfShareMPTID);
+    auto const share = MPTIssue(mptIssuanceID);
     if (share == assets.asset())
         return tefINTERNAL;
 
+    auto const sleIssuance = ctx.view.read(keylet::mptIssuance(mptIssuanceID));
+    if (!sleIssuance)
+        return tefINTERNAL;
+
     // Cannot deposit inside Vault an Asset frozen for the depositor
     if (isFrozen(ctx.view, account, vaultAsset))
         return vaultAsset.holds<Issue>() ? tecFROZEN : tecLOCKED;
@@ -80,17 +85,23 @@ VaultDeposit::preclaim(PreclaimContext const& ctx)
 
     if ((vault->getFlags() & tfVaultPrivate) && account != vault->at(sfOwner))
     {
-        // The authorization check below is based on DomainID stored in
-        // MPTokenIssuance. Had the vault shares been a regular MPToken, we
-        // would allow authorization granted by the issuer explicitly, but Vault
-        // does not have an MPT issuer (instead it uses pseudo-account, which is
-        // blackholed and cannot create any transactions).
-        //
-        // As per requireAuth documentation, we suppress tecEXPIRED error here,
-        // so we can delete any expired credentials inside doApply.
-        if (auto const ter = requireAuth(ctx.view, share, account);
-            !isTesSuccess(ter) && ter != tecEXPIRED)
-            return ter;
+        auto const maybeDomainID = sleIssuance->at(~sfDomainID);
+        // Since this is a private vault and the account is not its owner, we
+        // perform authorization check based on DomainID read from sleIssuance.
+        // Had the vault shares been a regular MPToken, we would allow
+        // authorization granted by the Issuer explicitly, but Vault uses Issuer
+        // pseudo-account, which cannot grant an authorization.
+        if (maybeDomainID)
+        {
+            // As per validDomain documentation, we suppress tecEXPIRED error
+            // here, so we can delete any expired credentials inside doApply.
+            if (auto const err =
+                    credentials::validDomain(ctx.view, *maybeDomainID, account);
+                !isTesSuccess(err) && err != tecEXPIRED)
+                return err;
+        }
+        else
+            return tecNO_AUTH;
     }
 
     if (auto const ter = std::visit(
@@ -158,13 +169,27 @@ VaultDeposit::doApply()
                     view(),
                     ctx_.journal,
                     {.priorBalance = mPriorBalance,
-                     // The operator-> gives the underlying STUInt192
-                     // whose value function returns a const&.
                      .mptIssuanceID = mptIssuanceID->value(),
                      .accountID = account_});
                 !isTesSuccess(err))
                 return err;
         }
+
+        // If the vault is private, set the authorized flag for the vault owner
+        if (vault->getFlags() & tfVaultPrivate)
+        {
+            if (auto const err = MPTokenAuthorize::authorize(
+                    view(),
+                    ctx_.journal,
+                    {
+                        .priorBalance = mPriorBalance,
+                        .mptIssuanceID = mptIssuanceID->value(),
+                        .accountID = sleIssuance->at(sfIssuer),
+                        .holderID = account_,
+                    });
+                !isTesSuccess(err))
+                return err;
+        }
     }
 
     // Compute exchange before transferring any amounts.

src/xrpld/ledger/detail/View.cpp

@@ -2275,42 +2275,46 @@ requireAuth(
                 return tecKILLED;  // VaultCreate looks for this error code
 
             auto const asset = sleVault->at(sfAsset);
-            return std::visit(
-                [&]<ValidIssueType TIss>(TIss const& issue) {
-                    if constexpr (std::is_same_v<TIss, Issue>)
-                        return requireAuth(view, issue, account);
-                    else
-                        return requireAuth(view, issue, account, depth + 1);
-                },
-                asset.value());
+            if (auto const err = std::visit(
+                    [&]<ValidIssueType TIss>(TIss const& issue) {
+                        if constexpr (std::is_same_v<TIss, Issue>)
+                            return requireAuth(view, issue, account);
+                        else
+                            return requireAuth(view, issue, account, depth + 1);
+                    },
+                    asset.value());
+                !isTesSuccess(err))
+                return err;
         }
     }
 
     auto const mptokenID = keylet::mptoken(mptID.key, account);
     auto const sleToken = view.read(mptokenID);
-    // Note, this is not amendment-gated because  we do not want to maintain in
-    // this file the list of all the amendments which can write to this field.
-    // This field is empty unless writing to it has been enabled by an amendment
+    // Note, this check is not amendment-gated because DomainID will be always
+    // empty **unless** writing to it has been enabled by an amendment
     auto const maybeDomainID = sleIssuance->at(~sfDomainID);
-    if (!maybeDomainID)
+    if (maybeDomainID)
     {
-        // if account has no MPToken, fail
-        if (!sleToken)
-            return tecNO_AUTH;
-
-        // mptoken must be authorized if issuance enabled requireAuth
-        if (sleIssuance->getFieldU32(sfFlags) & lsfMPTRequireAuth &&
-            !(sleToken->getFlags() & lsfMPTAuthorized))
-            return tecNO_AUTH;
-
-        return tesSUCCESS;
+        XRPL_ASSERT(
+            sleIssuance->getFieldU32(sfFlags) & lsfMPTRequireAuth,
+            "ripple::requireAuth issuance requires authorization");
+        // We ignore error from validDomain if we found sleToken, as it could
+        // belong to someone who is explicitly authorized e.g. a vault owner.
+        // err = tefINTERNAL | tecOBJECT_NOT_FOUND | tecNO_AUTH | tecEXPIRED
+        if (auto const ter =
+                credentials::validDomain(view, *maybeDomainID, account);
+            isTesSuccess(ter) || sleToken == nullptr)
+            return ter;
     }
 
-    // err = tefINTERNAL | tecOBJECT_NOT_FOUND | tecNO_AUTH | tecEXPIRED
-    if (auto const err =
-            credentials::validDomain(view, *maybeDomainID, account);
-        !isTesSuccess(err))
-        return err;
+    // if account has no MPToken, fail
+    if (!sleToken)
+        return tecNO_AUTH;
+
+    // mptoken must be authorized if issuance enabled requireAuth
+    if (sleIssuance->getFieldU32(sfFlags) & lsfMPTRequireAuth &&
+        !(sleToken->getFlags() & lsfMPTAuthorized))
+        return tecNO_AUTH;
 
     // We are authorized by permissioned domain.
     return tesSUCCESS;