diff --git a/prdoc/pr_10309.prdoc b/prdoc/pr_10309.prdoc new file mode 100644 index 0000000000000..86ede5e86d9de --- /dev/null +++ b/prdoc/pr_10309.prdoc @@ -0,0 +1,10 @@ +title: '[pallet-revive] evm remove contract storage slot when writing all zero bytes' +doc: +- audience: Runtime Dev + description: |- + fixes https://github.com/paritytech/contract-issues/issues/216 + + When writing all zero bytes to a storage item, the item shall be deleted and deposit refunded. +crates: +- name: pallet-revive + bump: patch diff --git a/substrate/frame/revive/src/tests/pvm.rs b/substrate/frame/revive/src/tests/pvm.rs index 5361efdd9f17f..43cc5e2f73b1a 100644 --- a/substrate/frame/revive/src/tests/pvm.rs +++ b/substrate/frame/revive/src/tests/pvm.rs @@ -5230,7 +5230,7 @@ fn consume_all_gas_works() { } #[test] -fn existential_deposit_shall_not_charged_twice() { +fn existential_deposit_shall_not_be_charged_twice() { let (code, _) = compile_module("dummy").unwrap(); let salt = [0u8; 32]; diff --git a/substrate/frame/revive/src/tests/sol/host.rs b/substrate/frame/revive/src/tests/sol/host.rs index eaef4fa753797..a62f2bb9120c0 100644 --- a/substrate/frame/revive/src/tests/sol/host.rs +++ b/substrate/frame/revive/src/tests/sol/host.rs @@ -19,14 +19,14 @@ use crate::{ address::AddressMapper, test_utils::{builder::Contract, ALICE, BOB, BOB_ADDR}, - tests::{builder, test_utils, ExtBuilder, RuntimeEvent, Test}, + tests::{builder, test_utils, test_utils::get_contract, ExtBuilder, RuntimeEvent, Test}, Code, Config, Error, Key, System, H256, U256, }; use frame_support::assert_err_ignore_postinfo; use alloy_core::sol_types::{SolCall, SolInterface}; use frame_support::traits::{fungible::Mutate, Get}; -use pallet_revive_fixtures::{compile_module_with_type, FixtureType, Host}; +use pallet_revive_fixtures::{compile_module_with_type, Caller, FixtureType, Host}; use pretty_assertions::assert_eq; use test_case::test_case; @@ -606,3 +606,178 @@ fn logs_denied_for_static_call(caller_type: FixtureType, callee_type: FixtureTyp assert_eq!(decoded_result.success, false); }); } + +#[test_case(FixtureType::Solc)] +#[test_case(FixtureType::Resolc)] +fn reading_empty_storage_item_returns_zero_simple(fixture_type: FixtureType) { + let (host_code, _) = compile_module_with_type("Host", fixture_type).unwrap(); + + ExtBuilder::default().build().execute_with(|| { + ::Currency::set_balance(&ALICE, 100_000_000_000); + + let Contract { addr: host_addr, .. } = + builder::bare_instantiate(Code::Upload(host_code)).build_and_unwrap_contract(); + + let index = 13u64; + + // read non-existent storage item + let result = builder::bare_call(host_addr) + .data(Host::sloadOpCall { slot: index }.abi_encode()) + .build_and_unwrap_result(); + + let decoded = Host::sloadOpCall::abi_decode_returns(&result.data).unwrap(); + assert_eq!(decoded, 0u64, "sloadOpCall should return zero for empty storage item"); + }) +} + +#[test_case(FixtureType::Solc, FixtureType::Solc; "solc->solc")] +#[test_case(FixtureType::Solc, FixtureType::Resolc; "solc->resolc")] +#[test_case(FixtureType::Resolc, FixtureType::Solc; "resolc->solc")] +#[test_case(FixtureType::Resolc, FixtureType::Resolc; "resolc->resolc")] +fn reading_empty_storage_item_returns_zero_delegatecall( + caller_type: FixtureType, + callee_type: FixtureType, +) { + let (caller_code, _) = compile_module_with_type("Caller", caller_type).unwrap(); + let (host_code, _) = compile_module_with_type("Host", callee_type).unwrap(); + + ExtBuilder::default().build().execute_with(|| { + ::Currency::set_balance(&ALICE, 100_000_000_000); + + let Contract { addr: host_addr, .. } = + builder::bare_instantiate(Code::Upload(host_code)).build_and_unwrap_contract(); + + let Contract { addr: caller_addr, .. } = + builder::bare_instantiate(Code::Upload(caller_code)).build_and_unwrap_contract(); + + let index = 13u64; + + // read non-existent storage item + let result = builder::bare_call(caller_addr) + .data( + Caller::delegateCall { + _callee: host_addr.0.into(), + _data: Host::sloadOpCall { slot: index }.abi_encode().into(), + _gas: u64::MAX, + } + .abi_encode(), + ) + .build_and_unwrap_result(); + assert!(!result.did_revert(), "delegateCall reverted"); + let result = Caller::delegateCall::abi_decode_returns(&result.data).unwrap(); + + assert!(result.success, "sloadOpCall did not succeed"); + let decoded = Host::sloadOpCall::abi_decode_returns(&result.output).unwrap(); + assert_eq!(decoded, 0u64, "sloadOpCall should return zero for empty storage item"); + }) +} + +#[test_case(FixtureType::Solc)] +#[test_case(FixtureType::Resolc)] +fn storage_item_zero_shall_refund_deposit_simple(fixture_type: FixtureType) { + let (host_code, _) = compile_module_with_type("Host", fixture_type).unwrap(); + + ExtBuilder::default().build().execute_with(|| { + ::Currency::set_balance(&ALICE, 100_000_000_000); + + let Contract { addr: host_addr, .. } = + builder::bare_instantiate(Code::Upload(host_code)).build_and_unwrap_contract(); + + let index = 13u64; + + let base_deposit = get_contract(&host_addr).total_deposit(); + + // write storage item + let result = builder::bare_call(host_addr) + .data(Host::sstoreOpCall { slot: index, value: 17u64 }.abi_encode()) + .build_and_unwrap_result(); + assert!(!result.did_revert(), "sstoreOpCall reverted"); + + // 32 for key, 32 for value, 2 for item + assert_eq!( + get_contract(&host_addr).total_deposit(), + base_deposit + 32 + 32 + 2, + "Unexpected deposit sum charged for non-zero storage item" + ); + + // write storage item to all zeros + let result = builder::bare_call(host_addr) + .data(Host::sstoreOpCall { slot: index, value: 0u64 }.abi_encode()) + .build_and_unwrap_result(); + assert!(!result.did_revert(), "sstoreOpCall reverted"); + + assert_eq!( + get_contract(&host_addr).total_deposit(), + base_deposit, + "contract should refund deposit on zeroing storage item" + ); + }); +} + +#[test_case(FixtureType::Solc, FixtureType::Solc; "solc->solc")] +#[test_case(FixtureType::Solc, FixtureType::Resolc; "solc->resolc")] +#[test_case(FixtureType::Resolc, FixtureType::Solc; "resolc->solc")] +#[test_case(FixtureType::Resolc, FixtureType::Resolc; "resolc->resolc")] +fn storage_item_zero_shall_refund_deposit_delegatecall( + caller_type: FixtureType, + callee_type: FixtureType, +) { + let (caller_code, _) = compile_module_with_type("Caller", caller_type).unwrap(); + let (host_code, _) = compile_module_with_type("Host", callee_type).unwrap(); + + ExtBuilder::default().build().execute_with(|| { + ::Currency::set_balance(&ALICE, 100_000_000_000); + + let Contract { addr: host_addr, .. } = + builder::bare_instantiate(Code::Upload(host_code)).build_and_unwrap_contract(); + + let Contract { addr: caller_addr, .. } = + builder::bare_instantiate(Code::Upload(caller_code)).build_and_unwrap_contract(); + + let index = 13u64; + + let base_deposit = get_contract(&caller_addr).total_deposit(); + + // write storage item + let result = builder::bare_call(caller_addr) + .data( + Caller::delegateCall { + _callee: host_addr.0.into(), + _data: Host::sstoreOpCall { slot: index, value: 17u64 }.abi_encode().into(), + _gas: u64::MAX, + } + .abi_encode(), + ) + .build_and_unwrap_result(); + let result = Caller::delegateCall::abi_decode_returns(&result.data).unwrap(); + assert!(result.success, "delegateCall did not succeed"); + + // 32 for key, 32 for value, 2 for item + assert_eq!( + get_contract(&caller_addr).total_deposit(), + base_deposit + 32 + 32 + 2, + "Unexpected deposit sum charged for non-zero storage item" + ); + + // write storage item to all zeros + let result = builder::bare_call(caller_addr) + .data( + Caller::delegateCall { + _callee: host_addr.0.into(), + _data: Host::sstoreOpCall { slot: index, value: 0u64 }.abi_encode().into(), + _gas: u64::MAX, + } + .abi_encode(), + ) + .build_and_unwrap_result(); + + let result = Caller::delegateCall::abi_decode_returns(&result.data).unwrap(); + assert!(result.success, "delegateCall did not succeed"); + + assert_eq!( + get_contract(&caller_addr).total_deposit(), + base_deposit, + "contract should refund deposit on zeroing storage item" + ); + }); +} diff --git a/substrate/frame/revive/src/vm/evm/instructions/host.rs b/substrate/frame/revive/src/vm/evm/instructions/host.rs index 160d6c3d6dd2f..b3b3c1fefba26 100644 --- a/substrate/frame/revive/src/vm/evm/instructions/host.rs +++ b/substrate/frame/revive/src/vm/evm/instructions/host.rs @@ -146,16 +146,16 @@ fn store_helper<'ext, E: Ext>( let charged_amount = interpreter.ext.charge_or_halt(cost_before)?; let key = Key::Fix(index.to_big_endian()); let take_old = false; - let Ok(write_outcome) = - set_function(interpreter.ext, &key, Some(value.to_big_endian().to_vec()), take_old) + let value_to_store = if value.is_zero() { None } else { Some(value.to_big_endian().to_vec()) }; + let Ok(write_outcome) = set_function(interpreter.ext, &key, value_to_store.clone(), take_old) else { return ControlFlow::Break(Error::::ContractTrapped.into()); }; - interpreter - .ext - .gas_meter_mut() - .adjust_gas(charged_amount, adjust_cost(32, write_outcome.old_len())); + interpreter.ext.gas_meter_mut().adjust_gas( + charged_amount, + adjust_cost(value_to_store.unwrap_or_default().len() as u32, write_outcome.old_len()), + ); ControlFlow::Continue(()) }