Fix(engine): do not panic when user has insufficient balance to cover gas

src/lib.rs

@@ -77,13 +77,13 @@ mod contract {
     use borsh::{BorshDeserialize, BorshSerialize};
 
     use crate::connector::EthConnectorContract;
-    use crate::engine::{Engine, EngineState};
+    use crate::engine::{Engine, EngineState, GasPaymentError};
     #[cfg(feature = "evm_bully")]
     use crate::parameters::{BeginBlockArgs, BeginChainArgs};
     use crate::parameters::{
         DeployErc20TokenArgs, ExpectUtf8, FunctionCallArgs, GetErc20FromNep141CallArgs,
         GetStorageAtArgs, InitCallArgs, IsUsedProofCallArgs, NEP141FtOnTransferArgs, NewCallArgs,
-        PauseEthConnectorCallArgs, SetContractDataCallArgs, TransactionStatus,
+        PauseEthConnectorCallArgs, SetContractDataCallArgs, SubmitResult, TransactionStatus,
         TransferCallCallArgs, ViewCallArgs,
     };
 
@@ -251,14 +251,25 @@ mod contract {
         // Pay for gas
         let gas_price = signed_transaction.gas_price();
         let prepaid_amount =
-            Engine::charge_gas_limit(&sender, signed_transaction.gas_limit(), gas_price)
-                .map_err(|e| {
-                    // In the error case we still need to increment the nonce since the transaction
-                    // was valid except for a property of the current state (the balance)
+            match Engine::charge_gas_limit(&sender, signed_transaction.gas_limit(), gas_price) {
+                Ok(amount) => amount,
+                // If the account does not have enough funds to cover the gas cost then we still
+                // must increment the nonce to prevent the transaction from being replayed in the
+                // future when the state may have changed such that it could pass.
+                Err(GasPaymentError::OutOfFund) => {
                     Engine::increment_nonce(&sender);
-                    e
-                })
-                .sdk_unwrap();
+                    let result = SubmitResult {
+                        status: TransactionStatus::OutOfFund,
+                        gas_used: 0,
+                        logs: crate::prelude::Vec::new(),
+                    };
+                    sdk::return_output(&result.try_to_vec().unwrap());
+                    return;
+                }
+                // If an overflow happens then the transaction is statically invalid
+                // (i.e. validity does not depend on state), so we do not need to increment the nonce.
+                Err(err) => sdk::panic_utf8(err.as_ref()),
+            };
 
         // Figure out what kind of a transaction this is, and execute it:
         let mut engine = Engine::new_with_state(state, sender);

src/tests/sanity.rs

@@ -1,6 +1,7 @@
 use crate::parameters::{SubmitResult, TransactionStatus};
 use crate::prelude::{Address, U256};
 use crate::test_utils;
+use crate::tests::state_migration;
 use crate::types::{self, Wei, ERC20_MINT_SELECTOR};
 use borsh::BorshSerialize;
 use secp256k1::SecretKey;
@@ -231,11 +232,10 @@ fn test_eth_transfer_charging_gas_not_enough_balance() {
     test_utils::validate_address_balance_and_nonce(&runner, dest_address, Wei::zero(), 0.into());
 
     // attempt transfer
-    let err = runner
+    let result = runner
         .submit_with_signer(&mut source_account, transaction)
-        .unwrap_err();
-    let error_message = format!("{:?}", err);
-    assert!(error_message.contains("ERR_OUT_OF_FUND"));
+        .unwrap();
+    assert_eq!(result.status, TransactionStatus::OutOfFund);
 
     // validate post-state
     let relayer =
@@ -325,35 +325,45 @@ fn test_block_hash_contract() {
 //       test does not need to be written twice.
 #[test]
 fn test_eth_transfer_insufficient_balance_sim() {
-    use crate::tests::state_migration;
+    let (aurora, mut signer, address) = initialize_evm_sim();
 
-    // initalize engine contract
-    let aurora = state_migration::deploy_evm();
-    let mut signer = test_utils::Signer::random();
-    let address = test_utils::address_from_secret_key(&signer.secret_key);
-
-    let args = (address.0, INITIAL_NONCE, INITIAL_BALANCE.raw().low_u64());
-    aurora
-        .call("mint_account", &args.try_to_vec().unwrap())
-        .assert_success();
+    // Run transaction which will fail (transfer more than current balance)
+    let nonce = signer.use_nonce();
+    let tx = test_utils::transfer(
+        Address([1; 20]),
+        INITIAL_BALANCE + INITIAL_BALANCE,
+        nonce.into(),
+    );
+    let signed_tx = test_utils::sign_transaction(
+        tx,
+        Some(test_utils::AuroraRunner::default().chain_id),
+        &signer.secret_key,
+    );
+    let call_result = aurora.call("submit", rlp::encode(&signed_tx).as_ref());
+    let result: SubmitResult = call_result.unwrap_borsh();
+    assert_eq!(result.status, TransactionStatus::OutOfFund);
 
-    // validate pre-state
+    // validate post-state
     assert_eq!(
-        query_address(&address, "get_nonce", &aurora),
-        U256::from(INITIAL_NONCE),
+        query_address_sim(&address, "get_nonce", &aurora),
+        U256::from(INITIAL_NONCE + 1),
     );
     assert_eq!(
-        query_address(&address, "get_balance", &aurora),
+        query_address_sim(&address, "get_balance", &aurora),
         INITIAL_BALANCE.raw(),
     );
+}
 
-    // Run transaction which will fail (transfer more than current balance)
+// Same as `test_eth_transfer_charging_gas_not_enough_balance` but run through `near-sdk-sim`.
+#[test]
+fn test_eth_transfer_charging_gas_not_enough_balance_sim() {
+    let (aurora, mut signer, address) = initialize_evm_sim();
+
+    // Run transaction which will fail (not enough balance to cover gas)
     let nonce = signer.use_nonce();
-    let tx = test_utils::transfer(
-        Address([1; 20]),
-        INITIAL_BALANCE + INITIAL_BALANCE,
-        nonce.into(),
-    );
+    let mut tx = test_utils::transfer(Address([1; 20]), TRANSFER_AMOUNT, nonce.into());
+    tx.gas = 3_000_000.into();
+    tx.gas_price = GAS_PRICE.into();
     let signed_tx = test_utils::sign_transaction(
         tx,
         Some(test_utils::AuroraRunner::default().chain_id),
@@ -365,26 +375,46 @@ fn test_eth_transfer_insufficient_balance_sim() {
 
     // validate post-state
     assert_eq!(
-        query_address(&address, "get_nonce", &aurora),
+        query_address_sim(&address, "get_nonce", &aurora),
         U256::from(INITIAL_NONCE + 1),
     );
     assert_eq!(
-        query_address(&address, "get_balance", &aurora),
+        query_address_sim(&address, "get_balance", &aurora),
+        INITIAL_BALANCE.raw(),
+    );
+}
+
+fn initialize_evm_sim() -> (state_migration::AuroraAccount, test_utils::Signer, Address) {
+    let aurora = state_migration::deploy_evm();
+    let signer = test_utils::Signer::random();
+    let address = test_utils::address_from_secret_key(&signer.secret_key);
+
+    let args = (address.0, INITIAL_NONCE, INITIAL_BALANCE.raw().low_u64());
+    aurora
+        .call("mint_account", &args.try_to_vec().unwrap())
+        .assert_success();
+
+    // validate pre-state
+    assert_eq!(
+        query_address_sim(&address, "get_nonce", &aurora),
+        U256::from(INITIAL_NONCE),
+    );
+    assert_eq!(
+        query_address_sim(&address, "get_balance", &aurora),
         INITIAL_BALANCE.raw(),
     );
 
-    // helper function
-    fn query_address(
-        address: &Address,
-        method: &str,
-        aurora: &state_migration::AuroraAccount,
-    ) -> U256 {
-        let x = aurora.call(method, &address.0);
-        match &x.outcome().status {
-            near_sdk_sim::transaction::ExecutionStatus::SuccessValue(b) => {
-                U256::from_big_endian(&b)
-            }
-            other => panic!("Unexpected outcome: {:?}", other),
-        }
+    (aurora, signer, address)
+}
+
+fn query_address_sim(
+    address: &Address,
+    method: &str,
+    aurora: &state_migration::AuroraAccount,
+) -> U256 {
+    let x = aurora.call(method, &address.0);
+    match &x.outcome().status {
+        near_sdk_sim::transaction::ExecutionStatus::SuccessValue(b) => U256::from_big_endian(&b),
+        other => panic!("Unexpected outcome: {:?}", other),
     }
 }