From a61a88ea8104aa48a503dc5543d4738ed395be48 Mon Sep 17 00:00:00 2001
From: Rome Reginelli <rome@ripple.com>
Date: Fri, 1 Sep 2023 13:44:48 -0700
Subject: [PATCH] AMMBid: use tecINTERNAL for 'impossible' errors (#4674)

Modify two error cases in AMMBid transactor to return `tecINTERNAL` to
more clearly indicate that these errors should not be possible unless
operating in unforeseen circumstances. It likely indicates a bug.

The log level has been updated to `fatal()` since it indicates a
(potentially network-wide) unexpected condition when either of these
errors occurs.

Details:

The two specific transaction error cases changed are:

- `tecAMM_BALANCE` - In this case, this error (total LP Tokens
  outstanding is lower than the amount to be burned for the bid) is a
  subset of the case where the user doesn't have enough LP Tokens to pay
  for the bid. When this case is reached, the bidder's LP Tokens balance
  has already been checked first. The user's LP Tokens should always be
  a subset of total LP Tokens issued, so this should be impossible.
- `tecINSUFFICIENT_PAYMENT` - In this case, the amount to be refunded as
  a result of the bid is greater than the price paid for the auction
  slot. This should never occur unless something is wrong with the math
  for calculating the refund amount.

Both error cases in question are "defense in depth" measures meant to
protect against making things worse if the code has already reached a
state that is supposed to be impossible, likely due to a bug elsewhere.

Such "shouldn't ever occur" checks should use an error code that
categorically indicates a larger problem. This is similar to how
`tecINVARIANT_FAILED` is a warning sign that something went wrong and
likely could've been worse, but since there isn't an Invariant Check
applying here, `tecINTERNAL` is the appropriate error code.

This is "debatably" a transaction processing change since it could
hypothetically change how transactions are processed if there's a bug we
don't know about.
---
 src/ripple/app/tx/impl/AMMBid.cpp | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/src/ripple/app/tx/impl/AMMBid.cpp b/src/ripple/app/tx/impl/AMMBid.cpp
index d059f88c76c..822e72203a6 100644
--- a/src/ripple/app/tx/impl/AMMBid.cpp
+++ b/src/ripple/app/tx/impl/AMMBid.cpp
@@ -223,9 +223,11 @@ applyBid(
             lptAMMBalance, toSTAmount(lptAMMBalance.issue(), burn), false);
         if (saBurn >= lptAMMBalance)
         {
-            JLOG(ctx_.journal.debug())
-                << "AMM Bid: invalid burn " << burn << " " << lptAMMBalance;
-            return tecAMM_BALANCE;
+            // This error case should never occur.
+            JLOG(ctx_.journal.fatal())
+                << "AMM Bid: LP Token burn exceeds AMM balance " << burn << " "
+                << lptAMMBalance;
+            return tecINTERNAL;
         }
         auto res =
             redeemIOU(sb, account_, saBurn, lpTokens.issue(), ctx_.journal);
@@ -316,9 +318,10 @@ applyBid(
         auto const refund = fractionRemaining * pricePurchased;
         if (refund > *payPrice)
         {
-            JLOG(ctx_.journal.debug())
-                << "AMM Bid: invalid refund " << refund << " " << *payPrice;
-            return {tecINSUFFICIENT_PAYMENT, false};
+            // This error case should never occur.
+            JLOG(ctx_.journal.fatal()) << "AMM Bid: refund exceeds payPrice "
+                                       << refund << " " << *payPrice;
+            return {tecINTERNAL, false};
         }
         res = accountSend(
             sb,