Octopus Network Anchor - Octopus Network

The set_owner() method implemented in the AppchainAnchor Struct, which can be found in “appchain-anchor/src/lib.rs”, does not validate that the AccountId value passed to it actually contains a valid AccountId following the NEAR’s account ID rules. As a result, an owner who wishes to update pass ownership to another user can erroneously call the function with a string pointing to an invalid NEAR account ID, resulting in complete and irreversible loss of control over the contract from that point forward.

Code Location

appchain-anchor/src/lib.rs

fn set_owner(&mut self, owner: AccountId) {
    self.assert_owner();
    self.owner = owner;
}

The following is a test case developed as a PoC, notice that the test prints Owner is: th!$1$!nv@|!d when finished:

fn test_set_invalid_owner(){
    let total_supply = common::to_oct_amount(TOTAL_SUPPLY);
    let (root, _, _registry, anchor, _) = common::init(total_supply, false);
    anchor.contract.set_owner("test".to_string());
    let result1 = call!(root, anchor.set_owner("th!$_1$_!nv@|!d".to_string()));
    result1.assert_success();
    let result = view!(anchor.get_owner());
    println!("New owner is: {}", result.unwrap_json::<String>());
}

SOLVED: The Octopus Network team solved the issue in commit ef2219a37c5be402cec720d9db03501981c2ca80

The change_maximum_validator_stake_percent() method in \"appchain-anchor/src/user\textunderscore actions/settings_manager.rs\" checks that the percentage value passed to it is less than a 100 and reverts otherwise. However, all the remaining functions allowing the owner to change other percentage values do not perform such checks, allowing percentages to exceed 100%, which would probably cause the contract to crash and panic while rewards are being distributed.

Code Location

appchain-anchor/src/user_actions/settings_manager.rs

fn change_maximum_market_value_percent_of_near_fungible_tokens(&mut self, value: u16) {
    self.assert_owner();
    let mut protocol_settings = self.protocol_settings.get().unwrap();
    assert!(
        value != protocol_settings.maximum_market_value_percent_of_near_fungible_tokens,
        "The value is not changed."
    );
    protocol_settings.maximum_market_value_percent_of_near_fungible_tokens = value;
    self.protocol_settings.set(&protocol_settings);
}

appchain-anchor/src/user_actions/settings_manager.rs

fn change_maximum_market_value_percent_of_wrapped_appchain_token(&mut self, value: u16) {
    self.assert_owner();
    let mut protocol_settings = self.protocol_settings.get().unwrap();
    assert!(
        value != protocol_settings.maximum_market_value_percent_of_wrapped_appchain_token,
        "The value is not changed."
    );
    protocol_settings.maximum_market_value_percent_of_wrapped_appchain_token = value;
    self.protocol_settings.set(&protocol_settings);
}

appchain-anchor/src/user_actions/settings_manager.rs

fn change_validator_commission_percent(&mut self, value: u16) {
    self.assert_owner();
    let mut protocol_settings = self.protocol_settings.get().unwrap();
    assert!(
        value != protocol_settings.maximum_market_value_percent_of_near_fungible_tokens,
        "The value is not changed."
    );
    protocol_settings.maximum_market_value_percent_of_near_fungible_tokens = value;
    self.protocol_settings.set(&protocol_settings);
}

PARTIALLY SOLVED: The Octopus Network team partially solved the issue in commit ef2219a37c5be402cec720d9db03501981c2ca80

Then some checks were removed in commit eaa2a5109bca0522f6a285f53ebe1e366475bbc6

The register_near_fungible_token() function in \"appchain-anchor/src/assets/near_fungible_tokens.rs\" only checks if the symbol of the token passed to it already exists, however the check is case-sensitive, so it can be bypassed. This allows the owner to register the same token more than once, which can lead to users distributing their funds under different NEAR token contracts instead of one, reducing liquidity and the rewards.

Code Location

appchain-anchor/src/assets/near_fungible_tokens.rs

fn register_near_fungible_token(
    &mut self,
    symbol: String,
    name: String,
    decimals: u8,
    contract_account: AccountId,
    price: U128,
) {
    self.assert_owner();
    let mut near_fungible_tokens = self.near_fungible_tokens.get().unwrap();
    assert!(
        !near_fungible_tokens.contains(&symbol),
        "Token '{}' is already registered.",
        &symbol
    );
    near_fungible_tokens.insert(&NearFungibleToken {
        metadata: FungibleTokenMetadata {
            spec: "ft-1.0.0".to_string(),
            symbol,
            name,
            decimals,
            icon: None,
            reference: None,
            reference_hash: None,
        },
        contract_account,
        price_in_usd: price,
        locked_balance: U128::from(0),
        bridging_state: BridgingState::Active,
    });
    self.near_fungible_tokens.set(&near_fungible_tokens);
}

The following test case reproduces the issue and prints the two tokens that were registered with similar names:

#[test]
fn test_same_token_registeration(){
    let total_supply = common::to_oct_amount(TOTAL_SUPPLY);
    let (root, _, _registry, anchor, _) = common::init(total_supply, false);
    let result = call!(root, anchor.register_near_fungible_token(
        "HLB".to_string(), "Halborn".to_string(),
        2, "test1".to_string(), U128::from(1)
    ));
    result.assert_success();
    let result = call!(root, anchor.register_near_fungible_token(
        "hlb".to_string(), "halborn".to_string(),
        2, "test2".to_string(), U128::from(5)
    ));
    result.assert_success();
    let result2 = view!(anchor.get_near_fungible_tokens());
    let tokens = result2.unwrap_json::<Vec<NearFungibleToken>>();
    tokens.iter().for_each(|token|{
        println!("Token name: {}, symbol: {}", token.metadata.name, token.metadata.symbol);
    });
}

SOLVED: The Octopus Network team solved the issue in commit ef2219a37c5be402cec720d9db03501981c2ca80

Checks should be implemented on amount and price values to make sure they are not set to invalid values, including setting such fields to zero. The set_price_of_oct_token() method implemented in the AppchainAnchor struct in \"appchain-anchor/src/lib.rs\" does not employ such checks to validate that the price of the OCT token does not drop to 0.

Code Location

appchain-anchor/src/lib.rs

pub fn set_price_of_oct_token(&mut self, price: U128) {
    let anchor_settings = self.anchor_settings.get().unwrap();
    assert_eq!(
        env::predecessor_account_id(),
        anchor_settings.token_price_maintainer_account,
        "Only '{}' can call this function.",
        anchor_settings.token_price_maintainer_account
    );
    let mut oct_token = self.oct_token.get().unwrap();
    oct_token.price_in_usd = price;
    self.oct_token.set(&oct_token);
}

NOT APPLICABLE: The Octopus Network team marked the issue as not applicable, as setting OCT to 0 is needed to remove the cross-chain asset transfer restriction.

The change_unlock_period_of_delegator_deposit() and change_unlock_period_of_validator_deposit() functions in \"appchain-anchor/src/user_actions/settings_manager.rs\" do not check for an upper bound for the values passed to them. These functions allow the owner to set the number of days before validators/delegators can withdraw their rewards.

By not checking for an upper bound, the owner can set the values to big numbers that would correspond to years before validators/delegators can actually withdraw their balances.

Code Location

appchain-anchor/src/user_actions/settings_manager.rs

fn change_unlock_period_of_delegator_deposit(&mut self, value: U64) {
    self.assert_owner();
    let mut protocol_settings = self.protocol_settings.get().unwrap();
    assert!(
        value.0 != protocol_settings.unlock_period_of_delegator_deposit.0,
        "The value is not changed."
    );
    protocol_settings.unlock_period_of_delegator_deposit = value;
    self.protocol_settings.set(&protocol_settings);
}

appchain-anchor/src/user_actions/settings_manager.rs

fn change_unlock_period_of_validator_deposit(&mut self, value: U64) {
    self.assert_owner();
    let mut protocol_settings = self.protocol_settings.get().unwrap();
    assert!(
        value.0 != protocol_settings.unlock_period_of_validator_deposit.0,
        "The value is not changed."
    );
    protocol_settings.unlock_period_of_validator_deposit = value;
    self.protocol_settings.set(&protocol_settings);
}

RISK ACCEPTED: The Octopus Network team accepted the risk of this finding.

The change_minimum_validator_count() method in \"appchain-anchor/src/user_actions/settings_manager.rs\" does not guarantee that the minimum validator count set by the owner is not less than 0 or 1 validators at the same time, that directly impacts the security of the chain. A minimum validators constant should be set and checked in that function to make sure validator counts cannot go below that threshold.

Code Location

appchain-anchor/src/user_actions/settings_manager.rs

fn change_minimum_validator_count(&mut self, value: U64) {
    self.assert_owner();
    let mut protocol_settings = self.protocol_settings.get().unwrap();
    assert!(
        value.0 != protocol_settings.minimum_validator_count.0,
        "The value is not changed."
    );
    protocol_settings.minimum_validator_count = value;
    self.protocol_settings.set(&protocol_settings);
}

RISK ACCEPTED: The Octopus Network team accepted the risk stating \"The minimum validator count of the protocol settings will only affect the verification process in the go_booting function, it is just a reference value. Other than that, it does not affect any other functions in the contract or the actual actions/status of the application chain. In some special cases, we can set it to 0 or 1, to change the state of the contract for further operations.\"

The get_market_value_of() function in \"appchain-anchor/src/assets/near_fungible_tokens.rs\" and \"appchain-anchor/src/assets/wrapped_appchain_token.rs\" uses the decimals value in the respective token as an exponent while raising a power and the value is then stored in a u128 variable. The issue here is that the decimals value is not validated before being used when the token is created, leading to the ability to pass any value in the range of 0-255, which allows the token creator to cause a panic case whenever the get_market_value_of() function is called since the pow() function call would yield a value well beyond the possible range of u128.

Code Location

appchain-anchor/src/assets/near_fungible_tokens.rs

pub fn get_market_value_of(&self, symbol: &String, amount: u128) -> Balance {
    if let Some(near_fungible_token) = self.tokens.get(&symbol) {
        amount / u128::pow(10, u32::from(near_fungible_token.metadata.decimals))
            * near_fungible_token.price_in_usd.0
    } else {
        0
    }
}

appchain-anchor/src/assets/wrapped_appchain_token.rs

pub fn get_market_value_of(&self, amount: u128) -> Balance {
    amount / u128::pow(10, u32::from(self.metadata.decimals)) * self.price_in_usd.0
}

RISK ACCEPTED: The Octopus Network team accepted the risk of this finding.

The implementation detail document states that newly registered NEAR fungible tokens should have their bridging state set to Closed. However, the register_near_fungible_token() function in \"appchain-anchor/src/assets/near_fungible_tokens.rs\" registers the token with the bridging state of Active.

Code Location

appchain-anchor/src/assets/near_fungible_tokens.rs

fn register_near_fungible_token(
    &mut self,
    symbol: String,
    name: String,
    decimals: u8,
    contract_account: AccountId,
    price: U128,
) {
    self.assert_owner();
    let mut near_fungible_tokens = self.near_fungible_tokens.get().unwrap();
    assert!(
        !near_fungible_tokens.contains(&symbol),
        "Token '{}' is already registered.",
        &symbol
    );
    near_fungible_tokens.insert(&NearFungibleToken {
        metadata: FungibleTokenMetadata {
            spec: "ft-1.0.0".to_string(),
            symbol,
            name,
            decimals,
            icon: None,
            reference: None,
            reference_hash: None,
        },
        contract_account,
        price_in_usd: price,
        locked_balance: U128::from(0),
        bridging_state: BridgingState::Active,
    });
    self.near_fungible_tokens.set(&near_fungible_tokens);
}

SOLVED: The Octopus Network team solved the issue in commit ef2219a37c5be402cec720d9db03501981c2ca80

The set_rpc_endpoint() and set_subql_endpoint() functions in \"appchain-anchor/src/user_actions/settings_manager.rs\" do not validate the values passed before setting them, allowing the owner to set the URLs to any values that may not follow correct URL forms.

Code Location

appchain-anchor/src/user_actions/settings_manager.rs

fn set_rpc_endpoint(&mut self, rpc_endpoint: String) {
    self.assert_owner();
    let mut appchain_settings = self.appchain_settings.get().unwrap();
    appchain_settings.rpc_endpoint = rpc_endpoint;
    self.appchain_settings.set(&appchain_settings);
}
//
fn set_subql_endpoint(&mut self, subql_endpoint: String) {
    self.assert_owner();
    let mut appchain_settings = self.appchain_settings.get().unwrap();
    appchain_settings.subql_endpoint = subql_endpoint;
    self.appchain_settings.set(&appchain_settings);
}

RISK ACCEPTED: The Octopus Network team accepted the risk of this finding.

The change_maximum_era_count_of_unwithdrawn_reward() function in \"appchain-anchor/src/user_actions/settings_manager.rs\" doesn’t check for an upper limit to the passed value before setting it, allowing the owner to pass a big enough value that is always higher than the end_era value used subsequently in withdraw_delegator_rewards() and withdraw_validator_rewards() (residing in \"appchain-anchor/src/user_actions/staking.rs\") and compared with the maximum_era_count_of_unwithdrawn_reward value. The outcome of this behavior would be looping over a bigger space each time each of these functions is called, which would consume more gas.

Code Location

appchain-anchor/src/user_actions/settings_manager.rs

fn change_maximum_era_count_of_unwithdrawn_reward(&mut self, value: U64) {
    self.assert_owner();
    let mut protocol_settings = self.protocol_settings.get().unwrap();
    assert!(
        value.0 != protocol_settings.maximum_era_count_of_unwithdrawn_reward.0,
        "The value is not changed."
    );
    protocol_settings.maximum_era_count_of_unwithdrawn_reward = value;
    self.protocol_settings.set(&protocol_settings);
}

RISK ACCEPTED: The Octopus Network team accepted the risk of this finding.