More precompile corrections

crates/humanode-runtime/src/tests/frontier_precompiles.rs

@@ -17,49 +17,87 @@ static GAS_PRICE: Lazy<U256> =
 
 const INIT_BALANCE: Balance = 10u128.pow(18 + 6);
 
-/// The address at which the forwarder contract is deployed at genesis.
-static FORWARDER_ADDRESS: Lazy<H160> = Lazy::new(|| H160::from_low_u64_be(0xf0f0f0));
+/// The kind of call the forwarder contract issues to its target.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+enum ForwardKind {
+    DelegateCall,
+    CallCode,
+    StaticCall,
+}
+
+/// All the forwarder kinds.
+const FORWARD_KINDS: [ForwardKind; 3] = [
+    ForwardKind::DelegateCall,
+    ForwardKind::CallCode,
+    ForwardKind::StaticCall,
+];
+
+/// The address at which the forwarder contract of the given kind is deployed at genesis.
+fn forwarder_address(kind: ForwardKind) -> H160 {
+    H160::from_low_u64_be(0xf0f0f0 + kind as u64)
+}
 
-/// A minimal contract that `DELEGATECALL`s the target with the provided input and bubbles up
-/// the result (return data on success, revert data on failure).
+/// A minimal contract that forwards the provided input to the target via the given call kind
+/// and bubbles up the result (return data on success, revert data on failure).
 ///
 /// Calldata layout: `abi.encode(target)` (32 bytes, address right-aligned) followed by the raw
 /// input to forward.
-const DELEGATECALL_FORWARDER_CODE: &[u8] = &[
-    // size = CALLDATASIZE - 32
-    0x60, 0x20, // PUSH1 0x20
-    0x36, // CALLDATASIZE
-    0x03, // SUB
-    // CALLDATACOPY(dest = 0, offset = 32, size)
-    0x80, // DUP1
-    0x60, 0x20, // PUSH1 0x20
-    0x60, 0x00, // PUSH1 0x00
-    0x37, // CALLDATACOPY
-    // DELEGATECALL(gas, target, args_offset = 0, args_size = size, ret_offset = 0, ret_size = 0)
-    0x60, 0x00, // PUSH1 0x00
-    0x60, 0x00, // PUSH1 0x00
-    0x82, // DUP3
-    0x60, 0x00, // PUSH1 0x00
-    0x60, 0x00, // PUSH1 0x00
-    0x35, // CALLDATALOAD
-    0x5a, // GAS
-    0xf4, // DELEGATECALL
-    // RETURNDATACOPY(dest = 0, offset = 0, size = RETURNDATASIZE)
-    0x3d, // RETURNDATASIZE
-    0x60, 0x00, // PUSH1 0x00
-    0x60, 0x00, // PUSH1 0x00
-    0x3e, // RETURNDATACOPY
+fn forwarder_code(kind: ForwardKind) -> Vec<u8> {
+    let mut code = vec![
+        // size = CALLDATASIZE - 32
+        0x60, 0x20, // PUSH1 0x20
+        0x36, // CALLDATASIZE
+        0x03, // SUB
+        // CALLDATACOPY(dest = 0, offset = 32, size)
+        0x80, // DUP1
+        0x60, 0x20, // PUSH1 0x20
+        0x60, 0x00, // PUSH1 0x00
+        0x37, // CALLDATACOPY
+        // Call arguments, pushed in reverse: ret_size = 0, ret_offset = 0, args_size = size,
+        // args_offset = 0.
+        0x60, 0x00, // PUSH1 0x00
+        0x60, 0x00, // PUSH1 0x00
+        0x82, // DUP3
+        0x60, 0x00, // PUSH1 0x00
+    ];
+    if kind == ForwardKind::CallCode {
+        // CALLCODE additionally takes a value, which we set to zero.
+        code.extend([0x60, 0x00]); // PUSH1 0x00
+    }
+    code.extend([
+        // target = CALLDATALOAD(0), gas = GAS
+        0x60,
+        0x00, // PUSH1 0x00
+        0x35, // CALLDATALOAD
+        0x5a, // GAS
+        match kind {
+            ForwardKind::DelegateCall => 0xf4, // DELEGATECALL
+            ForwardKind::CallCode => 0xf2,     // CALLCODE
+            ForwardKind::StaticCall => 0xfa,   // STATICCALL
+        },
+        // RETURNDATACOPY(dest = 0, offset = 0, size = RETURNDATASIZE)
+        0x3d, // RETURNDATASIZE
+        0x60,
+        0x00, // PUSH1 0x00
+        0x60,
+        0x00, // PUSH1 0x00
+        0x3e, // RETURNDATACOPY
+    ]);
     // if !success { REVERT(0, RETURNDATASIZE) } else { RETURN(0, RETURNDATASIZE) }
-    0x3d, // RETURNDATASIZE
-    0x60, 0x00, // PUSH1 0x00
-    0x82, // DUP3
-    0x15, // ISZERO
-    0x60, 0x25, // PUSH1 0x25
-    0x57, // JUMPI
-    0xf3, // RETURN
-    0x5b, // JUMPDEST (0x25)
-    0xfd, // REVERT
-];
+    let jumpdest = u8::try_from(code.len() + 9).unwrap();
+    code.extend([
+        0x3d, // RETURNDATASIZE
+        0x60, 0x00, // PUSH1 0x00
+        0x82, // DUP3
+        0x15, // ISZERO
+        0x60, jumpdest, // PUSH1 jumpdest
+        0x57,     // JUMPI
+        0xf3,     // RETURN
+        0x5b,     // JUMPDEST
+        0xfd,     // REVERT
+    ]);
+    code
+}
 
 /// The addresses of the custom Humanode precompiles.
 const HUMANODE_PRECOMPILES: [u64; 4] = [
@@ -137,27 +175,27 @@ fn new_test_ext_with() -> sp_io::TestExternalities {
                 evm_endowed_accounts
                     .into_iter()
                     .map(|k| (k, init_genesis_account.clone()))
-                    .chain([
-                        (
-                            EvmToNativeSwapBridgePot::account_id(),
-                            fp_evm::GenesisAccount {
-                                balance: <EvmBalances as Inspect<EvmAccountId>>::minimum_balance()
-                                    .into(),
-                                code: Default::default(),
-                                nonce: Default::default(),
-                                storage: Default::default(),
-                            },
-                        ),
+                    .chain([(
+                        EvmToNativeSwapBridgePot::account_id(),
+                        fp_evm::GenesisAccount {
+                            balance: <EvmBalances as Inspect<EvmAccountId>>::minimum_balance()
+                                .into(),
+                            code: Default::default(),
+                            nonce: Default::default(),
+                            storage: Default::default(),
+                        },
+                    )])
+                    .chain(FORWARD_KINDS.into_iter().map(|kind| {
                         (
-                            *FORWARDER_ADDRESS,
+                            forwarder_address(kind),
                             fp_evm::GenesisAccount {
                                 balance: INIT_BALANCE.into(),
-                                code: DELEGATECALL_FORWARDER_CODE.to_vec(),
+                                code: forwarder_code(kind),
                                 nonce: Default::default(),
                                 storage: Default::default(),
                             },
-                        ),
-                    ])
+                        )
+                    }))
                     .collect()
             },
         },
@@ -190,11 +228,31 @@ fn evm_call(to: H160, data: Vec<u8>, value: U256) -> fp_evm::CallInfo {
     .unwrap()
 }
 
-/// Run `input` against `target` via a `DELEGATECALL` issued by the forwarder contract.
-fn delegate_call(target: H160, input: &[u8]) -> fp_evm::CallInfo {
+/// Run `input` against `target` via a call of the given kind issued by the forwarder contract.
+fn forward_call(kind: ForwardKind, target: H160, input: &[u8]) -> fp_evm::CallInfo {
     let mut data = EvmDataWriter::new().write(Address(target)).build();
     data.extend_from_slice(input);
-    evm_call(*FORWARDER_ADDRESS, data, U256::zero())
+    evm_call(forwarder_address(kind), data, U256::zero())
+}
+
+/// Run `input` against `target` via a `DELEGATECALL` issued by the forwarder contract.
+fn delegate_call(target: H160, input: &[u8]) -> fp_evm::CallInfo {
+    forward_call(ForwardKind::DelegateCall, target, input)
+}
+
+/// A sample state-changing input for the custom Humanode precompiles that have one.
+fn humanode_precompile_state_changing_input(precompile: u64) -> Option<Vec<u8>> {
+    match precompile {
+        NATIVE_CURRENCY => Some(
+            EvmDataWriter::new_with_selector(precompile_native_currency::Action::Transfer)
+                .write(Address(evm_account_id("EvmBob")))
+                .write(U256::from(1))
+                .build(),
+        ),
+        EVM_TO_NATIVE_SWAP => Some(humanode_precompile_input(precompile)),
+        BIOAUTH | EVM_ACCOUNTS_MAPPING => None,
+        _ => unreachable!("not a Humanode precompile: {precompile:#x}"),
+    }
 }
 
 /// A sample valid input for each of the custom Humanode precompiles.
@@ -218,26 +276,100 @@ fn humanode_precompile_input(precompile: u64) -> Vec<u8> {
     }
 }
 
-/// This test verifies that a `DELEGATECALL` to each custom Humanode precompile reverts.
+/// This test verifies that a `DELEGATECALL` or a `CALLCODE` to each custom Humanode precompile
+/// reverts.
+#[test]
+fn humanode_precompiles_reject_delegate_call_and_call_code() {
+    // Build the state from the config.
+    new_test_ext_with().execute_with(move || {
+        for kind in [ForwardKind::DelegateCall, ForwardKind::CallCode] {
+            for precompile in HUMANODE_PRECOMPILES {
+                let execinfo = forward_call(
+                    kind,
+                    hash(precompile),
+                    &humanode_precompile_input(precompile),
+                );
+                assert_eq!(
+                    execinfo.exit_reason,
+                    fp_evm::ExitReason::Revert(fp_evm::ExitRevert::Reverted),
+                    "precompile {precompile:#x} did not revert on {kind:?}"
+                );
+                assert_eq!(
+                    execinfo.value,
+                    b"cannot be called with DELEGATECALL or CALLCODE".to_vec(),
+                    "precompile {precompile:#x} reverted with an unexpected message on {kind:?}"
+                );
+                assert!(
+                    execinfo.logs.is_empty(),
+                    "precompile {precompile:#x} emitted logs on {kind:?}"
+                );
+            }
+        }
+    })
+}
+
+/// This test verifies that a `STATICCALL` to a state-changing function of a custom Humanode
+/// precompile reverts, and that no state changes take place.
 #[test]
-fn humanode_precompiles_reject_delegate_call() {
+fn humanode_precompiles_reject_static_call_for_state_changes() {
     // Build the state from the config.
     new_test_ext_with().execute_with(move || {
+        let forwarder = forwarder_address(ForwardKind::StaticCall);
+        let forwarder_balance_before =
+            <EvmBalances as Inspect<EvmAccountId>>::total_balance(&forwarder);
+        let bob_balance_before =
+            <EvmBalances as Inspect<EvmAccountId>>::total_balance(&evm_account_id("EvmBob"));
+
         for precompile in HUMANODE_PRECOMPILES {
-            let execinfo = delegate_call(hash(precompile), &humanode_precompile_input(precompile));
+            let Some(input) = humanode_precompile_state_changing_input(precompile) else {
+                continue;
+            };
+            let execinfo = forward_call(ForwardKind::StaticCall, hash(precompile), &input);
             assert_eq!(
                 execinfo.exit_reason,
                 fp_evm::ExitReason::Revert(fp_evm::ExitRevert::Reverted),
-                "precompile {precompile:#x} did not revert on DELEGATECALL"
+                "precompile {precompile:#x} did not revert on STATICCALL"
             );
             assert_eq!(
                 execinfo.value,
-                b"cannot be called with DELEGATECALL or CALLCODE".to_vec(),
-                "precompile {precompile:#x} reverted with an unexpected message"
+                b"can't call non-static function in static context".to_vec(),
+                "precompile {precompile:#x} reverted with an unexpected message on STATICCALL"
             );
             assert!(
                 execinfo.logs.is_empty(),
-                "precompile {precompile:#x} emitted logs on DELEGATECALL"
+                "precompile {precompile:#x} emitted logs on STATICCALL"
+            );
+        }
+
+        // Assert state changes.
+        assert_eq!(
+            <EvmBalances as Inspect<EvmAccountId>>::total_balance(&forwarder),
+            forwarder_balance_before
+        );
+        assert_eq!(
+            <EvmBalances as Inspect<EvmAccountId>>::total_balance(&evm_account_id("EvmBob")),
+            bob_balance_before
+        );
+    })
+}
+
+/// This test verifies that a `STATICCALL` to a view function of a custom Humanode precompile
+/// still works.
+#[test]
+fn humanode_precompiles_accept_static_call_for_views() {
+    // Build the state from the config.
+    new_test_ext_with().execute_with(move || {
+        for precompile in [BIOAUTH, EVM_ACCOUNTS_MAPPING, NATIVE_CURRENCY] {
+            let execinfo = forward_call(
+                ForwardKind::StaticCall,
+                hash(precompile),
+                &humanode_precompile_input(precompile),
+            );
+            assert_eq!(
+                execinfo.exit_reason,
+                fp_evm::ExitReason::Succeed(fp_evm::ExitSucceed::Returned),
+                "precompile {precompile:#x} failed on STATICCALL: {:?}",
+                String::from_utf8_lossy(&execinfo.value)
             );
         }
     })

crates/precompile-evm-to-native-swap/Cargo.toml

@@ -18,6 +18,7 @@ sp-core = { workspace = true }
 [dev-dependencies]
 pallet-evm-balances = { path = "../pallet-evm-balances", features = ["default"] }
 pallet-evm-system = { path = "../pallet-evm-system", features = ["default"] }
+precompile-utils = { path = "../precompile-utils", features = ["testing"] }
 
 frame-system = { workspace = true }
 hex-literal = { workspace = true }

crates/precompile-evm-to-native-swap/src/lib.rs

@@ -15,7 +15,8 @@ use pallet_evm::{
     ExitError, Precompile, PrecompileFailure, PrecompileHandle, PrecompileOutput, PrecompileResult,
 };
 use precompile_utils::{
-    keccak256, succeed, EvmDataWriter, EvmResult, LogExt, LogsBuilder, PrecompileHandleExt,
+    keccak256, succeed, EvmDataWriter, EvmResult, FunctionModifier, LogExt, LogsBuilder,
+    PrecompileHandleExt,
 };
 use sp_core::{Get, H160, H256, U256};
 
@@ -114,6 +115,8 @@ where
 {
     /// Swap EVM tokens to native chain tokens.
     fn swap(handle: &mut impl PrecompileHandle) -> EvmResult<PrecompileOutput> {
+        handle.check_function_modifier(FunctionModifier::Payable)?;
+
         let mut input = handle.read_input()?;
 
         let fp_evm::Context {

crates/precompile-evm-to-native-swap/src/tests.rs

@@ -4,7 +4,7 @@
 use fp_evm::{ExitError, ExitReason};
 use frame_support::{assert_noop, traits::fungible::Unbalanced};
 use pallet_evm::Runner;
-use precompile_utils::{EvmDataWriter, LogsBuilder};
+use precompile_utils::{testing::*, EvmDataWriter, LogsBuilder};
 use sp_core::H256;
 
 use crate::{mock::*, *};
@@ -460,3 +460,23 @@ fn runner_fail_value_overflow() {
         );
     });
 }
+
+/// This test verifies that the swap reverts in the static context (i.e. under `STATICCALL`),
+/// as the EVM itself does not prevent the precompile from writing to the Substrate storage.
+#[test]
+fn swap_fail_static_context() {
+    new_test_ext().execute_with_ext(|_| {
+        let swap_action = EvmDataWriter::new_with_selector(Action::Swap)
+            .write(H256::from(target_swap_native_account().as_ref()))
+            .build();
+
+        PrecompilesValue::get()
+            .prepare_test(source_swap_evm_account(), *PRECOMPILE_ADDRESS, swap_action)
+            .with_static_call(true)
+            .expect_cost(200)
+            .expect_no_logs()
+            .execute_reverts(|output| {
+                output == b"can't call non-static function in static context"
+            });
+    });
+}