From f960d0d05587f41a0daf202a0c858f5dc91d0e97 Mon Sep 17 00:00:00 2001
From: George Kudrayvtsev <george@stellar.org>
Date: Tue, 20 Feb 2024 16:18:28 -0800
Subject: [PATCH] Add tests for various overflow cases

---
 cmd/crates/soroban-rpc/src/txn.rs | 110 ++++++++++++++++++++++++------
 1 file changed, 90 insertions(+), 20 deletions(-)

diff --git a/cmd/crates/soroban-rpc/src/txn.rs b/cmd/crates/soroban-rpc/src/txn.rs
index 313caf80..8352375c 100644
--- a/cmd/crates/soroban-rpc/src/txn.rs
+++ b/cmd/crates/soroban-rpc/src/txn.rs
@@ -323,8 +323,8 @@ pub fn assemble(
                 + u32::try_from(simulation.min_resource_fee)
                     .map_err(|_| Error::LargeFee(simulation.min_resource_fee))?,
         ),
-    ) / 100
-        * 115;
+    ) * 115
+        / 100;
     tx.fee = u32::try_from(padded_fee).map_err(|_| Error::LargeFee(padded_fee))?;
 
     tx.operations = vec![op].try_into()?;
@@ -612,6 +612,27 @@ mod tests {
         }
     }
 
+    fn non_soroban_transaction() -> Transaction {
+        let source_bytes = Ed25519PublicKey::from_string(SOURCE).unwrap().0;
+        Transaction {
+            source_account: MuxedAccount::Ed25519(Uint256(source_bytes)),
+            fee: 100,
+            seq_num: SequenceNumber(0),
+            cond: Preconditions::None,
+            memo: Memo::None,
+            operations: vec![Operation {
+                source_account: None,
+                body: OperationBody::ChangeTrust(ChangeTrustOp {
+                    line: ChangeTrustAsset::Native,
+                    limit: 0,
+                }),
+            }]
+            .try_into()
+            .unwrap(),
+            ext: TransactionExt::V0,
+        }
+    }
+
     #[test]
     fn test_assemble_transaction_updates_tx_data_from_simulation_response() {
         let sim = simulation_response();
@@ -669,24 +690,7 @@ mod tests {
 
     #[test]
     fn test_assemble_transaction_errors_for_non_invokehostfn_ops() {
-        let source_bytes = Ed25519PublicKey::from_string(SOURCE).unwrap().0;
-        let txn = Transaction {
-            source_account: MuxedAccount::Ed25519(Uint256(source_bytes)),
-            fee: 100,
-            seq_num: SequenceNumber(0),
-            cond: Preconditions::None,
-            memo: Memo::None,
-            operations: vec![Operation {
-                source_account: None,
-                body: OperationBody::ChangeTrust(ChangeTrustOp {
-                    line: ChangeTrustAsset::Native,
-                    limit: 0,
-                }),
-            }]
-            .try_into()
-            .unwrap(),
-            ext: TransactionExt::V0,
-        };
+        let txn = non_soroban_transaction();
 
         let result = assemble(
             &txn,
@@ -725,4 +729,70 @@ mod tests {
             r => panic!("expected UnexpectedSimulateTransactionResultSize error, got: {r:#?}"),
         }
     }
+
+    #[test]
+    fn test_assemble_transaction_overflow_behavior() {
+        // Test three separate cases:
+        //
+        //  1. Given a near-max (U32_MAX) resource fee, ensure the "wiggle room"
+        //     doesn't cause an overflow due to correct math order.
+        //     (Specifically, do U32_MAX - 15% - 100 - 1, so the final fee is
+        //     U32_MAX-1.)
+        //
+        //  3. Given a near-max (U32_MAX) resource fee that will ONLY exceed
+        //     U32_MAX *with* the wiggle room, ensure the overflow is caught
+        //     with an error rather than silently ignored.
+        //
+        //  2. Given a large resource fee that WILL exceed on its own, ensure
+        //     the overflow is caught with an error rather than silently
+        //     ignored.
+        //
+        let txn = single_contract_fn_transaction();
+        let mut response = simulation_response();
+
+        // sanity check so these can be adjusted if the above helper changes
+        assert_eq!(txn.fee, 100, "modified txn.fee: update the math below");
+
+        // 1: wiggle room math overflows but result fits
+        let mut resource_fee = ((u32::MAX as f64) * 0.85).floor() as u64 - 100 - 1;
+        assert_eq!(resource_fee, 3650722099);
+        response.min_resource_fee = resource_fee;
+
+        match assemble(&txn, &response) {
+            Ok(_) => {}
+            r => {
+                panic!("expected success, got: {r:#?}")
+            }
+        }
+
+        // 2: combo works but wiggle room overflows
+        resource_fee = ((u32::MAX as f64) * 0.90).floor() as u64;
+        assert_eq!(resource_fee, 3865470565);
+        response.min_resource_fee = resource_fee;
+
+        match assemble(&txn, &response) {
+            Err(Error::LargeFee(fee)) => {
+                let expected = (((resource_fee + 100) as f64) * 1.15).floor() as u64;
+                assert_eq!(expected, fee, "expected {} != {} actual", expected, fee);
+            }
+            r => {
+                panic!("expected LargeFee error, got: {r:#?}")
+            }
+        }
+
+        // 2: combo overflows
+        resource_fee = (u32::MAX - 100) as u64;
+        assert_eq!(resource_fee, 4294967195);
+        response.min_resource_fee = resource_fee;
+
+        match assemble(&txn, &response) {
+            Err(Error::LargeFee(fee)) => {
+                let expected = (((resource_fee + 100) as f64) * 1.15).floor() as u64;
+                assert_eq!(expected, fee, "expected {} != {} actual", expected, fee);
+            }
+            r => {
+                panic!("expected LargeFee error, got: {r:#?}")
+            }
+        }
+    }
 }