From 0a0156ce3686db969d7924de65c990f42aa7a56a Mon Sep 17 00:00:00 2001 From: Michael Birch Date: Wed, 15 Nov 2023 20:39:46 +0100 Subject: [PATCH 1/2] Feat: XCC uses native bridge logic to unwrap wNEAR --- engine/src/xcc.rs | 38 ++++++++++++++--------------- etc/xcc-router/src/lib.rs | 50 ++++----------------------------------- 2 files changed, 24 insertions(+), 64 deletions(-) diff --git a/engine/src/xcc.rs b/engine/src/xcc.rs index 6a39e69a1..63e1a62fc 100644 --- a/engine/src/xcc.rs +++ b/engine/src/xcc.rs @@ -25,7 +25,7 @@ pub const CODE_KEY: &[u8] = b"router_code"; pub const VERSION_UPDATE_GAS: NearGas = NearGas::new(5_000_000_000_000); pub const INITIALIZE_GAS: NearGas = NearGas::new(15_000_000_000_000); pub const UPGRADE_GAS: NearGas = NearGas::new(20_000_000_000_000); -pub const UNWRAP_AND_REFUND_GAS: NearGas = NearGas::new(25_000_000_000_000); +pub const REFUND_GAS: NearGas = NearGas::new(5_000_000_000_000); pub const WITHDRAW_GAS: NearGas = NearGas::new(40_000_000_000_000); /// Solidity selector for the `withdrawToNear` function /// `https://www.4byte.directory/signatures/?bytes4_signature=0x6b351848` @@ -312,22 +312,20 @@ pub fn handle_precompile_promise( AddressVersionStatus::DeployNeeded { create_needed } => create_needed, AddressVersionStatus::UpToDate => false, }; - let args = format!( - r#"{{"amount": "{}", "refund_needed": {}}}"#, - required_near.as_u128(), - refund_needed, - ); - let unwrap_call = PromiseCreateArgs { - target_account_id: promise.target_account_id.clone(), - method: "unwrap_and_refund_storage".into(), - args: args.into_bytes(), - attached_balance: ZERO_YOCTO, - attached_gas: UNWRAP_AND_REFUND_GAS, - }; - // Safety: This call is safe because the router's `unwrap_and_refund_storage` method - // does not violate any security invariants. It only interacts with the wrap.near contract - // to obtain NEAR from WNEAR. - unsafe { Some(handler.promise_attach_callback(id, &unwrap_call)) } + if refund_needed { + let refund_call = PromiseCreateArgs { + target_account_id: promise.target_account_id.clone(), + method: "send_refund".into(), + args: Vec::new(), + attached_balance: ZERO_YOCTO, + attached_gas: REFUND_GAS, + }; + // Safety: This call is safe because the router's `send_refund` method + // does not violate any security invariants. It only sends NEAR back to this contract. + unsafe { Some(handler.promise_attach_callback(id, &refund_call)) } + } else { + Some(id) + } }; // 3. Finally we can do the call the user wanted to do. @@ -457,8 +455,9 @@ impl AddressVersionStatus { } fn withdraw_to_near_args(recipient: &AccountId, amount: Yocto) -> Vec { + let recipient_with_msg = format!("{recipient}:unwrap"); let args = ethabi::encode(&[ - ethabi::Token::Bytes(recipient.as_bytes().to_vec()), + ethabi::Token::Bytes(recipient_with_msg.into_bytes()), ethabi::Token::Uint(U256::from(amount.as_u128())), ]); [&WITHDRAW_TO_NEAR_SELECTOR, args.as_slice()].concat() @@ -537,6 +536,7 @@ mod tests { #[test] fn test_withdraw_to_near_encoding() { let recipient: AccountId = "some_account.near".parse().unwrap(); + let recipient_with_msg = format!("{recipient}:unwrap"); let amount = Yocto::new(1332654); #[allow(deprecated)] let withdraw_function = ethabi::Function { @@ -559,7 +559,7 @@ mod tests { }; let expected_tx_data = withdraw_function .encode_input(&[ - ethabi::Token::Bytes(recipient.as_bytes().to_vec()), + ethabi::Token::Bytes(recipient_with_msg.into_bytes()), ethabi::Token::Uint(U256::from(amount.as_u128())), ]) .unwrap(); diff --git a/etc/xcc-router/src/lib.rs b/etc/xcc-router/src/lib.rs index 096b38e7d..d3d19324b 100644 --- a/etc/xcc-router/src/lib.rs +++ b/etc/xcc-router/src/lib.rs @@ -4,7 +4,7 @@ use aurora_engine_types::parameters::{ }; use near_sdk::borsh::{self, BorshDeserialize, BorshSerialize}; use near_sdk::collections::{LazyOption, LookupMap}; -use near_sdk::json_types::{U128, U64}; +use near_sdk::json_types::U64; use near_sdk::BorshStorageKey; use near_sdk::{ env, near_bindgen, AccountId, Gas, PanicOnDefault, Promise, PromiseIndex, PromiseResult, @@ -27,15 +27,9 @@ const CURRENT_VERSION: u32 = std::include!("VERSION"); const ERR_ILLEGAL_CALLER: &str = "ERR_ILLEGAL_CALLER"; const INITIALIZE_GAS: Gas = Gas(15_000_000_000_000); -/// Gas cost estimated from mainnet data. Cost seems to consistently be 3 Tgas, but we add a -/// little more to be safe. Example: -/// https://explorer.mainnet.near.org/transactions/3U9SKbGKM3MchLa2hLTNuYLdErcEDneJGbGv1cHZXuvE#HsHabUdJ7DRJcseNa4GQTYwm8KtbB4mqsq2AUssJWWv6 -const WNEAR_WITHDRAW_GAS: Gas = Gas(5_000_000_000_000); /// Gas cost estimated from mainnet data. Example: /// https://explorer.mainnet.near.org/transactions/5NbZ7SfrodNxeLcSkCmLAEdbZfbkk9cjqz3zSDwktKrk#D7un3c3Nxv7Ee3JpQSKiM97LbwCDFPbMo5iLoijGPXPM const WNEAR_REGISTER_GAS: Gas = Gas(5_000_000_000_000); -/// Gas cost estimated from simulation tests. -const REFUND_GAS: Gas = Gas(5_000_000_000_000); /// Registration amount computed from FT token source code, see /// https://github.com/near/near-sdk-rs/blob/master/near-contract-standards/src/fungible_token/core_impl.rs#L50 /// https://github.com/near/near-sdk-rs/blob/master/near-contract-standards/src/fungible_token/storage_impl.rs#L101 @@ -160,37 +154,6 @@ impl Router { env::promise_return(promise_id) } - /// The router will receive wNEAR deposits from its user. This function is to - /// unwrap that wNEAR into NEAR. Additionally, this function will transfer some - /// NEAR back to its parent, if needed. This transfer is done because the parent - /// must cover the storage staking cost with the router account is first created, - /// but the user ultimately is responsible to pay for it. - pub fn unwrap_and_refund_storage(&self, amount: U128, refund_needed: bool) { - self.require_parent_caller(); - - let args = format!(r#"{{"amount": "{}"}}"#, amount.0); - let id = env::promise_create( - self.wnear_account.clone(), - "near_withdraw", - args.as_bytes(), - 1, - WNEAR_WITHDRAW_GAS, - ); - let final_id = if refund_needed { - env::promise_then( - id, - env::current_account_id(), - "send_refund", - &[], - 0, - REFUND_GAS, - ) - } else { - id - }; - env::promise_return(final_id); - } - /// Allows the parent contract to trigger an update to the logic of this contract /// (by deploying a new contract to this account); #[payable] @@ -209,19 +172,14 @@ impl Router { env::promise_return(promise_id); } - #[private] pub fn send_refund(&self) -> Promise { - let parent = self - .parent - .get() - .unwrap_or_else(|| env::panic_str("ERR_CONTRACT_NOT_INITIALIZED")); - + let parent = self.require_parent_caller(); Promise::new(parent).transfer(REFUND_AMOUNT) } } impl Router { - fn require_parent_caller(&self) { + fn require_parent_caller(&self) -> AccountId { let caller = env::predecessor_account_id(); let parent = self .parent @@ -238,6 +196,8 @@ impl Router { env::panic_str("ERR_CALLBACK_OF_FAILED_PROMISE"); } } + + parent } fn promise_create(promise: PromiseArgs) -> PromiseIndex { From c847479fff268be0c45d881ecd3c3ce993411d8a Mon Sep 17 00:00:00 2001 From: Michael Birch Date: Thu, 16 Nov 2023 15:57:54 +0100 Subject: [PATCH 2/2] Refactor require_parent_caller in XCC router --- etc/xcc-router/src/lib.rs | 88 +++++++++++++++++++++++++++++---------- 1 file changed, 66 insertions(+), 22 deletions(-) diff --git a/etc/xcc-router/src/lib.rs b/etc/xcc-router/src/lib.rs index d3d19324b..d55a3ced4 100644 --- a/etc/xcc-router/src/lib.rs +++ b/etc/xcc-router/src/lib.rs @@ -123,7 +123,7 @@ impl Router { /// contract calls is used by the address associated with the sub-account this router contract /// is deployed at. pub fn execute(&self, #[serializer(borsh)] promise: PromiseArgs) { - self.require_parent_caller(); + self.assert_preconditions(); let promise_id = Router::promise_create(promise); env::promise_return(promise_id) @@ -131,7 +131,7 @@ impl Router { /// Similar security considerations here as for `execute`. pub fn schedule(&mut self, #[serializer(borsh)] promise: PromiseArgs) { - self.require_parent_caller(); + self.assert_preconditions(); let nonce = self.nonce.get().unwrap_or_default(); self.scheduled_promises.insert(&nonce, &promise); @@ -158,7 +158,7 @@ impl Router { /// (by deploying a new contract to this account); #[payable] pub fn deploy_upgrade(&mut self, #[serializer(borsh)] args: DeployUpgradeParams) { - self.require_parent_caller(); + self.assert_preconditions(); let promise_id = env::promise_batch_create(&env::current_account_id()); env::promise_batch_action_deploy_contract(promise_id, &args.code); @@ -173,31 +173,37 @@ impl Router { } pub fn send_refund(&self) -> Promise { - let parent = self.require_parent_caller(); + let parent = self.get_parent().unwrap_or_else(env_panic); + + require_caller(&parent) + .and_then(|_| require_no_failed_promises()) + .unwrap_or_else(env_panic); + Promise::new(parent).transfer(REFUND_AMOUNT) } } impl Router { - fn require_parent_caller(&self) -> AccountId { - let caller = env::predecessor_account_id(); - let parent = self - .parent - .get() - .unwrap_or_else(|| env::panic_str("ERR_CONTRACT_NOT_INITIALIZED")); - if caller != parent { - 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"); - } - } + fn get_parent(&self) -> Result { + self.parent.get().ok_or(Error::ContractNotInitialized) + } + + /// Checks the following preconditions: + /// 1. Contract is initialized + /// 2. predecessor_account_id == self.parent + /// 3. There are no failed promise results + /// These preconditions must be checked on methods where are important for + /// the security of the contract (e.g. `execute`). + fn require_preconditions(&self) -> Result<(), Error> { + let parent = self.get_parent()?; + require_caller(&parent)?; + require_no_failed_promises()?; + Ok(()) + } - parent + /// Panics if any of the preconditions checked in `require_preconditions` are not met. + fn assert_preconditions(&self) { + self.require_preconditions().unwrap_or_else(env_panic); } fn promise_create(promise: PromiseArgs) -> PromiseIndex { @@ -359,3 +365,41 @@ fn to_sdk_pk(key: &aurora_engine_types::parameters::NearPublicKey) -> near_sdk:: // Unwrap should be safe because we only encode valid public keys data.try_into().unwrap() } + +fn require_caller(caller: &AccountId) -> Result<(), Error> { + if caller != &env::predecessor_account_id() { + return Err(Error::IllegalCaller); + } + Ok(()) +} + +fn require_no_failed_promises() -> Result<(), Error> { + let num_promises = env::promise_results_count(); + for index in 0..num_promises { + if let PromiseResult::Failed | PromiseResult::NotReady = env::promise_result(index) { + return Err(Error::CallbackOfFailedPromise); + } + } + Ok(()) +} + +fn env_panic(e: Error) -> T { + env::panic_str(e.as_ref()) +} + +#[derive(Debug)] +enum Error { + ContractNotInitialized, + IllegalCaller, + CallbackOfFailedPromise, +} + +impl AsRef for Error { + fn as_ref(&self) -> &str { + match self { + Self::ContractNotInitialized => "ERR_CONTRACT_NOT_INITIALIZED", + Self::IllegalCaller => ERR_ILLEGAL_CALLER, + Self::CallbackOfFailedPromise => "ERR_CALLBACK_OF_FAILED_PROMISE", + } + } +}