From 04503c9fa4ce1582946c8875e832ed26d5ddb4a5 Mon Sep 17 00:00:00 2001
From: Bronek Kozicki <brok@incorrekt.com>
Date: Tue, 25 Feb 2025 19:37:01 +0000
Subject: [PATCH] Enforce non-negative amounts

---
 src/test/app/Vault_test.cpp               | 49 +++++++++++++++++++++++
 src/xrpld/app/tx/detail/VaultClawback.cpp |  5 +++
 src/xrpld/app/tx/detail/VaultDeposit.cpp  |  3 ++
 src/xrpld/app/tx/detail/VaultWithdraw.cpp |  3 ++
 4 files changed, 60 insertions(+)

diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp
index 6bd6e94be6..3863887878 100644
--- a/src/test/app/Vault_test.cpp
+++ b/src/test/app/Vault_test.cpp
@@ -59,6 +59,25 @@ class Vault_test : public beast::unit_test::suite
                 env(tx, ter(tecINSUFFICIENT_FUNDS));
             }
 
+            {
+                testcase(prefix + " fail to deposit negative amount");
+                auto tx = vault.deposit(
+                    {.depositor = depositor,
+                     .id = keylet.key,
+                     .amount = PrettyAmount(
+                         STAmount(asset.raw(), 1ul, 0, true), "")});
+                env(tx, ter(temBAD_AMOUNT));
+            }
+
+            {
+                testcase(prefix + " fail to deposit zero amount");
+                auto tx = vault.deposit(
+                    {.depositor = depositor,
+                     .id = keylet.key,
+                     .amount = asset(0)});
+                env(tx, ter(temBAD_AMOUNT));
+            }
+
             {
                 testcase(prefix + " deposit non-zero amount");
                 auto tx = vault.deposit(
@@ -120,6 +139,25 @@ class Vault_test : public beast::unit_test::suite
                 env(tx, ter(tecLIMIT_EXCEEDED));
             }
 
+            {
+                testcase(prefix + " fail to withdraw negative amount");
+                auto tx = vault.withdraw(
+                    {.depositor = depositor,
+                     .id = keylet.key,
+                     .amount = PrettyAmount(
+                         STAmount(asset.raw(), 1ul, 0, true), "")});
+                env(tx, ter(temBAD_AMOUNT));
+            }
+
+            {
+                testcase(prefix + " fail to withdraw zero amount");
+                auto tx = vault.withdraw(
+                    {.depositor = depositor,
+                     .id = keylet.key,
+                     .amount = asset(0)});
+                env(tx, ter(temBAD_AMOUNT));
+            }
+
             {
                 testcase(prefix + " fail to withdraw more than assets held");
                 auto tx = vault.withdraw(
@@ -149,6 +187,17 @@ class Vault_test : public beast::unit_test::suite
                 env(tx, ter(tecNO_PERMISSION));
             }
 
+            {
+                testcase(prefix + " fail to clawback negative amount");
+                auto tx = vault.clawback(
+                    {.issuer = issuer,
+                     .id = keylet.key,
+                     .holder = depositor,
+                     .amount = PrettyAmount(
+                         STAmount(asset.raw(), 1ul, 0, true), "")});
+                env(tx, ter(temBAD_AMOUNT));
+            }
+
             {
                 testcase(prefix + " clawback");
                 auto code = asset.raw().native() ? ter(tecNO_PERMISSION)
diff --git a/src/xrpld/app/tx/detail/VaultClawback.cpp b/src/xrpld/app/tx/detail/VaultClawback.cpp
index 9cc8200d13..4d7fb97fdb 100644
--- a/src/xrpld/app/tx/detail/VaultClawback.cpp
+++ b/src/xrpld/app/tx/detail/VaultClawback.cpp
@@ -23,6 +23,7 @@
 #include <xrpl/beast/utility/instrumentation.h>
 #include <xrpl/protocol/Feature.h>
 #include <xrpl/protocol/STNumber.h>
+#include <xrpl/protocol/TER.h>
 #include <xrpl/protocol/TxFlags.h>
 
 namespace ripple {
@@ -39,6 +40,10 @@ VaultClawback::preflight(PreflightContext const& ctx)
     if (ctx.tx.getFlags() & tfUniversalMask)
         return temINVALID_FLAG;
 
+    // Note, zero amount is valid, it means "all". It is also the default.
+    if (ctx.tx[sfAmount] < beast::zero)
+        return temBAD_AMOUNT;
+
     return preflight2(ctx);
 }
 
diff --git a/src/xrpld/app/tx/detail/VaultDeposit.cpp b/src/xrpld/app/tx/detail/VaultDeposit.cpp
index aaad348ebe..ccd1e1cc8e 100644
--- a/src/xrpld/app/tx/detail/VaultDeposit.cpp
+++ b/src/xrpld/app/tx/detail/VaultDeposit.cpp
@@ -44,6 +44,9 @@ VaultDeposit::preflight(PreflightContext const& ctx)
     if (ctx.tx.getFlags() & tfUniversalMask)
         return temINVALID_FLAG;
 
+    if (ctx.tx[sfAmount] <= beast::zero)
+        return temBAD_AMOUNT;
+
     return preflight2(ctx);
 }
 
diff --git a/src/xrpld/app/tx/detail/VaultWithdraw.cpp b/src/xrpld/app/tx/detail/VaultWithdraw.cpp
index 14dae4ce26..92215c1576 100644
--- a/src/xrpld/app/tx/detail/VaultWithdraw.cpp
+++ b/src/xrpld/app/tx/detail/VaultWithdraw.cpp
@@ -41,6 +41,9 @@ VaultWithdraw::preflight(PreflightContext const& ctx)
     if (ctx.tx.getFlags() & tfUniversalMask)
         return temINVALID_FLAG;
 
+    if (ctx.tx[sfAmount] <= beast::zero)
+        return temBAD_AMOUNT;
+
     return preflight2(ctx);
 }