diff --git a/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp b/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp index a473ac7c36..e79c288792 100644 --- a/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp +++ b/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp @@ -41,10 +41,20 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx) if (!vault) return tecNO_ENTRY; - auto const assets = ctx.tx[sfAmount]; + auto const mptIssuanceID = vault->at(sfShareMPTID); + auto const sleIssuance = ctx.view.read(keylet::mptIssuance(mptIssuanceID)); + if (!sleIssuance) + { + // LCOV_EXCL_START + JLOG(ctx.j.error()) << "VaultWithdraw: missing issuance of vault shares."; + return tefINTERNAL; + // LCOV_EXCL_STOP + } + + 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); @@ -65,8 +75,26 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx) // LCOV_EXCL_STOP } - if (auto const ret = canWithdraw(ctx.view, ctx.tx)) - return ret; + if (amount.asset() == vaultShare) + { + // If the user specified shares, we need to first convert them to asset amount before + // checking whether they can be withdrawn + auto const maybeAssets = sharesToAssetsWithdraw(vault, sleIssuance, amount); + if (!maybeAssets) + return tecINTERNAL; // LCOV_EXCL_LINE + + auto const from = ctx.tx[sfAccount]; + auto const to = ctx.tx[~sfDestination].value_or(from); + + if (auto const ret = canWithdraw( + ctx.view, from, to, *maybeAssets, ctx.tx.isFieldPresent(sfDestinationTag))) + return ret; + } + 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 06f97972db..59235ac8df 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -5350,6 +5350,79 @@ 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"); + + Env env{*this, testable_amendments() | featureSingleAssetVault}; + Account const owner{"owner"}; + Account const issuer{"issuer"}; + Account const depositor{"depositor"}; + Account const charlie{"charlie"}; + Vault 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. + { + 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. This should also be rejected.< + { + auto withdrawTx = vault.withdraw( + {.depositor = depositor, .id = keylet.key, .amount = STAmount(share, 10'000'000)}); + withdrawTx[sfDestination] = charlie.human(); + env(withdrawTx, ter{tecNO_LINE}); + env.close(); + + // Verify that charlie received IOU beyond their trustline limit + // (their limit is 5, but they now hold 10). + auto const charlieBalanceAfter = env.balance(charlie, asset.raw().get()); + BEAST_EXPECT(charlieBalanceAfter == charlieBalanceBefore); + } + } + public: void run() override @@ -5370,6 +5443,7 @@ public: testVaultClawbackBurnShares(); testVaultClawbackAssets(); testAssetsMaximum(); + testBug6_LimitBypassWithShares(); } };