Merge pull request #14 from aurora-is-near/fix/audit-aur-03

Fix (audit AUR-03): signer_id for: `deposit` and `withdraw`

eth-connector-tests/src/utils.rs

@@ -25,37 +25,16 @@ pub struct TestContract {
 
 impl TestContract {
     pub async fn new() -> anyhow::Result<TestContract> {
-        use std::str::FromStr;
-
-        let (contract, root_account) = Self::deploy_aurora_contract().await?;
-
-        let prover_account: AccountId = contract.id().clone();
-        let eth_custodian_address = CUSTODIAN_ADDRESS;
-        let metadata = Self::metadata_default();
-        let account_with_access_right: AccountId = AccountId::from_str(CONTRACT_ACC).unwrap();
-        // Init eth-connector
-        let res = contract
-            .call("new")
-            .args_json((
-                prover_account,
-                eth_custodian_address,
-                metadata,
-                account_with_access_right,
-            ))
-            .gas(DEFAULT_GAS)
-            .transact()
-            .await?;
-        assert!(res.is_success());
-
-        Ok(Self {
-            contract,
-            root_account,
-        })
+        Self::new_with_custodian_and_owner(CUSTODIAN_ADDRESS, CONTRACT_ACC).await
     }
 
-    pub async fn new_with_custodian(eth_custodian_address: &str) -> anyhow::Result<TestContract> {
+    pub async fn new_with_custodian_and_owner(
+        eth_custodian_address: &str,
+        owner_id: &str,
+    ) -> anyhow::Result<TestContract> {
         use std::str::FromStr;
         let (contract, root_account) = Self::deploy_aurora_contract().await?;
+        let owner_id: AccountId = AccountId::from_str(owner_id).unwrap();
 
         let prover_account: AccountId = contract.id().clone();
         let metadata = Self::metadata_default();
@@ -66,6 +45,7 @@ impl TestContract {
             .args_json(json!({
                 "prover_account": prover_account,
                 "account_with_access_right": account_with_access_right,
+                "owner_id": owner_id,
                 "eth_custodian_address": eth_custodian_address,
                 "metadata": metadata,
             }))
@@ -208,21 +188,21 @@ impl TestContract {
             .view()
             .await?
             .borsh::<bool>()
-            .unwrap();
+            .expect("call_is_used_proof");
         Ok(res)
     }
 
     pub async fn call_deposit_eth_to_aurora(&self) -> anyhow::Result<()> {
         let proof: Proof = serde_json::from_str(PROOF_DATA_ETH).unwrap();
         let res = self.deposit_with_proof(&proof).await?;
-        assert!(res.is_success());
+        assert!(res.is_success(), "call_deposit_eth_to_aurora: {:#?}", res);
         Ok(())
     }
 
     pub async fn call_deposit_eth_to_near(&self) -> anyhow::Result<()> {
         let proof: Proof = self.get_proof(PROOF_DATA_NEAR);
         let res = self.deposit_with_proof(&proof).await?;
-        assert!(res.is_success());
+        assert!(res.is_success(), "call_deposit_eth_to_near: {:#?}", res);
         Ok(())
     }
 
@@ -234,7 +214,7 @@ impl TestContract {
             .view()
             .await?
             .json::<U128>()
-            .unwrap();
+            .expect("get_eth_on_near_balance");
         Ok(res)
     }
 
@@ -245,7 +225,7 @@ impl TestContract {
             .view()
             .await?
             .json::<U128>()
-            .unwrap())
+            .expect("total_supply"))
     }
 
     fn metadata_default() -> FungibleTokenMetadata {
@@ -320,7 +300,12 @@ impl TestContract {
         Ok(())
     }
 
-    pub fn mock_proof(recipient_id: &AccountId, deposit_amount: u128) -> String {
+    pub fn mock_proof(
+        &self,
+        recipient_id: &AccountId,
+        deposit_amount: u128,
+        proof_index: u64,
+    ) -> Proof {
         use aurora_engine_types::{
             types::{Fee, NEP141Wei},
             H160, H256, U256,
@@ -364,21 +349,19 @@ impl TestContract {
                 ethabi::Token::Uint(U256::from(deposit_event.fee.as_u128())),
             ]),
         };
-        let data = Proof {
-            log_index: 1,
+        Proof {
+            log_index: proof_index,
             // Only this field matters for the purpose of this test
             log_entry_data: rlp::encode(&log_entry).to_vec(),
             receipt_index: 1,
             receipt_data: Vec::new(),
             header_data: Vec::new(),
             proof: Vec::new(),
-        };
-        serde_json::to_string(&data).expect("failed serialize proof")
+        }
     }
 
     pub async fn call_deposit_contract(&self) -> anyhow::Result<()> {
-        let proof: Proof =
-            self.get_proof(&Self::mock_proof(self.contract.id(), DEPOSITED_CONTRACT));
+        let proof: Proof = self.mock_proof(self.contract.id(), DEPOSITED_CONTRACT, 1);
         let res = self.deposit_with_proof(&proof).await?;
         assert!(res.is_success(), "call_deposit_contract: {:#?}", res);
         Ok(())

eth-connector/src/admin_controlled.rs

@@ -18,18 +18,15 @@ pub trait AdminControlled {
     /// called by owner of the contract.
     fn set_paused_flags(&mut self, paused: PausedMask);
 
-    /// Return if the contract is paused for the current flag and user
-    fn is_paused(&self, flag: PausedMask, is_owner: bool) -> bool {
-        (self.get_paused_flags() & flag) != 0 && !is_owner
+    /// Return if the contract is paused for the current flag.
+    /// If it's owner, result always `false` - unpaused.
+    fn is_paused(&self, flag: PausedMask) -> bool {
+        (self.get_paused_flags() & flag) != 0 && !self.is_owner()
     }
 
     /// Asserts the passed paused flag is not set. Returns `PausedError` if paused.
-    fn assert_not_paused(
-        &self,
-        flag: PausedMask,
-        is_owner: bool,
-    ) -> Result<(), error::AdminControlledError> {
-        if self.is_paused(flag, is_owner) {
+    fn assert_not_paused(&self, flag: PausedMask) -> Result<(), error::AdminControlledError> {
+        if self.is_paused(flag) {
             Err(error::AdminControlledError::Paused)
         } else {
             Ok(())
@@ -45,13 +42,28 @@ pub trait AdminControlled {
     /// Check access right for predecessor account
     fn assert_access_right(&self) -> Result<(), error::AdminControlledError> {
         if self.get_access_right() == near_sdk::env::predecessor_account_id()
+            || self.is_owner()
             || near_sdk::env::predecessor_account_id() == near_sdk::env::current_account_id()
         {
             Ok(())
         } else {
             Err(error::AdminControlledError::AccessRight)
         }
     }
+
+    /// Asseert only owners of contract access right
+    fn assert_owner_access_right(&self) -> Result<(), error::AdminControlledError> {
+        if self.is_owner()
+            || near_sdk::env::predecessor_account_id() == near_sdk::env::current_account_id()
+        {
+            Ok(())
+        } else {
+            Err(error::AdminControlledError::AccessRight)
+        }
+    }
+
+    /// Check is predecessor account ID is owner
+    fn is_owner(&self) -> bool;
 }
 
 pub mod error {

eth-connector/src/connector_impl.rs

@@ -1,6 +1,6 @@
 use crate::{
     admin_controlled::PAUSE_DEPOSIT,
-    connector::{ext_funds_finish, ext_proof_verifier, ConnectorDeposit},
+    connector::{ext_funds_finish, ext_proof_verifier},
     deposit_event::{DepositedEvent, TokenMessageData},
     errors, log, panic_err,
     proof::Proof,
@@ -58,6 +58,9 @@ pub struct EthConnector {
 
     /// Account with access right for current contract
     pub account_with_access_right: AccountId,
+
+    /// Owner account ID
+    pub owner_id: AccountId,
 }
 
 impl AdminControlled for EthConnector {
@@ -76,16 +79,19 @@ impl AdminControlled for EthConnector {
     fn get_access_right(&self) -> AccountId {
         self.account_with_access_right.clone()
     }
+
+    fn is_owner(&self) -> bool {
+        self.owner_id == env::predecessor_account_id()
+            || env::current_account_id() == env::predecessor_account_id()
+    }
 }
 
-impl ConnectorDeposit for EthConnector {
-    fn deposit(&mut self, raw_proof: Proof) -> Promise {
+impl EthConnector {
+    pub(crate) fn deposit(&mut self, raw_proof: Proof) -> Promise {
         let current_account_id = env::current_account_id();
-        let predecessor_account_id = env::predecessor_account_id();
-        // Check is current account owner
-        let is_owner = current_account_id == predecessor_account_id;
+
         // Check is current flow paused. If it's owner account just skip it.
-        self.assert_not_paused(PAUSE_DEPOSIT, is_owner).sdk_unwrap();
+        self.assert_not_paused(PAUSE_DEPOSIT).sdk_unwrap();
 
         log!("[Deposit tokens]");
         let proof = raw_proof.clone();

eth-connector/src/lib.rs

@@ -166,6 +166,7 @@ impl EthConnectorContract {
         eth_custodian_address: String,
         metadata: FungibleTokenMetadata,
         account_with_access_right: AccountId,
+        owner_id: AccountId,
     ) -> Self {
         require!(!env::state_exists(), "Already initialized");
         metadata.assert_valid();
@@ -177,6 +178,7 @@ impl EthConnectorContract {
             paused_mask,
             eth_custodian_address: Address::decode(&eth_custodian_address).unwrap(),
             account_with_access_right,
+            owner_id,
         };
         let owner_id = env::current_account_id();
         let mut this = Self {
@@ -446,19 +448,23 @@ impl AdminControlled for EthConnectorContract {
         self.connector.get_paused_flags()
     }
 
-    #[private]
     fn set_paused_flags(&mut self, #[serializer(borsh)] paused: PausedMask) {
+        self.connector.assert_owner_access_right().sdk_unwrap();
         self.connector.set_paused_flags(paused)
     }
 
-    #[private]
     fn set_access_right(&mut self, account: &AccountId) {
+        self.connector.assert_owner_access_right().sdk_unwrap();
         self.connector.set_access_right(account)
     }
 
     fn get_access_right(&self) -> AccountId {
         self.connector.get_access_right()
     }
+
+    fn is_owner(&self) -> bool {
+        self.connector.is_owner()
+    }
 }
 
 #[near_bindgen]
@@ -473,12 +479,9 @@ impl ConnectorWithdraw for EthConnectorContract {
     ) -> WithdrawResult {
         self.assert_access_right().sdk_unwrap();
         assert_one_yocto();
-        let predecessor_account_id = env::predecessor_account_id();
-        let current_account_id = env::current_account_id();
-        // Check is current account id is owner
-        let is_owner = current_account_id == predecessor_account_id;
-        // Check is current flow paused. If it's owner just skip assertion.
-        self.assert_not_paused(PAUSE_WITHDRAW, is_owner)
+
+        // Check is current flow paused. If it's owner just skip asserrion.
+        self.assert_not_paused(PAUSE_WITHDRAW)
             .map_err(|_| "WithdrawErrorPaused")
             .sdk_unwrap();
         // Burn tokens to recipient