From 02fa55df8d13be61d2d66e5c1905a0883d404d7d Mon Sep 17 00:00:00 2001 From: Vito Tumas <5780819+Tapanito@users.noreply.github.com> Date: Wed, 1 Apr 2026 21:31:45 +0200 Subject: [PATCH] fix: Check trustline limits for share-denominated vault withdrawals (#6645) --- .../tx/transactors/vault/VaultWithdraw.cpp | 53 +++++++++- src/test/app/Vault_test.cpp | 97 +++++++++++++++++++ 2 files changed, 146 insertions(+), 4 deletions(-) diff --git a/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp b/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp index e4c671b462..f6b8c98788 100644 --- a/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp +++ b/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp @@ -43,10 +43,10 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx) if (!vault) return tecNO_ENTRY; - auto const assets = ctx.tx[sfAmount]; + auto const amount = ctx.tx[sfAmount]; auto const vaultAsset = vault->at(sfAsset); auto const vaultShare = vault->at(sfShareMPTID); - if (assets.asset() != vaultAsset && assets.asset() != vaultShare) + if (amount.asset() != vaultAsset && amount.asset() != vaultShare) return tecWRONG_ASSET; auto const& vaultAccount = vault->at(sfAccount); @@ -67,8 +67,53 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx) // LCOV_EXCL_STOP } - if (auto const ret = canWithdraw(ctx.view, ctx.tx)) - return ret; + if (ctx.view.rules().enabled(fixSecurity3_1_3) && amount.asset() == vaultShare) + { + // Post-fixSecurity3_1_3: if the user specified shares, convert + // to the equivalent asset amount before checking withdrawal + // limits. Pre-amendment the limit check was skipped for + // share-denominated withdrawals. + auto const sleIssuance = ctx.view.read(keylet::mptIssuance(vaultShare)); + if (!sleIssuance) + { + // LCOV_EXCL_START + JLOG(ctx.j.error()) << "VaultWithdraw: missing issuance of vault shares."; + return tefINTERNAL; + // LCOV_EXCL_STOP + } + + try + { + auto const maybeAssets = sharesToAssetsWithdraw(vault, sleIssuance, amount); + if (!maybeAssets) + return tefINTERNAL; // LCOV_EXCL_LINE + + if (auto const ret = canWithdraw( + ctx.view, + account, + dstAcct, + *maybeAssets, + ctx.tx.isFieldPresent(sfDestinationTag))) + return ret; + } + catch (std::overflow_error const&) + { + // It's easy to hit this exception from Number with large enough Scale + // so we avoid spamming the log and only use debug here. + JLOG(ctx.j.debug()) // + << "VaultWithdraw: overflow error with" + << " scale=" << (int)vault->at(sfScale) // + << ", assetsTotal=" << vault->at(sfAssetsTotal) + << ", sharesTotal=" << sleIssuance->at(sfOutstandingAmount) + << ", amount=" << amount.value(); + return tecPATH_DRY; + } + } + else + { + if (auto const ret = canWithdraw(ctx.view, ctx.tx)) + return ret; + } // If sending to Account (i.e. not a transfer), we will also create (only // if authorized) a trust line or MPToken as needed, in doApply(). diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index 50417305b4..823cf7aafd 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -5231,6 +5231,102 @@ class Vault_test : public beast::unit_test::suite } } + // Reproduction: canWithdraw IOU limit check bypassed when + // withdrawal amount is specified in shares (MPT) rather than in assets. + void + testBug6_LimitBypassWithShares() + { + using namespace test::jtx; + testcase("Bug6 - limit bypass with share-denominated withdrawal"); + + auto const allAmendments = testable_amendments() | featureSingleAssetVault; + + for (auto const& features : {allAmendments, allAmendments - fixSecurity3_1_3}) + { + bool const withFix = features[fixSecurity3_1_3]; + + Env env{*this, features}; + Account const owner{"owner"}; + Account const issuer{"issuer"}; + Account const depositor{"depositor"}; + Account const charlie{"charlie"}; + Vault const vault{env}; + + env.fund(XRP(1000), issuer, owner, depositor, charlie); + env(fset(issuer, asfAllowTrustLineClawback)); + env.close(); + + PrettyAsset const asset = issuer["IOU"]; + env.trust(asset(1000), owner); + env.trust(asset(1000), depositor); + env(pay(issuer, owner, asset(200))); + env(pay(issuer, depositor, asset(200))); + env.close(); + + // Charlie gets a LOW trustline limit of 5 + env.trust(asset(5), charlie); + env.close(); + + auto const [tx, keylet] = vault.create({.owner = owner, .asset = asset}); + env(tx); + env.close(); + + auto const depositTx = + vault.deposit({.depositor = depositor, .id = keylet.key, .amount = asset(100)}); + env(depositTx); + env.close(); + + // Get the share MPT info + auto const vaultSle = env.le(keylet); + if (!BEAST_EXPECT(vaultSle)) + return; + auto const mptIssuanceID = vaultSle->at(sfShareMPTID); + MPTIssue const shares(mptIssuanceID); + PrettyAsset const share(shares); + + // CONTROL: Withdraw 10 IOU (asset-denominated) to charlie. + // Charlie's limit is 5, so this should be rejected with tecNO_LINE + // regardless of the amendment. + { + auto withdrawTx = + vault.withdraw({.depositor = depositor, .id = keylet.key, .amount = asset(10)}); + withdrawTx[sfDestination] = charlie.human(); + env(withdrawTx, ter{tecNO_LINE}); + env.close(); + } + auto const charlieBalanceBefore = env.balance(charlie, asset.raw().get()); + + // Withdraw the equivalent amount in shares to charlie. + // Post-fix: rejected (tecNO_LINE) because the share amount is + // converted to assets and the trustline limit is checked. + // Pre-fix: succeeds (tesSUCCESS) because the limit check was + // skipped for share-denominated withdrawals. + { + auto withdrawTx = vault.withdraw( + {.depositor = depositor, + .id = keylet.key, + .amount = STAmount(share, 10'000'000)}); + withdrawTx[sfDestination] = charlie.human(); + env(withdrawTx, ter{withFix ? TER{tecNO_LINE} : TER{tesSUCCESS}}); + env.close(); + + auto const charlieBalanceAfter = env.balance(charlie, asset.raw().get()); + if (withFix) + { + // Post-fix: charlie's balance is unchanged — the withdrawal + // was correctly rejected despite being share-denominated. + BEAST_EXPECT(charlieBalanceAfter == charlieBalanceBefore); + } + else + { + // Pre-fix: charlie received the assets, bypassing the + // trustline limit. + BEAST_EXPECT(charlieBalanceAfter > charlieBalanceBefore); + } + } + } + } + public: void run() override @@ -5251,6 +5347,7 @@ public: testVaultClawbackBurnShares(); testVaultClawbackAssets(); testAssetsMaximum(); + testBug6_LimitBypassWithShares(); } };