Fix(xcc): Ensure near_withdraw comes after ft_transfer (#864)

birchmd · web-flow · commit 0d2acf081937 · 2023-11-10T19:45:18.000Z
## Description

The XCC feature was designed to allow users to spend their own wNEAR
ERC-20 tokens on Aurora in Near native interaction as if it were the
base token. This works by bridging the wNEAR from Aurora out to the
user's XCC account, then unwrapping it. The Rainbow bridge team noticed
an issue where it is possible for the `wrap.near:withdraw_near` promise
to resolve before the `wrap.near:ft_transfer` promise. This causes the
XCC flow to fail if the user's XCC account does not carry a wNEAR
balance because we attempt to withdraw tokens we don't yet have.

This PR aims to solve that issue. To see why this fix works, we need to
know why the issue happens in the first place. The problem is the XCC
flow used to use the `call` entry point to trigger the exit to Near
function on the wNEAR ERC-20 token. That function invokes the exit to
Near precompile which creates a promise to transfer the corresponding
NEP-141 token from `aurora` to the destination account. However, that
promise is not returned from `call` because instead it must return the
EVM `SubmitResult` (the normal use-case for `call` is simply to invoke
the EVM).

By not returning the `ft_transfer` promise, it is disconnected from the
subsequent execution graph and therefore Near does not make any
guarantees about when it will resolve relative to other promises the
execution will create. Under normal (non-congested) conditions, the
`ft_transfer` does resolve first because there is one block before the
`wrap.near:withdraw_near` call is created (since after `aurora:call`
comes `xcc_router:unwrap_and_refund_storage` which then makes the
withdraw call). However, if the shard containing `wrap.near` is
congested then the `ft_transfer` call can delayed by one block and then
need to execute in the same block as `near_withdraw`, resulting in a 50%
chance of failure.

Therefore, to fix the issue we must make sure the promise from the exit
precompile is given as the return value of the call in the XCC flow to
make sure it stays connected with the rest of the execution graph. Doing
this will ensure `wrap.near:ft_transfer` resolves before
`xcc_router:unwrap_and_refund_storage` is allowed to execute.

To that end, in this PR I introduce a new private function called
`withdraw_wnear_to_router`. The only purpose of this function is to make
the call to the exit precompile while capturing its promise and then
return that promise. With that context, this change should be pretty
easy to follow. The new function is defined in `contract_methods::xcc`,
and that logic is applied in both `lib.rs` and the standalone engine.

## Performance / NEAR gas cost considerations

All costs should remain unchanged. The same work is done, just in a
different method to allow the promise return.

## Testing

The bug described above only occurs under congested conditions, so I do
not know how to write a good test for it in near-workspaces. I am
relying on the existing XCC tests to at least be sure this change does
not break the feature.
diff --git a/engine-precompiles/src/xcc.rs b/engine-precompiles/src/xcc.rs
@@ -52,6 +52,7 @@ mod consts {
     pub(super) const ERR_SERIALIZE: &str = "ERR_XCC_CALL_SERIALIZE";
     pub(super) const ERR_STATIC: &str = "ERR_INVALID_IN_STATIC";
     pub(super) const ERR_DELEGATE: &str = "ERR_INVALID_IN_DELEGATE";
+    pub(super) const ERR_XCC_ACCOUNT_ID: &str = "ERR_FAILED_TO_CREATE_XCC_ACCOUNT_ID";
     pub(super) const ROUTER_EXEC_NAME: &str = "execute";
     pub(super) const ROUTER_SCHEDULE_NAME: &str = "schedule";
     /// Solidity selector for the ERC-20 transferFrom function
@@ -130,7 +131,7 @@ impl<I: IO> HandleBasedPrecompile for CrossContractCall<I> {
         }
 
         let sender = context.caller;
-        let target_account_id = create_target_account_id(sender, self.engine_account_id.as_ref());
+        let target_account_id = create_target_account_id(sender, self.engine_account_id.as_ref())?;
         let args = CrossContractCallArgs::try_from_slice(input)
             .map_err(|_| ExitError::Other(Cow::from(consts::ERR_INVALID_INPUT)))?;
         let (promise, attached_near) = match args {
@@ -295,10 +296,13 @@ fn transfer_from_args(from: H160, to: H160, amount: U256) -> Vec<u8> {
     [&consts::TRANSFER_FROM_SELECTOR, args.as_slice()].concat()
 }
 
-fn create_target_account_id(sender: H160, engine_account_id: &str) -> AccountId {
+fn create_target_account_id(
+    sender: H160,
+    engine_account_id: &str,
+) -> Result<AccountId, PrecompileFailure> {
     format!("{}.{}", hex::encode(sender.as_bytes()), engine_account_id)
         .parse()
-        .unwrap_or_default()
+        .map_err(|_| revert_with_message(consts::ERR_XCC_ACCOUNT_ID))
 }
 
 fn revert_with_message(message: &str) -> PrecompileFailure {
diff --git a/engine-standalone-storage/src/sync/mod.rs b/engine-standalone-storage/src/sync/mod.rs
@@ -189,6 +189,10 @@ pub fn parse_transaction_kind(
             })?;
             TransactionKind::FactorySetWNearAddress(address)
         }
+        TransactionKindTag::WithdrawWnearToRouter => {
+            let args = xcc::WithdrawWnearToRouterArgs::try_from_slice(&bytes).map_err(f)?;
+            TransactionKind::WithdrawWnearToRouter(args)
+        }
         TransactionKindTag::SetUpgradeDelayBlocks => {
             let args = parameters::SetUpgradeDelayBlocksArgs::try_from_slice(&bytes).map_err(f)?;
             TransactionKind::SetUpgradeDelayBlocks(args)
@@ -644,6 +648,12 @@ fn non_submit_execute<I: IO + Copy>(
 
             None
         }
+        TransactionKind::WithdrawWnearToRouter(_) => {
+            let mut handler = crate::promise::NoScheduler { promise_data };
+            let result = contract_methods::xcc::withdraw_wnear_to_router(io, env, &mut handler)?;
+
+            Some(TransactionExecutionResult::Submit(Ok(result)))
+        }
         TransactionKind::Unknown => None,
         // Not handled in this function; is handled by the general `execute_transaction` function
         TransactionKind::Submit(_) | TransactionKind::SubmitWithArgs(_) => unreachable!(),
diff --git a/engine-standalone-storage/src/sync/types.rs b/engine-standalone-storage/src/sync/types.rs
@@ -5,6 +5,7 @@ use aurora_engine::xcc::{AddressVersionUpdateArgs, FundXccArgs};
 use aurora_engine_transactions::{EthTransactionKind, NormalizedEthTransaction};
 use aurora_engine_types::account_id::AccountId;
 use aurora_engine_types::parameters::silo;
+use aurora_engine_types::parameters::xcc::WithdrawWnearToRouterArgs;
 use aurora_engine_types::types::Address;
 use aurora_engine_types::{
     borsh::{self, BorshDeserialize, BorshSerialize},
@@ -146,6 +147,8 @@ pub enum TransactionKind {
     FactoryUpdateAddressVersion(AddressVersionUpdateArgs),
     FactorySetWNearAddress(Address),
     FundXccSubAccount(FundXccArgs),
+    /// Self-call used during XCC flow to move wNEAR tokens to user's XCC account
+    WithdrawWnearToRouter(WithdrawWnearToRouterArgs),
     /// Pause the contract
     PauseContract,
     /// Resume the contract
@@ -374,6 +377,31 @@ impl TransactionKind {
                     },
                 )
             }
+            Self::WithdrawWnearToRouter(args) => {
+                let recipient = AccountId::new(&format!(
+                    "{}.{}",
+                    args.target.encode(),
+                    engine_account.as_ref()
+                ))
+                .unwrap_or_else(|_| engine_account.clone());
+                let wnear_address = storage
+                    .with_engine_access(block_height, transaction_position, &[], |io| {
+                        aurora_engine_precompiles::xcc::state::get_wnear_address(&io)
+                    })
+                    .result;
+                let call_args = aurora_engine::xcc::withdraw_wnear_call_args(
+                    &recipient,
+                    args.amount,
+                    wnear_address,
+                );
+                Self::Call(call_args).eth_repr(
+                    engine_account,
+                    caller,
+                    block_height,
+                    transaction_position,
+                    storage,
+                )
+            }
             Self::Deposit(_) => Self::no_evm_execution("deposit"),
             Self::FtTransferCall(_) => Self::no_evm_execution("ft_transfer_call"),
             Self::FinishDeposit(_) => Self::no_evm_execution("finish_deposit"),
@@ -550,6 +578,8 @@ pub enum TransactionKindTag {
     RemoveEntryFromWhitelist,
     #[strum(serialize = "mirror_erc20_token_callback")]
     MirrorErc20TokenCallback,
+    #[strum(serialize = "withdraw_wnear_to_router")]
+    WithdrawWnearToRouter,
     Unknown,
 }
 
@@ -592,6 +622,7 @@ impl TransactionKind {
             Self::NewEngine(args) => args.try_to_vec().unwrap_or_default(),
             Self::FactoryUpdateAddressVersion(args) => args.try_to_vec().unwrap_or_default(),
             Self::FundXccSubAccount(args) => args.try_to_vec().unwrap_or_default(),
+            Self::WithdrawWnearToRouter(args) => args.try_to_vec().unwrap_or_default(),
             Self::PauseContract | Self::ResumeContract | Self::Unknown => Vec::new(),
             Self::SetKeyManager(args) => args.try_to_vec().unwrap_or_default(),
             Self::AddRelayerKey(args) | Self::RemoveRelayerKey(args) => {
@@ -641,6 +672,7 @@ impl From<&TransactionKind> for TransactionKindTag {
             TransactionKind::FactoryUpdate(_) => Self::FactoryUpdate,
             TransactionKind::FactoryUpdateAddressVersion(_) => Self::FactoryUpdateAddressVersion,
             TransactionKind::FactorySetWNearAddress(_) => Self::FactorySetWNearAddress,
+            TransactionKind::WithdrawWnearToRouter(_) => Self::WithdrawWnearToRouter,
             TransactionKind::SetOwner(_) => Self::SetOwner,
             TransactionKind::SubmitWithArgs(_) => Self::SubmitWithArgs,
             TransactionKind::SetUpgradeDelayBlocks(_) => Self::SetUpgradeDelayBlocks,
@@ -886,6 +918,7 @@ enum BorshableTransactionKind<'a> {
     SetWhitelistStatus(Cow<'a, silo::WhitelistStatusArgs>),
     SetEthConnectorContractAccount(Cow<'a, parameters::SetEthConnectorContractAccountArgs>),
     MirrorErc20TokenCallback(Cow<'a, parameters::MirrorErc20TokenArgs>),
+    WithdrawWnearToRouter(Cow<'a, WithdrawWnearToRouterArgs>),
 }
 
 impl<'a> From<&'a TransactionKind> for BorshableTransactionKind<'a> {
@@ -924,6 +957,9 @@ impl<'a> From<&'a TransactionKind> for BorshableTransactionKind<'a> {
             TransactionKind::FactorySetWNearAddress(address) => {
                 Self::FactorySetWNearAddress(*address)
             }
+            TransactionKind::WithdrawWnearToRouter(x) => {
+                Self::WithdrawWnearToRouter(Cow::Borrowed(x))
+            }
             TransactionKind::Unknown => Self::Unknown,
             TransactionKind::PausePrecompiles(x) => Self::PausePrecompiles(Cow::Borrowed(x)),
             TransactionKind::ResumePrecompiles(x) => Self::ResumePrecompiles(Cow::Borrowed(x)),
@@ -1051,6 +1087,9 @@ impl<'a> TryFrom<BorshableTransactionKind<'a>> for TransactionKind {
             BorshableTransactionKind::MirrorErc20TokenCallback(x) => {
                 Ok(Self::MirrorErc20TokenCallback(x.into_owned()))
             }
+            BorshableTransactionKind::WithdrawWnearToRouter(x) => {
+                Ok(Self::WithdrawWnearToRouter(x.into_owned()))
+            }
         }
     }
 }
diff --git a/engine-types/src/parameters/xcc.rs b/engine-types/src/parameters/xcc.rs
@@ -1,6 +1,6 @@
 use crate::account_id::AccountId;
 use crate::borsh::{self, BorshDeserialize, BorshSerialize};
-use crate::types::Address;
+use crate::types::{Address, Yocto};
 
 #[derive(Debug, Clone, PartialEq, Eq, BorshDeserialize, BorshSerialize)]
 pub struct AddressVersionUpdateArgs {
@@ -14,6 +14,12 @@ pub struct FundXccArgs {
     pub wnear_account_id: Option<AccountId>,
 }
 
+#[derive(Debug, Clone, PartialEq, Eq, BorshDeserialize, BorshSerialize)]
+pub struct WithdrawWnearToRouterArgs {
+    pub target: Address,
+    pub amount: Yocto,
+}
+
 /// Type wrapper for version of router contracts.
 #[derive(
     Debug, Clone, Copy, Default, PartialEq, Eq, PartialOrd, Ord, BorshDeserialize, BorshSerialize,
diff --git a/engine/src/contract_methods/evm_transactions.rs b/engine/src/contract_methods/evm_transactions.rs
@@ -56,17 +56,6 @@ pub fn call<I: IO + Copy, E: Env, H: PromiseHandler>(
         let current_account_id = env.current_account_id();
         let predecessor_account_id = env.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 handler.promise_result_check() {
-                Some(true) | None => Ok(()),
-                Some(false) => Err(b"ERR_CALLBACK_OF_FAILED_PROMISE"),
-            };
-            check_promise?;
-        }
-
         let mut engine: Engine<_, E, AuroraModExp> = Engine::new_with_state(
             state,
             predecessor_address(&predecessor_account_id),
diff --git a/engine/src/contract_methods/xcc.rs b/engine/src/contract_methods/xcc.rs
@@ -1,17 +1,69 @@
 use crate::{
-    contract_methods::{require_owner_only, require_running, ContractError},
+    contract_methods::{predecessor_address, require_owner_only, require_running, ContractError},
+    engine::Engine,
     errors,
-    hashchain::with_hashchain,
+    hashchain::{with_hashchain, with_logs_hashchain},
     state, xcc,
 };
+use aurora_engine_modexp::AuroraModExp;
 use aurora_engine_sdk::{
     env::Env,
     io::{StorageIntermediate, IO},
     promise::PromiseHandler,
 };
-use aurora_engine_types::{borsh::BorshSerialize, types::Address};
+use aurora_engine_types::{
+    account_id::AccountId,
+    borsh::BorshSerialize,
+    format,
+    parameters::{engine::SubmitResult, xcc::WithdrawWnearToRouterArgs},
+    types::Address,
+};
 use function_name::named;
 
+#[named]
+pub fn withdraw_wnear_to_router<I: IO + Copy, E: Env, H: PromiseHandler>(
+    io: I,
+    env: &E,
+    handler: &mut H,
+) -> Result<SubmitResult, ContractError> {
+    with_logs_hashchain(io, env, function_name!(), |io| {
+        let state = state::get_state(&io)?;
+        require_running(&state)?;
+        env.assert_private_call()?;
+        if matches!(handler.promise_result_check(), Some(false)) {
+            return Err(b"ERR_CALLBACK_OF_FAILED_PROMISE".into());
+        }
+        let args: WithdrawWnearToRouterArgs = io.read_input_borsh()?;
+        let current_account_id = env.current_account_id();
+        let recipient = AccountId::new(&format!(
+            "{}.{}",
+            args.target.encode(),
+            current_account_id.as_ref()
+        ))?;
+        let wnear_address = aurora_engine_precompiles::xcc::state::get_wnear_address(&io);
+        let mut engine: Engine<_, E, AuroraModExp> = Engine::new_with_state(
+            state,
+            predecessor_address(&current_account_id),
+            current_account_id,
+            io,
+            env,
+        );
+        let (result, ids) = xcc::withdraw_wnear_to_router(
+            &recipient,
+            args.amount,
+            wnear_address,
+            &mut engine,
+            handler,
+        )?;
+        if !result.status.is_ok() {
+            return Err(b"ERR_WITHDRAW_FAILED".into());
+        }
+        let id = ids.last().ok_or(b"ERR_NO_PROMISE_CREATED")?;
+        handler.promise_return(*id);
+        Ok(result)
+    })
+}
+
 #[named]
 pub fn factory_update<I: IO + Copy, E: Env>(io: I, env: &E) -> Result<(), ContractError> {
     with_hashchain(io, env, function_name!(), |mut io| {
diff --git a/engine/src/lib.rs b/engine/src/lib.rs
@@ -388,6 +388,19 @@ mod contract {
             .sdk_unwrap();
     }
 
+    /// A private function (only callable by the contract itself) used as part of the XCC flow.
+    /// This function uses the exit to Near precompile to move wNear from Aurora to a user's
+    /// XCC account.
+    #[no_mangle]
+    pub extern "C" fn withdraw_wnear_to_router() {
+        let io = Runtime;
+        let env = Runtime;
+        let mut handler = Runtime;
+        contract_methods::xcc::withdraw_wnear_to_router(io, &env, &mut handler)
+            .map_err(ContractError::msg)
+            .sdk_unwrap();
+    }
+
     /// Mirror existing ERC-20 token on the main Aurora contract.
     /// Notice: It works if the SILO mode is on.
     #[no_mangle]
diff --git a/engine/src/xcc.rs b/engine/src/xcc.rs
