From d70e5c234fe84243822b897711abedb70a5a7a4a Mon Sep 17 00:00:00 2001 From: kiseln <3428059+kiseln@users.noreply.github.com> Date: Thu, 3 Apr 2025 19:15:52 +0400 Subject: [PATCH 1/3] Minor audit fixes --- near/omni-bridge/src/lib.rs | 15 ++++++++++++--- near/omni-bridge/src/storage.rs | 1 + near/omni-tests/src/fast_transfer.rs | 4 ++-- near/omni-types/src/lib.rs | 2 +- 4 files changed, 16 insertions(+), 6 deletions(-) diff --git a/near/omni-bridge/src/lib.rs b/near/omni-bridge/src/lib.rs index ca8342b18..1f7755c71 100644 --- a/near/omni-bridge/src/lib.rs +++ b/near/omni-bridge/src/lib.rs @@ -443,7 +443,9 @@ impl Contract { fee_recipient, }; - let payload = near_sdk::env::keccak256_array(&borsh::to_vec(&transfer_payload).unwrap()); + let payload = near_sdk::env::keccak256_array( + &borsh::to_vec(&transfer_payload).sdk_expect("ERR_BORSH"), + ); ext_signer::ext(self.mpc_signer.clone()) .with_static_gas(MPC_SIGNING_GAS) @@ -640,6 +642,7 @@ impl Contract { if let OmniAddress::Near(recipient) = fast_fin_transfer_msg.recipient { let storage_deposit_amount = fast_fin_transfer_msg .storage_deposit_amount + .map(|amount| amount.0) .unwrap_or_default(); if storage_deposit_amount > 0 { self.update_storage_balance( @@ -652,7 +655,9 @@ impl Contract { let deposit_action = StorageDepositAction { account_id: recipient, token_id, - storage_deposit_amount: fast_fin_transfer_msg.storage_deposit_amount, + storage_deposit_amount: fast_fin_transfer_msg + .storage_deposit_amount + .map(|amount| amount.0), }; PromiseOrValue::Promise( Self::check_or_pay_ft_storage( @@ -1320,6 +1325,7 @@ impl Contract { predecessor_account_id == status.relayer, "ERR_FAST_TRANSFER_PERFORMED_BY_ANOTHER_RELAYER" ); + require!(!status.finalised, "ERR_FAST_TRANSFER_ALREADY_FINALISED"); (status.relayer, String::new(), true) } None => (recipient, transfer_message.msg.clone(), false), @@ -1433,6 +1439,7 @@ impl Contract { predecessor_account_id == status.relayer, "ERR_FAST_TRANSFER_PERFORMED_BY_ANOTHER_RELAYER" ); + require!(!status.finalised, "ERR_FAST_TRANSFER_ALREADY_FINALISED"); Some(status.relayer) } None => None, @@ -1652,7 +1659,9 @@ impl Contract { } fn mark_fast_transfer_as_finalised(&mut self, fast_transfer_id: &FastTransferId) { - let mut status = self.get_fast_transfer_status(fast_transfer_id).unwrap(); + let mut status = self + .get_fast_transfer_status(fast_transfer_id) + .sdk_expect("ERR_FAST_TRANSFER_NOT_FOUND"); status.finalised = true; self.fast_transfers .insert(fast_transfer_id, &FastTransferStatusStorage::V0(status)); diff --git a/near/omni-bridge/src/storage.rs b/near/omni-bridge/src/storage.rs index 6f5ec26b4..fc45b09bd 100644 --- a/near/omni-bridge/src/storage.rs +++ b/near/omni-bridge/src/storage.rs @@ -110,6 +110,7 @@ impl Contract { storage } + #[payable] pub fn storage_unregister(&mut self, force: Option) -> bool { assert_one_yocto(); let account_id = env::predecessor_account_id(); diff --git a/near/omni-tests/src/fast_transfer.rs b/near/omni-tests/src/fast_transfer.rs index b6429ba5c..1005dec0a 100644 --- a/near/omni-tests/src/fast_transfer.rs +++ b/near/omni-tests/src/fast_transfer.rs @@ -581,7 +581,7 @@ mod tests { let transfer_msg = get_transfer_msg_to_near(&env, transfer_amount); let mut fast_transfer_msg = get_fast_transfer_msg(&env, transfer_msg); fast_transfer_msg.storage_deposit_amount = - Some(NEP141_DEPOSIT.saturating_mul(100).as_yoctonear()); + Some(U128(NEP141_DEPOSIT.saturating_mul(100).as_yoctonear())); let relayer_balance_before = get_balance(&env.token_contract, env.relayer_account.id()).await?; @@ -1070,7 +1070,7 @@ mod tests { fee: transfer_msg.fee, msg: transfer_msg.msg, storage_deposit_amount: match transfer_msg.recipient.get_chain() { - ChainKind::Near => Some(NEP141_DEPOSIT.as_yoctonear()), + ChainKind::Near => Some(U128(NEP141_DEPOSIT.as_yoctonear())), _ => None, }, relayer: env.relayer_account.id().clone(), diff --git a/near/omni-types/src/lib.rs b/near/omni-types/src/lib.rs index de379fed7..10838e28b 100644 --- a/near/omni-types/src/lib.rs +++ b/near/omni-types/src/lib.rs @@ -405,7 +405,7 @@ pub struct FastFinTransferMsg { pub recipient: OmniAddress, pub fee: Fee, pub msg: String, - pub storage_deposit_amount: Option, + pub storage_deposit_amount: Option, pub relayer: AccountId, } From d30c2a96b753d7b1f9ecff5d65c3f4bcaf394283 Mon Sep 17 00:00:00 2001 From: kiseln <3428059+kiseln@users.noreply.github.com> Date: Mon, 7 Apr 2025 17:14:25 +0400 Subject: [PATCH 2/3] Fix storage_withdraw --- near/omni-bridge/src/storage.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/near/omni-bridge/src/storage.rs b/near/omni-bridge/src/storage.rs index fc45b09bd..a232e0096 100644 --- a/near/omni-bridge/src/storage.rs +++ b/near/omni-bridge/src/storage.rs @@ -107,6 +107,9 @@ impl Contract { .sdk_expect("The amount is greater than the available storage balance"); self.accounts_balances.insert(&account_id, &storage); + + Promise::new(account_id).transfer(to_withdraw); + storage } From 35a6332e49f9d53b3f0f2714cd176ac203384b23 Mon Sep 17 00:00:00 2001 From: kiseln <3428059+kiseln@users.noreply.github.com> Date: Thu, 10 Apr 2025 14:39:19 +0400 Subject: [PATCH 3/3] feat: add native fee restriction (based on implementation from main) --- near/omni-bridge/src/lib.rs | 8 + near/omni-tests/src/lib.rs | 1 + near/omni-tests/src/native_fee_role.rs | 495 +++++++++++++++++++++++++ 3 files changed, 504 insertions(+) create mode 100644 near/omni-tests/src/native_fee_role.rs diff --git a/near/omni-bridge/src/lib.rs b/near/omni-bridge/src/lib.rs index 1f7755c71..e5fda02b6 100644 --- a/near/omni-bridge/src/lib.rs +++ b/near/omni-bridge/src/lib.rs @@ -95,6 +95,7 @@ pub enum Role { MetadataManager, UnrestrictedRelayer, TokenControllerUpdater, + NativeFeeRestricted, } #[ext_contract(ext_token)] @@ -470,6 +471,13 @@ impl Contract { amount: U128, init_transfer_msg: InitTransferMsg, ) -> U128 { + // Avoid extra storage read by verifying native fee before checking the role + if init_transfer_msg.native_token_fee.0 > 0 + && self.acl_has_role(Role::NativeFeeRestricted.into(), storage_payer.clone()) + { + env::panic_str("ERR_ACCOUNT_RESTRICTED_FROM_USING_NATIVE_FEE"); + } + require!( init_transfer_msg.recipient.get_chain() != ChainKind::Near, "ERR_INVALID_RECIPIENT_CHAIN" diff --git a/near/omni-tests/src/lib.rs b/near/omni-tests/src/lib.rs index c69725c90..aeb06315e 100644 --- a/near/omni-tests/src/lib.rs +++ b/near/omni-tests/src/lib.rs @@ -2,4 +2,5 @@ mod fast_transfer; mod fin_transfer; mod helpers; mod init_transfer; +mod native_fee_role; mod omni_token; diff --git a/near/omni-tests/src/native_fee_role.rs b/near/omni-tests/src/native_fee_role.rs new file mode 100644 index 000000000..2ce6dfc84 --- /dev/null +++ b/near/omni-tests/src/native_fee_role.rs @@ -0,0 +1,495 @@ +#[cfg(test)] +mod tests { + use near_sdk::{ + json_types::U128, + serde_json::{self, json}, + }; + use near_workspaces::{types::NearToken, AccountId}; + use omni_types::{near_events::OmniBridgeEvent, InitTransferMsg, OmniAddress, TransferMessage}; + use rstest::rstest; + + use crate::helpers::tests::{ + account_n, eth_eoa_address, eth_factory_address, get_event_data, locker_wasm, + mock_prover_wasm, mock_token_wasm, NEP141_DEPOSIT, + }; + + struct TestEnv { + worker: near_workspaces::Worker, + token_contract: near_workspaces::Contract, + locker_contract: near_workspaces::Contract, + sender_account: near_workspaces::Account, + } + + impl TestEnv { + async fn new( + mock_token_wasm: Vec, + mock_prover_wasm: Vec, + locker_wasm: Vec, + ) -> anyhow::Result { + let worker = near_workspaces::sandbox().await?; + + // Deploy and initialize FT token + let token_contract = worker.dev_deploy(&mock_token_wasm).await?; + token_contract + .call("new_default_meta") + .args_json(json!({ + "owner_id": token_contract.id(), + "total_supply": U128(u128::MAX) + })) + .max_gas() + .transact() + .await? + .into_result()?; + + let prover_contract = worker.dev_deploy(&mock_prover_wasm).await?; + + // Deploy and initialize locker + let locker_contract = worker.dev_deploy(&locker_wasm).await?; + locker_contract + .call("new") + .args_json(json!({ + "prover_account": prover_contract.id(), + "mpc_signer": "mpc.testnet", + "nonce": U128(0), + "wnear_account_id": "wnear.testnet", + })) + .max_gas() + .transact() + .await? + .into_result()?; + + // Create admin account (this will be our DAO account) + let admin_account = worker + .create_tla(account_n(99), worker.dev_generate().await.1) + .await? + .unwrap(); + + // Grant DAO role to admin account + locker_contract + .call("acl_grant_role") + .args_json(json!({ + "role": "DAO", + "account_id": admin_account.id(), + })) + .max_gas() + .transact() + .await? + .into_result()?; + + // Create sender account + let sender_account = worker + .create_tla(account_n(1), worker.dev_generate().await.1) + .await? + .unwrap(); + + // Register the accounts in the token contract + token_contract + .call("storage_deposit") + .args_json(json!({ + "account_id": locker_contract.id(), + "registration_only": true, + })) + .deposit(NEP141_DEPOSIT) + .max_gas() + .transact() + .await? + .into_result()?; + + token_contract + .call("storage_deposit") + .args_json(json!({ + "account_id": sender_account.id(), + "registration_only": true, + })) + .deposit(NEP141_DEPOSIT) + .max_gas() + .transact() + .await? + .into_result()?; + + // Transfer initial tokens to the sender account + token_contract + .call("ft_transfer") + .args_json(json!({ + "receiver_id": sender_account.id(), + "amount": U128(1_000_000), + "memo": None::, + })) + .deposit(NearToken::from_yoctonear(1)) + .max_gas() + .transact() + .await? + .into_result()?; + + // Add the ETH factory address to the locker contract + let eth_factory_address = eth_factory_address(); + locker_contract + .call("add_factory") + .args_json(json!({ + "address": eth_factory_address, + })) + .max_gas() + .transact() + .await? + .into_result()?; + + Ok(Self { + worker, + token_contract, + locker_contract, + sender_account, + }) + } + + async fn grant_native_fee_restricted_role( + &self, + account_id: &AccountId, + ) -> anyhow::Result<()> { + self.locker_contract + .call("acl_grant_role") + .args_json(json!({ + "role": "NativeFeeRestricted", + "account_id": account_id, + })) + .max_gas() + .transact() + .await? + .into_result()?; + + Ok(()) + } + + async fn revoke_native_fee_restricted_role( + &self, + account_id: &AccountId, + ) -> anyhow::Result<()> { + self.locker_contract + .call("acl_revoke_role") + .args_json(json!({ + "role": "NativeFeeRestricted", + "account_id": account_id, + })) + .max_gas() + .transact() + .await? + .into_result()?; + + Ok(()) + } + + async fn initialize_transfer( + &self, + amount: u128, + native_fee: u128, + token_fee: u128, + should_succeed: bool, + ) -> anyhow::Result> { + // Prepare storage deposit for the sender + let required_balance_account: NearToken = self + .locker_contract + .view("required_balance_for_account") + .await? + .json()?; + + let init_transfer_msg = InitTransferMsg { + native_token_fee: U128(native_fee), + fee: U128(token_fee), + recipient: eth_eoa_address(), + }; + + let required_balance_init_transfer: NearToken = self + .locker_contract + .view("required_balance_for_init_transfer") + .args_json(json!({ + "recipient": init_transfer_msg.recipient, + "sender": OmniAddress::Near(self.sender_account.id().clone()), + })) + .await? + .json()?; + + // Deposit to storage + let storage_deposit_amount = required_balance_account + .saturating_add(NearToken::from_yoctonear(native_fee)) + .saturating_add(required_balance_init_transfer); + + self.sender_account + .call(self.locker_contract.id(), "storage_deposit") + .args_json(json!({ + "account_id": self.sender_account.id(), + })) + .deposit(storage_deposit_amount) + .max_gas() + .transact() + .await? + .into_result()?; + + // Initiate the transfer + let transfer_result = self + .sender_account + .call(self.token_contract.id(), "ft_transfer_call") + .args_json(json!({ + "receiver_id": self.locker_contract.id(), + "amount": U128(amount), + "memo": None::, + "msg": serde_json::to_string(&init_transfer_msg)?, + })) + .deposit(NearToken::from_yoctonear(1)) + .max_gas() + .transact() + .await?; + + // For the case where we expect failure + if !should_succeed { + // Check if any of the receipt outcomes contain our expected error message + let contains_expected_error = + transfer_result.receipt_outcomes().iter().any(|outcome| { + // Convert outcome to string to check for the error message + let outcome_str = format!("{outcome:?}"); + outcome_str.contains("ERR_ACCOUNT_RESTRICTED_FROM_USING_NATIVE_FEE") + }); + + assert!(contains_expected_error, + "Expected to find ERR_ACCOUNT_RESTRICTED_FROM_USING_NATIVE_FEE error in receipts"); + return Ok(None); + } + + // For successful case, extract the transfer message + let logs = transfer_result + .logs() + .iter() + .map(|s| (*s).to_string()) + .collect::>(); + + let log_refs = logs.iter().collect::>(); + + let omni_bridge_event: OmniBridgeEvent = serde_json::from_value( + get_event_data("InitTransferEvent", &log_refs)? + .ok_or_else(|| anyhow::anyhow!("InitTransferEvent not found"))?, + )?; + + let OmniBridgeEvent::InitTransferEvent { transfer_message } = omni_bridge_event else { + anyhow::bail!("InitTransferEvent is found in unexpected event") + }; + + Ok(Some(transfer_message)) + } + } + + #[rstest] + #[tokio::test] + async fn test_native_fee_restriction( + mock_token_wasm: Vec, + mock_prover_wasm: Vec, + locker_wasm: Vec, + ) -> anyhow::Result<()> { + let env = TestEnv::new(mock_token_wasm, mock_prover_wasm, locker_wasm).await?; + + // 1. Test that an account can set a native fee when not restricted + let transfer_amount = 100; + let native_fee = NearToken::from_near(1).as_yoctonear(); + let token_fee = 10; + + let transfer_message = env + .initialize_transfer( + transfer_amount, + native_fee, + token_fee, + true, // Should succeed + ) + .await? + .unwrap(); + + assert_eq!( + transfer_message.fee.native_fee.0, native_fee, + "Native fee was not set correctly" + ); + + // 2. Grant NativeFeeRestricted role to the sender account + env.grant_native_fee_restricted_role(env.sender_account.id()) + .await?; + + // 3. Test that the account cannot set a native fee when restricted + let result = env + .initialize_transfer( + transfer_amount, + native_fee, + token_fee, + false, // Should fail + ) + .await; + + assert!( + result.is_ok(), + "Transfer should have failed with the expected error" + ); + + // 4. Test that the account can still transfer with zero native fee + let transfer_message = env + .initialize_transfer( + transfer_amount, + 0, // Zero native fee + token_fee, + true, // Should succeed + ) + .await? + .unwrap(); + + assert_eq!( + transfer_message.fee.native_fee.0, 0, + "Native fee should be zero" + ); + + // 5. Revoke the NativeFeeRestricted role + env.revoke_native_fee_restricted_role(env.sender_account.id()) + .await?; + + // 6. Test that the account can set a native fee after role revocation + let transfer_message = env + .initialize_transfer( + transfer_amount, + native_fee, + token_fee, + true, // Should succeed + ) + .await? + .unwrap(); + + assert_eq!( + transfer_message.fee.native_fee.0, native_fee, + "Native fee was not set correctly after role revocation" + ); + + Ok(()) + } + + #[rstest] + #[tokio::test] + async fn test_role_persistence( + mock_token_wasm: Vec, + mock_prover_wasm: Vec, + locker_wasm: Vec, + ) -> anyhow::Result<()> { + let env = TestEnv::new(mock_token_wasm, mock_prover_wasm, locker_wasm).await?; + + // 1. Check role is not granted initially + let has_role: bool = env + .locker_contract + .view("acl_has_role") + .args_json(json!({ + "role": "NativeFeeRestricted", + "account_id": env.sender_account.id() + })) + .await? + .json()?; + + assert!( + !has_role, + "Account should not have NativeFeeRestricted role initially" + ); + + // 2. Grant the role + env.grant_native_fee_restricted_role(env.sender_account.id()) + .await?; + + // 3. Verify role is granted + let has_role: bool = env + .locker_contract + .view("acl_has_role") + .args_json(json!({ + "role": "NativeFeeRestricted", + "account_id": env.sender_account.id() + })) + .await? + .json()?; + + assert!( + has_role, + "Account should have NativeFeeRestricted role after granting" + ); + + // 4. Revoke the role + env.revoke_native_fee_restricted_role(env.sender_account.id()) + .await?; + + // 5. Verify role is revoked + let has_role: bool = env + .locker_contract + .view("acl_has_role") + .args_json(json!({ + "role": "NativeFeeRestricted", + "account_id": env.sender_account.id() + })) + .await? + .json()?; + + assert!( + !has_role, + "Account should not have NativeFeeRestricted role after revoking" + ); + + Ok(()) + } + + #[rstest] + #[tokio::test] + async fn test_admin_permissions( + mock_token_wasm: Vec, + mock_prover_wasm: Vec, + locker_wasm: Vec, + ) -> anyhow::Result<()> { + let env = TestEnv::new(mock_token_wasm, mock_prover_wasm, locker_wasm).await?; + + // Create a new account without special permissions + let unauthorized_account = env + .worker + .create_tla(account_n(42), env.worker.dev_generate().await.1) + .await? + .unwrap(); + + // Try to grant NativeFeeRestricted role using unauthorized account + let _result = unauthorized_account + .call(env.locker_contract.id(), "acl_grant_role") + .args_json(json!({ + "role": "NativeFeeRestricted", + "account_id": env.sender_account.id(), + })) + .max_gas() + .transact() + .await; + + // Verify that the role was NOT granted, regardless of whether the call succeeded or failed + let role_granted: bool = env + .locker_contract + .view("acl_has_role") + .args_json(json!({ + "role": "NativeFeeRestricted", + "account_id": env.sender_account.id() + })) + .await? + .json()?; + + assert!( + !role_granted, + "Role should not be granted by unauthorized account" + ); + + // Verify that authorized admin can grant the role + env.grant_native_fee_restricted_role(env.sender_account.id()) + .await?; + + // Verify role was successfully granted + let has_role: bool = env + .locker_contract + .view("acl_has_role") + .args_json(json!({ + "role": "NativeFeeRestricted", + "account_id": env.sender_account.id() + })) + .await? + .json()?; + + assert!(has_role, "DAO account should be able to grant roles"); + + Ok(()) + } +}