From 3c17a65cdcad88645c1d615391c74caf1bce7417 Mon Sep 17 00:00:00 2001 From: Michael Birch Date: Mon, 9 Aug 2021 19:16:12 +0000 Subject: [PATCH] Fix(engine): do not panic when user has insufficient balance to cover gas --- src/lib.rs | 29 ++++++++---- src/tests/sanity.rs | 108 ++++++++++++++++++++++++++++---------------- 2 files changed, 89 insertions(+), 48 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 608cfb565..46dab1121 100644 --- a/src/lib.rs +++ b/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); diff --git a/src/tests/sanity.rs b/src/tests/sanity.rs index a33cbc99d..2be54a850 100644 --- a/src/tests/sanity.rs +++ b/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), } }