Prepared by:
HALBORN
Last Updated 04/26/2024
Date of Engagement by: March 8th, 2022 - April 24th, 2022
92% of all REPORTED Findings have been addressed
All findings
12
Critical
0
High
0
Medium
6
Low
5
Informational
1
Octopus Network engaged Halborn to conduct a security assessment on their NEAR smart contracts beginning on March 8th, 2022 and ending April 24th, 2022. Octopus Network is a multichain interoperable cryptonetwork for launching and running Web3.0 Substrate-based application-specific blockchains, aka appchains.
Though this security audit's outcome is satisfactory, only the most essential aspects were tested and verified to achieve objectives and deliverables set in the scope due to time and resource constraints. It is essential to note the use of the best practices for secure development.
The team at Halborn was provided six weeks for the engagement and assigned one full-time security engineer to audit the security of the assets in scope. The engineer is a blockchain and smart contract security expert with advanced penetration testing, smart-contract hacking, and deep knowledge of multiple blockchain protocols.
The purpose of this audit is to achieve the following:
Identify potential security issues within the NEAR smart contracts.
In summary, Halborn identified few security risks that were mostly addressed by the Octopus Network team.
Halborn performed a combination of manual view of the code and automated security testing to balance efficiency, timeliness, practicality, and accuracy in regard to the scope of the smart contract audit. While manual testing is recommended to uncover flaws in logic, process, and implementation; automated testing techniques help enhance coverage of smart contracts and can quickly identify items that do not follow security best practices. The following phases and associated tools were used throughout the term of the audit:
Research into architecture, purpose, and use of the platform.
Manual code read and walkthrough.
Manual Assessment of use and safety for the critical Rust variables and functions in scope to identify any arithmetic related vulnerability classes.
Fuzz testing. (cargo fuzz)
Checking the unsafe code usage. (cargo-geiger)
Scanning of Rust files for vulnerabilities.(cargo audit)
Deployment to devnet through near-cli
Critical
0
High
0
Medium
6
Low
5
Informational
1
Impact x Likelihood
HAL-01
HAL-02
HAL-03
HAL-05
HAL-06
HAL-04
HAL-07
HAL-08
HAL-09
HAL-11
HAL-12
HAL-10
| Security analysis | Risk level | Remediation Date |
|---|---|---|
| TOKEN PRICE MAINTAINER AS WELL AS RELAYER CAN BE SET TO THE OWNER’S ACCOUNTID | Medium | Solved - 03/14/2022 |
| OWNER ACCOUNTID CAN BE SET TO AN INVALID VALUE | Medium | Solved - 03/14/2022 |
| LACK OF VALIDATION ALLOWS SETTING PERCENTAGES HIGHER THAN A HUNDRED | Medium | Partially Solved |
| CASE SENSITIVE CHECK ALLOWS ADDING THE SAME NEAR FUNGIBLE TOKEN MORE THAN ONCE | Medium | Solved - 03/14/2022 |
| MISSING ZERO CHECKS ON AMOUNTS AND PRICES | Medium | Not Applicable |
| LACK OF UPPER LIMIT CHECKS ALLOWS BLOCKING WITHDRAWALS | Medium | Risk Accepted |
| MINIMUM VALIDATOR COUNT CAN BE SET TO 0 OR 1 | Low | Risk Accepted |
| LACK OF UPPER BOUND CHECKS ON DECIMALS LEADS TO PANICS | Low | Risk Accepted |
| DEFAULT BRIDGING STATE FOR NEW NEAR FUNGIBLE TOKENS IS `ACTIVE` WHILE IT SHOULD BE `CLOSED` | Low | Solved - 03/14/2022 |
| NO URL VALIDATION ON SETTING RPC AND SubQL ENDPOINTS | Low | Risk Accepted |
| LACK OF UPPER LIMIT CHECKS MAY CAUSE RESOURCE EXHAUSTION | Low | Risk Accepted |
| LACK OF VALIDATOR ACCOUNTID VALIDATION ALLOWS USING INVALID ACCOUNTID | Informational | - |
// Medium
The set_token_price_maintainer_account() and set_relayer_account() methods implemented for the AppchainAnchor struct and can be found in appchain-anchor/src/user_actions/settings_manager.rs do not validate the AccountId passed to them to ensure that no matches that of the owner, allowing a malicious owner to bypass the privilege separation point in this case.
fn set_token_price_maintainer_account(&mut self, account_id: AccountId) {
self.assert_owner();
let mut anchor_settings = self.anchor_settings.get().unwrap();
anchor_settings.token_price_maintainer_account = account_id;
self.anchor_settings.set(&anchor_settings);
}
fn set_relayer_account(&mut self, account_id: AccountId) {
self.assert_owner();
let mut anchor_settings = self.anchor_settings.get().unwrap();
anchor_settings.relayer_account = account_id;
self.anchor_settings.set(&anchor_settings);
}
The following test case was created as a PoC:
fn test_set_owner_as_maintainer(){
let total_supply = common::to_oct_amount(TOTAL_SUPPLY);
let (root, _, _registry, anchor, _) = common::init(total_supply, false);
let result = settings_actions::set_token_price_maintainer_account(&root, &anchor, &root);
result.assert_success();
let result2 = view!(anchor.get_owner());
let owner = result2.unwrap_json::<String>();
let anchor_settings = anchor_viewer::get_anchor_settings(&anchor);
let maintainer = anchor_settings.token_price_maintainer_account;
assert!(owner == maintainer);
}
SOLVED: The Octopus Network team solved the issue in commit ef2219a37c5be402cec720d9db03501981c2ca80
// Medium
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.
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
// Medium
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.
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);
}
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);
}
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
// Medium
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.
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
// Medium
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.
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.
// Medium
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.
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);
}
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.
// Low
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.
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.\"
// Low
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.
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
}
}
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.
// Low
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.
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
// Low
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.
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.
// Low
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.
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.
// Informational
Halborn used automated security scanners to assist with detection of well-known security issues and vulnerabilities. Among the tools used was cargo audit, a security scanner for vulnerabilities reported to the RustSec Advisory Database. All vulnerabilities published in https://crates.io are stored in a repository named The RustSec Advisory Database. cargo audit is a human-readable version of the advisory database which performs a scanning on Cargo.lock. Security Detections are only in scope. All vulnerabilities shown here were already disclosed in the above report. However, to better assist the developers maintaining this code, the auditors are including the output with the dependencies tree, and this is included in the cargo audit output to better know the dependencies affected by unmaintained and vulnerable crates.
\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-2022-0013}{RUSTSEC-2022-0013} & regex & Regexes with large repetitions on empty sub-expressions take a very long time to parse \ \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}
Halborn strongly recommends conducting a follow-up assessment of the project either within six months or immediately following any material changes to the codebase, whichever comes first. This approach is crucial for maintaining the project’s integrity and addressing potential vulnerabilities introduced by code modifications.
// Download the full report
* Use Google Chrome for best results
** Check "Background Graphics" in the print settings if needed