Repository: stellar/rs-soroban-env
Pull request #513 (2022-10-03 15:50:43)

Forbid contract reentry
### What

Partially resolves https://github.com/stellar/rs-soroban-env/issues/336. 

~Add a `reentry: RawVal` argument to the `try_call` function. If set, it checks the host context for frames with the same contract id.~
Disallow contract reentry by checking the host context for frames with the same contract id. 
In a later follow up, we will add a `reentry: RawVal` flag to the `try_call` function and use that. 

### Why

- Why there is no checking function name? We assume if reentry is disabled, the same contract cannot be entered at all, even if the entry point is a different function than what had been called. 
- (In the future) Why only add it in `try_call`?  The `call` host function is the recommended (safe) path of invocation, where any failure will results in an host error.  The `try_call` is the more loose version that allows failure to be returned as a status. It makes sense to only enable re-entry in the `try_call` invocation path since that is a further loosening feature (@graydon has suggested it). 

### Known limitations

[TODO or N/A]


jayz22: I haven't bumped the env version because I haven't figured out how to make the sdk changes (proc-macro) in order to regen the wasm tests. 
leighmcculloch: > It makes sense to only enable re-entry in the try_call invocation path since that is a further loosening feature (@graydon has suggested it).

I don't think it makes sense to couple reentry as a capability with error handling, as there are disconnected reasons to choose one and the other.

In the SDK I hope to add support for "invocation options" on functions that call both call and try_call, and it will muddy the SDK API if these options are inconsistent between the two functions.

At the moment these options will only include the reentry flag, but I imagine them including other options in the future.

cc @graydon 

Patch:
diff --git a/soroban-env-host/src/host.rs b/soroban-env-host/src/host.rs
index 608cd50f..cb4ca87f 100644
--- a/soroban-env-host/src/host.rs
+++ b/soroban-env-host/src/host.rs
@@ -747,10 +747,36 @@ impl Host {
     }
 
     // Notes on metering: this is covered by the called components.
-    fn call_n(&self, contract: Object, func: Symbol, args: &[RawVal]) -> Result<RawVal, HostError> {
+    fn call_n(
+        &self,
+        contract: Object,
+        func: Symbol,
+        args: &[RawVal],
+        allow_reentry: bool,
+    ) -> Result<RawVal, HostError> {
         // Get contract ID
         let id = self.hash_from_obj_input("contract", contract)?;
 
+        if !allow_reentry {
+            for f in self.0.context.borrow().iter() {
+                let exist_id = match f {
+                    #[cfg(feature = "vm")]
+                    Frame::ContractVM(vm, _) => &vm.contract_id,
+                    Frame::Token(id, _) => id,
+                    #[cfg(feature = "testutils")]
+                    Frame::TestContract(id, _) => id,
+                    Frame::HostFunction(_) => continue,
+                };
+                if id == *exist_id {
+                    return Err(self.err_status_msg(
+                        // TODO: proper error code
+                        ScHostContextErrorCode::UnknownError,
+                        "Contract re-entry is not allowed",
+                    ));
+                }
+            }
+        }
+
         // "testutils" is not covered by budget metering.
         #[cfg(feature = "testutils")]
         {
@@ -788,7 +814,8 @@ impl Host {
                             .iter()
                             .map(|scv| self.to_host_val(scv).map(|hv| hv.val))
                             .collect::<Result<Vec<RawVal>, HostError>>()?;
-                        self.call_n(object, symbol, &args[..])
+                        // doesn't matter what `reentry` flag is passed, `HostFunction` must be the first frame
+                        self.call_n(object, symbol, &args[..], false)
                     })
                 } else {
                     Err(self.err_status_msg(
@@ -1971,10 +1998,12 @@ impl VmCallerCheckedEnv for Host {
 
         let buf = self.id_preimage_from_asset(a)?;
         let id = self.create_contract_with_id_preimage(ScContractCode::Token, buf)?;
+        // reentry is irrelavent here since this token contract has just been created
         self.call_n(
             id,
             Symbol::from_str("init_asset"),
             &[self.to_host_val(&arg)?.val],
+            false,
         )?;
 
         Ok(id)
@@ -1988,11 +2017,9 @@ impl VmCallerCheckedEnv for Host {
         func: Symbol,
         args: Object,
     ) -> Result<RawVal, HostError> {
-        let args: Vec<RawVal> = self.visit_obj(args, |hv: &HostVec| {
-            self.charge_budget(CostType::CallArgsUnpack, hv.len() as u64)?;
-            Ok(hv.iter().map(|a| a.to_raw()).collect())
-        })?;
-        let res = self.call_n(contract, func, args.as_slice());
+        let args = self.call_args_from_obj(args)?;
+        // this is the recommanded path of calling a contract, with `reentry` always set `false`
+        let res = self.call_n(contract, func, args.as_slice(), false);
         if let Err(e) = &res {
             let evt = DebugEvent::new()
                 .msg("contract call invocation resulted in error {}")
@@ -2010,9 +2037,21 @@ impl VmCallerCheckedEnv for Host {
         func: Symbol,
         args: Object,
     ) -> Result<RawVal, HostError> {
-        match self.call(vmcaller, contract, func, args) {
+        let args = self.call_args_from_obj(args)?;
+        // this is the "loosened" path of calling a contract.
+        // TODO: A `reentry` flag will be passed from `try_call` into here.
+        // For now, we are passing in `false` to disable reentry.
+        let res = self.call_n(contract, func, args.as_slice(), false);
+        match res {
             Ok(rv) => Ok(rv),
-            Err(e) => Ok(e.status.into()),
+            Err(e) => {
+                let status: RawVal = e.status.into();
+                let evt = DebugEvent::new()
+                    .msg("contract call invocation resulted in error {}")
+                    .arg(status);
+                self.record_debug_event(evt)?;
+                Ok(status)
+            }
         }
     }
 
diff --git a/soroban-env-host/src/host/conversion.rs b/soroban-env-host/src/host/conversion.rs
index 4ef10b9f..98ba04f5 100644
--- a/soroban-env-host/src/host/conversion.rs
+++ b/soroban-env-host/src/host/conversion.rs
@@ -7,7 +7,7 @@ use crate::xdr::{
 use crate::{
     budget::CostType,
     events::{DebugError, CONTRACT_EVENT_TOPICS_LIMIT, TOPIC_BYTES_LENGTH_LIMIT},
-    host_object::HostObject,
+    host_object::{HostObject, HostVec},
     Host, HostError, Object, RawVal, Tag,
 };
 use ed25519_dalek::{PublicKey, Signature, SIGNATURE_LENGTH};
@@ -333,4 +333,11 @@ impl Host {
             }
         }
     }
+
+    pub(crate) fn call_args_from_obj(&self, args: Object) -> Result<Vec<RawVal>, HostError> {
+        self.visit_obj(args, |hv: &HostVec| {
+            self.charge_budget(CostType::CallArgsUnpack, hv.len() as u64)?;
+            Ok(hv.iter().map(|a| a.to_raw()).collect())
+        })
+    }
 }


Test patch:
diff --git a/soroban-env-host/src/test/invocation.rs b/soroban-env-host/src/test/invocation.rs
index 7b5a4e59..da7a3ec2 100644
--- a/soroban-env-host/src/test/invocation.rs
+++ b/soroban-env-host/src/test/invocation.rs
@@ -1,4 +1,7 @@
-use soroban_env_common::{xdr::ScVmErrorCode, RawVal};
+use soroban_env_common::{
+    xdr::{ScHostContextErrorCode, ScVmErrorCode},
+    RawVal,
+};
 
 use crate::{
     budget::Budget,
@@ -244,3 +247,30 @@ fn invoke_cross_contract_indirect_err() -> Result<(), HostError> {
 
     Ok(())
 }
+
+#[test]
+fn invoke_contract_with_reentry() -> Result<(), HostError> {
+    let dummy_id0 = [0; 32]; // the calling contract
+    let budget = Budget::default();
+    let storage = Host::test_storage_with_contracts(
+        vec![dummy_id0.into()],
+        vec![INVOKE_CONTRACT],
+        budget.clone(),
+    );
+    let host = Host::with_storage_and_budget(storage, budget);
+    // prepare arguments
+    let id0_obj = host.test_bin_obj(&dummy_id0)?;
+    let sym = Symbol::from_str("add_with");
+    let args = host.test_vec_obj::<i32>(&[i32::MAX, 1])?;
+    let args = host.vec_push_back(args.val, id0_obj.clone().into())?; // trying to call its own `add` function
+
+    // try call -- add will trap, and add_with will trap, but we will get a status
+    let res = host.call(id0_obj.clone().to_object(), sym.into(), args.clone().into());
+    let status = host.try_call(id0_obj.clone().to_object(), sym.into(), args.clone().into())?;
+    let code = ScHostContextErrorCode::UnknownError;
+    let exp: Status = code.into();
+    assert!(HostError::result_matches_err_status(res, code));
+    assert_eq!(status.get_payload(), exp.to_raw().get_payload());
+
+    Ok(())
+}
diff --git a/soroban-test-wasms/wasm-workspace/invoke_contract/src/lib.rs b/soroban-test-wasms/wasm-workspace/invoke_contract/src/lib.rs
index 1cc6bee3..4d40ccae 100644
--- a/soroban-test-wasms/wasm-workspace/invoke_contract/src/lib.rs
+++ b/soroban-test-wasms/wasm-workspace/invoke_contract/src/lib.rs
@@ -5,6 +5,10 @@ pub struct Contract;
 
 #[contractimpl]
 impl Contract {
+    pub fn add(a: i32, b: i32) -> i32 {
+        a + b
+    }
+
     pub fn add_with(env: Env, x: i32, y: i32, contract_id: BytesN<32>) -> i32 {
         env.invoke_contract(
             &contract_id,