diff --git a/src/libxrpl/tx/transactors/token/MPTokenAuthorize.cpp b/src/libxrpl/tx/transactors/token/MPTokenAuthorize.cpp index 0aeb6f33d1..c19b8f64d7 100644 --- a/src/libxrpl/tx/transactors/token/MPTokenAuthorize.cpp +++ b/src/libxrpl/tx/transactors/token/MPTokenAuthorize.cpp @@ -37,6 +37,7 @@ MPTokenAuthorize::preclaim(PreclaimContext const& ctx) { auto const accountID = ctx.tx[sfAccount]; auto const holderID = ctx.tx[~sfHolder]; + auto const sleMptIssuance = ctx.view.read(keylet::mptokenIssuance(ctx.tx[sfMPTokenIssuanceID])); // if non-issuer account submits this tx, then they are trying either: // 1. Unauthorize/delete MPToken @@ -51,9 +52,8 @@ MPTokenAuthorize::preclaim(PreclaimContext const& ctx) // There is an edge case where all holders have zero balance, issuance // is legally destroyed, then outstanding MPT(s) are deleted afterwards. - // Thus, there is no need to check for the existence of the issuance if - // the MPT is being deleted with a zero balance. Check for unauthorize - // before fetching the MPTIssuance object. + // Thus, the unauthorize/delete path below does not require the issuance + // to exist when the MPT is being deleted with a zero balance. // if holder wants to delete/unauthorize a mpt if (ctx.tx.isFlag(tfMPTUnauthorize)) @@ -63,8 +63,6 @@ MPTokenAuthorize::preclaim(PreclaimContext const& ctx) if ((*sleMpt)[sfMPTAmount] != 0) { - auto const sleMptIssuance = - ctx.view.read(keylet::mptokenIssuance(ctx.tx[sfMPTokenIssuanceID])); if (!sleMptIssuance) return tefINTERNAL; // LCOV_EXCL_LINE @@ -73,21 +71,24 @@ MPTokenAuthorize::preclaim(PreclaimContext const& ctx) if ((*sleMpt)[~sfLockedAmount].value_or(0) != 0) { - auto const sleMptIssuance = - ctx.view.read(keylet::mptokenIssuance(ctx.tx[sfMPTokenIssuanceID])); if (!sleMptIssuance) return tefINTERNAL; // LCOV_EXCL_LINE return tecHAS_OBLIGATIONS; } - if (ctx.view.rules().enabled(featureSingleAssetVault) && sleMpt->isFlag(lsfMPTLocked)) + if (ctx.view.rules().enabled(fixCleanup3_4_0)) + { + if (sleMptIssuance && sleMpt->isFlag(lsfMPTLocked)) + return tecNO_PERMISSION; + } + else if ( + ctx.view.rules().enabled(featureSingleAssetVault) && sleMpt->isFlag(lsfMPTLocked)) + { return tecNO_PERMISSION; + } if (ctx.view.rules().enabled(featureConfidentialTransfer)) { - auto const sleMptIssuance = - ctx.view.read(keylet::mptokenIssuance(ctx.tx[sfMPTokenIssuanceID])); - // if there still existing encrypted balances of MPT in // circulation if (sleMptIssuance && @@ -106,9 +107,6 @@ MPTokenAuthorize::preclaim(PreclaimContext const& ctx) } // Now test when the holder wants to hold/create/authorize a new MPT - auto const sleMptIssuance = - ctx.view.read(keylet::mptokenIssuance(ctx.tx[sfMPTokenIssuanceID])); - if (!sleMptIssuance) return tecOBJECT_NOT_FOUND; @@ -126,7 +124,6 @@ MPTokenAuthorize::preclaim(PreclaimContext const& ctx) if (!sleHolder) return tecNO_DST; - auto const sleMptIssuance = ctx.view.read(keylet::mptokenIssuance(ctx.tx[sfMPTokenIssuanceID])); if (!sleMptIssuance) return tecOBJECT_NOT_FOUND; diff --git a/src/test/app/MPToken_test.cpp b/src/test/app/MPToken_test.cpp index b392dca758..7086adf743 100644 --- a/src/test/app/MPToken_test.cpp +++ b/src/test/app/MPToken_test.cpp @@ -789,7 +789,7 @@ class MPToken_test : public beast::unit_test::Suite // locks up bob's mptoken again mptAlice.set({.account = alice, .holder = bob, .flags = tfMPTLock}); - if (!features[featureSingleAssetVault]) + if (!features[featureSingleAssetVault] && !features[fixCleanup3_4_0]) { // Delete bob's mptoken even though it is locked mptAlice.authorize({.account = bob, .flags = tfMPTUnauthorize}); @@ -7657,6 +7657,56 @@ class MPToken_test : public beast::unit_test::Suite 0, tecNO_PERMISSION, tecNO_PERMISSION, tecNO_PERMISSION, tecNO_PERMISSION); } + void + testLockedMPTokenDestroyedIssuance(FeatureBitset features) + { + testcase("Locked MPToken with destroyed issuance"); + + using namespace test::jtx; + Account const alice("alice"); // issuer + Account const bob("bob"); // holder + + Env env{*this, features}; + env.fund(XRP(1'000), alice, bob); + env.close(); + MPTTester mptAlice( + {.env = env, .issuer = alice, .holders = {bob}, .flags = kMptDexFlags | tfMPTCanLock}); + + // alice locks bob's mptoken individually + mptAlice.set({.account = alice, .holder = bob, .flags = tfMPTLock}); + + // alice destroys her issuance. This succeeds: MPTokenIssuanceDestroy + // only requires that the issuance has no outstanding balance; it does + // not require that all holder MPTokens have been deleted first. + mptAlice.destroy({.ownerCount = 0}); + + if (!features[featureSingleAssetVault] || features[fixCleanup3_4_0]) + { + // pre SAV or post Cleanup340 amendment: bob deletes the dangling locked MPToken + mptAlice.authorize({.account = bob, .holderCount = 0, .flags = tfMPTUnauthorize}); + BEAST_EXPECT(ownerCount(env, bob) == 0); + } + else + { + // bob cannot delete his locked MPToken, even though the issuance + // no longer exists. + mptAlice.authorize( + {.account = bob, .flags = tfMPTUnauthorize, .err = tecNO_PERMISSION}); + + // and the lock can never be cleared, because unlocking + // requires the (destroyed) issuance + mptAlice.set( + {.account = alice, + .holder = bob, + .flags = tfMPTUnlock, + .err = tecOBJECT_NOT_FOUND}); + + // the dangling locked MPToken survives + BEAST_EXPECT(env.current()->exists(keylet::mptoken(mptAlice.issuanceID(), bob.id()))); + BEAST_EXPECT(ownerCount(env, bob) == 1); + } + } + public: void run() override @@ -7703,7 +7753,9 @@ public: testSetValidation(all - featurePermissionedDomains); testSetValidation(all); + testSetEnabled(all - featureSingleAssetVault - fixCleanup3_4_0); testSetEnabled(all - featureSingleAssetVault); + testSetEnabled(all - fixCleanup3_4_0); testSetEnabled(all); // MPT clawback @@ -7770,6 +7822,10 @@ public: // Fixes testFixDoubleOwnerCount(all); + testLockedMPTokenDestroyedIssuance(all); + testLockedMPTokenDestroyedIssuance(all - fixCleanup3_4_0); + testLockedMPTokenDestroyedIssuance(all - featureSingleAssetVault); + testLockedMPTokenDestroyedIssuance(all - featureSingleAssetVault - fixCleanup3_4_0); } };