From d6d07e6fcf969de0af02e45c90e244ed6ed546c0 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Mon, 14 Apr 2025 17:12:31 +0100 Subject: [PATCH] Fix authorization issues --- src/test/app/Vault_test.cpp | 153 ++++++++++++++++++++++- src/xrpld/app/tx/detail/VaultDeposit.cpp | 53 +++++--- src/xrpld/ledger/detail/View.cpp | 58 +++++---- 3 files changed, 217 insertions(+), 47 deletions(-) diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index 478803dd62..4343e7d104 100644 --- a/src/test/app/Vault_test.cpp +++ b/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 { + 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 { 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(); diff --git a/src/xrpld/app/tx/detail/VaultDeposit.cpp b/src/xrpld/app/tx/detail/VaultDeposit.cpp index 2226b85330..70f38932f2 100644 --- a/src/xrpld/app/tx/detail/VaultDeposit.cpp +++ b/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() ? 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. diff --git a/src/xrpld/ledger/detail/View.cpp b/src/xrpld/ledger/detail/View.cpp index e90360620b..60e766a462 100644 --- a/src/xrpld/ledger/detail/View.cpp +++ b/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( - [&](TIss const& issue) { - if constexpr (std::is_same_v) - return requireAuth(view, issue, account); - else - return requireAuth(view, issue, account, depth + 1); - }, - asset.value()); + if (auto const err = std::visit( + [&](TIss const& issue) { + if constexpr (std::is_same_v) + 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;