Octopus Network

It was observed that the project is vulnerable to DoS of the main functionality. In NEAR, there is a validation that tells whether the account format is valid or not. During conclude_voting_score, the new sub_account is created by appending the appchain_id to the registry account:

appchain-registry/src/registry_owner_actions.rs

fn conclude_voting_score(&mut self) {
        self.assert_owner();
        assert!(
            !self.top_appchain_id_in_queue.is_empty(),
            "There is no appchain on the top of queue yet."
        );
        // Set the appchain with the largest voting score to go `staging`
        let sub_account_id = format!(
            "{}.{}",
            &self.top_appchain_id_in_queue,
            env::current_account_id()
        );

Then, at the end, smart contracts creates a new create_account promise action to create new sub account:

appchain-registry/src/registry_owner_actions.rs

   Promise::new(sub_account_id)
            .create_account()
            .transfer(APPCHAIN_ANCHOR_INIT_BALANCE)
            .add_full_access_key(self.owner_pk.clone());
    }

The issue is that no check ensures that the appchain_id complies with NEAR's validation rules. Therefore, if invalid appchain_id became top_appchain_id_in_queue and used during the creation of sub_account, the smart contract will inevitably panic during the creation of the account. Since there is no functionality to remove top_appchain_id_in_queue, the smart contract won't conclude votes anymore. The smart contract will get stuck it at that appchain_id.

Code Location

appchain-registry/src/registry_owner_actions.rs

   Promise::new(sub_account_id)
            .create_account()
            .transfer(APPCHAIN_ANCHOR_INIT_BALANCE)
            .add_full_access_key(self.owner_pk.clone());
    }

SOLVED: The Octopus Network team solved the issue by using ValidAccountId helper class.

Fixed Code:

appchain-registry/src/lib.rs

        assert!(
            !appchain_id.trim().is_empty(),
            "Missing necessary field 'appchain_id'."
        );
        assert!(appchain_id.find(".").is_none(), "Invalid 'appchain_id'.");
        assert!(
            ValidAccountId::try_from(format!("{}.{}", appchain_id, env::current_account_id()))
                .is_ok(),
            "Invalid 'appchain_id'."
        );

It was observed that most of the privileged functionality is controlled by the owner. Additional authorization levels are needed to implement the least privilege principle, also known as least-authority, which ensures only authorized processes, users, or programs can access the necessary resources or information. The ownership role is helpful in a simple system, but more complex projects require more roles by using role-based access control.

Code Location

The owner can access those functions:

• All functions in sudo_actions.rs
• All functions in registry_settings_actions.rs
• All functions in registry_owner_actions.rs except count_voting_score
• set_owner in lib.rs

SOLVED: The Octopus Network team solved the issue by adding role based access control functionality..

It was observed that the owner could set itself as a voter_operator. This functionality violates the principle of least privilege giving the owner additional privileges.

Code Location

appchain-registry/src/registry_settings_actions.rs

   fn change_operator_of_counting_voting_score(&mut self, operator_account: AccountId) {
        self.assert_owner();
        let mut registry_settings = self.registry_settings.get().unwrap();
        registry_settings.operator_of_counting_voting_score.clear();
        registry_settings
            .operator_of_counting_voting_score
            .push_str(&operator_account);
        self.registry_settings.set(&registry_settings);
    }

SOLVED: The Octopus Network team solved the issue by adding relevant check.

Fixed Code:

appchain-registry/src/registry_settings_actions.rs

    fn change_operator_of_counting_voting_score(&mut self, operator_account: AccountId) {
        self.assert_owner();
        assert_ne!(
            operator_account, self.owner,
            "The account should NOT be the owner."
        );
        let mut registry_settings = self.registry_settings.get().unwrap();
        assert_ne!(
            operator_account, registry_settings.operator_of_counting_voting_score,
            "The account is not changed."
        );
        registry_settings.operator_of_counting_voting_score = operator_account;
        self.registry_settings.set(&registry_settings);
    }

It was observed that the owner could be set as an appchain_owner. This functionality violates the principle of least privilege giving the owner additional privileges.

Code Location

appchain-registry/src/lib.rs: register_appchain

• sender_id should not be equal to the registry owner

appchain-registry/src/appchain_owner_actions.rs: transfer_appchain_ownership

• new_owner should not be equal to the registry owner

PARTIALLY SOLVED: The Octopus Network team partially solved the issue by adding the required check only to appchain-registry/src/lib.rs.

Fixed Code:

appchain-registry/src/lib.rs

appchain-registry/src/lib.rs

 fn register_appchain(
        &mut self,
        sender_id: AccountId,
        appchain_id: AppchainId,
        register_deposit: Balance,
        website_url: String,
        function_spec_url: String,
        github_address: String,
        github_release: String,
        contact_email: String,
        premined_wrapped_appchain_token_beneficiary: AccountId,
        premined_wrapped_appchain_token: U128,
        ido_amount_of_wrapped_appchain_token: U128,
        initial_era_reward: U128,
        fungible_token_metadata: FungibleTokenMetadata,
        custom_metadata: HashMap<String, String>,
    ) {
        assert_ne!(
            sender_id, self.owner,
            "The register account should NOT be the contract owner."
        );

It was observed that the env::signer_account_id() was used in the assert_appchain_owner to assert whether the caller is the appchain_owner.

• env::signer_account_id(): The id of the account that either signed the original transaction or issued the initial cross-contract call.

• env::predecessor_account_id(): The id of the account that was the previous contract in the chain of cross-contract calls. If this is the first contract, it is equal to signer_account_id.

From their definitions above, we can derive that the usage of env::signer_account_id() is risky in access control scenarios. There is a risk that the appchain owner can be phished to sign the cross contract call and hence unknowingly let the malicious contract execute functions in the project's contract under that owner's role.

Code Location

appchain-registry/src/lib.rs

fn assert_appchain_owner(&self, appchain_id: &AppchainId) {
        let appchain_basedata = self.get_appchain_basedata(appchain_id);
        assert_eq!(
            env::signer_account_id(),
            appchain_basedata.owner().clone(),
            "Function can only be called by appchain owner."
        );
    }

SOLVED: The Octopus Network team solved the issue by changing env::signer_account_id() to env::predecessor_account_id().

Fixed Code:

appchain-registry/src/lib.rs

fn assert_appchain_owner(&self, appchain_id: &AppchainId) {
        let appchain_basedata = self.get_appchain_basedata(appchain_id);
        assert_eq!(
            env::predecessor_account_id(),
            appchain_basedata.owner().clone(),
            "Function can only be called by appchain owner."
        );
    }

It was observed that it is possible to register an appchain without providing any core details such as appchain_id, website_url, and so on. Those details are needed for intended functionality of the application.

Code Location

Existence of those fields has to be enforced:

appchain-registry/src/lib.rs: register_appchain

• appchain_id
• website_url
• function_spec_url
• github_address
• github_release
• contact_email
• premined_wrapped_appchain_token_beneficiary
• fungible_token_metadata.name
• fungible_token_metadata.symbol

SOLVED: The Octopus Network team solved the issue by enforcing required fields.

Fixed Code:

appchain-registry/src/lib.rs

assert!(
            !appchain_id.trim().is_empty(),
            "Missing necessary field 'appchain_id'."
        );
        assert!(
            !website_url.trim().is_empty(),
            "Missing necessary field 'website_url'."
        );
        assert!(
            !function_spec_url.trim().is_empty(),
            "Missing necessary field 'function_spec_url'."
        );
        assert!(
            !github_address.trim().is_empty(),
            "Missing necessary field 'github_address'."
        );
        assert!(
            !github_release.trim().is_empty(),
            "Missing necessary field 'github_release'."
        );
        assert!(
            !contact_email.trim().is_empty(),
            "Missing necessary field 'contact_email'."
        );
        assert!(
            !premined_wrapped_appchain_token_beneficiary
                .trim()
                .is_empty(),
            "Missing necessary field 'premined_wrapped_appchain_token_beneficiary'."
        );
        fungible_token_metadata.assert_valid();
        assert!(
            !fungible_token_metadata.name.trim().is_empty(),
            "Missing necessary field 'fungible token name'."
        );
        assert!(
            !fungible_token_metadata.symbol.trim().is_empty(),
            "Missing necessary field 'fungible token symbol'."
        );

It was observed that there is no overflow-checks=true in Cargo.toml. By default, overflow checks are disabled in optimized release builds. Hence, if there is an overflow in release builds, it will be silenced, leading to unexpected behavior of an application. Even if checked arithmetic is used through checked_*, it is recommended to have that check in Cargo.toml.

Code Location

• Cargo.toml

SOLVED: The Octopus Network team solved the issue by adding overflow-checks=true.

The project lacks ability to pause contracts. It is advised that in case of unexpected events temporarily disable some important functions to prevent further damage.

SOLVED: The Octopus Network team solved the issue by adding the pausability to smart contracts.

It was observed that the project uses crates with known vulnerabilities.

Code Location

\begin{center} \begin{tabular}{|l|p{2cm}|p{9cm}|} \hline \textbf{ID} & \textbf{package} & \textbf{Short Description} \ \hline \href{https://rustsec.org/advisories/RUSTSEC-2020-0159}{RUSTSEC-2020-0159} & chrono & Potential segfault in localtime\textunderscore r invocations \ \hline \href{https://rustsec.org/advisories/RUSTSEC-2021-0067}{RUSTSEC-2021-0067} & cranelift-codegen & Memory access due to code generation flaw in Cranelift module\ \hline \href{https://rustsec.org/advisories/RUSTSEC-2021-0013}{RUSTSEC-2021-0013} & raw-cpuid & Soundness issues in raw-cpuid \ \hline \href{https://rustsec.org/advisories/RUSTSEC-2021-0089}{RUSTSEC-2021-0089} & raw-cpuid & Optional Deserialize implementations lacking validation \ \hline \href{https://rustsec.org/advisories/RUSTSEC-2020-0071}{RUSTSEC-2020-0071} & time & Potential segfault in the time crate \ \hline \href{https://rustsec.org/advisories/RUSTSEC-2021-0110}{RUSTSEC-2021-0110} & wasmtime & Multiple Vulnerabilities in Wasmtime \ \hline \end{tabular} \end{center}

RISK ACCEPTED: The Octopus Network team acknowledged the issue and is working on fixing it.

There are functions within the project that should have a zero value check.

Code Location

appchain-registry/src/registry_settings_actions.rs

• change_minimum_register_deposit
  • value should be greater than 0

• change_counting_interval_in_seconds

  • value should be greater than 0. Possibly bigger than 3600 seconds.

• value should be greater than 0

• value should be greater than 0. Possibly bigger than 3600 seconds.

appchain-registry/src/voter_actions.rs

• withdraw_upvote_deposit_of

  • amount should be greater than 0.

• withdraw_downvote_deposit_of

  • amount should be greater than 0.

• amount should be greater than 0.

• amount should be greater than 0.

SOLVED: The Octopus Network team solved the issue by adding necessary zero-value checks.

It was observed that there is a redundant code in withdraw_upvote_deposit_of and withdraw_downvote_deposit_of in the voter_actions module. Even though there is already a variable called voter = env::predecessor_account_id(); new variable called account_id = env::predecessor_account_id(); is created and the same actions are performed for both of those variables for voter_upvote.

Code Location

appchain-appchain-registry/src/voter_actions.rs

fn withdraw_upvote_deposit_of(&mut self, appchain_id: AppchainId, amount: U128) {
        let voter = env::predecessor_account_id();
        let voter_upvote = self
            .upvote_deposits
            .get(&(appchain_id.clone(), voter.clone()))
            .unwrap_or_default();
        assert!(
            voter_upvote >= amount.0,
            "Not enough upvote deposit to withdraw."
        );
        let mut appchain_basedata = self.get_appchain_basedata(&appchain_id);
        let account_id = env::predecessor_account_id();
        let voter_upvote = self
            .upvote_deposits
            .get(&(appchain_id.clone(), account_id.clone()))
            .unwrap_or_default();
        appchain_basedata.decrease_upvote_deposit(amount.0);
        self.appchain_basedatas
            .insert(&appchain_id, &appchain_basedata);
        if amount.0 == voter_upvote {
            self.upvote_deposits
                .remove(&(appchain_id.clone(), account_id.clone()));
        } else {
            self.upvote_deposits.insert(
                &(appchain_id.clone(), account_id.clone()),
                &(voter_upvote - amount.0),
            );
        }

SOLVED: The Octopus Network team solved the issue by removing redundant code.

Fixed Code:

appchain-appchain-registry/src/voter_actions.rs

fn withdraw_upvote_deposit_of(&mut self, appchain_id: AppchainId, amount: U128) {
        let voter = env::predecessor_account_id();
        let voter_upvote = self
            .upvote_deposits
            .get(&(appchain_id.clone(), voter.clone()))
            .unwrap_or_default();
        assert!(
            voter_upvote >= amount.0,
            "Not enough upvote deposit to withdraw."
        );
        let mut appchain_basedata = self.get_appchain_basedata(&appchain_id);
        appchain_basedata.decrease_upvote_deposit(amount.0);
        self.appchain_basedatas
            .insert(&appchain_id, &appchain_basedata);
        if amount.0 == voter_upvote {
            self.upvote_deposits
                .remove(&(appchain_id.clone(), voter.clone()));
        } else {
            self.upvote_deposits.insert(
                &(appchain_id.clone(), voter.clone()),
                &(voter_upvote - amount.0),
            );
        }

It was observed that the project is missing reassignment checks. Reassignment checks make sure that redundant operations are not performed by not letting the reassignment of the existing value.

Code Location

appchain-registry/src/registry_settings_actions.rs

• change_operator_of_counting_voting_score: value

appchain-registry/src/lib.rs

• set_owner: owner

SOLVED: The Octopus Network team added all necessary reassignment checks.

It was observed that the project is manually restricts the usage of uninitialized smart contract. However, near_sdk already provides a PanicOnDefault macro that generates that code for you.

Code Location

appchain-registry/src/lib.rs

impl Default for AppchainRegistry {
    fn default() -> Self {
        env::panic(b"The contract needs be initialized before use.")
    }
}

SOLVED: The Octopus Network team solved the issue by adding PanicOnDefault macro.

It was observed that the project is using outdated rust edition(2018). Recently, 2021 rust edition came out, which includes a lot of stability improvements and new features that might make the code more readable.

Code Location

appchain-registry/Cargo.toml

[package]
name = "appchain-registry"
version = "1.0.5"
authors = ["Octopus Network"]
edition = "2018"

ACKNOWLEDGED: The Octopus Network team acknowledged the issue and is working on fixing it.