diff --git a/prdoc/pr_9823.prdoc b/prdoc/pr_9823.prdoc new file mode 100644 index 0000000000000..fe07007a015ee --- /dev/null +++ b/prdoc/pr_9823.prdoc @@ -0,0 +1,8 @@ +title: bugfix revm set_storage gas cost +doc: +- audience: Runtime Dev + description: "Fixes bug in revm gasmetering where the initial charge was less than\ + \ the adjusted charge.\r\n" +crates: +- name: pallet-revive + bump: patch diff --git a/substrate/frame/revive/src/tests/sol/host.rs b/substrate/frame/revive/src/tests/sol/host.rs index 7ee487dda90df..d18f1cb18f5e4 100644 --- a/substrate/frame/revive/src/tests/sol/host.rs +++ b/substrate/frame/revive/src/tests/sol/host.rs @@ -364,6 +364,59 @@ fn sload_works() { } } +#[test] +fn sload_error_reading_non_32_byte_value() { + let (code, _) = compile_module_with_type("Host", FixtureType::Solc).unwrap(); + + let index = U256::from(13); + let expected_value = U256::from(17); + + ExtBuilder::default().build().execute_with(|| { + ::Currency::set_balance(&ALICE, 100_000_000_000); + + let Contract { addr, .. } = + builder::bare_instantiate(Code::Upload(code)).build_and_unwrap_contract(); + + { + // Test that reading storage value of 31 bytes results in contract trapped + let contract_info = test_utils::get_contract(&addr); + let key = Key::Fix(index.to_be_bytes()); + contract_info + .write(&key, Some(expected_value.to_be_bytes::<32>()[..31].to_vec()), None, false) + .unwrap(); + + let result = builder::bare_call(addr) + .data(Host::HostCalls::sloadOp(Host::sloadOpCall { slot: index }).abi_encode()) + .build(); + assert!(result.result.is_err(), "test should error"); + let err = result.result.unwrap_err(); + let sp_runtime::DispatchError::Module(module_err) = &err else { + panic!("expected Module error (ContractTrapped), got {:?}", err) + }; + assert_eq!(module_err.message, Some("ContractTrapped")); + } + + { + // Test that reading storage value of 33 bytes results in contract trapped + let contract_info = test_utils::get_contract(&addr); + let key = Key::Fix(index.to_be_bytes()); + let mut bytes = expected_value.to_be_bytes::<32>().to_vec(); + bytes.push(0u8); + contract_info.write(&key, Some(bytes), None, false).unwrap(); + + let result = builder::bare_call(addr) + .data(Host::HostCalls::sloadOp(Host::sloadOpCall { slot: index }).abi_encode()) + .build(); + assert!(result.result.is_err(), "test should error"); + let err = result.result.unwrap_err(); + let sp_runtime::DispatchError::Module(module_err) = &err else { + panic!("expected Module error (ContractTrapped), got {:?}", err) + }; + assert_eq!(module_err.message, Some("ContractTrapped")); + } + }); +} + #[test] fn sstore_works() { for fixture_type in [FixtureType::Solc, FixtureType::Resolc] { diff --git a/substrate/frame/revive/src/vm/evm/instructions/host.rs b/substrate/frame/revive/src/vm/evm/instructions/host.rs index dbb65c89de64a..6e2a3abb8bd16 100644 --- a/substrate/frame/revive/src/vm/evm/instructions/host.rs +++ b/substrate/frame/revive/src/vm/evm/instructions/host.rs @@ -117,6 +117,7 @@ pub fn sload<'ext, E: Ext>(context: Context<'_, 'ext, E>) { *index = if let Some(storage_value) = value { // sload always reads a word let Ok::<[u8; 32], _>(bytes) = storage_value.try_into() else { + log::debug!(target: crate::LOG_TARGET, "sload read invalid storage value length. Expected 32."); context.interpreter.halt(InstructionResult::FatalExternalError); return }; @@ -169,9 +170,10 @@ fn store_helper<'ext, E: Ext>( /// /// Stores a word to storage. pub fn sstore<'ext, E: Ext>(context: Context<'_, 'ext, E>) { + let old_bytes = context.interpreter.extend.max_value_size(); store_helper( context, - RuntimeCosts::SetStorage { new_bytes: 32, old_bytes: 0 }, + RuntimeCosts::SetStorage { new_bytes: 32, old_bytes }, |ext, key, value, take_old| ext.set_storage(key, value, take_old), |new_bytes, old_bytes| RuntimeCosts::SetStorage { new_bytes, old_bytes }, ); @@ -180,9 +182,10 @@ pub fn sstore<'ext, E: Ext>(context: Context<'_, 'ext, E>) { /// EIP-1153: Transient storage opcodes /// Store value to transient storage pub fn tstore<'ext, E: Ext>(context: Context<'_, 'ext, E>) { + let old_bytes = context.interpreter.extend.max_value_size(); store_helper( context, - RuntimeCosts::SetTransientStorage { new_bytes: 32, old_bytes: 0 }, + RuntimeCosts::SetTransientStorage { new_bytes: 32, old_bytes }, |ext, key, value, take_old| ext.set_transient_storage(key, value, take_old), |new_bytes, old_bytes| RuntimeCosts::SetTransientStorage { new_bytes, old_bytes }, ); @@ -199,6 +202,7 @@ pub fn tload<'ext, E: Ext>(context: Context<'_, 'ext, E>) { *index = if let Some(storage_value) = bytes { if storage_value.len() != 32 { // tload always reads a word + log::error!(target: crate::LOG_TARGET, "tload read invalid storage value length. Expected 32."); context.interpreter.halt(InstructionResult::FatalExternalError); return; }