diff --git a/include/xrpl/ledger/View.h b/include/xrpl/ledger/View.h index ece574e52d..5e6fb8f5ce 100644 --- a/include/xrpl/ledger/View.h +++ b/include/xrpl/ledger/View.h @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -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( + [&](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. diff --git a/src/libxrpl/ledger/View.cpp b/src/libxrpl/ledger/View.cpp index 9b6a8723c5..88bffae413 100644 --- a/src/libxrpl/ledger/View.cpp +++ b/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, diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index 37490906ff..24cb63a3cf 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -5588,11 +5588,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; @@ -5612,7 +5612,7 @@ protected: env.close(); - PrettyAsset const asset = mpt["BUG"]; + PrettyAsset const asset = mpt["MPT"]; mpt.authorize({.account = alice}); env.close(); @@ -5646,21 +5646,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() { @@ -6855,10 +6892,10 @@ public: run() override { #if LOANTODO - testCoverDepositAllowsNonTransferableMPT(); testLoanPayLateFullPaymentBypassesPenalties(); testLoanCoverMinimumRoundingExploit(); #endif + testCoverDepositWithdrawNonTransferableMPT(); testPoC_UnsignedUnderflowOnFullPayAfterEarlyPeriodic(); testDisabled(); diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index a4a3fdd26a..499584a43d 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -2,6 +2,7 @@ #include #include #include +#include #include #include @@ -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}); @@ -2483,6 +2487,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 @@ -2495,6 +2546,7 @@ class Vault_test : public beast::unit_test::suite int initialXRP = 1000; Number initialIOU = 200; double transferRate = 1.0; + bool charlieRipple = true; }; auto testCase = @@ -2515,13 +2567,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(); @@ -2899,6 +2965,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 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().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, @@ -3217,6 +3366,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)); @@ -3646,6 +3796,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(); diff --git a/src/xrpld/app/tx/detail/LoanBrokerCoverDeposit.cpp b/src/xrpld/app/tx/detail/LoanBrokerCoverDeposit.cpp index 25075582c5..974bb36a93 100644 --- a/src/xrpld/app/tx/detail/LoanBrokerCoverDeposit.cpp +++ b/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; diff --git a/src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp b/src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp index aebda09e0b..58204b70d5 100644 --- a/src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp +++ b/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. diff --git a/src/xrpld/app/tx/detail/VaultDeposit.cpp b/src/xrpld/app/tx/detail/VaultDeposit.cpp index 3e5ae741e3..aeaf890126 100644 --- a/src/xrpld/app/tx/detail/VaultDeposit.cpp +++ b/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()) + auto const& vaultAccount = vault->at(sfAccount); + if (auto ter = canTransfer(ctx.view, vaultAsset, account, vaultAccount); + !isTesSuccess(ter)) { - auto mptID = vaultAsset.get().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()) - { - 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); diff --git a/src/xrpld/app/tx/detail/VaultWithdraw.cpp b/src/xrpld/app/tx/detail/VaultWithdraw.cpp index dc8cbde7c0..de84a372e7 100644 --- a/src/xrpld/app/tx/detail/VaultWithdraw.cpp +++ b/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()) + 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().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()) - { - 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_);