Compare commits

...

3 Commits

Author SHA1 Message Date
Vito
cdecd5d36d Merge origin/develop; replace fixAssortedFixes with fixSecurity3_1_3 2026-03-26 10:41:50 +01:00
Vito
8d3f7e2552 fix: Address review feedback for vault withdraw shares
Move sleIssuance ledger read inside the fixAssortedFixes branch
where it is actually needed, avoiding an unnecessary read on the
pre-amendment and asset-denominated paths. Reuse existing account
and dstAcct variables instead of recomputing from and to. Add
pre-amendment test to verify the old behavior (share-denominated
withdrawal bypasses trustline limit check) alongside the post-fix
behavior. Fix stale comments in the test.
2026-03-25 13:48:25 +01:00
Vito
f92277cb3d fix: Check trustline limits for share-denominated vault withdrawals
VaultWithdraw preclaim skipped the canWithdraw trustline limit check
when the withdrawal amount was specified in shares (MPT) rather than
assets. Convert shares to the equivalent asset amount before calling
canWithdraw so that the destination's trustline limit is enforced
regardless of denomination.

Gated behind fixAssortedFixes for ledger replay compatibility.
2026-03-25 12:31:39 +01:00
2 changed files with 128 additions and 4 deletions

View File

@@ -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,35 @@ 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 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 maybeAssets = sharesToAssetsWithdraw(vault, sleIssuance, amount);
if (!maybeAssets)
return tecINTERNAL; // LCOV_EXCL_LINE
if (auto const ret = canWithdraw(
ctx.view, account, dstAcct, *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().

View File

@@ -5230,6 +5230,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 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<Issue>());
// 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<Issue>());
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
@@ -5250,6 +5346,7 @@ public:
testVaultClawbackBurnShares();
testVaultClawbackAssets();
testAssetsMaximum();
testBug6_LimitBypassWithShares();
}
};