Skip to content

Commit

Permalink
runtime-sdk/modules/evm: Simplify revert reason encoding
Browse files Browse the repository at this point in the history 
Instead of attempting to decode simple ABI-encoded error strings, just
return the entire data, base64-encoded.
  • Loading branch information
@kostko
kostko committed Sep 11, 2023
1 parent 9573807 commit 85408ad
Show file tree
Hide file tree
Showing 9 changed files with 216 additions and 138 deletions.
74 changes: 21 additions & 53 deletions runtime-sdk/modules/evm/src/lib.rs
View file
Expand Up @@ -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(
Expand Down Expand Up @@ -210,56 +213,6 @@ impl From<evm::ExitFatal> 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<u8>) -> Result<Vec<u8>, 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 {}
Expand Down Expand Up @@ -538,6 +491,7 @@ impl<Cfg: Config> Module<Cfg> {
) -> (evm::ExitReason, Vec<u8>),
C: TxContext,
{
let is_query = ctx.is_check_only();
let cfg = Cfg::evm_config(estimate_gas);
let gas_limit: u64 = <C::Runtime as Runtime>::Core::remaining_tx_gas(ctx);
let gas_price: primitive_types::U256 = ctx.tx_auth_info().fee.gas_price().into();
Expand All @@ -557,9 +511,23 @@ impl<Cfg: Config> Module<Cfg> {
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(),

Check warning on line 527 in runtime-sdk/modules/evm/src/lib.rs

View check run for this annotation

Codecov / codecov/patch

runtime-sdk/modules/evm/src/lib.rs#L527 

Added line #L527 was not covered by tests
_ => unreachable!("already handled above"),
};

<C::Runtime as Runtime>::Core::use_tx_gas(ctx, gas_used)?;
Cfg::Accounts::set_refund_unused_tx_fee(ctx, Cfg::REFUND_UNUSED_FEE);
return Err(err);
Expand Down
37 changes: 37 additions & 0 deletions runtime-sdk/modules/evm/src/mock.rs
View file
Expand Up @@ -85,3 +85,40 @@ pub fn load_contract_bytecode(raw: &str) -> Vec<u8> {
Vec::from_hex(raw.split_whitespace().collect::<String>())
.expect("compiled contract should be a valid hex string")
}

/// Decode a basic revert reason.
pub fn decode_reverted(msg: &str) -> Option<String> {
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<Vec<ethabi::Token>> {
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<Vec<u8>> {
// Trim the optional reverted prefix.
let msg = msg.trim_start_matches("reverted: ");

base64::decode(msg).ok()
}
6 changes: 3 additions & 3 deletions runtime-sdk/modules/evm/src/precompile/subcall.rs
View file
Expand Up @@ -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 _,
};
Expand Down Expand Up @@ -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");
}
Expand Down Expand Up @@ -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");
}
Expand Down
174 changes: 92 additions & 82 deletions runtime-sdk/modules/evm/src/test.rs
View file
Expand Up @@ -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.
Expand Down Expand Up @@ -811,84 +810,6 @@ fn test_c10l_evm_runtime() {
do_test_evm_runtime::<ConfidentialEVMConfig>();
}

#[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();
Expand Down Expand Up @@ -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");
}
Expand All @@ -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::<EVMRuntime<EVMConfig>>(context::Mode::ExecuteTx, true);
let mut signer = EvmSigner::new(0, keys::dave::sigspec());

EVMRuntime::<EVMConfig>::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<u8> = 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<u8> = 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");
}
}
6 changes: 6 additions & 0 deletions tests/e2e/contracts/retval/Makefile
View file
@@ -0,0 +1,6 @@
contract = retval.sol
abi = retval.abi
hex = retval.hex

include ../contracts.mk

1 change: 1 addition & 0 deletions tests/e2e/contracts/retval/retval.abi
View file
@@ -0,0 +1 @@
[{"inputs":[],"name":"testRevert","outputs":[],"stateMutability":"view","type":"function"},{"inputs":[],"name":"testSuccess","outputs":[{"internalType":"bytes","name":"","type":"bytes"}],"stateMutability":"view","type":"function"}]

0 comments on commit 85408ad

Please sign in to comment.