From e4796a17177d657ebbd464dc7d50fee249231444 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Sun, 24 Mar 2024 13:00:43 -0400 Subject: [PATCH] fix: improper handling of large synthetic AMM offers: A large synthetic offer was not handled correctly in the payment engine. This patch fixes that issue and introduces a new invariant check while processing synthetic offers. --- src/ripple/app/misc/AMMHelpers.h | 2 +- src/ripple/app/paths/AMMLiquidity.h | 13 ++++-- src/ripple/app/paths/AMMOffer.h | 13 ++++-- src/ripple/app/paths/impl/AMMLiquidity.cpp | 54 +++++++++++++++++----- src/ripple/app/paths/impl/AMMOffer.cpp | 46 ++++++++++++++++-- src/ripple/app/paths/impl/BookStep.cpp | 11 +++++ src/ripple/app/tx/impl/Offer.h | 9 ++++ src/ripple/protocol/Feature.h | 3 +- src/ripple/protocol/impl/Feature.cpp | 3 +- 9 files changed, 128 insertions(+), 26 deletions(-) diff --git a/src/ripple/app/misc/AMMHelpers.h b/src/ripple/app/misc/AMMHelpers.h index 24c25922800..e84c6c535ea 100644 --- a/src/ripple/app/misc/AMMHelpers.h +++ b/src/ripple/app/misc/AMMHelpers.h @@ -134,7 +134,7 @@ withinRelativeDistance( template requires( std::is_same_v || std::is_same_v || - std::is_same_v) + std::is_same_v || std::is_same_v) bool withinRelativeDistance(Amt const& calc, Amt const& req, Number const& dist) { diff --git a/src/ripple/app/paths/AMMLiquidity.h b/src/ripple/app/paths/AMMLiquidity.h index 7757bd4684d..f1be112b2d6 100644 --- a/src/ripple/app/paths/AMMLiquidity.h +++ b/src/ripple/app/paths/AMMLiquidity.h @@ -137,10 +137,17 @@ class AMMLiquidity TAmounts generateFibSeqOffer(TAmounts const& balances) const; - /** Generate max offer + /** Generate max offer. + * If `fixAMMOverflowOffer` is active, the offer is generated as: + * takerGets = 99% * balances.out takerPays = swapOut(takerGets). + * Return nullopt if takerGets is 0 or takerGets == balances.out. + * + * If `fixAMMOverflowOffer` is not active, the offer is generated as: + * takerPays = max input amount; + * takerGets = swapIn(takerPays). */ - AMMOffer - maxOffer(TAmounts const& balances) const; + std::optional> + maxOffer(TAmounts const& balances, Rules const& rules) const; }; } // namespace ripple diff --git a/src/ripple/app/paths/AMMOffer.h b/src/ripple/app/paths/AMMOffer.h index 10e6017dd96..426ba96a772 100644 --- a/src/ripple/app/paths/AMMOffer.h +++ b/src/ripple/app/paths/AMMOffer.h @@ -50,9 +50,8 @@ class AMMOffer // the swap out of the entire side of the pool, in which case // the swap in amount is infinite. TAmounts const amounts_; - // If seated then current pool balances. Used in one-path limiting steps - // to swap in/out. - std::optional> const balances_; + // Current pool balances. + TAmounts const balances_; // The Spot Price quality if balances != amounts // else the amounts quality Quality const quality_; @@ -63,7 +62,7 @@ class AMMOffer AMMOffer( AMMLiquidity const& ammLiquidity, TAmounts const& amounts, - std::optional> const& balances, + TAmounts const& balances, Quality const& quality); Quality @@ -142,6 +141,12 @@ class AMMOffer // AMM doesn't pay transfer fee on Payment tx return {ofrInRate, QUALITY_ONE}; } + + /** Check the new pool product is greater or equal to the old pool + * product or if decreases then within some threshold. + */ + bool + checkInvariant(TAmounts const& consumed, beast::Journal j) const; }; } // namespace ripple diff --git a/src/ripple/app/paths/impl/AMMLiquidity.cpp b/src/ripple/app/paths/impl/AMMLiquidity.cpp index 3f22ebacec5..bcc086e23da 100644 --- a/src/ripple/app/paths/impl/AMMLiquidity.cpp +++ b/src/ripple/app/paths/impl/AMMLiquidity.cpp @@ -93,6 +93,7 @@ AMMLiquidity::generateFibSeqOffer( return cur; } +namespace { template constexpr T maxAmount() @@ -105,16 +106,41 @@ maxAmount() return STAmount(STAmount::cMaxValue / 2, STAmount::cMaxOffset); } +template +T +maxOut(T const& out, Issue const& iss) +{ + Number const res = out * Number{99, -2}; + return toAmount(iss, res, Number::rounding_mode::downward); +} +} // namespace + template -AMMOffer -AMMLiquidity::maxOffer(TAmounts const& balances) const +std::optional> +AMMLiquidity::maxOffer( + TAmounts const& balances, + Rules const& rules) const { - return AMMOffer( - *this, - {maxAmount(), - swapAssetIn(balances, maxAmount(), tradingFee_)}, - balances, - Quality{balances}); + if (!rules.enabled(fixAMMOverflowOffer)) + { + return AMMOffer( + *this, + {maxAmount(), + swapAssetIn(balances, maxAmount(), tradingFee_)}, + balances, + Quality{balances}); + } + else + { + auto const out = maxOut(balances.out, issueOut()); + if (out <= TOut{0} || out >= balances.out) + return std::nullopt; + return AMMOffer( + *this, + {swapAssetOut(balances, out, tradingFee_), out}, + balances, + Quality{balances}); + } } template @@ -167,15 +193,16 @@ AMMLiquidity::getOffer( if (clobQuality && Quality{amounts} < clobQuality) return std::nullopt; return AMMOffer( - *this, amounts, std::nullopt, Quality{amounts}); + *this, amounts, balances, Quality{amounts}); } else if (!clobQuality) { // If there is no CLOB to compare against, return the largest // amount, which doesn't overflow. The size is going to be // changed in BookStep per either deliver amount limit, or - // sendmax, or available output or input funds. - return maxOffer(balances); + // sendmax, or available output or input funds. Might return + // nullopt if the pool is small. + return maxOffer(balances, view.rules()); } else if ( auto const amounts = @@ -188,7 +215,10 @@ AMMLiquidity::getOffer( catch (std::overflow_error const& e) { JLOG(j_.error()) << "AMMLiquidity::getOffer overflow " << e.what(); - return maxOffer(balances); + if (!view.rules().enabled(fixAMMOverflowOffer)) + return maxOffer(balances, view.rules()); + else + return std::nullopt; } catch (std::exception const& e) { diff --git a/src/ripple/app/paths/impl/AMMOffer.cpp b/src/ripple/app/paths/impl/AMMOffer.cpp index 10b75b78565..759851b7afe 100644 --- a/src/ripple/app/paths/impl/AMMOffer.cpp +++ b/src/ripple/app/paths/impl/AMMOffer.cpp @@ -27,7 +27,7 @@ template AMMOffer::AMMOffer( AMMLiquidity const& ammLiquidity, TAmounts const& amounts, - std::optional> const& balances, + TAmounts const& balances, Quality const& quality) : ammLiquidity_(ammLiquidity) , amounts_(amounts) @@ -110,7 +110,7 @@ AMMOffer::limitOut( // Change the offer size according to the conservation function. The offer // quality is increased in this case, but it doesn't matter since there is // only one path. - return {swapAssetOut(*balances_, limit, ammLiquidity_.tradingFee()), limit}; + return {swapAssetOut(balances_, limit, ammLiquidity_.tradingFee()), limit}; } template @@ -122,7 +122,7 @@ AMMOffer::limitIn( // See the comments above in limitOut(). if (ammLiquidity_.multiPath()) return quality().ceil_in(offrAmt, limit); - return {limit, swapAssetIn(*balances_, limit, ammLiquidity_.tradingFee())}; + return {limit, swapAssetIn(balances_, limit, ammLiquidity_.tradingFee())}; } template @@ -132,7 +132,45 @@ AMMOffer::getQualityFunc() const if (ammLiquidity_.multiPath()) return QualityFunction{quality(), QualityFunction::CLOBLikeTag{}}; return QualityFunction{ - *balances_, ammLiquidity_.tradingFee(), QualityFunction::AMMTag{}}; + balances_, ammLiquidity_.tradingFee(), QualityFunction::AMMTag{}}; +} + +template +bool +AMMOffer::checkInvariant( + TAmounts const& consumed, + beast::Journal j) const +{ + if (consumed.in > amounts_.in || consumed.out > amounts_.out) + { + JLOG(j.error()) << "AMMOffer::checkInvariant failed: consumed " + << to_string(consumed.in) << " " + << to_string(consumed.out) << " amounts " + << to_string(amounts_.in) << " " + << to_string(amounts_.out); + + return false; + } + + Number const product = balances_.in * balances_.out; + auto const newBalances = TAmounts{ + balances_.in + consumed.in, balances_.out - consumed.out}; + Number const newProduct = newBalances.in * newBalances.out; + + if (newProduct >= product || + withinRelativeDistance(product, newProduct, Number{1, -7})) + return true; + + JLOG(j.error()) << "AMMOffer::checkInvariant failed: balances " + << to_string(balances_.in) << " " + << to_string(balances_.out) << " new balances " + << to_string(newBalances.in) << " " + << to_string(newBalances.out) << " product/newProduct " + << product << " " << newProduct << " diff " + << (product != Number{0} + ? to_string((product - newProduct) / product) + : "undefined"); + return false; } template class AMMOffer; diff --git a/src/ripple/app/paths/impl/BookStep.cpp b/src/ripple/app/paths/impl/BookStep.cpp index dd6abe577f5..358dac4c796 100644 --- a/src/ripple/app/paths/impl/BookStep.cpp +++ b/src/ripple/app/paths/impl/BookStep.cpp @@ -793,6 +793,17 @@ BookStep::consumeOffer( TAmounts const& stepAmt, TOut const& ownerGives) const { + if (!offer.checkInvariant(ofrAmt, j_)) + { + // purposely written as separate if statements so we get logging even + // when the amendment isn't active. + if (sb.rules().enabled(fixAMMOverflowOffer)) + { + Throw( + tecINVARIANT_FAILED, "AMM pool product invariant failed."); + } + } + // The offer owner gets the ofrAmt. The difference between ofrAmt and // stepAmt is a transfer fee that goes to book_.in.account { diff --git a/src/ripple/app/tx/impl/Offer.h b/src/ripple/app/tx/impl/Offer.h index 027030ec8d0..bdae4d2b155 100644 --- a/src/ripple/app/tx/impl/Offer.h +++ b/src/ripple/app/tx/impl/Offer.h @@ -163,6 +163,15 @@ class TOffer : private TOfferBase // CLOB offer pays the transfer fee return {ofrInRate, ofrOutRate}; } + + /** Check any required invariant. Limit order book offer + * always returns true. + */ + bool + checkInvariant(TAmounts const&, beast::Journal j) const + { + return true; + } }; using Offer = TOffer<>; diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index 8e6483b1dbd..a1e34952ea6 100644 --- a/src/ripple/protocol/Feature.h +++ b/src/ripple/protocol/Feature.h @@ -74,7 +74,7 @@ namespace detail { // Feature.cpp. Because it's only used to reserve storage, and determine how // large to make the FeatureBitset, it MAY be larger. It MUST NOT be less than // the actual number of amendments. A LogicError on startup will verify this. -static constexpr std::size_t numFeatures = 67; +static constexpr std::size_t numFeatures = 68; /** Amendments that this server supports and the default voting behavior. Whether they are enabled depends on the Rules defined in the validated @@ -354,6 +354,7 @@ extern uint256 const featureDID; extern uint256 const fixFillOrKill; extern uint256 const fixNFTokenReserve; extern uint256 const fixInnerObjTemplate; +extern uint256 const fixAMMOverflowOffer; } // namespace ripple diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index ab36983edd7..bf6706b118e 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -460,7 +460,8 @@ REGISTER_FIX (fixDisallowIncomingV1, Supported::yes, VoteBehavior::De REGISTER_FEATURE(DID, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX(fixFillOrKill, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX (fixNFTokenReserve, Supported::yes, VoteBehavior::DefaultNo); -REGISTER_FIX(fixInnerObjTemplate, Supported::yes, VoteBehavior::DefaultNo); +REGISTER_FIX (fixInnerObjTemplate, Supported::yes, VoteBehavior::DefaultNo); +REGISTER_FIX (fixAMMOverflowOffer, Supported::yes, VoteBehavior::DefaultYes); // The following amendments are obsolete, but must remain supported // because they could potentially get enabled.