Fix a bug in TransferSui (MystenLabs#3890)

crates/sui-core/src/transaction_input_checker.rs

@@ -5,6 +5,7 @@ use crate::authority::SuiDataStore;
 use serde::{Deserialize, Serialize};
 use std::collections::HashSet;
 use std::fmt::Debug;
+use sui_types::messages::TransactionKind;
 use sui_types::{
     base_types::{ObjectID, SequenceNumber, SuiAddress},
     error::{SuiError, SuiResult},
@@ -30,7 +31,7 @@ where
         transaction.gas_payment_object_ref().0,
         transaction.data.gas_budget,
         transaction.data.gas_price,
-        transaction.data.kind.is_system_tx(),
+        &transaction.data.kind,
     )
     .await?;
 
@@ -55,12 +56,12 @@ async fn check_gas<S>(
     gas_payment_id: ObjectID,
     gas_budget: u64,
     computation_gas_price: u64,
-    is_system_tx: bool,
+    tx_kind: &TransactionKind,
 ) -> SuiResult<SuiGasStatus<'static>>
 where
     S: Eq + Debug + Serialize + for<'de> Deserialize<'de>,
 {
-    if is_system_tx {
+    if tx_kind.is_system_tx() {
         Ok(SuiGasStatus::new_unmetered())
     } else {
         let gas_object = store.get_object(&gas_payment_id)?;
@@ -74,8 +75,18 @@ where
             .parameters
             .storage_gas_price;
 
+        // If the transaction is TransferSui, we ensure that the gas balance is enough to cover
+        // both gas budget and the transfer amount.
+        let extra_amount =
+            if let TransactionKind::Single(SingleTransactionKind::TransferSui(t)) = tx_kind {
+                t.amount.unwrap_or_default()
+            } else {
+                0
+            };
+        // TODO: We should revisit how we compute gas price and compare to gas budget.
         let gas_price = std::cmp::max(computation_gas_price, storage_gas_price);
-        gas::check_gas_balance(&gas_object, gas_budget, gas_price)?;
+
+        gas::check_gas_balance(&gas_object, gas_budget, gas_price, extra_amount)?;
         let gas_status =
             gas::start_gas_metering(gas_budget, computation_gas_price, storage_gas_price)?;
         Ok(gas_status)

crates/sui-core/src/unit_tests/authority_tests.rs

@@ -26,7 +26,7 @@ use sui_types::{
     crypto::{get_key_pair, Signature},
     crypto::{AccountKeyPair, AuthorityKeyPair, KeypairTraits},
     messages::Transaction,
-    object::{Owner, OBJECT_START_VERSION},
+    object::{Owner, GAS_VALUE_FOR_TESTING, OBJECT_START_VERSION},
     sui_system_state::SuiSystemState,
     SUI_SYSTEM_STATE_OBJECT_ID,
 };
@@ -737,6 +737,34 @@ async fn test_handle_transfer_transaction_double_spend() {
     compare_transaction_info_responses(&signed_transaction, &double_spend_signed_transaction);
 }
 
+#[tokio::test]
+async fn test_handle_transfer_sui_with_amount_insufficient_gas() {
+    let (sender, sender_key): (_, AccountKeyPair) = get_key_pair();
+    let recipient = dbg_addr(2);
+    let object_id = ObjectID::random();
+    let authority_state = init_state_with_ids(vec![(sender, object_id)]).await;
+    let object = authority_state
+        .get_object(&object_id)
+        .await
+        .unwrap()
+        .unwrap();
+    let data = TransactionData::new_transfer_sui(
+        recipient,
+        sender,
+        Some(GAS_VALUE_FOR_TESTING),
+        object.compute_object_reference(),
+        200,
+    );
+    let signature = Signature::new(&data, &sender_key);
+    let transaction = Transaction::new(data, signature);
+
+    let result = authority_state.handle_transaction(transaction).await;
+    assert!(matches!(
+        result.unwrap_err(),
+        SuiError::InsufficientGas { .. }
+    ));
+}
+
 #[tokio::test]
 async fn test_handle_confirmation_transaction_unknown_sender() {
     let recipient = dbg_addr(2);

crates/sui-types/src/gas.rs

@@ -305,7 +305,12 @@ impl<'a> SuiGasStatus<'a> {
 /// 2. If it's enough to pay the flat minimum transaction fee
 /// 3. If it's less than the max gas budget allowed
 /// 4. If the gas_object actually has enough balance to pay for the budget.
-pub fn check_gas_balance(gas_object: &Object, gas_budget: u64, gas_price: u64) -> SuiResult {
+pub fn check_gas_balance(
+    gas_object: &Object,
+    gas_budget: u64,
+    gas_price: u64,
+    extra_amount: u64,
+) -> SuiResult {
     ok_or_gas_error!(
         matches!(gas_object.owner, Owner::AddressOwner(_)),
         "Gas object must be owned Move object".to_owned()
@@ -323,7 +328,7 @@ pub fn check_gas_balance(gas_object: &Object, gas_budget: u64, gas_price: u64) -
     )?;
 
     let balance = get_gas_balance(gas_object)?;
-    let total_amount: u128 = (gas_budget as u128) * (gas_price as u128);
+    let total_amount = (gas_budget as u128) * (gas_price as u128) + extra_amount as u128;
     ok_or_gas_error!(
         (balance as u128) >= total_amount,
         format!("Gas balance is {balance}, not enough to pay {total_amount} with gas price of {gas_price}")