fix(AMM): prevent orphaned objects, inconsistent ledger state: (XRPLF…

…#4626)

When an AMM account is deleted, the owner directory entries must be
deleted in order to ensure consistent ledger state.

* When deleting AMM account:
  * Clean up AMM owner dir, linking AMM account and AMM object
  * Delete trust lines to AMM
* Disallow `CheckCreate` to AMM accounts
  * AMM cannot cash a check
* Constrain entries in AuthAccounts array to be accounts
  * AuthAccounts is an array of objects for the AMMBid transaction
* SetTrust (TrustSet): Allow on AMM only for LP tokens
  * If the destination is an AMM account and the trust line doesn't
    exist, then:
    * If the asset is not the AMM LP token, then fail the tx with
      `tecNO_PERMISSION`
    * If the AMM is in empty state, then fail the tx with `tecAMM_EMPTY`
      * This disallows trustlines to AMM in empty state
* Add AMMID to AMM root account
  * Remove lsfAMM flag and use sfAMMID instead
* Remove owner dir entry for ltAMM
* Add `AMMDelete` transaction type to handle amortized deletion
  * Limit number of trust lines to delete on final withdraw + AMMDelete
  * Put AMM in empty state when LPTokens is 0 upon final withdraw
  * Add `tfTwoAssetIfEmpty` deposit option in AMM empty state
  * Fail all AMM transactions in AMM empty state except special deposit
  * Add `tecINCOMPLETE` to indicate that not all AMM trust lines are
    deleted (i.e. partial deletion)
    * This is handled in Transactor similar to deleted offers
  * Fail AMMDelete with `tecINTERNAL` if AMM root account is nullptr
  * Don't validate for invalid asset pair in AMMDelete
* AMMWithdraw deletes AMM trust lines and AMM account/object only if the
  number of trust lines is less than max
  * Current `maxDeletableAMMTrustLines` = 512
  * Check no directory left after AMM trust lines are deleted
  * Enable partial trustline deletion in AMMWithdraw
* Add `tecAMM_NOT_EMPTY` to fail any transaction that expects an AMM in
  empty state
* Clawback considerations
  * Disallow clawback out of AMM account
  * Disallow AMM create if issuer can claw back

This patch applies to the AMM implementation in XRPLF#4294.

Acknowledgements:
Richard Holland and Nik Bougalis for responsibly disclosing this issue.

Bug Bounties and Responsible Disclosures:
We welcome reviews of the project code and urge researchers to
responsibly disclose any issues they may find.

To report a bug, please send a detailed report to:

    [email protected]

API-CHANGELOG.md

@@ -47,6 +47,28 @@ Additions are intended to be non-breaking (because they are purely additive).
   - Adds the [Clawback transaction type](https://github.com/XRPLF/XRPL-Standards/blob/master/XLS-39d-clawback/README.md#331-clawback-transaction), containing these fields:
     - `Account`: The issuer of the asset being clawed back. Must also be the sender of the transaction.
     - `Amount`: The amount being clawed back, with the `Amount.issuer` being the token holder's address.
+- Adds [AMM](https://github.com/XRPLF/XRPL-Standards/discussions/78) ([#4294](https://github.com/XRPLF/rippled/pull/4294), [#4626](https://github.com/XRPLF/rippled/pull/4626)) feature:
+    - Adds `amm_info` API to retrieve AMM information for a given tokens pair.
+    - Adds `AMMCreate` transaction type to create `AMM` instance.
+    - Adds `AMMDeposit` transaction type to deposit funds into `AMM` instance.
+    - Adds `AMMWithdraw` transaction type to withdraw funds from `AMM` instance.
+    - Adds `AMMVote` transaction type to vote for the trading fee of `AMM` instance.
+    - Adds `AMMBid` transaction type to bid for the Auction Slot of `AMM` instance.
+    - Adds `AMMDelete` transaction type to delete `AMM` instance.
+    - Adds `sfAMMID` to `AccountRoot` to indicate that the account is `AMM`'s account. `AMMID` is used to fetch `ltAMM`.
+    - Adds `lsfAMMNode` `TrustLine` flag to indicate that one side of the `TrustLine` is `AMM` account.
+    - Adds `tfLPToken`, `tfSingleAsset`, `tfTwoAsset`, `tfOneAssetLPToken`, `tfLimitLPToken`, `tfTwoAssetIfEmpty`,
+      `tfWithdrawAll`, `tfOneAssetWithdrawAll` which allow a trader to specify different fields combination 
+      for `AMMDeposit` and `AMMWithdraw` transactions.
+    - Adds new transaction result codes:
+       - tecUNFUNDED_AMM: insufficient balance to fund AMM. The account does not have funds for liquidity provision.
+       -  tecAMM_BALANCE: AMM has invalid balance. Calculated balances greater than the current pool balances.
+       -  tecAMM_FAILED: AMM transaction failed. Fails due to a processing failure.
+       -  tecAMM_INVALID_TOKENS: AMM invalid LP tokens. Invalid input values, format, or calculated values.
+       -  tecAMM_EMPTY: AMM is in empty state. Transaction expects AMM in non-empty state (LP tokens > 0).
+       -  tecAMM_NOT_EMPTY: AMM is not in empty state. Transaction expects AMM in empty state (LP tokens == 0).
+       -  tecAMM_ACCOUNT: AMM account. Clawback of AMM account.
+       -  tecINCOMPLETE: Some work was completed, but more submissions required to finish. AMMDelete partially deletes the trustlines.
 
 ## XRP Ledger version 1.11.0
 

Builds/CMake/RippledCore.cmake

@@ -476,6 +476,7 @@ target_sources (rippled PRIVATE
   src/ripple/app/rdb/impl/Wallet.cpp
   src/ripple/app/tx/impl/AMMBid.cpp
   src/ripple/app/tx/impl/AMMCreate.cpp
+  src/ripple/app/tx/impl/AMMDelete.cpp
   src/ripple/app/tx/impl/AMMDeposit.cpp
   src/ripple/app/tx/impl/AMMVote.cpp
   src/ripple/app/tx/impl/AMMWithdraw.cpp

src/ripple/app/misc/AMMUtils.h

@@ -93,6 +93,26 @@ ammAccountHolds(
     AccountID const& ammAccountID,
     Issue const& issue);
 
+/** Delete trustlines to AMM. If all trustlines are deleted then
+ * AMM object and account are deleted. Otherwise tecIMPCOMPLETE is returned.
+ */
+TER
+deleteAMMAccount(
+    Sandbox& view,
+    Issue const& asset,
+    Issue const& asset2,
+    beast::Journal j);
+
+/** Initialize Auction and Voting slots and set the trading/discounted fee.
+ */
+void
+initializeFeeAuctionVote(
+    ApplyView& view,
+    std::shared_ptr<SLE>& ammSle,
+    AccountID const& account,
+    Issue const& lptIssue,
+    std::uint16_t tfee);
+
 }  // namespace ripple
 
 #endif  // RIPPLE_APP_MISC_AMMUTILS_H_INLCUDED

src/ripple/app/misc/impl/AMMUtils.cpp

@@ -188,4 +188,122 @@ ammAccountHolds(
     return STAmount{issue};
 }
 
+static TER
+deleteAMMTrustLines(
+    Sandbox& sb,
+    AccountID const& ammAccountID,
+    std::uint16_t maxTrustlinesToDelete,
+    beast::Journal j)
+{
+    return cleanupOnAccountDelete(
+        sb,
+        keylet::ownerDir(ammAccountID),
+        [&](LedgerEntryType nodeType,
+            uint256 const&,
+            std::shared_ptr<SLE>& sleItem) -> TER {
+            // Should only have the trustlines
+            if (nodeType != LedgerEntryType::ltRIPPLE_STATE)
+            {
+                JLOG(j.error())
+                    << "deleteAMMTrustLines: deleting non-trustline "
+                    << nodeType;
+                return tecINTERNAL;
+            }
+
+            // Trustlines must have zero balance
+            if (sleItem->getFieldAmount(sfBalance) != beast::zero)
+            {
+                JLOG(j.error())
+                    << "deleteAMMTrustLines: deleting trustline with "
+                       "non-zero balance.";
+                return tecINTERNAL;
+            }
+
+            return deleteAMMTrustLine(sb, sleItem, ammAccountID, j);
+        },
+        j,
+        maxTrustlinesToDelete);
+}
+
+TER
+deleteAMMAccount(
+    Sandbox& sb,
+    Issue const& asset,
+    Issue const& asset2,
+    beast::Journal j)
+{
+    auto ammSle = sb.peek(keylet::amm(asset, asset2));
+    if (!ammSle)
+    {
+        JLOG(j.error()) << "deleteAMMAccount: AMM object does not exist "
+                        << asset << " " << asset2;
+        return tecINTERNAL;
+    }
+
+    auto const ammAccountID = (*ammSle)[sfAccount];
+    auto sleAMMRoot = sb.peek(keylet::account(ammAccountID));
+    if (!sleAMMRoot)
+    {
+        JLOG(j.error()) << "deleteAMMAccount: AMM account does not exist "
+                        << to_string(ammAccountID);
+        return tecINTERNAL;
+    }
+
+    if (auto const ter =
+            deleteAMMTrustLines(sb, ammAccountID, maxDeletableAMMTrustLines, j);
+        ter != tesSUCCESS)
+        return ter;
+
+    auto const ownerDirKeylet = keylet::ownerDir(ammAccountID);
+    if (sb.exists(ownerDirKeylet) && !sb.emptyDirDelete(ownerDirKeylet))
+    {
+        JLOG(j.error()) << "deleteAMMAccount: cannot delete root dir node of "
+                        << toBase58(ammAccountID);
+        return tecINTERNAL;
+    }
+
+    sb.erase(ammSle);
+    sb.erase(sleAMMRoot);
+
+    return tesSUCCESS;
+}
+
+void
+initializeFeeAuctionVote(
+    ApplyView& view,
+    std::shared_ptr<SLE>& ammSle,
+    AccountID const& account,
+    Issue const& lptIssue,
+    std::uint16_t tfee)
+{
+    // AMM creator gets the voting slot.
+    STArray voteSlots;
+    STObject voteEntry{sfVoteEntry};
+    if (tfee != 0)
+        voteEntry.setFieldU16(sfTradingFee, tfee);
+    voteEntry.setFieldU32(sfVoteWeight, VOTE_WEIGHT_SCALE_FACTOR);
+    voteEntry.setAccountID(sfAccount, account);
+    voteSlots.push_back(voteEntry);
+    ammSle->setFieldArray(sfVoteSlots, voteSlots);
+    // AMM creator gets the auction slot for free.
+    auto& auctionSlot = ammSle->peekFieldObject(sfAuctionSlot);
+    auctionSlot.setAccountID(sfAccount, account);
+    // current + sec in 24h
+    auto const expiration = std::chrono::duration_cast<std::chrono::seconds>(
+                                view.info().parentCloseTime.time_since_epoch())
+                                .count() +
+        TOTAL_TIME_SLOT_SECS;
+    auctionSlot.setFieldU32(sfExpiration, expiration);
+    auctionSlot.setFieldAmount(sfPrice, STAmount{lptIssue, 0});
+    // Set the fee
+    if (tfee != 0)
+        ammSle->setFieldU16(sfTradingFee, tfee);
+    else if (ammSle->isFieldPresent(sfTradingFee))
+        ammSle->makeFieldAbsent(sfTradingFee);
+    if (auto const dfee = tfee / AUCTION_SLOT_DISCOUNTED_FEE_FRACTION)
+        auctionSlot.setFieldU16(sfDiscountedFee, dfee);
+    else if (auctionSlot.isFieldPresent(sfDiscountedFee))
+        auctionSlot.makeFieldAbsent(sfDiscountedFee);
+}
+
 }  // namespace ripple

src/ripple/app/paths/impl/BookStep.cpp

@@ -99,7 +99,8 @@ class BookStep : public StepImp<TIn, TOut, BookStep<TIn, TOut, TDerived>>
         , ownerPaysTransferFee_(ctx.ownerPaysTransferFee)
         , j_(ctx.j)
     {
-        if (auto const ammSle = ctx.view.read(keylet::amm(in, out)))
+        if (auto const ammSle = ctx.view.read(keylet::amm(in, out));
+            ammSle && ammSle->getFieldAmount(sfLPTokenBalance) != beast::zero)
             ammLiquidity_.emplace(
                 ctx.view,
                 (*ammSle)[sfAccount],

src/ripple/app/tx/impl/AMMBid.cpp

@@ -94,6 +94,10 @@ AMMBid::preclaim(PreclaimContext const& ctx)
         return terNO_AMM;
     }
 
+    auto const lpTokensBalance = (*ammSle)[sfLPTokenBalance];
+    if (lpTokensBalance == beast::zero)
+        return tecAMM_EMPTY;
+
     if (ctx.tx.isFieldPresent(sfAuthAccounts))
     {
         for (auto const& account : ctx.tx.getFieldArray(sfAuthAccounts))
@@ -114,7 +118,6 @@ AMMBid::preclaim(PreclaimContext const& ctx)
         JLOG(ctx.j.debug()) << "AMM Bid: account is not LP.";
         return tecAMM_INVALID_TOKENS;
     }
-    auto const lpTokensBalance = (*ammSle)[sfLPTokenBalance];
 
     auto const bidMin = ctx.tx[~sfBidMin];
 
@@ -204,10 +207,10 @@ applyBid(
                           Number const& burn) -> TER {
         auctionSlot.setAccountID(sfAccount, account_);
         auctionSlot.setFieldU32(sfExpiration, current + TOTAL_TIME_SLOT_SECS);
-        if (fee == 0)
-            auctionSlot.makeFieldAbsent(sfDiscountedFee);
-        else
+        if (fee != 0)
             auctionSlot.setFieldU16(sfDiscountedFee, fee);
+        else if (auctionSlot.isFieldPresent(sfDiscountedFee))
+            auctionSlot.makeFieldAbsent(sfDiscountedFee);
         auctionSlot.setFieldAmount(
             sfPrice, toSTAmount(lpTokens.issue(), minPrice));
         if (ctx_.tx.isFieldPresent(sfAuthAccounts))

src/ripple/app/tx/impl/AMMCreate.cpp

@@ -173,7 +173,7 @@ AMMCreate::preclaim(PreclaimContext const& ctx)
     auto isLPToken = [&](STAmount const& amount) -> bool {
         if (auto const sle =
                 ctx.view.read(keylet::account(amount.issue().account)))
-            return (sle->getFlags() & lsfAMM);
+            return sle->isFieldPresent(sfAMMID);
         return false;
     };
 
@@ -184,7 +184,21 @@ AMMCreate::preclaim(PreclaimContext const& ctx)
         return tecAMM_INVALID_TOKENS;
     }
 
-    return tesSUCCESS;
+    // Disallow AMM if the issuer has clawback enabled
+    auto clawbackDisabled = [&](Issue const& issue) -> TER {
+        if (isXRP(issue))
+            return tesSUCCESS;
+        if (auto const sle = ctx.view.read(keylet::account(issue.account));
+            !sle)
+            return tecINTERNAL;
+        else if (sle->getFlags() & lsfAllowTrustLineClawback)
+            return tecNO_PERMISSION;
+        return tesSUCCESS;
+    };
+
+    if (auto const ter = clawbackDisabled(amount.issue()); ter != tesSUCCESS)
+        return ter;
+    return clawbackDisabled(amount2.issue());
 }
 
 static std::pair<TER, bool>
@@ -247,54 +261,26 @@ applyCreate(
     // A user can only receive LPTokens through affirmative action -
     // either an AMMDeposit, TrustSet, crossing an offer, etc.
     sleAMMRoot->setFieldU32(
-        sfFlags, lsfAMM | lsfDisableMaster | lsfDefaultRipple | lsfDepositAuth);
+        sfFlags, lsfDisableMaster | lsfDefaultRipple | lsfDepositAuth);
+    // Link the root account and AMM object
+    sleAMMRoot->setFieldH256(sfAMMID, ammKeylet.key);
     sb.insert(sleAMMRoot);
 
     // Calculate initial LPT balance.
     auto const lpTokens = ammLPTokens(amount, amount2, lptIss);
 
     // Create ltAMM
     auto ammSle = std::make_shared<SLE>(ammKeylet);
-    if (ctx_.tx[sfTradingFee] != 0)
-        ammSle->setFieldU16(sfTradingFee, ctx_.tx[sfTradingFee]);
     ammSle->setAccountID(sfAccount, *ammAccount);
     ammSle->setFieldAmount(sfLPTokenBalance, lpTokens);
     auto const& [issue1, issue2] = std::minmax(amount.issue(), amount2.issue());
     ammSle->setFieldIssue(sfAsset, STIssue{sfAsset, issue1});
     ammSle->setFieldIssue(sfAsset2, STIssue{sfAsset2, issue2});
-    // AMM creator gets the voting slot.
-    STArray voteSlots;
-    STObject voteEntry{sfVoteEntry};
-    if (ctx_.tx[sfTradingFee] != 0)
-        voteEntry.setFieldU16(sfTradingFee, ctx_.tx[sfTradingFee]);
-    voteEntry.setFieldU32(sfVoteWeight, 100000);
-    voteEntry.setAccountID(sfAccount, account_);
-    voteSlots.push_back(voteEntry);
-    ammSle->setFieldArray(sfVoteSlots, voteSlots);
-    // AMM creator gets the auction slot for free.
-    auto& auctionSlot = ammSle->peekFieldObject(sfAuctionSlot);
-    auctionSlot.setAccountID(sfAccount, account_);
-    // current + sec in 24h
-    auto const expiration =
-        std::chrono::duration_cast<std::chrono::seconds>(
-            ctx_.view().info().parentCloseTime.time_since_epoch())
-            .count() +
-        TOTAL_TIME_SLOT_SECS;
-    auctionSlot.setFieldU32(sfExpiration, expiration);
-    auctionSlot.setFieldAmount(sfPrice, STAmount{lpTokens.issue(), 0});
+    // AMM creator gets the auction slot and the voting slot.
+    initializeFeeAuctionVote(
+        ctx_.view(), ammSle, account_, lptIss, ctx_.tx[sfTradingFee]);
     sb.insert(ammSle);
 
-    // Add owner directory to link the root account and AMM object.
-    if (auto const page = sb.dirInsert(
-            keylet::ownerDir(*ammAccount),
-            ammSle->key(),
-            describeOwnerDir(*ammAccount));
-        !page)
-    {
-        JLOG(j_.debug()) << "AMM Instance: failed to insert owner dir";
-        return {tecDIR_FULL, false};
-    }
-
     // Send LPT to LP.
     auto res = accountSend(sb, *ammAccount, account_, lpTokens, ctx_.journal);
     if (res != tesSUCCESS)