From 596365d05d9cda37f525f3b1514c02def6f0101e Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Mon, 17 Nov 2025 00:32:21 -0500 Subject: [PATCH] fixup! Rip out about half the code: levels, enforcement, and STAmount changes --- include/xrpl/basics/Number.h | 1 - include/xrpl/protocol/Asset.h | 9 ++- include/xrpl/protocol/MPTAmount.h | 2 +- include/xrpl/protocol/STAmount.h | 3 +- include/xrpl/protocol/STNumber.h | 12 ---- src/libxrpl/protocol/STNumber.cpp | 18 ------ src/test/app/Vault_test.cpp | 42 +++++++++++--- src/test/basics/Number_test.cpp | 11 ++-- src/xrpld/app/tx/detail/VaultClawback.cpp | 14 ++--- src/xrpld/app/tx/detail/VaultDeposit.cpp | 68 ++++++++++++++++++++--- src/xrpld/app/tx/detail/VaultWithdraw.cpp | 11 ++-- 11 files changed, 123 insertions(+), 68 deletions(-) diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index 6632a456ab..314754371a 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -24,7 +24,6 @@ isPowerOfTen(T value) class Number { -private: using rep = std::int64_t; rep mantissa_{0}; int exponent_{std::numeric_limits::lowest()}; diff --git a/include/xrpl/protocol/Asset.h b/include/xrpl/protocol/Asset.h index 0a7f41880a..7ad1e70256 100644 --- a/include/xrpl/protocol/Asset.h +++ b/include/xrpl/protocol/Asset.h @@ -81,7 +81,14 @@ public: bool native() const { - return holds() && get().native(); + return std::visit( + [&](TIss const& issue) { + if constexpr (std::is_same_v) + return issue.native(); + if constexpr (std::is_same_v) + return false; + }, + issue_); } bool diff --git a/include/xrpl/protocol/MPTAmount.h b/include/xrpl/protocol/MPTAmount.h index 3ca1718cdc..46fa4e98b9 100644 --- a/include/xrpl/protocol/MPTAmount.h +++ b/include/xrpl/protocol/MPTAmount.h @@ -62,7 +62,7 @@ public: explicit constexpr operator bool() const noexcept; - operator Number() const + operator Number() const noexcept { return {value(), true}; } diff --git a/include/xrpl/protocol/STAmount.h b/include/xrpl/protocol/STAmount.h index 85e1fd4a99..e8eded314f 100644 --- a/include/xrpl/protocol/STAmount.h +++ b/include/xrpl/protocol/STAmount.h @@ -552,7 +552,6 @@ STAmount::operator=(Number const& number) mValue = mIsNegative ? -number.mantissa() : number.mantissa(); mOffset = number.exponent(); canonicalize(); - return *this; } @@ -568,7 +567,7 @@ STAmount::clear() { // The -100 is used to allow 0 to sort less than a small positive values // which have a negative exponent. - mOffset = integral() ? 0 : -100; + mOffset = native() ? 0 : -100; mValue = 0; mIsNegative = false; } diff --git a/include/xrpl/protocol/STNumber.h b/include/xrpl/protocol/STNumber.h index 2d9be9d91e..2ec3d66fd1 100644 --- a/include/xrpl/protocol/STNumber.h +++ b/include/xrpl/protocol/STNumber.h @@ -56,18 +56,6 @@ public: bool isDefault() const override; - /// Sets the flag on the underlying number - void - setIsInteger(bool isInteger); - - /// Gets the flag value on the underlying number - bool - isInteger() const noexcept; - - /// Checks the underlying number - bool - valid() const noexcept; - operator Number() const { return value_; diff --git a/src/libxrpl/protocol/STNumber.cpp b/src/libxrpl/protocol/STNumber.cpp index c9aebaee58..c353f6b795 100644 --- a/src/libxrpl/protocol/STNumber.cpp +++ b/src/libxrpl/protocol/STNumber.cpp @@ -94,24 +94,6 @@ STNumber::isDefault() const return value_ == Number(); } -void -STNumber::setIsInteger(bool isInteger) -{ - value_.setIsInteger(isInteger); -} - -bool -STNumber::isInteger() const noexcept -{ - return value_.isInteger(); -} - -bool -STNumber::valid() const noexcept -{ - return value_.valid(); -} - std::ostream& operator<<(std::ostream& out, STNumber const& rhs) { diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index 37ceffe5b2..3cb8991a33 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -3684,7 +3684,7 @@ class Vault_test : public beast::unit_test::suite }); testCase(18, [&, this](Env& env, Data d) { - testcase("MPT scale deposit overflow"); + testcase("MPT scale deposit over maxIntValue"); // The computed number of shares can not be represented as an MPT // without truncation @@ -3699,7 +3699,7 @@ class Vault_test : public beast::unit_test::suite }); testCase(13, [&, this](Env& env, Data d) { - testcase("MPT scale deposit overflow on first deposit"); + testcase("MPT scale deposit over maxIntValue on first deposit"); auto tx = d.vault.deposit( {.depositor = d.depositor, .id = d.keylet.key, @@ -3709,7 +3709,7 @@ class Vault_test : public beast::unit_test::suite }); testCase(13, [&, this](Env& env, Data d) { - testcase("MPT scale deposit overflow on second deposit"); + testcase("MPT scale deposit over maxIntValue on second deposit"); { auto tx = d.vault.deposit( @@ -3725,13 +3725,13 @@ class Vault_test : public beast::unit_test::suite {.depositor = d.depositor, .id = d.keylet.key, .amount = d.asset(10)}); - env(tx, ter{tecPATH_DRY}); + env(tx, ter{tecPRECISION_LOSS}); env.close(); } }); testCase(13, [&, this](Env& env, Data d) { - testcase("No MPT scale deposit overflow on total shares"); + testcase("MPT scale deposit over maxIntValue on total shares"); { auto tx = d.vault.deposit( @@ -3747,7 +3747,7 @@ class Vault_test : public beast::unit_test::suite {.depositor = d.depositor, .id = d.keylet.key, .amount = d.asset(5)}); - env(tx); + env(tx, ter(tecPRECISION_LOSS)); env.close(); } }); @@ -4051,7 +4051,7 @@ class Vault_test : public beast::unit_test::suite }); testCase(13, [&, this](Env& env, Data d) { - testcase("MPT scale withdraw overflow"); + testcase("MPT scale withdraw over maxIntValue"); { auto tx = d.vault.deposit( @@ -4063,11 +4063,22 @@ class Vault_test : public beast::unit_test::suite } { + // withdraws are allowed to be invalid... auto tx = d.vault.withdraw( {.depositor = d.depositor, .id = d.keylet.key, .amount = STAmount(d.asset, Number(10, 0))}); - env(tx, ter{tecPATH_DRY}); + env(tx, ter{tecINSUFFICIENT_FUNDS}); + env.close(); + } + + { + // ...but they are not allowed to be unrepresentable + auto tx = d.vault.withdraw( + {.depositor = d.depositor, + .id = d.keylet.key, + .amount = STAmount(d.asset, Number(1000, 0))}); + env(tx, ter{tecPRECISION_LOSS}); env.close(); } }); @@ -4304,14 +4315,27 @@ class Vault_test : public beast::unit_test::suite } { + // clawbacks are allowed to be invalid... auto tx = d.vault.clawback( {.issuer = d.issuer, .id = d.keylet.key, .holder = d.depositor, .amount = STAmount(d.asset, Number(10, 0))}); - env(tx, ter{tecPATH_DRY}); + env(tx); env.close(); } + + { + // ...but they are not allowed to be unrepresentable + auto tx = d.vault.clawback( + {.issuer = d.issuer, + .id = d.keylet.key, + .holder = d.depositor, + .amount = STAmount(d.asset, Number(1000, 0))}); + env(tx, ter{tecPRECISION_LOSS}); + env.close(); + } + }); testCase(1, [&, this](Env& env, Data d) { diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index bc0bbeab60..539ac8bf0e 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -2,7 +2,6 @@ #include #include #include -#include #include #include @@ -772,17 +771,21 @@ public: BEAST_EXPECT(a.isInteger()); BEAST_EXPECT(a.valid()); BEAST_EXPECT(a.representable()); - a *= Number{1, 16}; + a *= Number{1, 13}; BEAST_EXPECT(a.isInteger()); BEAST_EXPECT(!a.valid()); BEAST_EXPECT(a.representable()); - // Intermittent value rounding can be lost, but the result + a *= Number{1, 3}; + BEAST_EXPECT(a.isInteger()); + BEAST_EXPECT(!a.valid()); + BEAST_EXPECT(!a.representable()); + // Intermittent value precision can be lost, but the result // will be rounded, so that's fine. a /= Number{1, 5}; BEAST_EXPECT(a.isInteger()); BEAST_EXPECT(a.valid()); BEAST_EXPECT(a.representable()); - a = Number{1, 13} - 3; + a = Number{1, 14} - 3; BEAST_EXPECT(a.isInteger()); BEAST_EXPECT(a.valid()); BEAST_EXPECT(a.representable()); diff --git a/src/xrpld/app/tx/detail/VaultClawback.cpp b/src/xrpld/app/tx/detail/VaultClawback.cpp index 3dafa76629..4282877d1f 100644 --- a/src/xrpld/app/tx/detail/VaultClawback.cpp +++ b/src/xrpld/app/tx/detail/VaultClawback.cpp @@ -48,6 +48,8 @@ VaultClawback::preflight(PreflightContext const& ctx) << "VaultClawback: only asset issuer can clawback."; return temMALFORMED; } + else if (!amount->representableNumber()) + return temBAD_AMOUNT; } return tesSUCCESS; @@ -71,13 +73,8 @@ VaultClawback::preclaim(PreclaimContext const& ctx) } Asset const vaultAsset = vault->at(sfAsset); - if (auto const amount = ctx.tx[~sfAmount]) - { - if (vaultAsset != amount->asset()) - return tecWRONG_ASSET; - else if (!amount->validNumber()) - return tecPRECISION_LOSS; - } + if (auto const amount = ctx.tx[~sfAmount]; vaultAsset != amount->asset()) + return tecWRONG_ASSET; if (vaultAsset.native()) { @@ -199,6 +196,9 @@ VaultClawback::doApply() return tecINTERNAL; // LCOV_EXCL_LINE assetsRecovered = *maybeAssets; } + // Clawback amounts are allowed to be invalid, but not unrepresentable. + // Amounts over the "soft" limit help bring the numbers back into the + // valid range. if (!sharesDestroyed.representableNumber() || !assetsRecovered.representableNumber()) return tecPRECISION_LOSS; diff --git a/src/xrpld/app/tx/detail/VaultDeposit.cpp b/src/xrpld/app/tx/detail/VaultDeposit.cpp index e1d8e34dbd..878e018e95 100644 --- a/src/xrpld/app/tx/detail/VaultDeposit.cpp +++ b/src/xrpld/app/tx/detail/VaultDeposit.cpp @@ -23,7 +23,10 @@ VaultDeposit::preflight(PreflightContext const& ctx) return temMALFORMED; } - if (ctx.tx[sfAmount] <= beast::zero) + auto const amount = ctx.tx[sfAmount]; + if (amount <= beast::zero) + return temBAD_AMOUNT; + if (!amount.validNumber()) return temBAD_AMOUNT; return tesSUCCESS; @@ -42,9 +45,6 @@ VaultDeposit::preclaim(PreclaimContext const& ctx) if (assets.asset() != vaultAsset) return tecWRONG_ASSET; - if (!assets.validNumber()) - return tecPRECISION_LOSS; - if (vaultAsset.native()) ; // No special checks for XRP else if (vaultAsset.holds()) @@ -165,7 +165,8 @@ VaultDeposit::doApply() auto const amount = ctx_.tx[sfAmount]; // Make sure the depositor can hold shares. auto const mptIssuanceID = (*vault)[sfShareMPTID]; - auto const sleIssuance = view().read(keylet::mptIssuance(mptIssuanceID)); + SLE::const_pointer sleIssuance = + view().read(keylet::mptIssuance(mptIssuanceID)); if (!sleIssuance) { // LCOV_EXCL_START @@ -176,6 +177,7 @@ VaultDeposit::doApply() auto const& vaultAccount = vault->at(sfAccount); auto const& vaultAsset = vault->at(sfAsset); + // Note, vault owner is always authorized if (vault->isFlag(lsfVaultPrivate) && account_ != vault->at(sfOwner)) { @@ -248,8 +250,10 @@ VaultDeposit::doApply() } assetsDeposited = *maybeAssets; - if (!sharesCreated.representableNumber() || - !assetsDeposited.representableNumber()) + // Deposit needs to be more strict than the other Vault transactions + // that deal with asset <-> share calculations, because we don't + // want to go over the "soft" limit. + if (!sharesCreated.validNumber() || !assetsDeposited.validNumber()) return tecPRECISION_LOSS; } catch (std::overflow_error const&) @@ -257,7 +261,7 @@ VaultDeposit::doApply() // 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(j_.debug()) // - << "VaultDeposit: overflow error with" + << "VaultDeposit: overflow error computing shares with" << " scale=" << (int)vault->at(sfScale).value() // << ", assetsTotal=" << vault->at(sfAssetsTotal).value() << ", sharesTotal=" << sleIssuance->at(sfOutstandingAmount) @@ -280,7 +284,19 @@ VaultDeposit::doApply() assetsAvailableProxy += assetsDeposited; if (!assetsTotalProxy.value().valid() || !assetsAvailableProxy.value().valid()) - return tecLIMIT_EXCEEDED; + { + // 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(j_.warn()) // + << "VaultDeposit: integer overflow error in total assets with" + << " scale=" << (int)vault->at(sfScale).value() // + << ", assetsTotal=" << vault->at(sfAssetsTotal).value() + << ", sharesTotal=" << sleIssuance->at(sfOutstandingAmount) + << ", amount=" << amount; + + return tecPRECISION_LOSS; + } + view().update(vault); // A deposit must not push the vault over its limit. @@ -288,6 +304,9 @@ VaultDeposit::doApply() if (maximum != 0 && *assetsTotalProxy > maximum) return tecLIMIT_EXCEEDED; + // Reset the sleIssance ptr, since it's about to get invalidated + sleIssuance.reset(); + // Transfer assets from depositor to vault. if (auto const ter = accountSend( view(), @@ -325,6 +344,37 @@ VaultDeposit::doApply() !isTesSuccess(ter)) return ter; + { + // Load the updated issuance + sleIssuance = view().read(keylet::mptIssuance(mptIssuanceID)); + if (!sleIssuance) + { + // LCOV_EXCL_START + JLOG(j_.error()) + << "VaultDeposit: missing issuance of vault shares."; + return tefINTERNAL; + // LCOV_EXCL_STOP + } + + // Check if the deposit pushed the total over the integer Number limit. + // That is not a problem for the MPT itself, which is 64-bit, but for + // any computations that use it, such as converting assets to shares and + // vice-versa + STAmount const shareTotal{ + vault->at(sfShareMPTID), sleIssuance->at(sfOutstandingAmount)}; + if (!shareTotal.validNumber()) + { + JLOG(j_.warn()) // + << "VaultDeposit: integer overflow error in total shares with" + << " scale=" << (int)vault->at(sfScale).value() // + << ", assetsTotal=" << vault->at(sfAssetsTotal).value() + << ", sharesTotal=" << sleIssuance->at(sfOutstandingAmount) + << ", amount=" << amount; + + return tecPRECISION_LOSS; + } + } + return tesSUCCESS; } diff --git a/src/xrpld/app/tx/detail/VaultWithdraw.cpp b/src/xrpld/app/tx/detail/VaultWithdraw.cpp index 2423d7128a..e99a1af20d 100644 --- a/src/xrpld/app/tx/detail/VaultWithdraw.cpp +++ b/src/xrpld/app/tx/detail/VaultWithdraw.cpp @@ -20,7 +20,10 @@ VaultWithdraw::preflight(PreflightContext const& ctx) return temMALFORMED; } - if (ctx.tx[sfAmount] <= beast::zero) + auto const amount = ctx.tx[sfAmount]; + if (amount <= beast::zero) + return temBAD_AMOUNT; + if (!amount.representableNumber()) return temBAD_AMOUNT; if (auto const destination = ctx.tx[~sfDestination]; @@ -50,9 +53,6 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx) if (assets.asset() != vaultAsset && assets.asset() != vaultShare) return tecWRONG_ASSET; - if (!assets.validNumber()) - return tecPRECISION_LOSS; - if (vaultAsset.native()) ; // No special checks for XRP else if (vaultAsset.holds()) @@ -190,6 +190,9 @@ VaultWithdraw::doApply() } else return tefINTERNAL; // LCOV_EXCL_LINE + // Withdraw amounts are allowed to be invalid, but not unrepresentable. + // Amounts over the "soft" limit help bring the numbers back into the + // valid range. if (!sharesRedeemed.representableNumber() || !assetsWithdrawn.representableNumber()) return tecPRECISION_LOSS;