diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index 59f3a212a6..4f447d5f98 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -53,7 +53,8 @@ public: constexpr static rep maxMantissa = minMantissa * 10 - 1; static_assert(maxMantissa == 9'999'999'999'999'999LL); - constexpr static rep maxIntValue = minMantissa / 10; + constexpr static rep maxIntValue = maxMantissa / 10; + static_assert(maxIntValue == 999'999'999'999'999LL); // The range for the exponent when normalized constexpr static int minExponent = -32768; diff --git a/include/xrpl/protocol/Asset.h b/include/xrpl/protocol/Asset.h index d0efe814a9..6765f7cf7d 100644 --- a/include/xrpl/protocol/Asset.h +++ b/include/xrpl/protocol/Asset.h @@ -84,6 +84,12 @@ public: return holds() && get().native(); } + bool + integral() const + { + return !holds() || get().native(); + } + friend constexpr bool operator==(Asset const& lhs, Asset const& rhs); diff --git a/include/xrpl/protocol/MPTAmount.h b/include/xrpl/protocol/MPTAmount.h index 1c7c7a25e7..5f552d839b 100644 --- a/include/xrpl/protocol/MPTAmount.h +++ b/include/xrpl/protocol/MPTAmount.h @@ -62,11 +62,17 @@ public: explicit constexpr operator bool() const noexcept; - operator Number() const noexcept + operator Number() const { return {value(), Number::strong}; } + Number + toNumber(Number::EnforceInteger enforce) const + { + return {value(), enforce}; + } + /** Return the sign of the amount */ constexpr int signum() const noexcept; diff --git a/include/xrpl/protocol/STAmount.h b/include/xrpl/protocol/STAmount.h index 83493efcdd..d0092fdbec 100644 --- a/include/xrpl/protocol/STAmount.h +++ b/include/xrpl/protocol/STAmount.h @@ -40,6 +40,12 @@ private: exponent_type mOffset; bool mIsNegative; + // The Enforce integer setting is not stored or serialized. If set, it is + // used during automatic conversions to Number. If not set, the default + // behavior is used. It can also be overridden when coverting by using + // toNumber(). + std::optional enforceConversion_; + public: using value_type = STAmount; @@ -135,9 +141,28 @@ public: STAmount(A const& asset, int mantissa, int exponent = 0); template - STAmount(A const& asset, Number const& number) + STAmount( + A const& asset, + Number const& number, + std::optional enforce = std::nullopt) : STAmount(asset, number.mantissa(), number.exponent()) { + enforceConversion_ = enforce; + if (!enforce) + { + // Use the default conversion behavior + [[maybe_unused]] + Number const n = *this; + } + else if (enforce == Number::strong) + { + // Throw if it's not valid + if (!validNumber()) + { + Throw( + "STAmount::STAmount integer Number lost precision"); + } + } } // Legacy support for new-style amounts @@ -145,6 +170,17 @@ public: STAmount(XRPAmount const& amount); STAmount(MPTAmount const& amount, MPTIssue const& mptIssue); operator Number() const; + Number + toNumber(Number::EnforceInteger enforce) const; + + void + setIntegerEnforcement(std::optional enforce); + + std::optional + integerEnforcement() const noexcept; + + bool + validNumber() const noexcept; //-------------------------------------------------------------------------- // @@ -155,6 +191,9 @@ public: int exponent() const noexcept; + bool + integral() const noexcept; + bool native() const noexcept; @@ -435,6 +474,12 @@ STAmount::exponent() const noexcept return mOffset; } +inline bool +STAmount::integral() const noexcept +{ + return mAsset.integral(); +} + inline bool STAmount::native() const noexcept { @@ -510,6 +555,8 @@ inline STAmount::operator bool() const noexcept inline STAmount::operator Number() const { + if (enforceConversion_) + return toNumber(*enforceConversion_); if (native()) return xrp(); if (mAsset.holds()) @@ -517,6 +564,17 @@ inline STAmount::operator Number() const return iou(); } +inline Number +STAmount::toNumber(Number::EnforceInteger enforce) const +{ + if (native()) + return xrp().toNumber(enforce); + if (mAsset.holds()) + return mpt().toNumber(enforce); + // It doesn't make sense to enforce limits on IOUs + return iou(); +} + inline STAmount& STAmount::operator=(beast::Zero) { @@ -538,6 +596,11 @@ STAmount::operator=(Number const& number) mValue = mIsNegative ? -number.mantissa() : number.mantissa(); mOffset = number.exponent(); canonicalize(); + + // Convert it back to a Number to check that it's valid + [[maybe_unused]] + Number n = *this; + return *this; } @@ -553,7 +616,7 @@ STAmount::clear() { // The -100 is used to allow 0 to sort less than a small positive values // which have a negative exponent. - mOffset = native() ? 0 : -100; + mOffset = integral() ? 0 : -100; mValue = 0; mIsNegative = false; } diff --git a/include/xrpl/protocol/STNumber.h b/include/xrpl/protocol/STNumber.h index 2ec3d66fd1..43b96a2b46 100644 --- a/include/xrpl/protocol/STNumber.h +++ b/include/xrpl/protocol/STNumber.h @@ -56,6 +56,18 @@ public: bool isDefault() const override; + /// Sets the flag on the underlying number + void + setIntegerEnforcement(Number::EnforceInteger enforce); + + /// Gets the flag value on the underlying number + Number::EnforceInteger + integerEnforcement() const noexcept; + + /// Checks the underlying number + bool + valid() const noexcept; + operator Number() const { return value_; diff --git a/include/xrpl/protocol/SystemParameters.h b/include/xrpl/protocol/SystemParameters.h index de78b65265..7a2c5a7f63 100644 --- a/include/xrpl/protocol/SystemParameters.h +++ b/include/xrpl/protocol/SystemParameters.h @@ -23,6 +23,7 @@ systemName() /** Number of drops in the genesis account. */ constexpr XRPAmount INITIAL_XRP{100'000'000'000 * DROPS_PER_XRP}; +static_assert(INITIAL_XRP.drops() == 100'000'000'000'000'000); /** Returns true if the amount does not exceed the initial XRP in existence. */ inline bool diff --git a/include/xrpl/protocol/XRPAmount.h b/include/xrpl/protocol/XRPAmount.h index f26bb7366f..159174accc 100644 --- a/include/xrpl/protocol/XRPAmount.h +++ b/include/xrpl/protocol/XRPAmount.h @@ -146,6 +146,12 @@ public: return {drops(), Number::weak}; } + Number + toNumber(Number::EnforceInteger enforce) const + { + return {value(), enforce}; + } + /** Return the sign of the amount */ constexpr int signum() const noexcept diff --git a/src/libxrpl/ledger/View.cpp b/src/libxrpl/ledger/View.cpp index 0175b099ea..92fd5ccc97 100644 --- a/src/libxrpl/ledger/View.cpp +++ b/src/libxrpl/ledger/View.cpp @@ -2878,13 +2878,17 @@ assetsToSharesDeposit( Number const assetTotal = vault->at(sfAssetsTotal); STAmount shares{vault->at(sfShareMPTID)}; + shares.setIntegerEnforcement(Number::weak); if (assetTotal == 0) return STAmount{ shares.asset(), Number(assets.mantissa(), assets.exponent() + vault->at(sfScale)) - .truncate()}; + .truncate(), + Number::weak}; - Number const shareTotal = issuance->at(sfOutstandingAmount); + Number const shareTotal{ + unsafe_cast(issuance->at(sfOutstandingAmount)), + Number::strong}; shares = (shareTotal * (assets / assetTotal)).truncate(); return shares; } @@ -2906,6 +2910,7 @@ sharesToAssetsDeposit( Number const assetTotal = vault->at(sfAssetsTotal); STAmount assets{vault->at(sfAsset)}; + assets.setIntegerEnforcement(Number::weak); if (assetTotal == 0) return STAmount{ assets.asset(), @@ -2913,7 +2918,9 @@ sharesToAssetsDeposit( shares.exponent() - vault->at(sfScale), false}; - Number const shareTotal = issuance->at(sfOutstandingAmount); + Number const shareTotal{ + unsafe_cast(issuance->at(sfOutstandingAmount)), + Number::strong}; assets = assetTotal * (shares / shareTotal); return assets; } @@ -2937,9 +2944,12 @@ assetsToSharesWithdraw( Number assetTotal = vault->at(sfAssetsTotal); assetTotal -= vault->at(sfLossUnrealized); STAmount shares{vault->at(sfShareMPTID)}; + shares.setIntegerEnforcement(Number::weak); if (assetTotal == 0) return shares; - Number const shareTotal = issuance->at(sfOutstandingAmount); + Number const shareTotal{ + unsafe_cast(issuance->at(sfOutstandingAmount)), + Number::strong}; Number result = shareTotal * (assets / assetTotal); if (truncate == TruncateShares::yes) result = result.truncate(); @@ -2965,9 +2975,12 @@ sharesToAssetsWithdraw( Number assetTotal = vault->at(sfAssetsTotal); assetTotal -= vault->at(sfLossUnrealized); STAmount assets{vault->at(sfAsset)}; + assets.setIntegerEnforcement(Number::weak); if (assetTotal == 0) return assets; - Number const shareTotal = issuance->at(sfOutstandingAmount); + Number const shareTotal{ + unsafe_cast(issuance->at(sfOutstandingAmount)), + Number::strong}; assets = assetTotal * (shares / shareTotal); return assets; } diff --git a/src/libxrpl/protocol/STAmount.cpp b/src/libxrpl/protocol/STAmount.cpp index 4aac551003..6f6355a145 100644 --- a/src/libxrpl/protocol/STAmount.cpp +++ b/src/libxrpl/protocol/STAmount.cpp @@ -255,6 +255,25 @@ STAmount::move(std::size_t n, void* buf) return emplace(n, buf, std::move(*this)); } +void +STAmount::setIntegerEnforcement(std::optional enforce) +{ + enforceConversion_ = enforce; +} + +std::optional +STAmount::integerEnforcement() const noexcept +{ + return enforceConversion_; +} + +bool +STAmount::validNumber() const noexcept +{ + Number n = toNumber(Number::EnforceInteger::weak); + return n.valid(); +} + //------------------------------------------------------------------------------ // // Conversion diff --git a/src/libxrpl/protocol/STNumber.cpp b/src/libxrpl/protocol/STNumber.cpp index 889dd9ee68..5486864e95 100644 --- a/src/libxrpl/protocol/STNumber.cpp +++ b/src/libxrpl/protocol/STNumber.cpp @@ -94,6 +94,24 @@ STNumber::isDefault() const return value_ == Number(); } +void +STNumber::setIntegerEnforcement(Number::EnforceInteger enforce) +{ + value_.setIntegerEnforcement(enforce); +} + +Number::EnforceInteger +STNumber::integerEnforcement() const noexcept +{ + return value_.integerEnforcement(); +} + +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 58b929f1a2..5b36ffaf8e 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -3597,7 +3597,32 @@ class Vault_test : public beast::unit_test::suite }); testCase(18, [&, this](Env& env, Data d) { - testcase("Scale deposit overflow on second deposit"); + testcase("MPT scale deposit overflow"); + // The computed number of shares can not be represented as an MPT + // without truncation + + { + auto tx = d.vault.deposit( + {.depositor = d.depositor, + .id = d.keylet.key, + .amount = d.asset(5)}); + env(tx, ter{tecPRECISION_LOSS}); + env.close(); + } + }); + + testCase(14, [&, this](Env& env, Data d) { + testcase("MPT scale deposit overflow on first deposit"); + auto tx = d.vault.deposit( + {.depositor = d.depositor, + .id = d.keylet.key, + .amount = d.asset(10)}); + env(tx, ter{tecPRECISION_LOSS}); + env.close(); + }); + + testCase(14, [&, this](Env& env, Data d) { + testcase("MPT scale deposit overflow on second deposit"); { auto tx = d.vault.deposit( @@ -3618,8 +3643,8 @@ class Vault_test : public beast::unit_test::suite } }); - testCase(18, [&, this](Env& env, Data d) { - testcase("Scale deposit overflow on total shares"); + testCase(14, [&, this](Env& env, Data d) { + testcase("No MPT scale deposit overflow on total shares"); { auto tx = d.vault.deposit( @@ -3635,7 +3660,7 @@ class Vault_test : public beast::unit_test::suite {.depositor = d.depositor, .id = d.keylet.key, .amount = d.asset(5)}); - env(tx, ter{tecPATH_DRY}); + env(tx); env.close(); } }); @@ -3919,6 +3944,28 @@ class Vault_test : public beast::unit_test::suite testCase(18, [&, this](Env& env, Data d) { testcase("Scale withdraw overflow"); + { + auto tx = d.vault.deposit( + {.depositor = d.depositor, + .id = d.keylet.key, + .amount = d.asset(5)}); + env(tx, ter{tecPRECISION_LOSS}); + env.close(); + } + + { + auto tx = d.vault.withdraw( + {.depositor = d.depositor, + .id = d.keylet.key, + .amount = STAmount(d.asset, Number(10, 0))}); + env(tx, ter{tecPRECISION_LOSS}); + env.close(); + } + }); + + testCase(14, [&, this](Env& env, Data d) { + testcase("MPT scale withdraw overflow"); + { auto tx = d.vault.deposit( {.depositor = d.depositor, @@ -4137,6 +4184,29 @@ class Vault_test : public beast::unit_test::suite testCase(18, [&, this](Env& env, Data d) { testcase("Scale clawback overflow"); + { + auto tx = d.vault.deposit( + {.depositor = d.depositor, + .id = d.keylet.key, + .amount = d.asset(5)}); + env(tx, ter(tecPRECISION_LOSS)); + env.close(); + } + + { + 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{tecPRECISION_LOSS}); + env.close(); + } + }); + + testCase(14, [&, this](Env& env, Data d) { + testcase("MPT Scale clawback overflow"); + { auto tx = d.vault.deposit( {.depositor = d.depositor, diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index 78c9b28952..a5fba8ef7d 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -858,7 +858,7 @@ public: { BEAST_EXPECT(e.what() == "Number::operator= integer overflow"s); // The throw is done _after_ the number is updated. - BEAST_EXPECT((a == Number{2, 14})); + BEAST_EXPECT((a == Number::maxIntValue * 2)); BEAST_EXPECT(!a.valid()); } try @@ -870,7 +870,7 @@ public: { BEAST_EXPECT(e.what() == "Number::Number integer overflow"s); // The Number doesn't get updated because the ctor throws - BEAST_EXPECT((a == Number{2, 14})); + BEAST_EXPECT((a == Number::maxIntValue * 2)); BEAST_EXPECT(!a.valid()); } a = Number(1, 10); diff --git a/src/xrpld/app/tx/detail/VaultClawback.cpp b/src/xrpld/app/tx/detail/VaultClawback.cpp index 7c56ca1d60..1c73d36bb0 100644 --- a/src/xrpld/app/tx/detail/VaultClawback.cpp +++ b/src/xrpld/app/tx/detail/VaultClawback.cpp @@ -71,9 +71,13 @@ VaultClawback::preclaim(PreclaimContext const& ctx) } Asset const vaultAsset = vault->at(sfAsset); - if (auto const amount = ctx.tx[~sfAmount]; - amount && vaultAsset != amount->asset()) - return tecWRONG_ASSET; + if (auto const amount = ctx.tx[~sfAmount]) + { + if (vaultAsset != amount->asset()) + return tecWRONG_ASSET; + else if (!amount->validNumber()) + return tecPRECISION_LOSS; + } if (vaultAsset.native()) { @@ -157,6 +161,8 @@ VaultClawback::doApply() MPTIssue const share{mptIssuanceID}; STAmount sharesDestroyed = {share}; STAmount assetsRecovered; + assetsRecovered.setIntegerEnforcement(Number::weak); + sharesDestroyed.setIntegerEnforcement(Number::weak); try { if (amount == beast::zero) @@ -169,6 +175,9 @@ VaultClawback::doApply() AuthHandling::ahIGNORE_AUTH, j_); + if (!sharesDestroyed.validNumber()) + return tecPRECISION_LOSS; + auto const maybeAssets = sharesToAssetsWithdraw(vault, sleIssuance, sharesDestroyed); if (!maybeAssets) @@ -184,6 +193,8 @@ VaultClawback::doApply() if (!maybeShares) return tecINTERNAL; // LCOV_EXCL_LINE sharesDestroyed = *maybeShares; + if (!sharesDestroyed.validNumber()) + return tecPRECISION_LOSS; } auto const maybeAssets = @@ -192,6 +203,8 @@ VaultClawback::doApply() return tecINTERNAL; // LCOV_EXCL_LINE assetsRecovered = *maybeAssets; } + if (!assetsRecovered.validNumber()) + return tecPRECISION_LOSS; // Clamp to maximum. if (assetsRecovered > *assetsAvailable) diff --git a/src/xrpld/app/tx/detail/VaultDeposit.cpp b/src/xrpld/app/tx/detail/VaultDeposit.cpp index 3e5ae741e3..66e144312f 100644 --- a/src/xrpld/app/tx/detail/VaultDeposit.cpp +++ b/src/xrpld/app/tx/detail/VaultDeposit.cpp @@ -42,6 +42,9 @@ 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()) @@ -217,6 +220,7 @@ VaultDeposit::doApply() } STAmount sharesCreated = {vault->at(sfShareMPTID)}, assetsDeposited; + sharesCreated.setIntegerEnforcement(Number::weak); try { // Compute exchange before transferring any amounts. @@ -227,14 +231,14 @@ VaultDeposit::doApply() return tecINTERNAL; // LCOV_EXCL_LINE sharesCreated = *maybeShares; } - if (sharesCreated == beast::zero) + if (sharesCreated == beast::zero || !sharesCreated.validNumber()) return tecPRECISION_LOSS; auto const maybeAssets = sharesToAssetsDeposit(vault, sleIssuance, sharesCreated); if (!maybeAssets) return tecINTERNAL; // LCOV_EXCL_LINE - else if (*maybeAssets > amount) + else if (*maybeAssets > amount || !maybeAssets->validNumber()) { // LCOV_EXCL_START JLOG(j_.error()) << "VaultDeposit: would take more than offered."; @@ -260,13 +264,22 @@ VaultDeposit::doApply() sharesCreated.asset() != assetsDeposited.asset(), "ripple::VaultDeposit::doApply : assets are not shares"); - vault->at(sfAssetsTotal) += assetsDeposited; - vault->at(sfAssetsAvailable) += assetsDeposited; + auto assetsTotalProxy = vault->at(sfAssetsTotal); + auto assetsAvailableProxy = vault->at(sfAssetsAvailable); + if (vault->at(sfAsset).value().integral()) + { + assetsTotalProxy.value().setIntegerEnforcement(Number::weak); + assetsAvailableProxy.value().setIntegerEnforcement(Number::weak); + } + assetsTotalProxy += assetsDeposited; + assetsAvailableProxy += assetsDeposited; + if (!assetsTotalProxy->valid() || !assetsAvailableProxy->valid()) + return tecLIMIT_EXCEEDED; view().update(vault); // A deposit must not push the vault over its limit. auto const maximum = *vault->at(sfAssetsMaximum); - if (maximum != 0 && *vault->at(sfAssetsTotal) > maximum) + if (maximum != 0 && *assetsTotalProxy > maximum) return tecLIMIT_EXCEEDED; // Transfer assets from depositor to vault. diff --git a/src/xrpld/app/tx/detail/VaultWithdraw.cpp b/src/xrpld/app/tx/detail/VaultWithdraw.cpp index 8cd3f5cd97..8806f7b236 100644 --- a/src/xrpld/app/tx/detail/VaultWithdraw.cpp +++ b/src/xrpld/app/tx/detail/VaultWithdraw.cpp @@ -50,6 +50,9 @@ 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()) @@ -154,6 +157,8 @@ VaultWithdraw::doApply() MPTIssue const share{mptIssuanceID}; STAmount sharesRedeemed = {share}; STAmount assetsWithdrawn; + assetsWithdrawn.setIntegerEnforcement(Number::weak); + sharesRedeemed.setIntegerEnforcement(Number::weak); try { if (amount.asset() == vaultAsset) @@ -167,13 +172,15 @@ VaultWithdraw::doApply() sharesRedeemed = *maybeShares; } - if (sharesRedeemed == beast::zero) + if (sharesRedeemed == beast::zero || !sharesRedeemed.validNumber()) return tecPRECISION_LOSS; auto const maybeAssets = sharesToAssetsWithdraw(vault, sleIssuance, sharesRedeemed); if (!maybeAssets) return tecINTERNAL; // LCOV_EXCL_LINE assetsWithdrawn = *maybeAssets; + if (!assetsWithdrawn.validNumber()) + return tecPRECISION_LOSS; } else if (amount.asset() == share) { @@ -184,6 +191,8 @@ VaultWithdraw::doApply() if (!maybeAssets) return tecINTERNAL; // LCOV_EXCL_LINE assetsWithdrawn = *maybeAssets; + if (!assetsWithdrawn.validNumber()) + return tecPRECISION_LOSS; } else return tefINTERNAL; // LCOV_EXCL_LINE