From f11c1f8420616170b5103e559e1d14a1386755e8 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 | 45 ++---------- runtime-sdk/modules/evm/src/mock.rs | 31 ++++++++ .../modules/evm/src/precompile/subcall.rs | 6 +- runtime-sdk/modules/evm/src/test.rs | 71 ++++--------------- 4 files changed, 52 insertions(+), 101 deletions(-) diff --git a/runtime-sdk/modules/evm/src/lib.rs b/runtime-sdk/modules/evm/src/lib.rs index 24e88611ba..4a30815293 100644 --- a/runtime-sdk/modules/evm/src/lib.rs +++ b/runtime-sdk/modules/evm/src/lib.rs @@ -214,47 +214,10 @@ impl From for Error { 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())) - } + // Encode raw revert reason as Base64, up to the maximum length of 1024 bytes. + evm::ExitReason::Revert(_) => Err(Error::Reverted(base64::encode( + &data[..data.len().clamp(0, 1024)], + ))), evm::ExitReason::Error(err) => Err(err.into()), evm::ExitReason::Fatal(err) => Err(err.into()), } diff --git a/runtime-sdk/modules/evm/src/mock.rs b/runtime-sdk/modules/evm/src/mock.rs index 0d62094f6d..5a4bfa3497 100644 --- a/runtime-sdk/modules/evm/src/mock.rs +++ b/runtime-sdk/modules/evm/src/mock.rs @@ -85,3 +85,34 @@ 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> { + // Trim the optional reverted prefix. + let msg = msg.trim_start_matches("reverted: "); + + let raw = base64::decode(msg).ok()?; + // Strip (and validate) error signature. + let signature = abi.signature(); + let raw = raw.strip_prefix(&signature.as_bytes()[..4])?; + + Some(abi.decode(raw).unwrap()) +} 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..5f3a4f7fc0 100644 --- a/runtime-sdk/modules/evm/src/test.rs +++ b/runtime-sdk/modules/evm/src/test.rs @@ -27,7 +27,7 @@ use oasis_runtime_sdk::{ use crate::{ derive_caller, - mock::{load_contract_bytecode, EvmSigner}, + mock::{decode_reverted, load_contract_bytecode, EvmSigner}, process_evm_result, types::{self, H160}, Config, Error, Genesis, Module as EVMModule, @@ -815,65 +815,17 @@ fn test_c10l_evm_runtime() { 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. + // Valid value. ( - "08c379a0\ - 0000000000000000000000000000000000000000000000000000000000000020\ - 0000000000000000000000000000000000000000000000000000000000000018\ - 4461692f696e73756666696369656e742d62616c616e63650000000000000000", - "Dai/insufficient-balance", + "00010203040506", + vec![0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06], ), - ( - "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]), + // Reason too long and should be truncated. + (&long_reason_hex, (&long_reason[..1024]).to_vec()), // 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==", - ), + ("", b"".to_vec()), ]; for tc in tcs { @@ -882,7 +834,9 @@ fn test_revert_reason_decoding() { .unwrap_err(); match err { Error::Reverted(reason) => { - assert_eq!(&reason, tc.1, "revert reason should be decoded correctly"); + let decoded = + base64::decode(reason).expect("revert reason should be base64 encoded"); + assert_eq!(decoded, tc.1, "revert reason should match expected value"); } _ => panic!("expected Error::Reverted(_) variant"), } @@ -994,7 +948,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"); }