XRPL AMM

CertiK21 Jun 2023Code version: not recorded (see the report)

Audit

CertiK reviewed XRP Ledger (XRPL AMM) on 21 Jun 2023: 31 findings, 0 critical; 29 marked fixed, 2 open.

No open critical findings

Open the report Open XRP Ledger’s security record

Report hosted on skynet.certik.com.

Trustblock does not review code.

Findings31

Statuses are the security firm’s own, from its report.

Severity	Open	Fixed
Critical	0	0
High	0	0
Medium	0	8
Low	2	21

Medium`AMMDeposit` doesn't provide a protection from prFixed

`AMMDeposit(tfTwoAsset)` allows specifying `amount` and `amount2`. They are used as maximum values consumed by the deposit operation. However, that doesn't allow the user to set the protection from front-running. Other implementations of AMM, like Uniswap, allow the user to set the minimum amounts accepted: ```solidity function addLiquidity( address tokenA, address tokenB, uint amountADesired, uint amountBDesired, uint amountAMin, uint amountBMin, address to, uint deadline ``` If `amountAMin`/`amountBMin` conditions are not satisfied, the transaction is rejected. This effectively protects the user from front-running and sudden pool spot-price changes for other reasons. Other modes of `AMMDeposit` also don't provide protection from price fluctuations.

Medium`applyBid()` treats `minBid` incorrectlyFixed

```cpp=242 if (computedPrice >= *bidMin && computedPrice <= *bidMax) return computedPrice; JLOG(ctx_.journal.debug()) << "AMM Bid: not in range " << computedPrice << *bidMin << " " << *bidMax; ``` `AMMBid` allows setting `sfBidMin` and `sfBidMax`. `computedPrice` is calculated based on the current slot state. The code doesn't allow raising the bid via setting `sfBinMin` if `sfBidMax` is also specified.

Medium`generateFibSeqOffer()` can return negative valueFixed

```cpp=81 cur.out = toAmount<TOut>( getIssue(balances.out), cur.out * fib[ammContext_.curIters() - 1], Number::rounding_mode::downward); cur.in = swapAssetOut(balances, cur.out, tradingFee_); ``` `generateFibSeqOffer()` is supposed to generate AMM offers with the offer size based on Fibonacci sequence. The starting `cur.in = InitialFibSeqPct * initialBalances_.in = 0.025% of initialBalances_.in`. `cur.out` is approximately the same. Then `cur.out` is multiplied by `fib` elements based on the iteration. Since `fib[17] = 4181`, `cur.out` will get bigger than `balances.out`. That is unexpected by `swapAssetOut()` - `cur.in` will become negative or overflow.

Medium`withdrawByTokens()` is inaccurateFixed

It is possible to perform a single asset withdrawal via `AMMWithdraw(tfOneAssetLPToken)`. However, the formula (8) is inaccurate. It is more advantageous for the user to perform `AMMWithdraw(tfLPToken)` and then swap the redundant token.

MediumFreeze status is not checked properly in `AMMWithFixed

In `AMMWithdraw::preclaim()` if `sfAmount`/`sfAmount2` are set, it is checked that * account is authorized to hold the tokens and the trustline exists * AMM account or currency is not frozen * account is not frozen However, if `tfLPToken` is set, it is not checked that account is authorized to hold the tokens. If `tfWithdrawAll` is set, all three checks are skipped. As a result, the frozen account can get tokens from AMM or tokens can be extracted from frozen AMM.

MediumSingle asset deposit fees are inaccurateFixed

It is possible to perform a single asset deposit via `AMMDeposit(tfSingleAsset)`. However, the formulas [(3) and (4)](https://github.com/XRPLF/XRPL-Standards/discussions/78) are inaccurate. Fees are always taken for half of the deposited asset and pool value change due to fees is not accounted for. The variable amount should be virtually swapped with the corresponding fee taken. So the users will prefer `AMMDeposit(tfSingleAsset)` then `AMMWithdraw(tfLPToken)` instead of normal swap to save some fees. Current logic behind single asset deposit formula **(3)**: $$\Delta_{LPTokens} = \Gamma_{LPTokens} * \left[ \left(1+ \frac{\Delta_{B}-TFee*(1-W_B)*\Delta_B}{\Gamma_B} \right) ^ {W_B} - 1 \right] $$ 1. The deposited amount $$ \Delta_B $$ is split into $$ W_B\Delta_B + (1-W_B)\Delta_B = W_B\Delta_B + W_A\Delta_B$$ 2. Then $$W_A\Delta_B$$ of AssetB is virtually swapped for AssetA 3. Pool spot-price $$ SP = \frac{\frac{\Gamma_A}{W_A}}{\frac{\Gamma_B}{W_B}} $$ (see **(II)**) is used to get $$ \Delta_A = W_B\Delta_B \frac{\Gamma_A}{\Gamma_B}$$ 4. Two assets $$\left( \Delta_A, W_B\Delta_B \right) = \left( W_B\Delta_B \frac{\Gamma_A}{\Gamma_B} , W_B\Delta_B \right)$$ are deposited to pool with $$\left( \Gamma_A, \Gamma_B \right)$$ reserves 5. The pool ratio $$ \frac{\Gamma_A}{\Gamma_B} $$ (and therefore pool spot-price) remains the same since $$ \frac{\Gamma'_A}{\Gamma'_B} = \frac{\Gamma_A + W_B\Delta_B \frac{\Gamma_A}{\Gamma_B}}{\Gamma_B + W_B\Delta_B} = \frac{\Gamma_A}{\Gamma_B} $$ 6. Since we virtually swapped $$W_A\Delta_B$$, only $$ TFee W_A $$ was taken, that's why $$ \Delta_B $$ in **(3)** is reduced by $$ \left( 1-TFee(1-W_B)\right) $$ However, the pool state changes during the swap operation, so we can't perform the swap using the spot-price and then perform a deposit using the same spot-price. In case $$ \Delta_B $$ is comparable to $$ \Gamma_B $$ and $$ Tfee $$ is significant, the resulting $$ \Delta_{LPTokens} $$ will be higher than obtainable via real swap and `AMMDeposit(tfTwoAsset)`. See the example in the Scenario section. The proposed logic of single asset deposit is: 1. Let $$ W_A = W_B = 0.5 $$ 2. We want to split $$ \Delta_B $$ into two parts $$ x + (\Delta_B - x) $$ and swap $$ x $$ for $$ \Delta_A = \Gamma_A \frac{x(1-TFee)}{\Gamma_B + x(1-TFee)} $$ (see **(9)**) 3. The $$x$$ should be so that the final pool reserves ratio $$\frac{\Gamma_A}{\Gamma_B+\Delta_B} $$ is equal to the ratio of assets deposited $$ \frac{\Delta_A}{\Delta_B - x} $$ and is equal to the pool reserves ratio after the swap $$ \frac{\Gamma_A - \Delta_A}{\Gamma_B + x} $$ 4. Using $$ \frac{\Gamma_A}{\Gamma_B + \Delta_B} = \frac{\Delta_A}{\Delta_B-x} $$ and substituting $$ \Delta_A $$ we get $$ (\Delta_B - x)(\Gamma_B + x(1-TFee)) = (\Gamma_B + \Delta_B)x(1-TFee) $$ 5. Solving it gives $$ x = \Gamma_B (\sqrt{ \left( \frac{1-TFee/2}{1-TFee} \right) ^2 + \frac{\Delta_B}{\Gamma_B(1-TFee)}} - \frac{1-TFee/2}{1-TFee} )$$ 6. After the swap of $$ x $$ of AssetB for $$ \Delta_A $$ of AssetA, we perform all assets deposit with $$ \Delta_{LPTokens} = \Gamma_{LPTokens} \frac{\Delta_A}{\Gamma_A - \Delta_A} = \Gamma_{LPTokens} \frac{\Delta_B-x}{\Gamma_B + x} $$ The final formula instead of **(3)** would be: $$ \Delta_{LPTokens} = \Gamma_{LPTokens} \frac{\Delta_B-x}{\Gamma_B + x} = \Gamma_{LPTokens} \frac{ \frac{\Delta_B}{\Gamma_B} - (\sqrt{ \left( \frac{1-TFee/2}{1-TFee} \right) ^2 + \frac{\Delta_B}{\Gamma_B(1-TFee)}} - \frac{1-TFee/2}{1-TFee} ) }{1 + {\sqrt{ \left( \frac{1-TFee/2}{1-TFee} \right) ^2 + \frac{\Delta_B}{\Gamma_B(1-TFee)}} - \frac{1-TFee/2}{1-TFee} } } $$

MediumSingle asset withdrawal is unfair for usersFixed

It is possible to perform a single asset withdrawal via `AMMWithdraw(tfSingleAsset)`. However, the formulas [(7) and (8)](https://github.com/XRPLF/XRPL-Standards/discussions/78) are inaccurate. It is more advantageous for the user to perform `AMMWithdraw(tfLPToken)` and then swap the redundant token. We propose to follow this logic: 1. Our goal is to get $$ \Delta_B = b_{withdrawn} + b_{swapped} $$ 2. Assuming we first withdraw $$ x $$ fraction of $$ \Gamma_{LPTokens} $$ 3. That gives $$ (x \Gamma_A, x\Gamma_B) $$ and pool in state $$ ((1-x)\Gamma_A, (1-x)\Gamma_B) $$ 4. Using **(9)** we can swap $$ x\Gamma_A $$ of AssetA for $$ b_{swapped} = \Gamma'_B \frac{x\Gamma_A(1-TFee)}{\Gamma'_A + x\Gamma_A(1-TFee)} $$ of AssetB 5. Using $$ \Delta_B = b_{withdrawn}+ b_{swapped} = x\Gamma_B + \Gamma'_B \frac{x\Gamma_A(1-TFee)}{\Gamma'_A + x\Gamma_A(1-TFee)} $$ we get $$ \Delta_B - x\Gamma_B = (1-x)\Gamma_B \frac{x\Gamma_A(1-TFee)}{(1-x)\Gamma_A + x\Gamma_A(1-TFee)} $$ 6. Simplifying that we get $$ (\frac{\Delta_B}{\Gamma_B} - x)(1-x + x(1-TFee)) = (1-x)x(1-TFee)$$ 7. Which gives $$ x^2 + x(TFee - 2 - \frac{\Delta_B}{\Gamma_B}Tfee) + \frac{\Delta_B}{\Gamma_B} = 0 $$ 8. Updated **(7')** is $$ \frac{\Delta_{LPTokens}}{\Gamma_{LPTokens}}= x = \frac{ \frac{\Delta_B}{\Gamma_B}Tfee + 2 - TFee - \sqrt{(\frac{\Delta_B}{\Gamma_B}Tfee + 2 - TFee)^2 - 4\frac{\Delta_B}{\Gamma_B}}}{2} $$ With current formulas, users should never `AMMWithdraw(tfSingleAsset)` to prevent extra spending.

MediumZero `MinSlotPrice` motivates participant to do nFixed

`MinSlotPrice` is set to zero. So, if `bidMin` is not specified, `sfPrice` is also set to zero according to the formula. There is no motivation for arbitrageurs to raise the price. They can always buy a slot for zero, perform the arbitrage and give up the slot to the next participant for free.

Low`asset_frozen` and `asset2_frozen` are `out` fielFixed

```h=168 JSS(asset_frozen); // in: amm_info JSS(asset2_frozen); // in: amm_info ``` `asset_frozen` and `asset2_frozen` fields are described as `in`, however, they are in fact `out`. `EPrice` is used not only by `AMMDeposit`, but also by `AMMWithdraw`.

Low`bidMin == bidMax` is forbiddenFixed

```cpp=151 if (bidMin && bidMax && bidMin >= bidMax) ``` It is disallowed to have `bidMin == bidMax` in `AMMBid::preclaim()`, however, that is a valid scenario: the user wants to pay exactly `bidMin`, not more.

Show 10 more

Low`invalidAMMAmount()` argument `nonNegative` nameFixed

`invalidAMMAmount()` has the argument `nonNegative` which is `false` by default. The comment states: ```cpp=60 * If zero is false and amount is beast::zero then invalid amount. ``` The code is: ```cpp=101 if (!nonNegative && amount <= beast::zero) return temBAD_AMOUNT; return tesSUCCESS; ``` As a result, the meaning of `!nonNegative` is `mustBePositive`. However, in math "nonNegative" means "zero or positive". Providing `true` as an argument to the function allows the `amount` to be zero. Using negations in bool variable names and negation of the variable itself decreases the code readability.

Low`multiply()` is not usedFixed

```cpp=652 auto const amount2Withdraw = amount2Balance * frac; ``` `multiply(STAmount, STAmount, Issue)` performs some checks and Number normalization, however, it is not used in `AMMWithdraw::equalWithdrawTokens()`.

Low`sfDiscountedFee` is `UINT32`Fixed

`sfDiscountedFee` is stored as `UINT32`, but `sfTradingFee` as `UINT16`. Discounted fee is supposed to be less or equal to normal trading fee. It is unclear why bigger type used.

Low`sfVoteSlots` not updated if not enough LPTokensFixed

The TradingFee of the AMM instance is a votable parameter. Any account that holds the corresponding LPTokens can cast a vote using the new `AMMVote` transaction. During `AMMVote` processing the weights of all the votes are recomputed, and the TradingFee for the AMM instance is updated. Up to 8 voting slots supported. However, `sfVoteSlots` is only updated if a voter is part of it. Consider the scenario: - The Griefer obtains a big amount of LPTokens via `AMMDeposit`. - The Griefer separates the amount into 8 equal parts, distributes it among 8 accounts, and performs `AMMVote(1%)` from each. - All 8 `sfVoteSlots` are now occupied by an account with a significant weight. - The Griefer combines the LPTokens and performs `AMMWithdraw`. - As a result, **no one can refresh the votes array** until they get a significant LPTokens balance.

Low`std::chrono` is not used in `ammAuctionTimeSlot(Fixed

`std::chrono` is not used in `ammAuctionTimeSlot()`.

Low`tFee` is declared as `uint32_t`Fixed

In `changeSpotPriceQuality()` and `withdrawByTokens()` the argument `tFee` is declared as `uint32_t`. In other code it is `uint16_t`.

Low`tfWithdrawAll` is used in `AMMDeposit::preclaim(Fixed

`tfWithdrawAll` flag is checked in `AMMDeposit::preclaim()`, however, it can't be used with `AMMDeposit`.

LowAuction price decay is negligibleFixed

The current slot price is calculated by the formula: $$ f(t) = B * 1.05*(1-t^{60}) + M $$ According to the description: "The slot price approaches `M` as the slot approaches expiration (i.e., as `t` approaches 1)." However, 1. During the first 18 slots the price drops gradually from $$ \approx B * 1.05 + M $$ (with `t = 0.05`) to $$ \approx B * 1.048 + M $$ (with `t = 0.90`) 2. Then drops significantly to $$ \approx B + M $$ (with `t = 0.95`) 3. And then drops to $$ M $$, which is currently set to zero So, the speed of descent is highly uneven.

LowCode can be simplifiedFixed

```cpp=192 return (xrpLiquid(ctx.view, accountID, !sle, ctx.j) >= deposit) ? TER(tesSUCCESS) : (!sle ? tecINSUF_RESERVE_LINE : tecAMM_UNFUNDED); ``` Expressions with ternary conditional operators are harder to read. The expression can be simplified using regular `if`s and `return`.

LowInaccurate commentsFixed

```cpp=122 // Returns the amount an account can spend of the currency type saDefault, or // returns saDefault if this account is the issuer of the the currency in // question. Should be used in favor of accountHolds when questioning how much // an account can spend while also allowing currency issuers to spend // unlimited amounts of their own currency (since they can always issue more). [[nodiscard]] STAmount accountHolds( ReadView const& view, ``` The comment describes `accountFunds()` function, not `accountHolds()`. ```cpp=38 * specified by Asset1In. This is essentially an equal asset deposit * and a swap. ``` In fact, that is a swap and a deposit. ```cpp=48 * a. amount of asset1 if specified in Asset1In specifies the maximum * amount of asset1 that the trader is willing to deposit ``` `amount` is always specified in this mode, however, it can be zero. ```cpp=80 /** Attempt to create the AMM instance. */ ``` The same comment is used in `AMMWithdraw.h`, `AMMDeposit.h`, and other files. It should be removed or updated. ```cpp=535 * Use equation 1 to compute the amount of , given the amount in Asset1In. ``` `t` is missing. ```cpp=153 * @param ctx ``` There is no argument `ctx`.

Show 9 more

LowInaccurate error messageFixed

```cpp=497 JLOG(j.fatal()) << "Invariant failed: account root created " "by a non-Payment or by an unsuccessful transaction"; ``` `ValidNewAccountRoot::finalize()` error message assumes the account can only be created by `ttPAYMENT` transaction. However, `ttAMM_CREATE` should also be mentioned. ```cpp=309 JLOG(j_.debug()) << "AMM Instance: success " << *ammAccount << " " << ammKeylet.key << " " << lptIss << " " << amount << " " << amount2; auto addOrderBook = ``` Using `lpTokens` instead of `lptIss` is preferable. ```cpp=245 << "AMM Bid: not in range " << computedPrice << *bidMin << " " ``` " " is absent between values.

LowLocal Variable ShadowingFixed

```cpp=567 auto const [withdrawAmount, withdraw2Amount] = getAmounts(frac); ``` ```cpp=577 auto const [withdrawAmount, withdraw2Amount] = getAmounts(frac); ``` `withdrawAmount` and `withdraw2Amount` are shadowed by the local variable with the same names.

LowMisleading `time_interval` returned if auction haFixed

```cpp=153 auto const timeSlot = ammAuctionTimeSlot( ledger->info().parentCloseTime.time_since_epoch().count(), auctionSlot); auction[jss::time_interval] = timeSlot ? *timeSlot : 0; ``` `ammAuctionTimeSlot()` returns 0-19 in case the auction has not yet expired and `std::nullopt` otherwise. In case of expired `auction[jss::time_interval]` is set 0. The same value is set during the first 1.2 hours. That could be misleading.

LowRounding error is not handled during auction slotFixed

```cpp=218 auto res = redeemIOU(sb, account_, saBurn, lpTokens.issue(), ctx_.journal); if (res != tesSUCCESS) { JLOG(ctx_.journal.debug()) << "AMM Bid: failed to redeem."; return res; } ammSle->setFieldAmount(sfLPTokenBalance, lptAMMBalance - saBurn); ``` `saBurn` LPTokens are redeemed from `account_` and then `sfLPTokenBalance` is reduced. However, these values can be different due to rounding error.

LowShort initializer list in `generateFibSeqOffer()`Fixed

```cpp=73 constexpr std::uint32_t fib[AMMContext::MaxIterations] = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89, 144, 233, 377, 610, 987, 1597, 2584, 4181, 6765, 10946, 17711, 28657, 46368, 75025, 121393, 196418, 317811, 514229, 832040}; ``` `fib[29]` is zero because the initializer list has only 29 elements. This can be unexpected.

LowThe slot owner always looses money if outbid immeFixed

```cpp=296 // Refund the previous owner. If the time slot is 0 then // the owner is refunded full amount. auto const refund = fractionRemaining * pricePurchased; if (refund > *payPrice) ``` Despite the comment states that the owner is refunded full amount during slot 0, in fact, `fractionRemaining` is maximum 0.95 and the owner always looses money even if outbid in the first second.

LowUsage of Magic NumbersFixed

Different magic numbers are used as-is in code.

LowUser has to `AMMVote` based on other votesFixed

The TradingFee of the AMM instance is a votable parameter. Any account that holds the corresponding LPTokens can cast a vote using the new `AMMVote` transaction. During `AMMVote` processing the weights of all the votes are recomputed, and the TradingFee for the AMM instance is updated. Up to 8 voting slots supported. The voter doesn't know what `sfTradingFee` to set. Consider the scenario: - Imagine that a Voter with weight 10 wants to set the fee to 0.5%. - At that moment all other voters with a sum weight of 10 voted for a 1% fee. - So, Voter has to submit `AMMVote(0%)` to get the desired 0.5%. - After some time other voters change the desired fee to 0%. - The Voter has to re-submit `AMMVote(1%)` to get the desired 0.5%. - As a result, the voter **can't set the desired fee** but rather sets the value that balances the other votes instead. - This can lead to more `AMMVote` submitted than expected.

LowWrong argument namesFixed

The argument `deposit` of `AMMDeposit::deposit()` should be named `view`. `lptTokensAMMBalance` should be named `lptAMMBalance`.