Fix: check promise results before executing XCC callbacks (#693)

engine-sdk/src/near_runtime.rs

@@ -470,6 +470,22 @@ impl crate::promise::PromiseHandler for Runtime {
     fn read_only(&self) -> Self::ReadOnly {
         Self
     }
+
+    // An optimized version of the default implementation where we do not copy over the
+    // result bytes of all the successful promise results.
+    fn promise_result_check(&self) -> Option<bool> {
+        let num_promises = self.promise_results_count();
+        if num_promises == 0 {
+            return None;
+        }
+        for index in 0..num_promises {
+            let status = unsafe { exports::promise_result(index, Self::PROMISE_REGISTER_ID.0) };
+            if status != 1 {
+                return Some(false);
+            }
+        }
+        Some(true)
+    }
 }
 
 /// Similar to NearPublicKey, except the first byte includes

engine-sdk/src/promise.rs

@@ -54,6 +54,25 @@ pub trait PromiseHandler {
     }
 
     fn read_only(&self) -> Self::ReadOnly;
+
+    /// Returns `None` if there were no prior promises
+    /// (i.e. the method was not called as a callback). Returns `Some(true)` if
+    /// there was at least one promise result and all results were successful.
+    /// Returns `Some(false)` if there was at least one failed promise result.
+    fn promise_result_check(&self) -> Option<bool> {
+        let num_promises = self.promise_results_count();
+        if num_promises == 0 {
+            return None;
+        }
+        for index in 0..num_promises {
+            if let Some(PromiseResult::Failed | PromiseResult::NotReady) =
+                self.promise_result(index)
+            {
+                return Some(false);
+            }
+        }
+        Some(true)
+    }
 }
 
 pub trait ReadOnlyPromiseHandler {

engine/src/lib.rs

@@ -273,8 +273,21 @@ mod contract {
         let bytes = io.read_input().to_vec();
         let args = CallArgs::deserialize(&bytes).sdk_expect(errors::ERR_BORSH_DESERIALIZE);
         let current_account_id = io.current_account_id();
+        let predecessor_account_id = io.predecessor_account_id();
+
+        // During the XCC flow the Engine will call itself to move wNEAR
+        // to the user's sub-account. We do not want this move to happen
+        // if prior promises in the flow have failed.
+        if current_account_id == predecessor_account_id {
+            let check_promise: Result<(), &[u8]> = match io.promise_result_check() {
+                Some(true) | None => Ok(()),
+                Some(false) => Err(b"ERR_CALLBACK_OF_FAILED_PROMISE"),
+            };
+            check_promise.sdk_unwrap();
+        }
+
         let mut engine = Engine::new(
-            predecessor_address(&io.predecessor_account_id()),
+            predecessor_address(&predecessor_account_id),
             current_account_id,
             io,
             &io,
@@ -349,9 +362,9 @@ mod contract {
     pub extern "C" fn factory_update_address_version() {
         let mut io = Runtime;
         io.assert_private_call().sdk_unwrap();
-        let check_deploy: Result<(), &[u8]> = match io.promise_result(0) {
-            Some(PromiseResult::Successful(_)) => Ok(()),
-            Some(_) => Err(b"ERR_ROUTER_DEPLOY_FAILED"),
+        let check_deploy: Result<(), &[u8]> = match io.promise_result_check() {
+            Some(true) => Ok(()),
+            Some(false) => Err(b"ERR_ROUTER_DEPLOY_FAILED"),
             None => Err(b"ERR_ROUTER_UPDATE_NOT_CALLBACK"),
         };
         check_deploy.sdk_unwrap();

etc/xcc-router/src/lib.rs

@@ -6,7 +6,9 @@ use near_sdk::borsh::{self, BorshDeserialize, BorshSerialize};
 use near_sdk::collections::{LazyOption, LookupMap};
 use near_sdk::json_types::{U128, U64};
 use near_sdk::BorshStorageKey;
-use near_sdk::{env, near_bindgen, AccountId, Gas, PanicOnDefault, Promise, PromiseIndex};
+use near_sdk::{
+    env, near_bindgen, AccountId, Gas, PanicOnDefault, Promise, PromiseIndex, PromiseResult,
+};
 
 #[cfg(not(target_arch = "wasm32"))]
 #[cfg(test)]
@@ -194,7 +196,15 @@ impl Router {
             .get()
             .unwrap_or_else(|| env::panic_str("ERR_CONTRACT_NOT_INITIALIZED"));
         if caller != parent {
-            env::panic_str(ERR_ILLEGAL_CALLER)
+            env::panic_str(ERR_ILLEGAL_CALLER);
+        }
+        // Any method that can only be called by the parent should also only be executed if
+        // the parent's execution was successful.
+        let num_promises = env::promise_results_count();
+        for index in 0..num_promises {
+            if let PromiseResult::Failed | PromiseResult::NotReady = env::promise_result(index) {
+                env::panic_str("ERR_CALLBACK_OF_FAILED_PROMISE");
+            }
         }
     }