From 85408ade13d8bd4c7099fcdbcc8bbdba01a88107 Mon Sep 17 00:00:00 2001 From: Jernej Kos Date: Fri, 8 Sep 2023 10:16:13 +0200 Subject: [PATCH] runtime-sdk/modules/evm: Simplify revert reason encoding Instead of attempting to decode simple ABI-encoded error strings, just return the entire data, base64-encoded. --- runtime-sdk/modules/evm/src/lib.rs | 74 +++----- runtime-sdk/modules/evm/src/mock.rs | 37 ++++ .../modules/evm/src/precompile/subcall.rs | 6 +- runtime-sdk/modules/evm/src/test.rs | 174 +++++++++--------- tests/e2e/contracts/retval/Makefile | 6 + tests/e2e/contracts/retval/retval.abi | 1 + tests/e2e/contracts/retval/retval.go | 36 ++++ tests/e2e/contracts/retval/retval.hex | 1 + tests/e2e/contracts/retval/retval.sol | 19 ++ 9 files changed, 216 insertions(+), 138 deletions(-) create mode 100644 tests/e2e/contracts/retval/Makefile create mode 100644 tests/e2e/contracts/retval/retval.abi create mode 100644 tests/e2e/contracts/retval/retval.go create mode 100644 tests/e2e/contracts/retval/retval.hex create mode 100644 tests/e2e/contracts/retval/retval.sol diff --git a/runtime-sdk/modules/evm/src/lib.rs b/runtime-sdk/modules/evm/src/lib.rs index 24e88611ba..29e4102ff5 100644 --- a/runtime-sdk/modules/evm/src/lib.rs +++ b/runtime-sdk/modules/evm/src/lib.rs @@ -70,6 +70,9 @@ pub trait Config: 'static { /// Whether to refund unused transaction fee. const REFUND_UNUSED_FEE: bool = true; + /// Maximum result size in bytes. + const MAX_RESULT_SIZE: usize = 1024; + /// Maps an Ethereum address into an SDK account address. fn map_address(address: primitive_types::H160) -> Address { Address::new( @@ -210,56 +213,6 @@ impl From for Error { } } -/// Process an EVM result to return either a successful result or a (readable) error reason. -fn process_evm_result(exit_reason: evm::ExitReason, data: Vec) -> Result, Error> { - match exit_reason { - evm::ExitReason::Succeed(_) => Ok(data), - evm::ExitReason::Revert(_) => { - if data.is_empty() { - return Err(Error::Reverted("no revert reason".to_string())); - } - - // Decode revert reason, format is as follows: - // - // 08c379a0 <- Function selector - // 0000000000000000000000000000000000000000000000000000000000000020 <- Offset of string return value - // 0000000000000000000000000000000000000000000000000000000000000047 <- Length of string return value (the revert reason) - // 6d7946756e6374696f6e206f6e6c79206163636570747320617267756d656e74 <- First 32 bytes of the revert reason - // 7320776869636820617265206772656174686572207468616e206f7220657175 <- Next 32 bytes of the revert reason - // 616c20746f203500000000000000000000000000000000000000000000000000 <- Last 7 bytes of the revert reason - // - const ERROR_STRING_SELECTOR: &[u8] = &[0x08, 0xc3, 0x79, 0xa0]; // Keccak256("Error(string)") - const FIELD_OFFSET_START: usize = 4; - const FIELD_LENGTH_START: usize = FIELD_OFFSET_START + 32; - const FIELD_REASON_START: usize = FIELD_LENGTH_START + 32; - const MIN_SIZE: usize = FIELD_REASON_START; - const MAX_REASON_SIZE: usize = 1024; - - let max_raw_len = data.len().clamp(0, MAX_REASON_SIZE); - if data.len() < MIN_SIZE || !data.starts_with(ERROR_STRING_SELECTOR) { - return Err(Error::Reverted(base64::encode(&data[..max_raw_len]))); - } - // Decode and validate length. - let mut length = - primitive_types::U256::from(&data[FIELD_LENGTH_START..FIELD_LENGTH_START + 32]) - .low_u32() as usize; - if FIELD_REASON_START + length > data.len() { - return Err(Error::Reverted(base64::encode(&data[..max_raw_len]))); - } - // Make sure that this doesn't ever return huge reason values as this is at least - // somewhat contract-controlled. - if length > MAX_REASON_SIZE { - length = MAX_REASON_SIZE; - } - let reason = - String::from_utf8_lossy(&data[FIELD_REASON_START..FIELD_REASON_START + length]); - Err(Error::Reverted(reason.to_string())) - } - evm::ExitReason::Error(err) => Err(err.into()), - evm::ExitReason::Fatal(err) => Err(err.into()), - } -} - /// Gas costs. #[derive(Clone, Debug, Default, cbor::Encode, cbor::Decode)] pub struct GasCosts {} @@ -538,6 +491,7 @@ impl Module { ) -> (evm::ExitReason, Vec), C: TxContext, { + let is_query = ctx.is_check_only(); let cfg = Cfg::evm_config(estimate_gas); let gas_limit: u64 = ::Core::remaining_tx_gas(ctx); let gas_price: primitive_types::U256 = ctx.tx_auth_info().fee.gas_price().into(); @@ -557,9 +511,23 @@ impl Module { let (exit_reason, exit_value) = f(&mut executor, gas_limit); let gas_used = executor.used_gas(); - let exit_value = match process_evm_result(exit_reason, exit_value) { - Ok(exit_value) => exit_value, - Err(err) => { + // Clamp data based on maximum allowed result size. + let exit_value = if !is_query && exit_value.len() > Cfg::MAX_RESULT_SIZE { + exit_value[..Cfg::MAX_RESULT_SIZE].to_vec() + } else { + exit_value + }; + + let exit_value = match exit_reason { + evm::ExitReason::Succeed(_) => exit_value, + err => { + let err = match err { + evm::ExitReason::Revert(_) => Error::Reverted(base64::encode(exit_value)), + evm::ExitReason::Error(err) => err.into(), + evm::ExitReason::Fatal(err) => err.into(), + _ => unreachable!("already handled above"), + }; + ::Core::use_tx_gas(ctx, gas_used)?; Cfg::Accounts::set_refund_unused_tx_fee(ctx, Cfg::REFUND_UNUSED_FEE); return Err(err); diff --git a/runtime-sdk/modules/evm/src/mock.rs b/runtime-sdk/modules/evm/src/mock.rs index 0d62094f6d..bf54ce0e66 100644 --- a/runtime-sdk/modules/evm/src/mock.rs +++ b/runtime-sdk/modules/evm/src/mock.rs @@ -85,3 +85,40 @@ pub fn load_contract_bytecode(raw: &str) -> Vec { Vec::from_hex(raw.split_whitespace().collect::()) .expect("compiled contract should be a valid hex string") } + +/// Decode a basic revert reason. +pub fn decode_reverted(msg: &str) -> Option { + decode_reverted_abi( + msg, + ethabi::AbiError { + name: "Error".to_string(), + inputs: vec![ethabi::Param { + name: "message".to_string(), + kind: ethabi::ParamType::String, + internal_type: None, + }], + }, + )? + .pop() + .unwrap() + .into_string() +} + +/// Decode a revert reason accoording to the given API. +pub fn decode_reverted_abi(msg: &str, abi: ethabi::AbiError) -> Option> { + let raw = decode_reverted_raw(msg)?; + + // Strip (and validate) error signature. + let signature = abi.signature(); + let raw = raw.strip_prefix(&signature.as_bytes()[..4])?; + + Some(abi.decode(raw).unwrap()) +} + +/// Decode a base64-encoded revert reason. +pub fn decode_reverted_raw(msg: &str) -> Option> { + // Trim the optional reverted prefix. + let msg = msg.trim_start_matches("reverted: "); + + base64::decode(msg).ok() +} diff --git a/runtime-sdk/modules/evm/src/precompile/subcall.rs b/runtime-sdk/modules/evm/src/precompile/subcall.rs index 1a11339cef..89e331d364 100644 --- a/runtime-sdk/modules/evm/src/precompile/subcall.rs +++ b/runtime-sdk/modules/evm/src/precompile/subcall.rs @@ -139,7 +139,7 @@ mod test { use crate as evm; use crate::{ - mock::{load_contract_bytecode, EvmSigner}, + mock::{decode_reverted, load_contract_bytecode, EvmSigner}, types::{self, H160}, Config as _, }; @@ -364,7 +364,7 @@ mod test { { assert_eq!(module, "evm"); assert_eq!(code, 8); - assert_eq!(message, "reverted: subcall failed"); + assert_eq!(decode_reverted(&message).unwrap(), "subcall failed"); } else { panic!("call should fail due to delegatecall"); } @@ -416,7 +416,7 @@ mod test { { assert_eq!(module, "evm"); assert_eq!(code, 8); - assert_eq!(message, "reverted: subcall failed"); + assert_eq!(decode_reverted(&message).unwrap(), "subcall failed"); } else { panic!("call should fail due to re-entrancy not being allowed"); } diff --git a/runtime-sdk/modules/evm/src/test.rs b/runtime-sdk/modules/evm/src/test.rs index 1ff0b049f2..e05a4d042c 100644 --- a/runtime-sdk/modules/evm/src/test.rs +++ b/runtime-sdk/modules/evm/src/test.rs @@ -27,10 +27,9 @@ use oasis_runtime_sdk::{ use crate::{ derive_caller, - mock::{load_contract_bytecode, EvmSigner}, - process_evm_result, + mock::{decode_reverted, decode_reverted_raw, load_contract_bytecode, EvmSigner}, types::{self, H160}, - Config, Error, Genesis, Module as EVMModule, + Config, Genesis, Module as EVMModule, }; /// Test contract code. @@ -811,84 +810,6 @@ fn test_c10l_evm_runtime() { do_test_evm_runtime::(); } -#[test] -fn test_revert_reason_decoding() { - let long_reason = vec![0x61; 1050]; - let long_reason_hex = hex::encode(&long_reason); - let long_reason_str = String::from_utf8(long_reason).unwrap(); - let long_reason_hex = &[ - "08c379a0\ - 0000000000000000000000000000000000000000000000000000000000000020\ - 000000000000000000000000000000000000000000000000000000000000041a", - &long_reason_hex, - ] - .concat(); - - let tcs = vec![ - // Valid values. - ( - "08c379a0\ - 0000000000000000000000000000000000000000000000000000000000000020\ - 0000000000000000000000000000000000000000000000000000000000000018\ - 4461692f696e73756666696369656e742d62616c616e63650000000000000000", - "Dai/insufficient-balance", - ), - ( - "08c379a0\ - 0000000000000000000000000000000000000000000000000000000000000020\ - 0000000000000000000000000000000000000000000000000000000000000047\ - 6d7946756e6374696f6e206f6e6c79206163636570747320617267756d656e74\ - 7320776869636820617265206772656174686572207468616e206f7220657175\ - 616c20746f203500000000000000000000000000000000000000000000000000", - "myFunction only accepts arguments which are greather than or equal to 5", - ), - // Valid value, empty reason. - ( - "08c379a0\ - 0000000000000000000000000000000000000000000000000000000000000020\ - 0000000000000000000000000000000000000000000000000000000000000000", - "", - ), - // Valid value, reason too long and should be truncated. - (long_reason_hex, &long_reason_str[..1024]), - // No revert reason. - ("", "no revert reason"), - // Malformed output, incorrect selector and bad length. - ( - "BADBADBADBADBADBAD", - "utututututut", - ), - // Malformed output, bad selector. - ( - "BAAAAAAD\ - 0000000000000000000000000000000000000000000000000000000000000020\ - 0000000000000000000000000000000000000000000000000000000000000018\ - 4461692f696e73756666696369656e742d62616c616e63650000000000000000", - "uqqqrQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABhEYWkvaW5zdWZmaWNpZW50LWJhbGFuY2UAAAAAAAAAAA==", - ), - // Malformed output, corrupted length. - ( - "08c379a0\ - 0000000000000000000000000000000000000000000000000000000000000020\ - 00000000000000000000000000000000000000000000000000000000FFFFFFFF\ - 4461692f696e73756666696369656e742d62616c616e63650000000000000000", - "CMN5oAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAP////9EYWkvaW5zdWZmaWNpZW50LWJhbGFuY2UAAAAAAAAAAA==", - ), - ]; - - for tc in tcs { - let raw = hex::decode(tc.0).unwrap(); - let err = process_evm_result(evm::ExitReason::Revert(evm::ExitRevert::Reverted), raw) - .unwrap_err(); - match err { - Error::Reverted(reason) => { - assert_eq!(&reason, tc.1, "revert reason should be decoded correctly"); - } - _ => panic!("expected Error::Reverted(_) variant"), - } - } -} - #[test] fn test_fee_refunds() { let mut mock = mock::Mock::default(); @@ -994,7 +915,10 @@ fn test_fee_refunds() { { assert_eq!(module, "evm"); assert_eq!(code, 8); - assert_eq!(message, "reverted: ERC20: transfer amount exceeds balance"); + assert_eq!( + decode_reverted(&message).unwrap(), + "ERC20: transfer amount exceeds balance" + ); } else { panic!("call should revert"); } @@ -1019,3 +943,89 @@ fn test_fee_refunds() { assert_eq!(events.len(), 1); assert_eq!(events[0].amount, 24_585); } + +#[test] +fn test_return_value_limits() { + let mut mock = mock::Mock::default(); + let mut ctx = + mock.create_ctx_for_runtime::>(context::Mode::ExecuteTx, true); + let mut signer = EvmSigner::new(0, keys::dave::sigspec()); + + EVMRuntime::::migrate(&mut ctx); + + // Give Dave some tokens. + Accounts::mint( + &mut ctx, + keys::dave::address(), + &token::BaseUnits(1_000_000_000, Denomination::NATIVE), + ) + .unwrap(); + + static RETVAL_CONTRACT_CODE_HEX: &str = + include_str!("../../../../tests/e2e/contracts/retval/retval.hex"); + + // Create contract. + let dispatch_result = signer.call( + &mut ctx, + "evm.Create", + types::Create { + value: 0.into(), + init_code: load_contract_bytecode(RETVAL_CONTRACT_CODE_HEX), + }, + ); + let result = dispatch_result.result.unwrap(); + let result: Vec = cbor::from_value(result).unwrap(); + let contract_address = H160::from_slice(&result); + + // Call the `testSuccess` method on the contract. + let dispatch_result = signer.call_evm_opts( + &mut ctx, + contract_address, + "testSuccess", + &[], + &[], + CallOptions { + fee: Fee { + amount: token::BaseUnits::new(1_000_000, Denomination::NATIVE), + gas: 100_000, + ..Default::default() + }, + }, + ); + let result: Vec = cbor::from_value(dispatch_result.result.unwrap()).unwrap(); + assert_eq!(result.len(), 1024, "result should be correctly trimmed"); + // Actual payload is ABI-encoded so the raw result starts at offset 64. + assert_eq!(result[64], 0xFF, "result should be correct"); + assert_eq!(result[1023], 0x42, "result should be correct"); + + // Call the `testRevert` method on the contract. + let dispatch_result = signer.call_evm_opts( + &mut ctx, + contract_address, + "testRevert", + &[], + &[], + CallOptions { + fee: Fee { + amount: token::BaseUnits::new(1_000_000, Denomination::NATIVE), + gas: 100_000, + ..Default::default() + }, + }, + ); + if let module::CallResult::Failed { + module, + code, + message, + } = dispatch_result.result + { + assert_eq!(module, "evm"); + assert_eq!(code, 8); + let message = decode_reverted_raw(&message).unwrap(); + // Actual payload is ABI-encoded so the raw result starts at offset 68. + assert_eq!(message[68], 0xFF, "result should be correct"); + assert_eq!(message[1023], 0x42, "result should be correct"); + } else { + panic!("call should revert"); + } +} diff --git a/tests/e2e/contracts/retval/Makefile b/tests/e2e/contracts/retval/Makefile new file mode 100644 index 0000000000..44b49ec0e1 --- /dev/null +++ b/tests/e2e/contracts/retval/Makefile @@ -0,0 +1,6 @@ +contract = retval.sol +abi = retval.abi +hex = retval.hex + +include ../contracts.mk + diff --git a/tests/e2e/contracts/retval/retval.abi b/tests/e2e/contracts/retval/retval.abi new file mode 100644 index 0000000000..bf083fb322 --- /dev/null +++ b/tests/e2e/contracts/retval/retval.abi @@ -0,0 +1 @@ +[{"inputs":[],"name":"testRevert","outputs":[],"stateMutability":"view","type":"function"},{"inputs":[],"name":"testSuccess","outputs":[{"internalType":"bytes","name":"","type":"bytes"}],"stateMutability":"view","type":"function"}] \ No newline at end of file diff --git a/tests/e2e/contracts/retval/retval.go b/tests/e2e/contracts/retval/retval.go new file mode 100644 index 0000000000..24dc207c90 --- /dev/null +++ b/tests/e2e/contracts/retval/retval.go @@ -0,0 +1,36 @@ +package retval + +import ( + _ "embed" + "encoding/hex" + "fmt" + "strings" + + ethABI "github.com/ethereum/go-ethereum/accounts/abi" +) + +// CompiledHex is the compiled subcall contract in hex encoding. +// +//go:embed retval.hex +var CompiledHex string + +// Compiled is the compiled subcall contract. +var Compiled = func() []byte { + contract, err := hex.DecodeString(strings.TrimSpace(CompiledHex)) + if err != nil { + panic(fmt.Errorf("failed to decode contract: %w", err)) + } + return contract +}() + +//go:embed retval.abi +var abiJSON string + +// ABI is the ABI of the subcall contract. +var ABI = func() ethABI.ABI { + abi, err := ethABI.JSON(strings.NewReader(abiJSON)) + if err != nil { + panic(err) + } + return abi +}() diff --git a/tests/e2e/contracts/retval/retval.hex b/tests/e2e/contracts/retval/retval.hex new file mode 100644 index 0000000000..035da90b32 --- /dev/null +++ b/tests/e2e/contracts/retval/retval.hex @@ -0,0 +1 @@ +608060405234801561000f575f80fd5b5061028e8061001d5f395ff3fe608060405234801561000f575f80fd5b5060043610610034575f3560e01c8063713d3e3e14610038578063a26388bb14610056575b5f80fd5b610040610060565b60405161004d919061022b565b60405180910390f35b61005e610117565b005b6040805161041a80825261044082019092526060915f919060208201818036833701905050905060ff60f81b815f8151811061009e5761009e610244565b60200101906001600160f81b03191690815f1a905350604260f81b816103bf815181106100cd576100cd610244565b60200101906001600160f81b03191690815f1a90535060ff60f81b81610419815181106100fc576100fc610244565b60200101906001600160f81b03191690815f1a905350919050565b6040805161041a80825261044082019092525f9160208201818036833701905050905060ff60f81b815f8151811061015157610151610244565b60200101906001600160f81b03191690815f1a905350604260f81b816103bb8151811061018057610180610244565b60200101906001600160f81b03191690815f1a90535060ff60f81b81610419815181106101af576101af610244565b60200101906001600160f81b03191690815f1a9053508060405162461bcd60e51b81526004016101df919061022b565b60405180910390fd5b5f81518084525f5b8181101561020c576020818501810151868301820152016101f0565b505f602082860101526020601f19601f83011685010191505092915050565b602081525f61023d60208301846101e8565b9392505050565b634e487b7160e01b5f52603260045260245ffdfea264697066735822122045af1d1d298c4319acc88b9be68426e9be8176226b8a3cd846ab346afb94170464736f6c63430008150033 \ No newline at end of file diff --git a/tests/e2e/contracts/retval/retval.sol b/tests/e2e/contracts/retval/retval.sol new file mode 100644 index 0000000000..7a4e2a729e --- /dev/null +++ b/tests/e2e/contracts/retval/retval.sol @@ -0,0 +1,19 @@ +pragma solidity ^0.8.0; + +contract Test { + function testSuccess() external view returns (bytes memory) { + bytes memory data = new bytes(1050); // Over the 1024 byte limit. + data[0] = 0xFF; + data[959] = 0x42; // Offset of 64 bytes. + data[1049] = 0xFF; + return data; + } + + function testRevert() external view { + bytes memory data = new bytes(1050); // Over the 1024 byte limit. + data[0] = 0xFF; + data[955] = 0x42; // Offset of 68 bytes. + data[1049] = 0xFF; + require(false, string(data)); + } +}