[ALPHANET] AMM Curves

[dangell7]

High Level Overview of Change

Context of Change

API Impact

• Public API: New feature (new methods and/or new fields)
• Public API: Breaking change (in general, breaking changes should only impact the next api_version)
• libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
• Peer protocol change (must be backward compatible or bump the peer protocol version)

[sentinel-ai-reviewer]

The guard if(NOT DEFINED SANITIZERS) only returns early when SANITIZERS is entirely absent from the CMake cache. If SANITIZERS is defined but empty — for example via -D SANITIZERS= on a direct cmake command line or a stale cache entry from a previous sanitizer build — NOT DEFINED SANITIZERS evaluates to FALSE, the code falls through, and SANITIZERS_ENABLED is incorrectly set to TRUE. On Clang builds this then unconditionally checks for the ignorelist file and emits FATAL_ERROR even though no sanitizer is actually active. The idiomatic CMake fix is if(NOT SANITIZERS), which treats both undefined and empty/false values as "disabled" — consistent with how every other CMake boolean guard in this codebase works.

Suggested fix

Replace if(NOT DEFINED SANITIZERS) with if(NOT SANITIZERS) to correctly guard against both undefined and empty-string values.

[sentinel-ai-reviewer]

The secp256k1 Conan recipe revision changes from b1f450b7f78a36fff75bb6934a356f3a to 481881709eb0bdd0185a12b912bbe8ad at the same source version (0.7.1). Conan recipe revisions control how a library is compiled — not its source, but its build flags, applied patches, and feature toggles. secp256k1 is the library that performs ECDSA signature verification for every transaction and validation message in rippled (secp256k1_ecdsa_verify in PublicKey.cpp, secp256k1_ec_seckey_tweak_add in SecretKey.cpp). A silent build configuration change could disable constant-time scalar multiplication or remove other hardening options without any visible source-level diff. The difference in recipe content should be audited before merging.

Suggested fix

Pull both recipe revisions from Conan Center Index and diff them. Confirm that security-relevant build options — constant-time operations, SECP256K1_ENABLE_MODULE_RECOVERY, ECMULT_STATIC_PRECOMPUTATION — are unchanged or that changes are benign before accepting this lockfile.

[sentinel-ai-reviewer]

OpenSSL is downgraded from 3.6.3 to 3.6.2. rippled uses OpenSSL across its entire network stack: TLS peer connections via boost::asio::ssl::stream in ConnectAttempt.cpp, handshake channel-binding cookies derived from SSL_get_finished/SSL_get_peer_finished in Handshake.cpp, and ephemeral self-signed cert generation via EVP in make_SSLContext.cpp. All of these are network-reachable from any peer. Any CVE fixed in 3.6.3 is reintroduced here. Additionally, the libraries are linked statically (shared=False in conanfile.py), so a future OS-level patch won't help — only a rebuild does. This downgrade needs an explicit justification or should be reverted.

Suggested fix

Restore openssl/3.6.3 or document that the specific CVEs addressed in 3.6.3 are not reachable through rippled's usage of OpenSSL. Never downgrade a security-critical library without a recorded rationale.

[sentinel-ai-reviewer]

gRPC is downgraded from 1.81.1 to 1.78.1. rippled exposes a gRPC server (GRPCServer.h/cpp) with TLS enabled (grpc/*:secure=True in conanfile.py), serving ledger queries including LedgerData, LedgerDiff, and LedgerEntry handlers. gRPC has a history of HTTP/2-layer CVEs (CONTINUATION flood, HPACK decoder limits) that affect servers regardless of application-level logic. Rolling back three minor versions without a documented reason is a red flag. Static linkage means the vulnerability is baked into the binary.

Suggested fix

Review the gRPC changelog between 1.78.1 and 1.81.1 for any security-relevant fixes before accepting this downgrade. If a specific incompatibility drove the rollback, document it in the PR; otherwise restore 1.81.1.

[sentinel-ai-reviewer]

The Conan package registry is redirected from the XRPL Foundation's official server (conan.xrplf.org/repository/conan/) to conan.ripplex.io — a different domain under different organizational control — creating a supply chain trust boundary shift. Critically, the upload workflows (upload-conan-deps.yml and reusable-upload-recipe.yml) still hardcode conan.xrplf.org as their destination, so packages are written to one server but this script generates lockfiles by fetching from a different server entirely. Any actor who controls conan.ripplex.io can serve Conan packages that differ from what was audited and uploaded to the Foundation's registry, which would compile malicious code into rippled production binaries across all three platforms (Linux, macOS, Windows). Align all upload and download references to the same server, verify conan.ripplex.io is under the same organizational governance as the original registry, and add explicit package checksum validation beyond what the lockfile provides.

Suggested fix

Ensure all Conan remote references (regenerate.sh, setup-conan/action.yml, upload-conan-deps.yml, reusable-upload-recipe.yml) point to the same verified server. If ripplex.io is the intended migration target, update all upload workflows to match and document the organizational ownership of the domain. If not, revert to conan.xrplf.org in regenerate.sh and setup-conan/action.yml to restore consistency.

[sentinel-ai-reviewer]

The condition compiler_version < 13 compares a string against an integer literal. compiler_version is assigned from detect_api.detect_default_compiler() (line 4) and detect_api.default_compiler_version() (line 6), both of which return string values in Conan 2.x — the lockfile profiles confirm this with values like "17.0" for Apple Clang that cannot be integers. In Python 3 / Jinja2, "12" < 13 raises TypeError: '<' not supported between instances of 'str' and 'int', so on any GCC system the entire profile fails to render rather than adding the flag. Since compiler == "gcc" is the left operand of and, this short-circuits on non-GCC systems and the error is silently hidden there — it only manifests on Linux/GCC, which is the primary CI environment. Fix the comparison to use the Jinja2 int filter: {% if compiler == "gcc" and compiler_version | int < 13 %}.

Suggested fix

Change compiler_version < 13 to compiler_version | int < 13 so the string is cast to an integer before comparison: {% if compiler == "gcc" and compiler_version | int < 13 %}

[sentinel-ai-reviewer]

The refactoring moved sanitizer_compiler_flags from a top-level initialization (visible everywhere) to assignments that only exist inside the {% if compiler == "gcc" %} and {% elif compiler == "clang" or compiler == "apple-clang" %} branches. For any other compiler — MSVC in particular, which is warned about at the top of the template but not blocked from continuing — the variable is never set, so {{sanitizer_compiler_flags}} and {{sanitizer_linker_flags}} on lines 93-95 and 100 silently render as empty strings under Conan 2.x's default Jinja2 Undefined mode. Before this change, sanitizer_compiler_flags was initialized to the base flag list before the compiler branches, so MSVC at least produced a syntactically coherent (if semantically wrong) [conf] block; now it produces cxxflags+=[''], silently dropping the user's sanitizer request with no Conan-level error. Add an {% else %} branch that either raises an explicit Jinja2 error or initialises both variables to safe fallback strings, so the failure is loud rather than silent.

Suggested fix

Add {% else %} {% set sanitizer_compiler_flags = '' %} {% set sanitizer_linker_flags = '' %} {% endif %} (or a Jinja2 {{ raise('Unsupported compiler for sanitizers') }} equivalent) so that reaching the [conf] section with an unknown compiler produces an explicit error rather than rendering empty strings into the Conan configuration.

[sentinel-ai-reviewer]

When getCurve returns nullptr or curve->swapOut returns an error (e.g., activeLiquidity == 0, tick cap hit, amendment disabled), curveSwapOut silently falls back to toMaxAmount<TIn> — the maximum representable value — with no indication of failure. This is asymmetric with curveSwapIn at line 222, which returns zero on failure; the zero is safely filtered by the > beast::kZero guard in AMMLiquidity::getOffer, but toMaxAmount is a large positive value that passes that guard. The resulting AMMOffer has a legitimate-looking out and a sky-high in, which flows from generateFibSeqOffer and maxOffer into AMMOffer::limitOut and from there directly into BookStep strand accounting without any saturation check. Callers have no way to distinguish a legitimately expensive swap from a failed one. The return type should be Expected<TIn, TER> so errors propagate explicitly, or the fallback should be unified to zero (with callers updated) so both failure modes are caught by the existing > beast::kZero filter.

Suggested fix

Change the return type of curveSwapOut to Expected<TIn, TER> and return Unexpected(tecAMM_FAILED) on the failure path, matching the CurveInterface::swapOut contract. Update call sites in AMMOffer::limitOut and AMMLiquidity::maxOffer/generateFibSeqOffer to handle the Expected and propagate the error upward rather than silently consuming a sentinel value.

[sentinel-ai-reviewer]

When getCurve() returns nullptr or when curve->swapIn()/swapOut() returns an error TER, both template functions silently substitute sentinel amounts instead of propagating the failure: curveSwapIn returns a zero-output amount and curveSwapOut returns the maximum representable input amount. None of the six call sites in AMMLiquidity.cpp and AMMOffer.cpp treat these sentinels as errors — they assign them directly into offer amounts and forward them to BookStep. The curveSwapOut max-input sentinel is especially dangerous: BookStep's subsequent mulRatio() call multiplies max-native or max-IOU amounts by the transfer rate, producing an STAmount overflow; even without overflow, the settlement input disagrees with the quality BookStep computed from pool balances, causing mispriced payment fills. Change both templates to return Expected<TOut,TER> / Expected<TIn,TER> so errors propagate cleanly to BookStep.

Suggested fix

Change the return type of curveSwapIn to Expected<TOut, TER> and curveSwapOut to Expected<TIn, TER>. Return the TER from the failed curve->swapIn/swapOut call instead of the sentinel. Update all callers in AMMLiquidity.cpp (generateFibSeqOffer lines 85/109, maxOffer lines 159/212) and AMMOffer.cpp (limitIn line 161, limitOut line 135) to propagate or handle the error.

[sentinel-ai-reviewer]

Both template functions unconditionally dereference getCurrentTransactionRules() — which returns std::optional<Rules> const& — without checking whether the optional is engaged. Every other AMM helper in the codebase (swapAssetIn, swapAssetOut, AMMHelpers.h, Offer.h, STAmount.cpp) guards this call with !rules || before accessing the value, because the optional IS empty outside a Transactor context. The code path path_find / ripple_path_find RPC → PathRequest → Pathfinder → RippleCalc::rippleCalculate → BookStep → AMMLiquidity::getOffer → curveSwapIn/curveSwapOut installs no CurrentTransactionRulesGuard anywhere along the chain, so the optional is always empty when called from RPC. Dereferencing an empty optional is undefined behavior; in practice it is a null-pointer read that crashes the node, giving any remote caller with RPC access a trivial denial-of-service against any validator running a non-constant-product AMM pool.

Suggested fix

Replace getCurve(curveType, *getCurrentTransactionRules()) with (getCurrentTransactionRules() ? getCurve(curveType, *getCurrentTransactionRules()) : nullptr), or restructure the block as if (auto const& rules = getCurrentTransactionRules()) { if (auto const* curve = getCurve(curveType, *rules)) { ... } }. The same fix is needed at line 238 in curveSwapOut.

[sentinel-ai-reviewer]

The default value for curveType uses the bare literal 0 rather than the named constant CtConstantProduct, which is inconsistent with ammLPHolds in the same diff. While numerically identical today, any future caller that omits curveType when deleting a non-CP pool (CL/StableSwap/Binned) will silently pass 0 to keylet::amm(asset, asset2, curveType), producing the CP-variant keylet — either finding the wrong pool to delete if a CP pool shares the same asset pair, or returning tecINTERNAL and leaving the non-CP pool in the ledger permanently. Remove the default entirely and require callers to pass curveType explicitly, mirroring the pattern already used in AMMDelete::doApply and AMMWithdraw::deleteAMMAccountIfEmpty.

Suggested fix

Remove the = 0 default from the declaration: deleteAMMAccount(Sandbox& view, Asset const& asset, Asset const& asset2, beast::Journal j, std::uint8_t curveType);. All existing call sites already pass curveType explicitly, so no callers need updating. If a default is truly required, use = CtConstantProduct for semantic clarity, matching the convention in ammLPHolds.

[sentinel-ai-reviewer]

The function accepts any std::int32_t tick but performs no bounds check against [minTick, maxTick] before computing the bitmap position. For a tick one below minTick (-887273), the signed addition produces -1, which wraps to UINT32_MAX on the unsigned cast, and the subsequent static_cast<std::uint16_t> then truncates to 65535 — a wordIndex at the far end of the address space rather than an error. Neither setTickBitmap nor clearTickBitmap add this guard either, so they silently address the wrong bitmap SLE and corrupt tick-presence state. Current callers (AMMDeposit preflight) do validate tick bounds upstream, so there is no active exploit path, but the function is marked noexcept with no internal defense and is a fragile trap for any future caller. Add XRPL_ASSERT(tick >= minTick && tick <= maxTick, "tickToBitmapPos: tick out of range") inside setTickBitmap/clearTickBitmap before calling this function.

Suggested fix

Add XRPL_ASSERT(tick >= minTick && tick <= maxTick, "setTickBitmap: tick out of range") at the top of both setTickBitmap and clearTickBitmap before the call to tickToBitmapPos. This catches misuse in debug builds and documents the precondition at the enforcement site rather than relying solely on distant preflight checks.

[sentinel-ai-reviewer]

The function passes tick directly to std::abs() then casts to unsigned with no prior bounds check against [minTick, maxTick]. For tick == INT32_MIN, std::abs(INT32_MIN) is signed integer overflow — undefined behavior in C++. Nine call sites in AMMCurve.cpp (swapIn, swapOut, spotPrice, applySwap, maxClOutputWithinCurrentRange) pass sfCurrentTick or findNextTick() results read directly from SLEs without re-checking the legal range at the call site. Under normal operation these values stay within bounds because the write paths are constrained, but the absence of a defensive check means any SLE with an out-of-range tick field causes UB inside a swap computation. The function should validate its input — either clamp to [minTick, maxTick] and document the contract, or return an error for out-of-range inputs so callers can propagate a tec* code instead of hitting UB.

Suggested fix

Add a bounds check at the top of tickToSqrtPrice: if (tick < minTick || tick > maxTick) return Number{0}; (or return an Expected<Number,TER>). Callers in AMMCurve.cpp that read sfCurrentTick should assert the value is within [minTick, maxTick] immediately after the read.