diff --git a/prdoc/pr_10192.prdoc b/prdoc/pr_10192.prdoc new file mode 100644 index 0000000000000..150d1fe60d2bb --- /dev/null +++ b/prdoc/pr_10192.prdoc @@ -0,0 +1,21 @@ +title: 'revive: Fix dust & child contract calls' +doc: +- audience: Runtime Dev + description: |- + When transferring to self we should just return early as it's a noop. + Not doing so cause bug in `transfer_with_dust` + + as we do + ``` + from.dust -= from.dust + to.dust += to.dust + ``` + + We end up with a value of dust - planck (that we burnt from to create dust amount) on the account + + fix https://github.com/paritytech/contract-issues/issues/211 +crates: +- name: pallet-revive-fixtures + bump: patch +- name: pallet-revive + bump: patch diff --git a/substrate/frame/revive/fixtures/contracts/call_self_with_dust.sol b/substrate/frame/revive/fixtures/contracts/call_self_with_dust.sol new file mode 100644 index 0000000000000..7d44f76d03fe2 --- /dev/null +++ b/substrate/frame/revive/fixtures/contracts/call_self_with_dust.sol @@ -0,0 +1,7 @@ +contract CallSelfWithDust { + function f() external payable {} + + function call() public payable { + this.f{value: 10}(); + } +} diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index 60901419c742d..06b13eb49567c 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -1001,7 +1001,7 @@ where return Ok(None); }, (None, Some(precompile)) if precompile.has_contract_info() => { - log::trace!(target: crate::LOG_TARGET, "found precompile for address {address:?}"); + log::trace!(target: LOG_TARGET, "found precompile for address {address:?}"); if let Some(info) = AccountInfo::::load_contract(&address) { CachedContract::Cached(info) } else { @@ -1526,8 +1526,6 @@ where to: &AccountIdOf, value: BalanceWithDust>, ) -> DispatchResult { - let (value, dust) = value.deconstruct(); - fn transfer_balance( from: &AccountIdOf, to: &AccountIdOf, @@ -1535,7 +1533,7 @@ where ) -> DispatchResult { T::Currency::transfer(from, to, value, Preservation::Preserve) .map_err(|err| { - log::debug!(target: crate::LOG_TARGET, "Transfer failed: from {from:?} to {to:?} (value: ${value:?}). Err: {err:?}"); + log::debug!(target: LOG_TARGET, "Transfer failed: from {from:?} to {to:?} (value: ${value:?}). Err: {err:?}"); Error::::TransferFailed })?; Ok(()) @@ -1552,13 +1550,21 @@ where Ok(()) } + let from_addr = >::to_address(from); + let mut from_info = AccountInfoOf::::get(&from_addr).unwrap_or_default(); + + if from_info.balance(from) < value { + log::debug!(target: LOG_TARGET, "Insufficient balance: from {from:?} to {to:?} (value: ${value:?}). Balance: ${:?}", from_info.balance(from)); + return Err(Error::::TransferFailed.into()) + } else if from == to || value.is_zero() { + return Ok(()) + } + + let (value, dust) = value.deconstruct(); if dust.is_zero() { return transfer_balance::(from, to, value) } - let from_addr = >::to_address(from); - let mut from_info = AccountInfoOf::::get(&from_addr).unwrap_or_default(); - let to_addr = >::to_address(to); let mut to_info = AccountInfoOf::::get(&to_addr).unwrap_or_default(); @@ -1573,7 +1579,7 @@ where Fortitude::Polite, ) .map_err(|err| { - log::debug!(target: crate::LOG_TARGET, "Burning 1 plank from {from:?} failed. Err: {err:?}"); + log::debug!(target: LOG_TARGET, "Burning 1 plank from {from:?} failed. Err: {err:?}"); Error::::TransferFailed })?; @@ -1673,7 +1679,7 @@ where /// Returns the *free* balance of the supplied AccountId. fn account_balance(&self, who: &T::AccountId) -> U256 { - let balance = AccountInfo::::balance(AccountIdOrAddress::AccountId(who.clone())); + let balance = AccountInfo::::balance_of(AccountIdOrAddress::AccountId(who.clone())); crate::Pallet::::convert_native_to_evm(balance) } @@ -1732,7 +1738,7 @@ where delete_code, ); - log::debug!(target: crate::LOG_TARGET, "Contract at {contract_address:?} registered termination. Beneficiary: {beneficiary_address:?}, delete_code: {delete_code}"); + log::debug!(target: LOG_TARGET, "Contract at {contract_address:?} registered termination. Beneficiary: {beneficiary_address:?}, delete_code: {delete_code}"); } /// Returns true if the current context has contract info. diff --git a/substrate/frame/revive/src/lib.rs b/substrate/frame/revive/src/lib.rs index 6833a19238646..8383e71730b0b 100644 --- a/substrate/frame/revive/src/lib.rs +++ b/substrate/frame/revive/src/lib.rs @@ -1860,7 +1860,7 @@ impl Pallet { /// /// Returns the spendable balance excluding the existential deposit. pub fn evm_balance(address: &H160) -> U256 { - let balance = AccountInfo::::balance((*address).into()); + let balance = AccountInfo::::balance_of((*address).into()); Self::convert_native_to_evm(balance) } diff --git a/substrate/frame/revive/src/primitives.rs b/substrate/frame/revive/src/primitives.rs index 2180fd27c467c..1b4a92e64b2f5 100644 --- a/substrate/frame/revive/src/primitives.rs +++ b/substrate/frame/revive/src/primitives.rs @@ -96,7 +96,7 @@ pub enum BalanceConversionError { /// A Balance amount along with some "dust" to represent the lowest decimals that can't be expressed /// in the native currency -#[derive(Default, Clone, Copy, Eq, PartialEq, Debug)] +#[derive(Default, Clone, Copy, Eq, PartialEq, Ord, PartialOrd, Debug)] pub struct BalanceWithDust { /// The value expressed in the native currency value: Balance, diff --git a/substrate/frame/revive/src/storage.rs b/substrate/frame/revive/src/storage.rs index 0033142ec5754..46c175a9b7155 100644 --- a/substrate/frame/revive/src/storage.rs +++ b/substrate/frame/revive/src/storage.rs @@ -32,6 +32,10 @@ use codec::{Decode, Encode, MaxEncodedLen}; use core::marker::PhantomData; use frame_support::{ storage::child::{self, ChildInfo}, + traits::{ + fungible::Inspect, + tokens::{Fortitude::Polite, Preservation::Preserve}, + }, weights::{Weight, WeightMeter}, CloneNoBound, DebugNoBound, DefaultNoBound, }; @@ -157,15 +161,15 @@ impl AccountInfo { } /// Returns the balance of the account at the given address. - pub fn balance(account: AccountIdOrAddress) -> BalanceWithDust> { - use frame_support::traits::{ - fungible::Inspect, - tokens::{Fortitude::Polite, Preservation::Preserve}, - }; + pub fn balance_of(account: AccountIdOrAddress) -> BalanceWithDust> { + let info = >::get(account.address()).unwrap_or_default(); + info.balance(&account.account_id()) + } - let value = T::Currency::reducible_balance(&account.account_id(), Preserve, Polite); - let dust = >::get(account.address()).map(|a| a.dust).unwrap_or_default(); - BalanceWithDust::new_unchecked::(value, dust) + /// Returns the balance of this account info. + pub fn balance(&self, account: &AccountIdOf) -> BalanceWithDust> { + let value = T::Currency::reducible_balance(account, Preserve, Polite); + BalanceWithDust::new_unchecked::(value, self.dust) } /// Loads the contract information for a given address. diff --git a/substrate/frame/revive/src/tests/pvm.rs b/substrate/frame/revive/src/tests/pvm.rs index 71ec22f129966..0cca103e66607 100644 --- a/substrate/frame/revive/src/tests/pvm.rs +++ b/substrate/frame/revive/src/tests/pvm.rs @@ -71,12 +71,15 @@ use sp_runtime::{ fn transfer_with_dust_works() { struct TestCase { description: &'static str, + from: H160, + to: H160, from_balance: BalanceWithDust, to_balance: BalanceWithDust, amount: BalanceWithDust, expected_from_balance: BalanceWithDust, expected_to_balance: BalanceWithDust, total_issuance_diff: i64, + expected_error: Option, } let plank: u32 = ::NativeToEthRatio::get(); @@ -84,73 +87,142 @@ fn transfer_with_dust_works() { let test_cases = vec![ TestCase { description: "without dust", + from: ALICE_ADDR, + to: BOB_ADDR, from_balance: BalanceWithDust::new_unchecked::(100, 0), to_balance: BalanceWithDust::new_unchecked::(0, 0), amount: BalanceWithDust::new_unchecked::(1, 0), expected_from_balance: BalanceWithDust::new_unchecked::(99, 0), expected_to_balance: BalanceWithDust::new_unchecked::(1, 0), total_issuance_diff: 0, + expected_error: None, }, TestCase { description: "with dust", + from: ALICE_ADDR, + to: BOB_ADDR, from_balance: BalanceWithDust::new_unchecked::(100, 0), to_balance: BalanceWithDust::new_unchecked::(0, 0), amount: BalanceWithDust::new_unchecked::(1, 10), expected_from_balance: BalanceWithDust::new_unchecked::(98, plank - 10), expected_to_balance: BalanceWithDust::new_unchecked::(1, 10), total_issuance_diff: 1, + expected_error: None, }, TestCase { description: "just dust", + from: ALICE_ADDR, + to: BOB_ADDR, from_balance: BalanceWithDust::new_unchecked::(100, 0), to_balance: BalanceWithDust::new_unchecked::(0, 0), amount: BalanceWithDust::new_unchecked::(0, 10), expected_from_balance: BalanceWithDust::new_unchecked::(99, plank - 10), expected_to_balance: BalanceWithDust::new_unchecked::(0, 10), total_issuance_diff: 1, + expected_error: None, }, TestCase { description: "with existing dust", + from: ALICE_ADDR, + to: BOB_ADDR, from_balance: BalanceWithDust::new_unchecked::(100, 5), to_balance: BalanceWithDust::new_unchecked::(0, plank - 5), amount: BalanceWithDust::new_unchecked::(1, 10), expected_from_balance: BalanceWithDust::new_unchecked::(98, plank - 5), expected_to_balance: BalanceWithDust::new_unchecked::(2, 5), total_issuance_diff: 0, + expected_error: None, }, TestCase { description: "with enough existing dust", + from: ALICE_ADDR, + to: BOB_ADDR, from_balance: BalanceWithDust::new_unchecked::(100, 10), to_balance: BalanceWithDust::new_unchecked::(0, plank - 10), amount: BalanceWithDust::new_unchecked::(1, 10), expected_from_balance: BalanceWithDust::new_unchecked::(99, 0), expected_to_balance: BalanceWithDust::new_unchecked::(2, 0), total_issuance_diff: -1, + expected_error: None, }, TestCase { description: "receiver dust less than 1 plank", + from: ALICE_ADDR, + to: BOB_ADDR, from_balance: BalanceWithDust::new_unchecked::(100, plank / 10), to_balance: BalanceWithDust::new_unchecked::(0, plank / 2), amount: BalanceWithDust::new_unchecked::(1, plank / 10 * 3), expected_from_balance: BalanceWithDust::new_unchecked::(98, plank / 10 * 8), expected_to_balance: BalanceWithDust::new_unchecked::(1, plank / 10 * 8), total_issuance_diff: 1, + expected_error: None, + }, + TestCase { + description: "insufficient balance", + from: ALICE_ADDR, + to: BOB_ADDR, + from_balance: BalanceWithDust::new_unchecked::(10, 0), + to_balance: BalanceWithDust::new_unchecked::(10, 0), + amount: BalanceWithDust::new_unchecked::(20, 0), + expected_from_balance: BalanceWithDust::new_unchecked::(10, 0), + expected_to_balance: BalanceWithDust::new_unchecked::(10, 0), + total_issuance_diff: 0, + expected_error: Some(Error::::TransferFailed.into()), + }, + TestCase { + description: "from = to with insufficient balance", + from: ALICE_ADDR, + to: ALICE_ADDR, + from_balance: BalanceWithDust::new_unchecked::(10, 0), + to_balance: BalanceWithDust::new_unchecked::(10, 0), + amount: BalanceWithDust::new_unchecked::(20, 0), + expected_from_balance: BalanceWithDust::new_unchecked::(10, 0), + expected_to_balance: BalanceWithDust::new_unchecked::(10, 0), + total_issuance_diff: 0, + expected_error: Some(Error::::TransferFailed.into()), + }, + TestCase { + description: "from = to with insufficient balance", + from: ALICE_ADDR, + to: ALICE_ADDR, + from_balance: BalanceWithDust::new_unchecked::(0, 10), + to_balance: BalanceWithDust::new_unchecked::(0, 10), + amount: BalanceWithDust::new_unchecked::(0, 20), + expected_from_balance: BalanceWithDust::new_unchecked::(0, 10), + expected_to_balance: BalanceWithDust::new_unchecked::(0, 10), + total_issuance_diff: 0, + expected_error: Some(Error::::TransferFailed.into()), + }, + TestCase { + description: "from = to", + from: ALICE_ADDR, + to: ALICE_ADDR, + from_balance: BalanceWithDust::new_unchecked::(0, 10), + to_balance: BalanceWithDust::new_unchecked::(0, 10), + amount: BalanceWithDust::new_unchecked::(0, 5), + expected_from_balance: BalanceWithDust::new_unchecked::(0, 10), + expected_to_balance: BalanceWithDust::new_unchecked::(0, 10), + total_issuance_diff: 0, + expected_error: None, }, ]; for TestCase { description, + from, + to, from_balance, to_balance, amount, expected_from_balance, expected_to_balance, total_issuance_diff, + expected_error, } in test_cases.into_iter() { ExtBuilder::default().build().execute_with(|| { - set_balance_with_dust(&ALICE_ADDR, from_balance); - set_balance_with_dust(&BOB_ADDR, to_balance); + set_balance_with_dust(&from, from_balance); + set_balance_with_dust(&to, to_balance); let total_issuance = ::Currency::total_issuance(); let evm_value = Pallet::::convert_native_to_evm(amount); @@ -159,18 +231,22 @@ fn transfer_with_dust_works() { assert_eq!(Pallet::::has_dust(evm_value), !dust.is_zero()); assert_eq!(Pallet::::has_balance(evm_value), !value.is_zero()); - let result = - builder::bare_call(BOB_ADDR).evm_value(evm_value).build_and_unwrap_result(); - assert_eq!(result, Default::default(), "{description} tx failed"); + let result = builder::bare_call(to).evm_value(evm_value).build(); + + if let Some(expected_error) = expected_error { + assert_err!(result.result, expected_error); + } else { + assert_eq!(result.result.unwrap(), Default::default(), "{description} tx failed"); + } assert_eq!( - Pallet::::evm_balance(&ALICE_ADDR), + Pallet::::evm_balance(&from), Pallet::::convert_native_to_evm(expected_from_balance), "{description}: invalid from balance" ); assert_eq!( - Pallet::::evm_balance(&BOB_ADDR), + Pallet::::evm_balance(&to), Pallet::::convert_native_to_evm(expected_to_balance), "{description}: invalid to balance" ); diff --git a/substrate/frame/revive/src/tests/sol.rs b/substrate/frame/revive/src/tests/sol.rs index 2d240f4a4baa0..fd58c9fa45491 100644 --- a/substrate/frame/revive/src/tests/sol.rs +++ b/substrate/frame/revive/src/tests/sol.rs @@ -31,6 +31,7 @@ use alloy_core::sol_types::{SolCall, SolInterface}; use frame_support::{assert_err, assert_ok, traits::fungible::Mutate}; use pallet_revive_fixtures::{compile_module_with_type, Fibonacci, FixtureType}; use pretty_assertions::assert_eq; +use test_case::test_case; use revm::bytecode::opcode::*; @@ -240,3 +241,27 @@ fn upload_evm_runtime_code_works() { assert_eq!(55u64, decoded, "Contract should correctly compute fibonacci(10)"); }); } + +#[test_case(FixtureType::Solc)] +#[test_case(FixtureType::Resolc)] +fn dust_work_with_child_calls(fixture_type: FixtureType) { + use pallet_revive_fixtures::CallSelfWithDust; + let (code, _) = compile_module_with_type("CallSelfWithDust", fixture_type).unwrap(); + + ExtBuilder::default().build().execute_with(|| { + let _ = ::Currency::set_balance(&ALICE, 100_000_000_000); + let Contract { addr, .. } = + builder::bare_instantiate(Code::Upload(code.clone())).build_and_unwrap_contract(); + + let value = 1_000_000_000.into(); + builder::bare_call(addr) + .data( + CallSelfWithDust::CallSelfWithDustCalls::call(CallSelfWithDust::callCall {}) + .abi_encode(), + ) + .evm_value(value) + .build_and_unwrap_result(); + + assert_eq!(crate::Pallet::::evm_balance(&addr), value); + }); +}