INTRODUCTION

BlockApex (Auditor) was contracted by BullionFX (Client) for the purpose of conducting a Smart Contract Audit/Code Review. This document presents the findings of our analysis which started on 20th Nov 2022.

Scope

The git-repository shared was checked for common code violations along with vulnerability-specific probing to detect major issues/vulnerabilities. Some specific checks are as follows:

Project Overview

Rain Protocol is a set of building blocks that enable your token economy including staking, vesting, emissions, escrow, order book, verification, sale, and membership. Rain VM uses EVM making it fully compatible with all the EVM chains. DEMI has integrated Rain protocol’s Staking contracts.

System Architecture

Staking Factory Contract:

The system uses a design pattern called the ‘factory method’ to deploy homogenous contracts from the same parent & keeps a record of the child contracts that have been deployed by the factory. In this particular case, the system deploys a staking contract.

Staking Contract:

Staking contract is a modified version of a vault contract tokenized. It inherits all of the properties of the ERC-4626 contract which standardizes the interface for easily managing deposited tokens & their shares within the system. The contract also introduces a custom logic for maintaining its own internal ledger for share calculation through the checkpointing mechanism.

Methodology & Scope

The codebase was audited using a filtered audit technique. A pair of two (2) auditors scanned the codebase in an iterative process spanning over a time of One (1) week.

Starting with the recon phase, a basic understanding was developed and the auditors worked on developing presumptions for the developed codebase and the relevant documentation/whitepaper. Furthermore, the audit moved on with the manual code reviews with the motive to find logical flaws in the codebase complemented with code optimizations, software, and security design patterns, code styles, best practices, and identifying false positives that were detected by automated analysis tools.

AUDIT REPORT

Executive Summary

Our team performed a technique called “Filtered Audit”, where the contract was separately audited by four individuals. After a thorough and rigorous process of manual testing, an automated review was carried out using slither for static analysis. All the flags raised were manually reviewed and re-tested to identify the false positives.

Our team found:

# of issues	Severity of the risk
0	Critical Risk issue(s)
0	High Risk issue(s)
0	Medium
1	Low Risk issue(s)
1	Informatory issue(s)

KEY FINDINGS

DETAILED OVERVIEW

Critical-risk issues

No critical issues were found.

High-risk issues

No High-risk issues were found.

Medium-risk issues

No Medium-risk issues were found.

Low-risk issues

Potential Loss of Precision

File: stake/Stake.sol#L15

Description: 

In contracts/math/FixedPointMath.sol the function scaleN will lead to precision loss if a number is scaled down to 18.
There may arise a scenario in future development where a deposited token having a higher decimal (i.e > 18) may need to scale down to 18 decimals for logic consistency. The conversion either from or to 18 will lead to a loss in precision which may result in the dust amount being locked in the contract.

However, this function is not used in the current implementation of the staking contract to either scale up or down.

Recommendation:

Introduce a mechanism to manage the difference that was lost during the conversion. One way could be by storing the difference & using it to convert back.

DEMI Staking Audit - Report

Dev’s Response:

As noted staking contract does not use scaleN. Worth also noting that ERC4626 that the staking contract is based on dedicates a lot of the spec to rounding/dust handling, so if scaleN would be hypothetically used in the future it would still need to be 4626 compliant (which means always leaving dust from the underlying asset in the vault in the case of rounding issues) that's largely what the mulDiv is handling in openzeppelin's implementation and we wrap in the other fixed point math functions

As scaleN is a library contract it has no storage of its own so there's nowhere for it to save information about the lost precision directly, the best it could do is return two values, one representing the scaled value and one representing the lost precision. Currently, scaleN is only used in expressions in the interpreter as a provided opcode, so I'm not sure if it's something on the average expression author's radar to be worrying about or ready to handle (juggling 2 values on the stack to avoid some dust).

it's probably worth documenting all this though as it's worth pointing out as something to consider for anyone who does care about it.

DEMI Staking Audit - Report

Auditor’s Response

The auditors agree with the devs.

Informatory issues and Optimizations

1. Inexplicable inclusion of unused library

File: stake/Stake.sol#L15

Description:

The staking contract imports a library called FixedPointMath.sol that is not used within the scope of the contract.

Recommendation: Remove the unused import along with its using statement.

Alleviation: This issue is fixed.

DISCLAIMER

The smart contracts provided by the client for audit purposes have been thoroughly analyzed in compliance with the global best practices to date w.r.t cybersecurity vulnerabilities and issues in smart contract code, the details of which are enclosed in this report.

This report is not an endorsement or indictment of the project or team, and they do not in any way guarantee the security of the particular object in context. This report is not considered, and should not be interpreted as an influence, on the potential economics of the token, its sale or any other aspect of the project.

Crypto assets/tokens are the results of the emerging blockchain technology in the domain of decentralized finance and they carry with them high levels of technical risk and uncertainty. No report provides any warranty or representation to any third-Party in any respect, including regarding the bug-free nature of code, the business model or proprietors of any such business model, and the legal compliance of any such business. No third party should rely on the reports in any way, including for the purpose of making any decisions to buy or sell any token, product, service, or other assets. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project.

Smart contracts are deployed and executed on a blockchain. The platform, its programming language, and other software related to the smart contract can have its vulnerabilities that can lead to hacks. The scope of our review is limited to a review of the Solidity code and only the Solidity code we note as being within the scope of our review within this report. The Solidity language itself remains under development and is subject to unknown risks and flaws. The review does not extend to the compiler layer or any other areas beyond Solidity that could present security risks.

This audit cannot be considered as a sucient assessment regarding the utility and safety of the code, bug-free status, or any other statements of the contract. While we have done our best in conducting the analysis and producing this report, it is important to note that you should not rely on this report only - we recommend proceeding with several independent audits and a public bug bounty program to ensure the security of smart contracts.

Also see other Audit reports.

INTRODUCTION

BlockApex (Auditor) was contacted by Phase Protocol (Client) for the purpose of conducting a Smart Contract Audit/Code Review. This document presents the findings of our analysis which started on  23st July 2022 

Scope

The git-repository shared was checked for common code violations along with vulnerability-specific probing to detect major issues/vulnerabilities. Some specific checks are as follows:

Project Overview

Phase Protocol, founded by DedMonke is an innovative escrow service that incentivizes credible projects teams for the community approved roadmaps and deliverables. They are a type of fundraising opportunity through the use of NFTs.

System Architecture

Fundraising Through NFTs
With the use of Phase protocol, development teams can easily raise funds for their projects by minting NFTs for their project deliverable. They are one of the finest examples of how NFTs can be used in different ways.

Treasury Control
All the funds are properly controlled by the treasury to assure the community about the transparency of the system and how much funds are currently allocated by the team. 

Fund Redistribution
In any event of failure the funds will be redistributed back to the current holders. There will also be a option to create a DAO in the near future.

Methodology & Scope

The codebase was audited using a filtered audit technique. A band of four (4) auditors scanned the codebase in an iterative process spanning over a time of two (2) weeks. 

Starting with the recon phase, a basic understanding was developed and the auditors worked on developing presumptions for the developed codebase and the relevant documentation/whitepaper. Furthermore, the audit moved on with the manual code reviews with the motive to find logical flaws in the codebase complemented with code optimizations, software and security design patterns, code styles, best practices and identifying false positives that were detected by automated analysis tools.

AUDIT REPORT

Executive Summary

The analysis indicates that the contracts under scope of audit are working properly excluding swap functionality which contains one recent issue.

Our team performed a technique called “Filtered Audit”, where the contract was separately audited by four individuals. After a thorough and rigorous process of manual testing, an automated review was carried out using cargo-audit & cargo-tarpaulin for static analysis and cargo-fuzz for fuzzing invariants. All the flags raised were manually reviewed and re-tested to identify the false positives. 

Our team found: 

# of issues	Severity of the risk
0	Critical Risk issue(s)
1	High Risk issue(s)
1	Medium Risk issue(s)
2	Low Risk issue(s)
2	Informatory issue(s)

Key Findings

Detailed Overview

Critical-risk issues

No issues were found.

High-risk issues

1. Lack Of Proper Checks Could Potentially Lead to Bypass When Changing Phases

File:  In the programs/phase-protocol/src/instructions/phase

Description: 

In the contracts for different phases under the given directory, when changing the state from one phase into another there is not any proper conditional verification and statements to check if the phase is even eligible to change the state. In the contract lock_phase_for_mint.rs only the phases that are approved could be locked, and the phases in the draft cannot be locked. Although the protection will not be bypassed in every scenario, there is a risk that this lack of checks could cause issues in the future. 

Remedy:

According to the documentation provided the phases usually go through the following phases

DRAFT —> APPROVED —> ACTIONED —> STAGED FOR COMPLETION —> COMPLETE.

Now before moving on to the next phase it should check whether the provided phase is even in the phase that is required. As stated above parameters type will usually stop these types of bypasses but adding a proper layer of checks is always good to keep everything properly secured, because optimization is not such an issue in Solana.

 assert!(
           availability.allow_place || availability.allow_cancel,
           ""
       );

Status: 
Fixed

Dev Response: 

Added in explicit state transition checks

Medium-risk issues

2. Too Much Usage Of The Helper Methods.

File: In the programs/phase-protocol/src

Description: 

There is a lot of usage of helper function unwrap(). inside every contract in the given directory. Although helper methods like unwrap(). are extremely helpful during the development and testing phase, making use of these functions in a production environment is an extremely bad practice that should be avoided because this usually causes the program to panic! and does not even show any helpful messages to the user to help solve or understand the problem. 

Remedy:

Some usage of unwrap(). is justified. Proper conditional statements or Some/ None should be made use of because they are more safe and secure.

Status: 

Fixed

Dev Response: 

Added Error handling and messages for client

Low-risk issues

3. Use of Insecure Math In Arithmetic Operations

File:  In the programs

Description: 

Overflow/ Underflow usually happens when the result of any arithmetic operation is outside the range of datatype. There is one example attached below, but it is recommended to use checked_mul(), checked_add() and checked_sub() in all the complex arithmetic operations because they make sure that the variables don’t overflow or underflow. The code below is from programs/phase-protocol/src/state/global_config.rs 

Remedy:

checked_mul(), checked_add() and checked_sub() in all arithmetic operations in the codebase.

Status: 

Fixed

Dev Response: 

Added in a max length constraint to any dynamic sized pda's. The rest are static sizes so there is not overflow issues from user input

4. Use Of Outdated And Vulnerable Crates

Description: 

We used the `cargo audit` to test and detect any outdated and vulnerable crates that are used by the phase protocol and we found out that the contract is utilizing the outdated version of `time` crate. The version in use is 0.1.44 while the latest version is 0.2.23.

Remedy:

We recommend that the given crate should be upgraded to the latest version in order to protect the contracts from any malicious actions. Moreover the old versions are usually less optimized when compared to newer releases.

Status: 

Fixed

Dev Response: 

Updated to latest version of solana-sdk to solve issues. But dep potential seg fault issues still exist in the latest versions

Informatory issues and Optimizations

5. Inexistent math overflows checks 

Description: 

In the overall code, there is a lack of underflow and overflow checks inside the cargo.toml.

Remedy:

In all the cargo.toml files, underflow and overflow checks should be defined. 

Status: 

Fixed

Dev Response: 

"overflow-checks = true" added to cargo.toml to ensure overflow checks are in release

6. Low Test Coverage

File:  In the programs/phase-protocol/tests

Description: 

The test coverage provided in the tests directory is low, although it covers most of the functionalities but it does not contain the negative test cases. Negative test cases are equally important as compared to positive test cases. The test coverage can be found via the command `cargo tarpaulin`. 

Moreover the test coverage provided contains the positive test cases and it does not have enough negative tests to fully test it out.

Remedy:

Writing proper test scenarios for each of the access points and functionality is among the best practices that must be included.

Status: 

Acknowledged

Dev Response: 

This will be worked on further in coming months but not currently able to be prioritized

DISCLAIMER

The smart contracts provided by the client for audit purposes have been thoroughly analyzed in compliance with the global best practices till date w.r.t cybersecurity vulnerabilities and issues in smart contract code, the details of which are enclosed in this report. 

This report is not an endorsement or indictment of the project or team, and they do not in any way guarantee the security of the particular object in context. This report is not considered, and should not be interpreted as an influence, on the potential economics of the token, its sale or any other aspect of the project. 

Crypto assets/tokens are results of the emerging blockchain technology in the domain of decentralized finance and they carry with them high levels of technical risk and uncertainty. No report provides any warranty or representation to any third-Party in any respect, including regarding the bug-free nature of code, the business model or proprietors of any such business model, and the legal compliance of any such business. No third-party should rely on the reports in any way, including for the purpose of making any decisions to buy or sell any token, product, service or other asset. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project.

Smart contracts are deployed and executed on a blockchain. The platform, its programming language, and other software related to the smart contract can have its vulnerabilities that can lead to hacks. The scope of our review is limited to a review of the Solidity code and only the Solidity code we note as being within the scope of our review within this report. The Solidity language itself remains under development and is subject to unknown risks and flaws. The review does not extend to the compiler layer, or any other areas beyond Solidity that could present security risks.

This audit cannot be considered as a sufficient assessment regarding the utility and safety of the code, bug-free status or any other statements of the contract. While we have done our best in conducting the analysis and producing this report, it is important to note that you should not rely on this report only - we recommend proceeding with several independent audits and a public bug bounty program to ensure security of smart contracts.

INTRODUCTION

BlockApex (Auditor) was contracted by Spin Finance (Client) for the purpose of conducting a Smart Contract Audit/Code Review. This document presents the findings of our analysis which started on  31st May 2022 . 

Scope

The git-repository shared was checked for common code violations along with vulnerability-specific probing to detect major issues/vulner abilities. Some specific checks are as follows:

Project Overview

Spin is a DeFi derivative infrastructure built on NEAR Protocol, a reliable and scalable L1 solution. The on-chain order book solution offered by Spin provides a CEX-competitive experience to DeFi users.

Founded in June 2021, Spin was the first product to offer an on-chain order book solution on NEAR Protocol. The advantages of the order book model include better user experience compared to AMM, flexible liquidity, easy access for institutional traders, secure and transparent on-chain verification, opportunity to price different types of instruments, and trading robots interoperability.

System Architecture

Central Limit Order Book Model
Spin uses a single-asset pool, the liquidity of which is sent for market making to the order book. Thus, traders will always have enough liquidity for comfortable trading, and investors can profit from the sophisticated market-making mechanism developed by the Spin team.

Spot trading
The current version of the Spin spot DEX on NEAR is the first order book implementation that supports on-chain order matching and NEAR wallet connection. Currently, the Spin spot DEX is already live on mainnet: https://trade.spin.fi. Spin also provides users with an opportunity to make instant token swaps at the market price in a single click. Spin boasts lower fees compared to AMMs. 

On Spin, for example, on USN/USDC, the taker fee is 0.04% and the maker’s rebate is -0.02%. At the same time, NEAR Protocol’s largest AMM Ref Finance charges 0.3% from swappers.

Methodology & Scope

The codebase was audited using a filtered audit technique. A band of four (4) auditors scanned the codebase in an iterative process spanning over a time of two (2) weeks. 

Starting with the recon phase, a basic understanding was developed and the auditors worked on developing presumptions for the developed codebase and the relevant documentation/whitepaper. Furthermore, the audit moved on with the manual code reviews with the motive to find logical flaws in the codebase complemented with code optimizations, software and security design patterns, code styles, best practices and identifying false positives that were detected by automated analysis tools.

AUDIT REPORT

Executive Summary

The analysis indicates that the contracts under scope of audit are working properly excluding swap functionality which contains one recent issue.

Our team performed a technique called “Filtered Audit”, where the contract was separately audited by four individuals. After a thorough and rigorous process of manual testing, an automated review was carried out using cargo-audit & cargo-tarpaulin for static analysis and cargo-fuzz for fuzzing invariants. All the flags raised were manually reviewed and re-tested to identify the false positives. 

Our team found: 

# of issues	Severity of the risk
0	Critical Risk issue(s)
1	High Risk issue(s)
1	Medium Risk issue(s)
3	Low Risk issue(s)
7	Informatory issue(s)

Key Findings

Detailed Overview

Critical-risk issues

No issues were found.

High-risk issues

1. Logic Flaw in set_availability() will lead wrong state of the market been set

File:  In the marketplace/src/market.rs

Description: 

In the latest commit a mechanism for set_availability() function is changed and an assert!() statement is included to make sure that markets are not set when “placing is on and canceling is off”, the problem here is if both the placement and canceling is off then this assert!() statement will return true because of !availability.allow_place and will eventually set the market in the set_market_option() function of marketplace.rs contract. So because of lack of implementation of proper checks, the market will be set. 

   pub fn set_availability(&mut self, availability: MarketAvailability) {
       assert!(
           !availability.allow_place || availability.allow_cancel,
           "Market state when placing is on and cancelling is off is not possible."
       );
       self.availability = availability;
   }

Remedy:

There should be two checks, the first one will verify whether both the allow_place and allow_cancel are not false at the same thing. After that the current assert!() statement will be called. Adding a statement like above the already present assert!() statement will solve it.

       assert!(
           availability.allow_place || availability.allow_cancel,
           ""
       );

Status: 

Acknowledged.

Dev Response:

Spin consider "allow_place==false and allow_cancel==false" a valid market state. Moreover, that's required for a market to be created with such a configuration, so we can 

1. ensure market was created with correct base-quote pairs

2. ensure the fees and limits were set correctly

2. Order expiration time can be exploited 

File:  In the marketplace/src/marketplace.rs

Description: 

order ttl can be exploited by delaying or keeping users' transactions in the mempool. As the expiry time is calculated by current time_stamp + ttl.Current_time is calculated at the time the transaction is included in the block. Interested parties can keep the order in waiting queues or just do not include it in the block. When the prices are favorable they can execute the order. Although to pull this kind of attack requires high technical resources, it still can be exploited.

fn make_order(&mut self, order: PlaceOrder, meta: PlaceOrderMeta, signer: AccountId) -> Order {
       let expiration_time = if let Some(ttl) = order.ttl {
           // to nanoseconds
           meta.timestamp + (ttl * 1_000_000_000)
       } else {
           Timestamp(0)
       };
       self.order_id_sequence += 1;
 
       Order {
           id: self.order_id_sequence,
           acc: signer,
           price: order.price,
           average_price: U128(0),
           quantity: order.quantity,
           remaining: order.quantity,
           updated_at: Timestamp(0),
           created_at: meta.timestamp,
           expiration_time,
           o_type: order.order_type,
           client_order_id: order.client_order_id,
       }
   }

Remedy:

let expiration_time = ttl;

Status: 

Fixed

3. Potential MultiSig Failure/ Phishable Deployment 

File:  In the spot/src/lib.rs

Description: 

env::signer_account_id() is the account that originates a transaction on the NEAR protocol. In case of a multisig governance account management may not work and the deployer can be phished to interact with malicious contracts. Since the below contracts check on the transaction originator, this check can be bypassed. For further info please take a look at this SWC . 

pub fn create_market(&mut self, base: AccountId, quote: AccountId) -> MarketID {
       let _session = self.make_session();
       let signer = env::signer_account_id();
 
       self.marketplace
           .create_market(signer.as_str(), base.to_string(), quote.to_string())
   }

Remedy:

Use the predecessor’s account so that a call coming through a multisig governance contract could be handled appropriately.

let caller = env::predecessor_account_id().to_string();

Status: 

Fixed

4. Potentially dangerous drop_state() method

File:  In the spot/src/lib.rs

Description: 

The drop_state() function should be omitted as it is already a very unsafe method. Moreover it is calling the clear() function from currency.rs contract. The clear() function is just removing all the markets at once.

pub fn drop_state(&mut self, keys: Vec<String>, keep_balances: bool) {
       let _session = self.make_session();
       let signer = env::signer_account_id();
       self.marketplace
           .drop_state(signer.as_str(), keys, keep_balances)
   }
 
   /// Set contract root state.
   /// Used to change the state of the contract when updating the contract.
   pub fn set_root_state(&mut self, state: String) {
       let signer = env::signer_account_id();
       self.marketplace.ensure_root(signer.as_str());
       self.marketplace.ensure_markets_are_stopped();
 
       Promise::new(current_account_id()).function_call(
           "set_root_state_callback".to_string(),
           json!({ "state": state }).to_string().into_bytes(),
           0,
           SINGLE_CALL_GAS,
       );
   }

Remedy:

The drop_state() method should be discarded

Status: 

Fixed

5. Inessential parameter optional visibility

File:  In the spot/src/lib.rs

Description: 

The parameter ttl in the place_ask() and place_bid() function should not be optional and there must be a mandatory limit for how long a specific order exists. Not giving the order a time-to-live will open it for many different attack scenarios, and the order would be at risk of MEV attacks.

pub fn place_bid(
       &mut self,
       market_id: MarketID,
       price: Price,
       quantity: Quantity,
       ttl: Option<u64>,
       market_order: bool,
       client_order_id: Option<u32>,
   ) -> Option<OrderID> {
       self.place_order(
           PlaceOrder {
               price,
               quantity,
               ttl,
               market_order,
               order_type: OrderSide::Bid,
               client_order_id,
           },
           market_id,
       )
   }

Remedy:

It is our recommendation from the security perspective that each order should always have a time-to-live.  

Status: 

Fixed

Medium-risk issues

6. Unoptimized loop with redundant ops

File: In the marketplace/src/market.rs

Description: 

Inside the handle_execution_effects() function of market.rs contract, a continue; statement should be placed at the end of the first loop, currently it is also executing the whole function even if the particular order is empty. This is a waste of resources and gas, to make the code more optimized and secure from redundancy a continue statement should be placed.

fn handle_execution_effects(
       &mut self,
       balances: &mut Balances<P>,
       effects: &ExecutionEffects,
   ) -> Price {
       let execution_price = effects.applied_price();
       let mut taker_base_amount = U128(0);
       let mut taker_quote_amount = U128(0);
       for match_effect in &effects.apply_to {
           if match_effect.order.is_empty() {
               Accounts::<P>::remove_order(
                   &match_effect.order.acc,
                   self.id,
                   match_effect.order.id,
               );
           }

Remedy:

So it is advised to use a continue statement here and end the loop for the particular order.

Status: 

Acknowledged

Low-risk issues

7. Impossible ownership transfer

Description: 

There exists no function or implementation for transfer ownership or renounce ownership. These two functions are very necessary for any protocol so that a trust in potential multisig governance is built. 

Remedy:

We recommend that these functions should be added in the codebase to make it more usable and resistant in the long term.

Status: 

Acknowledged

8. Illegal Max Fee Possibility

File:  In the marketplace/src/market.rs 

Description: 

Inside the set_fees() function, there should be a maximum cap on how much the fees could go up. Or what is the max limit on how much the fees could be set. Because currently it is just checking whether or not the taker_fee is greater than or equal to zero, and if the maker_fee is less than or equal to taker_fee. In the current implementation a fee of more than 100% could be set up, which is an indicator of rug-pull for the users. 

Moreover there should also be a lower cap on the maker_fee. This will increase the credibility of your code. 

pub fn set_fees(&mut self, fees: MarketFeesInput) {
       assert!(
           fees.taker_fee >= 0,
           "Taker fee should not be less than zero."
       );
       assert!(
           fees.taker_fee >= fees.maker_fee.abs(),
           "Taker fee should be greater than or equal to maker fee."
       );
       self.fees = MarketFees::new(fees);
   }

Remedy:

There should be a proper check for setting the max upper and lower limit for the fees and if the provided fees exceed the limit then it should discard it.

Status: 

Fixed

9. Ineffectual availability of market

File:  In the marketplace/src/market.rs 

Description: 

In the market.rs contract of order book there is a function is_running(). It will check whether or not the market is running or not, and even if a single user is available then it will return true. The current condition that it is checking is self.availability.allow_place || self.availability.allow_cancel

There is an inconsistency here that needs to be addressed, if the allow_cancel for any market is false and allow_place is true then it would return true as the return value for is_running() function. 

pub fn is_running(&self) -> bool {
       self.availability.allow_place || self.availability.allow_cancel
   }

Remedy:

It is therefore recommended that setting the availability of the marketplace should be restricted so that whenever allow_cancel = false it should also be that allow_place = false , hence a consistent pattern of availability is achieved. In fact, it would be highly inconsistent if the canceling orders is turned off but placing orders is turned on. The allow_place could be set as false independently of allow_cancel, but the vice versa should not be true.

Status: 

Acknowledged

10. Inexistent math overflows checks 

Description: 

In the overall code, there is a lack of underflow and overflow checks inside the cargo.toml.

Remedy:

In all the cargo.toml files, underflow and overflow checks should be defined. 

Status: 

Acknowledged

Informatory issues and Optimizations

11. Inconsistent asserts and panics

File:  In the spot/src/lib.rs

Description: 

On multiple occasions e.g. as the one below an inconsistent checking pattern is observed where the function asserts for an invariant and also uses panics in if blocks to panic the code execution. This is an anti-pattern check in case of panics and can be replaced with using asserts. 

assert_eq!(sender_id.as_str(), signer.as_str(), "Invalid signer");
      if amount.0 == 0 {
          Near::panic("Attached deposit balance must be greater than 0.");
      };

Remedy:

Specifically, in the above mentioned case the statement amount.0 == 0 is purely anti pattern and can be replaced with the assertion assert_ne! following appropriate checks.

Status: 

Acknowledged

12. Unoptimized error message pattern 

File:  In the spot/src/lib.rs

Description: 

No standardized error codes were found in the codebase. The error handling is not upto the mark at this stage of implementation following any best practice or coding style.

Err(_) => Near::panic("Failed to get currency data."),

Remedy:

Create an error.rs file and define all the error messages as standardized error codes with explanatory messages, part of optimized best practices.

// Signatures and access
#[msg("Access denied")]
AccessDenied,

Status: 

Acknowledged

13. Inconsistent arguments type

File:  In the spot/src/ft.rs 

Description: 

In the ft.rs file, the functions storage_deposit() and ft_metadata() implement a mismatching argument type deviating from the code style and best practices. The arguments received by both functions consist of empty vectors but the ways are different in both.

/// Storage deposit.
pub fn storage_deposit(currency: AccountId) {
   Promise::new(currency).function_call(
       "storage_deposit".to_string(),
"{}".as_bytes().to_vec(),
       Decimal::new(125.into(), 5).scale(NEAR_DECIMALS).value().0,
       SINGLE_CALL_GAS * 2,
   );
}

/// Returns promise for ft_metadata.
pub fn ft_metadata(address: AccountId) -> Promise {
   Promise::new(address).function_call("ft_metadata".to_string(), vec![], 0, SINGLE_CALL_GAS)vec![]
}

Remedy:

It is therefore recommended that the functions should implement a singular pattern of sending arguments to a low level function call so that consistency is maintained and readability is improved.

Status: 

Acknowledged

14. Inexplicable balances variable

File:  In the spot/src/lib.rs 

Description: 

In the lib.rs file in the spot directory, the drop_state() function asks for the keep_balances boolean variable to confirm whether to keep balances before dropping and clearing a market state. This check is assumed to be an obvious pattern as setting it negative will cost in losing the user’s internal balance collections.

pub fn drop_state(&mut self, keys: Vec<String>, keep_balances: bool) {
       let _session = self.make_session();
       let signer = env::signer_account_id();
       self.marketplace
           .drop_state(signer.as_str(), keys, keep_balances)
   }

Remedy:

It is suggested that the param be removed from the function. If the need is necessary then the following check should be modified to only remove non-zero balances.

for key in keys {
           let key = base64::decode(key).expect("Invalid key");
           if keep_balances && key.starts_with(&balances_key) {
               continue;
           }
           P::remove(&key);
       }

Status: 

Fixed

15. Inconsistent functions to handle errors

File:  In the marketplace/src/marketplace.rs

Description: 

In the get_orders() function, a `if Some` and `else` statement should be added so that if none of the orders are found for users in a particular market, then it should throw a panic() statement. This will make the code more consistent with the error handling. The panic statement was implemented in the get_order(0 function, so it should also be implemented in get_order().  

pub fn get_order(
       &self,
       market_id: MarketID,
       account_id: AccountId,
       order_id: OrderID,
   ) -> Order {
       let market = self.markets.get(&market_id).expect("Market not found.");
       if let Some(order) = market.get_order(account_id, order_id) {
           order
       } else {
           P::panic("Order not found.");
       }
   }

/// Returns list of user [Order].
   pub fn get_orders(&self, market_id: MarketID, account_id: AccountId) -> Vec<Order> {
       let market = self.markets.get(&market_id).expect("Market not found.");
       market.get_user_orders(&account_id)
   }

Remedy:

The Panic statement should also be implemented in the get_orders() function just like it was implemented in get_order().

Status: 

Acknowledged

16. Unhandled whitelisting removal

File:  In the marketplace/src/marketplace.rs

Description: 

Inside this contract, there are functions to set the whitelist and fetch them but there is no function to remove the whitelist. In the long run this could prove to be worrisome as it will keep on accumulating the whitelist but there will be no way to remove the old and unnecessary whitelist. It will also be effective on the storage as data that is useless will take up resources. 

Remedy:

A method that would be used to remove the whitelist in the marketplace.rs contract should be created. 

Status: 

Acknowledged

17. Quality of Test Cases

Description: 

The test cases that are written are only negative test cases and there is no positive test coverage for the provided code. A full positive test coverage should be provided for the functionalities. 

Remedy:

Positive test cases should be properly defined.

Status: 

Acknowledged

18. Incomplete condition evaluation

File:   In the marketplace/src/order_book/order.rs 

Description: 

In the from(b: u8) function it will return OrderSide: Bid if b will be 0 and for all the other numbers it will return OrderSide: Ask. 

impl From<u8> for OrderSide {
   fn from(b: u8) -> Self {
       if b == 0 {
           OrderSide::Bid
       } else {
           OrderSide::Ask
       }
   }

Remedy:

Our recommendation is an `else if` statement should be created that will check whether b==1 then it will return OrderSide: Ask and for the else it will throw an error.

Status:
Acknowledged

19. Misidentified default order behavior 

File:  In the marketplace/src/order_book/order.rs 

Description: 

Inside the default() function it is returning the OrderSide: Ask. The default behavior should not be OrderSide: Ask.

impl Default for OrderSide {
   fn default() -> Self {
       OrderSide::Ask
   }
}

Remedy:

There should not be any default OrderSide, instead all the OrderSide should be defined accordingly at runtime.

Status: 

Acknowledged

DISCLAIMER

The smart contracts provided by the client for audit purposes have been thoroughly analyzed in compliance with the global best practices till date w.r.t cybersecurity vulnerabilities and issues in smart contract code, the details of which are enclosed in this report. 

This report is not an endorsement or indictment of the project or team, and they do not in any way guarantee the security of the particular object in context. This report is not considered, and should not be interpreted as an influence, on the potential economics of the token, its sale or any other aspect of the project. 

Crypto assets/tokens are results of the emerging blockchain technology in the domain of decentralized finance and they carry with them high levels of technical risk and uncertainty. No report provides any warranty or representation to any third-Party in any respect, including regarding the bug-free nature of code, the business model or proprietors of any such business model, and the legal compliance of any such business. No third-party should rely on the reports in any way, including for the purpose of making any decisions to buy or sell any token, product, service or other asset. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project.

Smart contracts are deployed and executed on a blockchain. The platform, its programming language, and other software related to the smart contract can have its vulnerabilities that can lead to hacks. The scope of our review is limited to a review of the Solidity code and only the Solidity code we note as being within the scope of our review within this report. The Solidity language itself remains under development and is subject to unknown risks and flaws. The review does not extend to the compiler layer, or any other areas beyond Solidity that could present security risks.

This audit cannot be considered as a sufficient assessment regarding the utility and safety of the code, bug-free status or any other statements of the contract. While we have done our best in conducting the analysis and producing this report, it is important to note that you should not rely on this report only - we recommend proceeding with several independent audits and a public bug bounty program to ensure security of smart contracts.

INTRODUCTION

BlockApex (Auditor) was contracted by Chainpals (Client) for the purpose of conducting a Smart Contract Audit/Code Review. This document presents the findings of our analysis which took place on  15th June 2022. 

Scope

The git-repository shared was checked for common code violations along with vulnerability-specific probing to detect major issues/vulnerabilities. Some specific checks are as follows:

Project Overview

Chainpals transaction contract is responsible for handling the multi-phased transactions that take place between a buyer and a seller, each overlooked by escrow managers to make sure everything goes smoothly.

System Architecture

The trio of Chainpals contracts form a system that allows end-users to meet, setup transaction details (allowing payments in any BEP20 token) while making sure that the transaction proceeds only if both parties agree on the rules. The system also has their own BEP20 token called ChainpalsToken. The actors are incentivized to use these native tokens, which allows them to avail special discounts on fees. People are also encouraged to tell others about this protocol, for which they get bonuses in the form of NFTs and a share.

Methodology & Scope

The code came to us in the form of a zip, containing a truffle directory, the contract and the tests. Initially, we ran the contract and tested the functionality of all the functions manually. After that, we moved to Foundry to try all kinds of scenarios. After all the logical and functional testing, we moved to code optimizations and solidity design patterns to ensure consistency and readability.

AUDIT REPORT

Executive Summary

The analysis indicates that some of the functionalities in the contracts audited are working properly.

Our team performed a technique called “Filtered Audit”, where the contract was separately audited by two individuals. After their thorough and rigorous process of manual testing, an automated review was carried out using Surya. All the flags raised were manually reviewed and re-tested.

Our team found: 

# of issues	Severity of the risk
2	Critical Risk issue(s)
0	High-Risk issue(s)
0	Medium Risk issue(s)
2	Low-Risk issue(s)
5	Informatory issue(s)

Findings

Critical risk issues

1. Missing core functionality

Description:

The contract seems to have the functionality of deducting fees in BNB. The claim transaction amount function has a check that the feePaymentCurrency variable is BNB but the contract is missing the functionality of collecting fees in BNB. 

if (
           compareToIgnoreCase(
               transaction.transactionDetails.feePaymentCurrency,
               "BNB"
           )
       ) {
           sendBnb(feesHoldingWalletAddress, transaction.paymentAmountFees);
           transferFees(transaction);

Remedy:

Write a proper code to collect fees in BNB instead of CHP when the feePaymentCurrency is set to BNB.

Status:

Acknowledged 

Developer Response:

If a user has created a transaction using two different currency platforms Fees in BNB & Transaction payment in USDT then in the smart contract there is one function name: “onChainATransactionBNB” using we are collecting platform fees(BNB) and if the user has created a transaction in the same currency for payment and platform fees(BNB) then on the time of make payment (function name: payTransactionBNB ) will collect both platform fee & payment in a single transaction.

2. Misleading functionality

Description:

The contract contains a function called transferFees() which calls the function transferFunds() to send fees to five different wallets. The transferFunds() has a hardcoded fee token address set to ChainpalsToken. Regardless of what token the user sets, the fee is always deducted in terms of the ChainpalsTokens. This is misleading because it does not function the way it is mentioned in the document.

function transferFees(Transaction memory _transaction) private {
       if (_transaction.referralAddress != msg.sender) {
           transferFunds(
               _transaction.fees.referral,
               ChainpalsToken,
               _transaction.referralAddress
           );
       }
       transferFunds(
           _transaction.fees.staking,
           ChainpalsToken,
           paidStakingAddress
       );
       transferFunds(
           _transaction.fees.escrow,
           ChainpalsToken,
           escrowManagerAddress
       );

Remedy:

Write proper implementation to go forward with the method written in the documentation, or update the documentation to go along with the existing code.

Status:

Acknowledged 

Auditor’s Response:

Since the specs document was not clear and auditors made the wrong assumptions. It is necessary for the user to hold CHP tokens in order to create/claim transactions because protocol only supports CHP tokens for transactionFees. Also it is recommended to clear/mention this spec in public doc for users. 

Low-risk issues

 3. Missing zero address checks

Description:

The constructor accepts several address parameters, none of which are being checked for zero address. There is a validateNonZeroAddress() that checks for zero addresses, which can be used here. Here are some other functions missing zero address checks:

• createTransaction()
• createTransactionForUnregistered()
• addUnregisteredUserIntoTransaction()

Status:

Fixed as per BlockApex recommendation.

4. Unnecessary conversion

Description:

In the function onChainATransactionBnb(), there is a require statement which checks for (paymentAmountFees * 1 wei <= msg.value). This operation is unnecessary. It is like multiplying the entire amount with 1, which is inconsequential.

require(
                   msg.value >=
                       transaction.paymentAmount.add(
                           transaction.paymentAmountFees
                       ) *
                           1 wei,
                   "Invalid Amount"
               );

Remedy:

Remove the unnecessary conversion.

Status:

Fixed as per BlockApex recommendation.

Informatory issues and Optimization

5. Anti-pattern check

Description:

Conventionally, the global variable is on the left-hand side of the comparison operator, with the local variable or the function parameter on the right-hand side. Most checks in the code go against this. The code is not committed to one pattern, with the variables reversed in some cases. 

require(
           transaction.transactionDetails.buyer == msg.sender,
           "not valid buyer"
       );

require(
                   msg.value >=
                       transaction.paymentAmount.add(
                           transaction.paymentAmountFees
                       ) *
                           1 wei,
                   "Invalid Amount"
               );

Status:

Fixed as per BlockApex recommendation.

6. Inconsistency in error messages

Description:

The error messages in the require checks are inconsistent. Even for the same check, each function has a different error message. Also, the error messages should be meaningful. At the moment, some messages in the code do not tell the user what the error is supposed to mean.

require(
           msg.sender == transaction.transactionDetails.createdBy,
           "You cannot update"
       );

Status:

Fixed as per BlockApex recommendation.

7. Spelling mistakes

Description:

There are several cases of spelling mistakes in the code.

address public referelBonusAddress;

enum PAYMENT {
       INSTANT,
       MIESTONE
   }

require(isTransaction(_uid) == false, "Invalid transction id");

Status:

Fixed as per BlockApex recommendation.

8. Follow a solidity design pattern

Description:

As stated in the Solidity style guide, the functions should be grouped according to their visibility and ordered:

• constructor
• receive function (if exists)
• fallback function (if exists)
• external
• public
• internal
• Private

Status:

Fixed as per BlockApex recommendation.

9. Inconsistent code writing

Description:

The code has used both msg.sender and msgSender() from the Context library. It is suggested that you stick to one and use it throughout the code.

function resolveDispute(string memory _uid) external returns (bool) {
       require(isTransaction(_uid), "Invalid id");
 
       Transaction storage transaction = transactions[_uid];
       require(
           transaction.transactionDetails.buyer == _msgSender() ||
               owner() == _msgSender() ||
               escrowManagerAddress == _msgSender() ||
               adminAddress == _msgSender(),
           "User can not resolve the dispute."
       );

require(
           transaction.transactionDetails.seller == msg.sender,
           "not valid seller"
       );

Status:

Fixed as per BlockApex recommendation.

DISCLAIMER

The smart contracts provided by the client for audit purposes have been thoroughly analyzed in compliance with the global best practices till date w.r.t cybersecurity vulnerabilities and issues in smart contract code, the details of which are enclosed in this report. 

This report is not an endorsement or indictment of the project or team, and they do not in any way guarantee the security of the particular object in context. This report is not considered, and should not be interpreted as an influence, on the potential economics of the token, its sale or any other aspect of the project. 

Crypto assets/tokens are results of the emerging blockchain technology in the domain of decentralized finance and they carry with them high levels of technical risk and uncertainty. No report provides any warranty or representation to any third-Party in any respect, including regarding the bug-free nature of code, the business model or proprietors of any such business model, and the legal compliance of any such business. No third-party should rely on the reports in any way, including for the purpose of making any decisions to buy or sell any token, product, service or other asset. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project.

Smart contracts are deployed and executed on a blockchain. The platform, its programming language, and other software related to the smart contract can have its vulnerabilities that can lead to hacks. The scope of our review is limited to a review of the Solidity code and only the Solidity code we note as being within the scope of our review within this report. The Solidity language itself remains under development and is subject to unknown risks and flaws. The review does not extend to the compiler layer, or any other areas beyond Solidity that could present security risks.

This audit cannot be considered as a sufficient assessment regarding the utility and safety of the code, bug-free status or any other statements of the contract. While we have done our best in conducting the analysis and producing this report, it is important to note that you should not rely on this report only - we recommend proceeding with several independent audits and a public bug bounty program to ensure security of smart contracts.

INTRODUCTION

BlockApex (Auditor) was contracted by  Chainpals  (Client) for the purpose of conducting a Smart Contract Audit/Code Review. This document presents the findings of our analysis which took place on  30 May 2022. 

Scope

The git-repository shared was checked for common code violations along with vulnerability-specific probing to detect major issues/vulnerabilities. Some specific checks are as follows:

Project Overview

Chainpals Token is a BEP20 token contract. It works slightly differently from the traditional BEP20 contract.

System Architecture

The main contract is called Chainpals Token. The minting and transfer of the complete supply is done during deployment. This means new tokens can only be minted if the old ones are burnt. There is mention of sale and presale, meaning these features will be introduced in the future. The contract holds 35% of the total supply, while the rest is transferred to known actors.

Methodology & Scope

The code came to us in the form of a zip, containing a truffle directory, the contract, and the tests. Initially, we ran the contract and tested the functionality of all the functions manually. After that, we moved to Foundry to try all kinds of scenarios. After all the logical and functional testing, we moved to code optimizations and solidity design patterns to ensure consistency and readability.

AUDIT REPORT

Executive Summary

The analysis indicates that some of the functionalities in the contracts audited are working properly.

Our team performed a technique called “Filtered Audit”, where the contract was separately audited by two individuals. After their thorough and rigorous process of manual testing, an automated review was carried out using Surya. All the flags raised were manually reviewed and re-tested.

Our team found: 

Findings

Low-risk issues

1. Potential centralization risk.

Description:

The recoverWrongTokens() function allows the owner to pass an address, parse it into BEP20 and withdraw the tokens. This creates a centralization risk that the owner is able to withdraw 35% share of contract tokens. 

Status: Acknowledged.

2. No upper limit on fee percentage.

Description:

The setFees() function allows the owner to set the fee percentage and the fee reward address. There is no check to limit the fee percentage. Setting a very high fee percentage can basically drain funds in big amounts.

function setFees(uint256 _feeRewardPct, address _feeRewardAddress)
        public
        onlyOwner
    {
        require(
            _feeRewardAddress != address(0),
            "Fee reward address must not be zero address"
        );

        FeeRewardPct = _feeRewardPct;
        FeeRewardAddress = _feeRewardAddress;
    }

Remedy:

Make a require function that makes sure that the value sent as parameter is legitimate (ideally, under 10%).

Status:

Fixed as per BlockApex Recommendation.

Informatory issues and Optimization

3. Use struct to simplify readability.

Description:

The constructor accepts 16 parameters before deployment. 

Remedy:

Make a struct for all the parameters and send the struct into the constructor as a single parameter.

struct Deploy {
        uint256 _initialSupply;
        uint256 _feeRewardPct;
        address _feeRewardAddress;
        address _PresaleWallet;
        address _PrivateSaleWallet;
        address _SaleWallet;
        address _CommunityFutureWallet;
        address _BurnWallet;
        address _MarketingWallet;
        address _ProductDevelopmentWallet;
        address _FounderWallet;
        string _marketing;
        string _founder;
        string _productDevelopment;
        string _presale;
        string _sale;
}
    constructor(Deploy memory params) public {

        releaseAgent = msg.sender;
        _maxSupply = params._initialSupply;
        uint256 initialSupply = _maxSupply;
        PresaleWallet = params._PresaleWallet;
        PrivateSaleWallet = params._PrivateSaleWallet;
        SaleWallet = params._SaleWallet;
        CommunityFutureWallet = params._CommunityFutureWallet;
        BurnWallet = params._BurnWallet;
        MarketingWallet = params._MarketingWallet;
        ProductDevelopmentWallet = params._ProductDevelopmentWallet;
        FounderWallet = params._FounderWallet;
        tradingFeesApplicable = true;

Status:

Acknowledged.

4. Expensive uint comparison in require statements.

Description:

In many cases, we see this check:

(x > 0)

The x is uint in every case, meaning it will never go below 0. The lowest value it can have is 0 itself. We can change this condition to test the same thing using less gas.

Remedy:

This check can be replaced by:

(x != 0)

This condition will check whether x is 0 or not. If it is 0, the check returns false, otherwise, it proves that x is greater than 0 because uint values cannot be negative.

Status:

Fixed as per BlockApex Recommendation.

5. Use memory keyword instead of calldata on parameters inside external function.

Description:

Reference-type parameters can be assigned calldata keyword instead of memory. Calldata is another space for storing externally received parameters. It is less expensive than using the memory space.

Remedy:

Replace the memory keywords with calldata.

Developer Response:

There are very few instances where memory keyword can be changed to calldata. Also, replacing them will not make a big difference in the gas cost and hence, we suggest to keep it as it is.

Status:

Acknowledged.

6. V1 contract will hold both V1 and V2 tokens at the time of V2 token Migration.

Description:

The 35% share locked inside the V1 contract cannot be migrated. When V2 is launched, the 35% will not be able to migrate unless a mechanism is added in the future. 

Only the owner wallet is able to withdraw this 35% share of the contract. 

Developer Response:

V1 tokens will get redeemed by defined wallets within their planned unlock phases after tokens are created and if in future there is a requirement then will create V2 tokens which will give users a facility that they can migrate tokens from V1 to V2. To summarize, none tokens will be locked for only the owner wallet to withdraw while migration is on.

Status:

Acknowledged.

7. Solidity style guide:

These were some cases of inconsistency with the style guide:

The external functions should be between the public functions and the constructor.

The public view functions should be below the state-changing public functions.

In some functions, the starting parenthesis is not on the same line.

Status:

Acknowledged.

8. Loop running till the end:

Description:

Inside the updateWithdrawalRecord() function, the claimTokens() functions sends the name of the sale and the amount to claim. The function checks each slot and gives back the amount by iterating through all the slots. The for loop does not stop even when the requested amount is removed. The extra running will increase the gas cost over time as more slots are added.

function updateWithdrawalRecord(string memory _saleName, uint256 _amount)
       internal
       returns (bool)
   {
       SaleInfo storage s = sales[_saleName];
       uint256 amount = _amount;
       for (uint256 i = 0; i < s.TotalSlots; i++) {
           uint256 slotTokens = s.saleSlots[i].TotalTokens.sub(
               s.saleSlots[i].ClaimedTokens
           );
           if (
               slotTokens > 0 &&
               amount > 0 &&
               s.saleSlots[i].WithdrawalDate < block.timestamp
           ) {
               if (slotTokens > amount) {
                   s.saleSlots[i].ClaimedTokens = s
                       .saleSlots[i]
                       .ClaimedTokens
                       .add(amount);
                   amount = 0;
               } else if (slotTokens <= amount) {
                   s.saleSlots[i].ClaimedTokens = s
                       .saleSlots[i]
                       .ClaimedTokens
                       .add(slotTokens);
                   amount -= slotTokens;
               }
           }
       }
       s.TotalClaimed += _amount;
       return true;
   }

Remedy:

Add a break keyword inside the (slotTokens > amount) check so that once the requested amount is found inside the current slot, there is no need for the loop to keep running.

function updateWithdrawalRecord(string memory _saleName, uint256 _amount)
       internal
       returns (bool)
   {
       SaleInfo storage s = sales[_saleName];
       uint256 amount = _amount;
       for (uint256 i = 0; i < s.TotalSlots; i++) {
           uint256 slotTokens = s.saleSlots[i].TotalTokens.sub(
               s.saleSlots[i].ClaimedTokens
           );
           if (
               slotTokens > 0 &&
               amount > 0 &&
               s.saleSlots[i].WithdrawalDate < block.timestamp
           ) {
               if (slotTokens > amount) {
                   s.saleSlots[i].ClaimedTokens = s
                       .saleSlots[i]
                       .ClaimedTokens
                       .add(amount);
                   amount = 0;
			//Break here
               } else if (slotTokens <= amount) {
                   s.saleSlots[i].ClaimedTokens = s
                       .saleSlots[i]
                       .ClaimedTokens
                       .add(slotTokens);
                   amount -= slotTokens;
               }
           }
       }
       s.TotalClaimed += _amount;
       return true;
   }

Alternatively, you can change the claimTokens() to accept the slot number, so that the loop is removed from the scenario entirely and only the assigned slot is targeted, checked upon and used to give the requested tokens.

Status:

Fixed as per BlockApex Recommendation.

9. Zero address checks in constructor:

Description:

Constructor is taking a lot of addresses from parameter. There should be a zero/null address check to verify each address.

Remedy:

Place require statements to check each address.

Status:

Fixed as per BlockApex Recommendation.

Suggestions

10. setFeeExcludedAddress needs to contract only address

Description:

The excluded-address should be a contract address, not a wallet address. Since this is a community-centered project, there should be no extra privileges to selected community members.

Status: Fixed as per BlockApex Recommendation.

11. There is no view function to view ongoing stage

Description:

At the moment there is no way to check what stage the platform is in. It would be better to implement a view function that returns the current stage.

Developer Response:

To check status/phase of the token there are two public values released and paused. Using that we can determine that tokens are currently released/unreleased or paused/unpaused.

Status:

Acknowledged

12. Known parameters should be hardcoded

Description:

The string values that are sent to the constructor can be hardcoded if they are known from the start.

Status:

Acknowledged.

13. Potential destruction of blacklisted address’s funds

Description:

Blacklisted addresses will have their funds locked for as long as they are blacklisted. For the time that an address is blacklisted, their funds are basically destroyed. That is, no one can access those funds in that period, till the address is removed from the blacklist. 

Developer Response:

Under our token requirements we will need a functionality using which the owner wallet can set a user/wallet address as blacklisted or remove him from the blacklist. Our functionality doesn’t include destroying the tokens under a blacklisted wallet.

Status:

Acknowledged.

14. Make sure that the error messages clearly explain the reason for the error

Description:

In some cases, the error messages in the revert statements do not clearly deliver the message. They should be changed to clearly explain why a statement was reverted. Certain spelling mistakes were also found, which should be fixed.

Status:

Fixed as per BlockApex Recommendation.

15. transfer() should not deduct 1% fee from whitelisted addresses

Description:

The tokens are already pre-minted. While calling the claimTokens() function, it deducts 1% from the caller’s requested amount. The whitelisted addresses should be excluded from the fee deduction.

Status:

Fixed as per BlockApex Recommendation.

DISCLAIMER

The smart contracts provided by the client for audit purposes have been thoroughly analyzed in compliance with the global best practices till date w.r.t cybersecurity vulnerabilities and issues in smart contract code, the details of which are enclosed in this report. 

This report is not an endorsement or indictment of the project or team, and they do not in any way guarantee the security of the particular object in context. This report is not considered, and should not be interpreted as an influence, on the potential economics of the token, its sale or any other aspect of the project. 

Crypto assets/tokens are results of the emerging blockchain technology in the domain of decentralized finance and they carry with them high levels of technical risk and uncertainty. No report provides any warranty or representation to any third-Party in any respect, including regarding the bug-free nature of code, the business model or proprietors of any such business model, and the legal compliance of any such business. No third-party should rely on the reports in any way, including for the purpose of making any decisions to buy or sell any token, product, service or other asset. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project.

Smart contracts are deployed and executed on a blockchain. The platform, its programming language, and other software related to the smart contract can have its vulnerabilities that can lead to hacks. The scope of our review is limited to a review of the Solidity code and only the Solidity code we note as being within the scope of our review within this report. The Solidity language itself remains under development and is subject to unknown risks and flaws. The review does not extend to the compiler layer, or any other areas beyond Solidity that could present security risks.

This audit cannot be considered as a sufficient assessment regarding the utility and safety of the code, bug-free status or any other statements of the contract. While we have done our best in conducting the analysis and producing this report, it is important to note that you should not rely on this report only - we recommend proceeding with several independent audits and a public bug bounty program to ensure security of smart contracts.

INTRODUCTION

BlockApex (Auditor) was contracted by Chainpals (Client) for the purpose of conducting a Smart Contract Audit/Code Review. This document presents the findings of our analysis which took place on 3 June 2022. 

Scope

The git-repository shared was checked for common code violations along with vulnerability-specific probing to detect major issues/vulnerabilities. Some specific checks are as follows:

Project Overview

Chainpals Token is a BEP20 token contract. Our contract at hand is the Chainpals Presale contract that uses the BEP20 token.

System Architecture

The main contract is called ChainpalsPresale.sol. This contract contains the functionality for the presale of the BEP20-based Chainpals Token. The presale is supposed to go forward in three stages, each with fixed purchasable amounts and at a fixed cost. The cost starts off at 0.25 USD in the first phase, moves to 0.35 USD in the second phase and then to 0.45 in the last phase. Also, the contract holds the 35% of 20M initial supply of the Chainpals Tokens, which is fixed.

Methodology & Scope

The code came to us in the form of a zip, containing a truffle directory, the contract and the tests. Initially, we ran the contract and tested the functionality of all the functions manually. After that, we moved to Foundry to try all kinds of scenarios. After all the logical and functional testing, we moved to code optimizations and solidity design patterns to ensure consistency and readability.

AUDIT REPORT

Executive Summary

The analysis indicates that some of the functionalities in the contracts audited are working properly.

Our team performed a technique called “Filtered Audit”, where the contract was separately audited by two individuals. After their thorough and rigorous process of manual testing, an automated review was carried out using Surya. All the flags raised were manually reviewed and re-tested.

Our team found: 

# of issues	Severity of the risk
1	Critical Risk issue(s)
0	High-Risk issue(s)
3	Medium Risk issue(s)
5	Low-Risk issue(s)
9	Informatory issue(s)

Findings

Critical risk issues

1. Potential economic crash

Description:

The buyUsingBnbCoin() and buyUsingAltCoin() both only check for the total remaining amount of tokens in the contract. This includes the yet-to-be unlocked amount purchased by other users.

• Impact:

1. If all the tokens of the contract have been sold in presale, only x% amount will be withdrawn (where x is the instant unlock percentage). The contract checks the entire balance present, not taking into consideration whether any of that is the locked amount belonging to someone. The contract, therefore, allows others to potentially buy other people’s locked tokens. Ultimately, when the locked tokens are unlocked and a user comes to claim them, the contract would not have them.
2. The AvailableTokensForWithdrawal() and availableReferralTokensForWithdrawal() functions will return the valid amount to withdraw, but their associated withdrawal function also checks the balance of the CHP Token, which will not pass.

function buyUsingBnbCoin(address _referredBy)
        public
        payable
        nonReentrant
        returns (bool)
    {
        require(
            msg.value <= maxSingleInvestmentPurchaseBNB * 1 wei,  
            "_amount should be less than maximum single purchase BNB amount"
        );
        uint256 currentPhaseId = getCurrentPhaseId();
        PhaseInfo storage phase = phases[currentPhaseId];
        uint256 USDTValue = (getLatestBNBPrice().mul(msg.value)).div(1e18); // calculate transferred BNB value in USDT
        uint256 numberOfTokens = ((USDTValue).mul(1e18)).div(phase.CHPBNBValue); // multiplication with 1 CHP token in decimal form
        require(_referredBy != address(0), "_referredBy from the zero address");
        require(_referredBy != msg.sender, "_referredBy can not be callee");
        require(
            numberOfTokens >= minimumPresalePurchase,
            "Wrong investment amount!"
        );
        uint256 transaferableAmount = numberOfTokens 
            .mul(instantTokenUnlockedPercentage)
            .div(100);
        uint256 referralTokens = numberOfTokens.mul(3).div(100);
        fundRaisingWallet.transfer(msg.value);
        require(
            CHPToken.balanceOf(address(this)) > transaferableAmount,
            "Not Enough Pre-Sale Token"
        );

Status:

Acknowledged.

Medium risk issues

2. Unsupervised contract balance return

Description:

The getAvailablePresaleTokens() function allows a user to get the total available presale tokens. This function returns the total balance of the contract, which includes the yet-to-be unlocked tokens belonging to the other users. This unchecked value can cause problems in the long run when the tokens unlock and the token owners attempt to withdraw them.

function getAvailablePresaleTokens() public view returns (uint256) {
       return CHPToken.balanceOf(address(this));
   }

Remedy:

Each user already has a Struct to keep track of their purchases. This function should return the difference between all held tokens and the total tokens, which will be the true value of available presale tokens.

Status:

Fixed as per BlockApex Recommendation.

3. Potential out-of-gas scenario

Description:

Inside the WithdrawTokens() function, we have calls going to availableTokensForWithdrawal() and updateWithdrawalRecord(). Both of these functions contain nested loops, which may result in a potential out-of-gas problem. 

function withdrawTokens(uint256 _amount)
       public
       nonReentrant
       returns (bool)
   {
       uint256 availableTokensToClaim = AvailableTokensForWithdrawl(
           msg.sender
       );
       require(_amount <= availableTokensToClaim, "Wrong withdrawal amount");
       require(
           CHPToken.balanceOf(address(this)) > _amount,
           "Not Enough Pre-Sale Token"
       );
 
       if (updateWithdrawalRecord(address(msg.sender), _amount)) {
           CHPToken.safeTransfer(address(_msgSender()), _amount);
       }
       emit TokenWithdrawal(_msgSender(), _amount);
       return true;
   }

Remedy:

Create a mapping which will track time from user purchase to 1st redeem,  1st redeem to last redeem and run the loop according to the time return from mapping. 

Developer Response:

Functions referred in these two 2 points are functional operations and we will require to have loops in there to check if the tokens are unlocked for the user to claim/redeem.

Status:

Acknowledged.

4. Check does not fulfill the purpose of limit purchase

Description:

Inside the buy tokens functions, namely buyUsingBnbCoin() and buyUsingAltCoin(), the contract has a check to ensure that a user does not spend more than 10,000 stable coins or 35 BNB. This is a one time limit to purchase in presale, but it does not limit the user from buying in a loop or multiple times at once.

Remedy:

Limit the user from buying in a loop, or buying multiple times.  

Developer Response:

We’re limiting the user to purchase only 10,000 USD or 35BNB worth of tokens from presale is handled through the frontend interface of the system.

Status:

Acknowledged.

Low risk issues

5. Zero address check missing in recoverWrongTokens()

Description:

The function does not check for zero address.

 function recoverWrongTokens(address _tokenAddress) external onlyOwner {
        uint256 _tokenAmount = IBEP201(_tokenAddress).balanceOf(address(this)); 
        IBEP201(_tokenAddress).safeTransfer(address(msg.sender), _tokenAmount);

        emit AdminTokenRecovery(_tokenAddress, _tokenAmount);
    }

Remedy:

Place a zero address check inside the function to save a caller from redundant calls.

Status:

Fixed as per BlockApex Recommendation.

6. Missing zero address checks in the constructor

Description:

The constructor does not check if any of the input addresses are equal to the zero addresses.

Remedy:

Place a zero address check on all the addresses passing to the constructor.

Status::

Fixed as per BlockApex Recommendation.

7. Missing zero value checks in constructor

Description:

The constructor does not check for the value of the integer input in the constructor.

Remedy:

Place a zero value check on all the values passing to the constructor.

Status:

Fixed as per BlockApex Recommendation.

8. updateWithdrawalRecord(): Condition satisfaction does not terminate loop

Description:

This function runs a loop to check whether the user has enough funds to withdraw. Upon successfully withdrawing all the requested amount, the loop should end. Inside the contract, the loop continues to run till the end. This is a waste of gas.

Remedy:

Place a break statement inside the first if statement to terminate the loop.

Status:

Fixed as per BlockApex Recommendation.

9. updateReferralWithdrawRecord(): Condition satisfaction does not terminate loop

Description:

This function runs a loop to check whether the user has enough funds to withdraw. Upon successfully withdrawing all the requested amount, the loop should end. Inside the contract, the loop continues to run till the end. This is a waste of gas.

 function updateWithdrawalRecord(address _user, uint256 _amount)
        internal
        returns (bool)
    {
        uint256 claimedAmount = _amount;
        UserDetail storage user = users[_user];
        for (uint256 i = 0; i < user.TotalPurchases; i++) {
            for (
                uint256 j = 0;
                j < user.purchases[i].subTransactionCount;
                j++
            ) {
                uint256 remainingTokens = user
                    .purchases[i]
                    .subTransactions[j]
                    .NumberOfTokens
                    .sub(user.purchases[i].subTransactions[j].ClaimedTokens);

                if (
                    remainingTokens > 0 &&
                    _amount > 0 &&
                    user.purchases[i].subTransactions[j].TokenUnlockedDate <
                    block.timestamp
                ) {
                    if (remainingTokens > _amount) {
                        user
                            .purchases[i]
                            .subTransactions[j]
                            .ClaimedTokens = user
                            .purchases[i]
                            .subTransactions[j]
                            .ClaimedTokens
                            .add(_amount);
                            _amount = 0;
                    } else if (remainingTokens <= _amount) {
                        user
                            .purchases[i]
                            .subTransactions[j]
                            .ClaimedTokens = user
                            .purchases[i]
                            .subTransactions[j]
                            .ClaimedTokens
                            .add(remainingTokens);
                        _amount -= remainingTokens;
                    }
                }
            }
        }
        user.TotalClaimed = user.TotalClaimed.add(claimedAmount);
        return true;
    }

Remedy:

Place a break statement inside the first if-statement to terminate the loop.

Status:

Fixed as per BlockApex Recommendation.

Informatory issues and Optimization

10. withdrawReferralTokens() is gas-costly

Description:

This function makes a call to availableReferralTokensForWithdrawal() and updateReferralTokensRecord(). Both of these functions contain loops, which makes this function very expensive in terms of gas. It is possible the gas cost for looping through the mapping could become so significant that the gas limit for the block is reached.

Developer Response:

Functions referred in these two 2 points are functional operations and we will require to have loops in there to check if the tokens are unlocked for the user to claim/redeem.

11. Hardcode the USDT and BUSD token addresses

Description:

The addresses of the USDT Token and the BUSD Token remain the same throughout the life of the contract. Instead of accepting the address inside the constructor, it should be hardcoded.

Status:

Fixed as per BlockApex Recommendation.

12. Use struct to simplify readability.

Description:

The constructor accepts 14 parameters before deployment. 

13. buyUsingBnbCoin() and buyUsingAltCoin() should be set to external.

Description:

Both of these functions are supposed to be called from outside the contract. Therefore, it makes no sense to set it to public. Setting it to external allows us to save some gas as well.

Status:

Fixed as per BlockApex Recommendation.

14. Spelling mistakes in two functions

Description:

Spelling mistake found buyUsingBnbCoin() and buyUsingAltCoin().

Status:

Fixed as per BlockApex Recommendation.

15. Unnecessary conversion

Description:

It is unnecessary to multiply the value by 1 Wei. It is basically multiplying the value with 1, which will not change anything.

 require(
            msg.value <= maxSingleInvestmentPurchaseBNB * 1 wei,  
            "_amount should be less than maximum single purchase BNB amount"
        );

16. Put all external functions on the top, below the constructor.

Description:

As stated in the Solidity style guide, the functions should be grouped according to their visibility and ordered:

• constructor
• receive function (if exists)
• fallback function (if exists)
• external
• public
• internal
• Private

Status:

Fixed as per BlockApex Recommendation.

17. TreasuryWallet address check

Description:

updateTreasuryWallet() should have a check to ensure that the updated address is not equal to the owner address.

Status:

Fixed as per BlockApex Recommendation.

18. Make all necessary variables constant

Description:

There are several variables whose value remains the same throughout the life of the contract. All such variables should be marked as constant.

Status:

Acknowledged.

DISCLAIMER

The smart contracts provided by the client for audit purposes have been thoroughly analyzed in compliance with the global best practices till date w.r.t cybersecurity vulnerabilities and issues in smart contract code, the details of which are enclosed in this report. 

This report is not an endorsement or indictment of the project or team, and they do not in any way guarantee the security of the particular object in context. This report is not considered, and should not be interpreted as an influence, on the potential economics of the token, its sale or any other aspect of the project. 

Crypto assets/tokens are results of the emerging blockchain technology in the domain of decentralized finance and they carry with them high levels of technical risk and uncertainty. No report provides any warranty or representation to any third-Party in any respect, including regarding the bug-free nature of code, the business model or proprietors of any such business model, and the legal compliance of any such business. No third-party should rely on the reports in any way, including for the purpose of making any decisions to buy or sell any token, product, service or other asset. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project.

Smart contracts are deployed and executed on a blockchain. The platform, its programming language, and other software related to the smart contract can have its vulnerabilities that can lead to hacks. The scope of our review is limited to a review of the Solidity code and only the Solidity code we note as being within the scope of our review within this report. The Solidity language itself remains under development and is subject to unknown risks and flaws. The review does not extend to the compiler layer, or any other areas beyond Solidity that could present security risks.

This audit cannot be considered as a sufficient assessment regarding the utility and safety of the code, bug-free status or any other statements of the contract. While we have done our best in conducting the analysis and producing this report, it is important to note that you should not rely on this report only - we recommend proceeding with several independent audits and a public bug bounty program to ensure the security of smart contracts.

INTRODUCTION

BlockApex (Auditor) was contracted by Voir Studio (Client) for the purpose of conducting a Smart Contract Audit/Code Review. This document presents the findings of our analysis which started on  22nd June 2022. 

Scope

The git repository shared was checked for common code violations along with vulnerability-specific probing to detect major issues/vulnerabilities. Some specific checks are as follows:

Project Overview

Unipilot Staking is a Staking infrastructure built on Ethereum, a reliable and scalable L1 solution. The staking solution offered by Unipilot provides the stakers with a way to get incentives.

Founded in May 2021, Unipilot was the first of its kind liquidity optimizer product to offer users to further optimize their liquidity on Uniswap. The advantages of using a liquidity optimizer include better returns on the provided liquidity on Uniswap.

System Architecture

The Unipilot protocol earns 20% of all revenues generated on the platform. Under the old model, all of this revenue was sent to the Index Fund, which already has a value of $245,000 at the time of writing. With the introduction of staking, you will be able to stake your $PILOT tokens on the Unipilot dApp to earn a share of protocol revenues. Initially, 40% of Treasury revenues will be distributed to stakers, though this percentage could change in the future.

Rewards will be distributed with every block and paid in $ETH, as chosen by the community. Stakers will be able to collect their $ETH rewards at any time, and remain in full control of their $PILOT, with the ability to unstake at any time without penalty.

As staking rewards originate from revenue earned by the protocol and not from inflationary $PILOT token rewards, returns are therefore not fixed. As TVL rises, the protocol will earn more revenue, and staking rewards will increase.

Methodology & Scope

The codebase was audited using a filtered audit technique. A band of four (2) auditors scanned the codebase in an iterative process spanning over a time of two (1) weeks. 

Starting with the recon phase, a basic understanding was developed and the auditors worked on developing presumptions for the developed codebase and the relevant documentation/whitepaper. Furthermore, the audit moved on with the manual code reviews with the motive to find logical flaws in the codebase complemented with code optimizations, software, and security design patterns, code styles, best practices and identifying false positives that were detected by automated analysis tools.

AUDIT REPORT

Executive Summary

The analysis indicates that some of the functionalities in the contracts audited are working properly.

Our team performed a technique called “Filtered Audit”, where the contract was separately audited by four individuals. After a thorough and rigorous process of manual testing, an automated review was carried out using slither for static analysis and Hardhat and Foundry for edge case testing. All the flags raised were manually reviewed and re-tested to identify the false positives.

Our team found:

# of issues	Severity of the risk
0	Critical Risk issue(s)
3	High-Risk issue(s)
2	Medium Risk issue(s)
1	Low-Risk issue(s)
1	Informatory issue(s)

Key Findings

Detailed Overview

Critical-risk issues

No issues were found.

High-risk issues

1. Pending Rewards For Previous Reward Tokens are Getting Lost 

Description: 

If the reward token is changed by the governor then the pending reward for the previous reward tokens are lost, users will not receive their already accumulated rewards and the accumulated rewards will reset to 0. Contract does not hold any logic for giving out the pending rewards for previous token.

Developers Response:

Developers said that those pending rewards for the user will not be completely  lost and they would be paid out to users eventually through a merkle tree.

Remedy:

There must be a map that will check the previous pending reward that is not paid to the stakers.

Status:

Fixed as per BlockAPex recommendation. 

2. Incorrect Behavior For Pending Reward Tokens

Description: 

When the reward token is changed and the previous reward token already had a reward for the user in it, then the calculated pending reward for the new reward token will add previous token reward as well as new token reward and return it to the user. 

Remedy:

The contract must check if the whole reward this is provided is given out to the stakers.

Status:

Fixed as per BlockAPex recommendation.

3. Incorrect Behavior For Staking When No One has Staked Their Tokens

Description: 

If the staking in the contract has already started and no one has staked their tokens yet, then the reward for empty past blocks will not be assigned to any staker.

Developers Response:

Initial permanent PILOT stake will be done by the foundation to set the initial state of the contract, rewards earned by the foundation will be distributed back to staking participants.

Remedy:

This default behavior of the contract should be changed and is not as per the intended behavior.

Status:

Fixed as per BlockAPex recommendation.

Medium-risk issues

4. Arithmetic Overflow Error On Updating the Reward Token

Description: 

If the governance recalls the updateRewardToken() function with the address of the same reward token, then the contract is throwing an arithmetic overflow error.

Remedy:

The arithmetic overflow error should be checked properly.

Status:

Fixed as per BlockAPex recommendation.

5. Inconsistent Behavior if the Reward Token is Other Than 18 Decimals

Description: 

If the reward token decimal is 6, 8 and 12 then the contract won’t let the users to stake the PILOTs. There is a functionality in contract, when the user will come to stake their token, it will check whether the contract has the reward tokens in it or not, if the reward tokens will not be present then it will not let the user to stake their token. 

Now in case of different decimal places other than 18, the contract will get its own balance of reward token 

Remedy:

The property should be added for the 6,8 and 12 decimals , just like for the 18 decimals.

Status:

Fixed as per BlockAPex recommendation.

Low-risk issues

6. Less Optimized Checks

Description: 

In the updateRewards() function, it is checking the condition if (_rewardDurationInBlocks == 0) instead in should check  if (_rewardDurationInBlocks == 0 || _reward == 0) because even if the rewardDurationInBlocks are provided but rewards are zero then there is no point in moving forward. 7 lines below this condition it is checking if (_reward == 0). So it would be more optimized if both of these conditions are checked in the same line.

Remedy:

These checks should be made into more optimized ones to save gas.

Developers Response:

Not valid as passing zero indicates the contract to distribute the existing debt rewards

Status:

Acknowledged.

Informatory issues and Optimizations

7. Redundant Event Emissions

Description: 

There is no reason to emit the events at the end of the constructor, when the constructor is completed, two events GovernanceChanged() and RewardTokenChanged() are emitted. The code can be more optimized by removing these event emissions.

Remedy:

Redundant events fired in the constructor should be removed. 

Status:

Acknowledged.

DISCLAIMER

The smart contracts provided by the client for audit purposes have been thoroughly analyzed in compliance with the global best practices till date w.r.t cybersecurity vulnerabilities and issues in smart contract code, the details of which are enclosed in this report. 

This report is not an endorsement or indictment of the project or team, and they do not in any way guarantee the security of the particular object in context. This report is not considered, and should not be interpreted as an influence, on the potential economics of the token, its sale or any other aspect of the project. 

Crypto assets/tokens are results of the emerging blockchain technology in the domain of decentralized finance and they carry with them high levels of technical risk and uncertainty. No report provides any warranty or representation to any third-Party in any respect, including regarding the bug-free nature of code, the business model or proprietors of any such business model, and the legal compliance of any such business. No third-party should rely on the reports in any way, including for the purpose of making any decisions to buy or sell any token, product, service or other asset. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project.

Smart contracts are deployed and executed on a blockchain. The platform, its programming language, and other software related to the smart contract can have its vulnerabilities that can lead to hacks. The scope of our review is limited to a review of the Solidity code and only the Solidity code we note as being within the scope of our review within this report. The Solidity language itself remains under development and is subject to unknown risks and flaws. The review does not extend to the compiler layer, or any other areas beyond Solidity that could present security risks.

This audit cannot be considered as a sufficient assessment regarding the utility and safety of the code, bug-free status or any other statements of the contract. While we have done our best in conducting the analysis and producing this report, it is important to note that you should not rely on this report only - we recommend proceeding with several independent audits and a public bug bounty program to ensure security of smart contracts.

INTRODUCTION

BlockApex (Auditor) was contracted by Flower Fam (Client) for the purpose of conducting a Smart Contract Audit/ Code Review. This document presents the findings of our analysis which started on 19 May 2022. 

Scope

The Git-repository shared was checked for common code violations along with vulnerability-specific probing to detect major issues/vulnerabilities. Some specific checks are as follows:

Project Overview

FlowerFam is an NFT-based project, after you mint your NFT you can “harvest” them on weekly basis to get 60% royalties. It's quite simple: every flower has a 10% chance to win. The rarer the species of a flower. Besides the weekly harvest, flowers can make $honeycoin through a lot of other fun activities in the Oasis. You can earn $honeycoin by staking your Flower, catching bees, and buying seeds that grow into beautiful new Flowers.

System Architecture

FlowerFam is a single NFT minter contract which is composed of three other contracts, FloweFam.sol , FlowerFamMintPass.sol and FlowerFamEcosystem.sol. This contract is used to make users whitelist so that only whitelisted addresses would be able to mint NFTs.

Methodology & Scope

The codebase was audited in an iterative process. Fixes were applied on the way and updated contracts were examined for more bugs. We used a combination of static analysis tool (slither) and testing framework (Foundry) which indicated some of the critical bugs. We also did manual reviews of the code to find logical bugs, code optimizations, solidity design patterns, code style and the bugs/ issues detected by automated tools.

AUDIT REPORT

Executive Summary

The analysis indicates that some of the functionalities in the contracts audited are working properly.

Our team performed a technique called “Filtered Audit”, where the contract was separately audited by two individuals. After their thorough and rigorous process of manual testing, an automated review was carried out using Mythril, MythX, Surya and Slither. All the flags raised were manually reviewed and re-tested. 

Our team found:

# of issues	Severity of the risk
0	Critical Risk issue(s)
0	High-Risk issue(s)
1	Medium Risk issue(s)
4	Low-Risk issue(s)
4	Informatory issue(s)

Findings

Medium-risk issues

1. Centralization risk.

Description: 

Heavy centralization risk using onlyowner check on withdraw() function, assuming owner address is never compromised else it might lead to lost funds. 

function withdraw(address _to) external onlyOwner {
        uint256 balance = address(this).balance;
        require(balance > 0, "Balance zero");
        payable(_to).transfer(balance);
    }

Status:

Resolved

Low-risk issues

2. setMerkleRootOfRound() can overwrite mapping

Description: 

The current implementation of the above function can override the mapping roundToMerkleRoot which can lead to loss of round minting for any of the four rounds.

Status: 

Acknowledged

Remedy: 

Place a check to ensure the root can only be set if the round has completed the  preset timeline.

3. Configuration inconsistency:

Description: 

Calling the functions setMintLimitOfRound or setMaxSupplyOfRound during a round can lead to inconsistencies of assumed configurations of the minting system.

Status: 

Acknowledged

Remedy: 

Owner can change configurations during the minting rounds and can lead to inconsistent management of user NFTs.

4. Withdraw function argument validation check

Description: 

Withdraw() function is only callable by owner but still there is a chance of mistake. Function only checks for the amount it should also check for the value owner sends from the arguments.

Status: 

Fixed

Remedy: 

There should be a zero address check inside the function.

5. Recommendation for missing function in the contract file as received by the client.

Description: 

The contract IFlowerFamMintPass contains a function named validPasssesLeft which is incompatible with the original files received later (out of scope) containing a similar function named as userPassesLeft().

Status: 

Acknowledged

Remedy: 

Ensure the functions are named properly and following the implemented interface architecture of the file system.

Informatory issues

6. No NatSpec Documentation

Description: 

NatSpec documentation is an essential part of smart contract readability; it is therefore advised that the contract and following files should contain proper explanatory commenting;

• FlowerFamMinter.sol

Status:

Acknowledged

7. Order of Functions

Description: 

Move receive() function right below the constructor. Move most useable/callable (Public/External) functions right below the constructor and internal functions right below the public functions as suggested in the solidity docs:

“Ordering helps readers identify which functions they can call and to find the constructor and fallback definitions easier”.

Functions should be grouped according to their visibility and ordered:

constructor
receive function (if exists)
fallback function (if exists)
external
public
internal
private

Status:

Fixed

8. Unchecked setter values

Description: 

All setter values are unchecked and can lead to redundant calling setter functions. There should be a zero value check in following functions:

setMintDuration()
StartTimeWL()
setStartTimeGiveaway()
setStartTimeRaffle()
setStartTimeWaitlist()
setMaxSupplyOfRound() 
setMintLimitOfRound()
setMerkleRootOfRound()

Status:

Acknowledged

9. Interface Instead of Contract

Description: 

Contracts named IFlowerFam and IFlowerFamMintPass can be changed for interface type.

Status: 

Fixed

Remedy: 

Ensure that the contracts are retyped as interfaces and reflect all consequential changes e.g. 

• In IFlowerFamMintPass contract, the function balanceOf() be marked with external accessibility
• In the IFlowerFam contract, the function named ownerOf() can be marked as external accessible.

DISCLAIMER

The smart contracts provided by the client for audit purposes have been thoroughly analyzed in compliance with the global best practices till date w.r.t cybersecurity vulnerabilities and issues in smart contract code, the details of which are enclosed in this report. 

This report is not an endorsement or indictment of the project or team, and they do not in any way guarantee the security of the particular object in context. This report is not considered, and should not be interpreted as an influence, on the potential economics of the token, its sale or any other aspect of the project. 

Crypto assets/tokens are the results of the emerging blockchain technology in the domain of decentralized finance and they carry with them high levels of technical risk and uncertainty. No report provides any warranty or representation to any third-Party in any respect, including regarding the bug-free nature of code, the business model or proprietors of any such business model, and the legal compliance of any such business. No third party should rely on the reports in any way, including for the purpose of making any decisions to buy or sell any token, product, service or other asset. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project.

Smart contracts are deployed and executed on a blockchain. The platform, its programming language, and other software related to the smart contract can have its vulnerabilities that can lead to hacks. The scope of our review is limited to a review of the Solidity code and only the Solidity code we note as being within the scope of our review within this report. The Solidity language itself remains under development and is subject to unknown risks and flaws. The review does not extend to the compiler layer, or any other areas beyond Solidity that could present security risks.

This audit cannot be considered as a sufficient assessment regarding the utility and safety of the code, bug-free status or any other statements of the contract. While we have done our best in conducting the analysis and producing this report, it is important to note that you should not rely on this report only - we recommend proceeding with several independent audits and a public bug bounty program to ensure security of smart contracts.

INTRODUCTION

BlockApex (Auditor) was contracted by VoirStudio (Client) for the purpose of conducting a Smart Contract Audit/Code Review. This document presents the findings of our analysis which started from 26th Jan 2022. 

Scope

The git-repository shared was checked for common code violations along with vulnerability-specific probing to detect major issues/vulnerabilities. Some specific checks are as follows:

Project Overview

Unipilot is an automated liquidity manager designed to maximize ”in-range” intervals for capital through an optimized rebalancing mechanism of liquidity pools. Unipilot V2 also detects the volatile behavior of the pools and pulls liquidity until the pool gets stable to save the pool from impairment loss.

System Architecture

The protocol is built to support multiple dexes (decentralized exchanges) for liquidity management. Currently it supports only Uniswap v3’s liquidity. In future, the protocol will support other decentralized exchanges like Sushiswap (Trident). The architecture is designed to keep in mind the future releases.

The protocol has 6 main smart contracts and their dependent libraries.

UnipilotActiveFactory.sol

The smart contract is the entry point in the protocol. It allows users to create a vault if it's not present on protocol. Nevertheless Active vaults can only be created by governance.

UnipilotPassiveFactory.sol

The smart contract is the entry point in the protocol. It allows users to create a vault if it's not present on protocol. However passive vaults can be created by anyone.

UnipilotActiveVault.sol

Vault contract allows users to deposit, withdraw, readjustLiquidity and collect fees on liquidity. It mints an LPs to its users representing their individual shares. It also has a pullLiquidity function if liquidity is needed to be pulled.

UnipilotPassiveVault.sol

PassiveVault contract allows users to deposit, withdraw, readjustLiquidity and collect fees on liquidity. It mints an LPs to its users representing their individual shares.

UnipilotStrategy.sol

The smart contract to fetch and process ticks’ data from Uniswap. It also decides the bandwidth of the ticks to supply liquidity.

UnipilotMigrator.sol

The smart contract aids to migrate users liquidity from other Uniswap V3 Liquidity Optimizer Protocols to Unipilot V2 Protocol.

Methodology & Scope

The codebase was audited in an iterative process. Fixes were applied on the way and updated contracts were examined for more bugs. We used a combination of static analysis tool (slither) and Automated testing tool (Foundry) which indicated some of the critical bugs in the code. We also did manual reviews of the code to find logical bugs, code optimizations, solidity design patterns, code style and the bugs/ issues detected by automated tools.

Privileged Roles

In a production environment, the unipilot protocol sets the address for a governance that exercises a privileged position over the factory and vault contracts in the system. The governor has the power to initiate a transfer of the governor role to a new address.

The governance address is capable of executing a set of actions including:

• Controlling the various whitelists for vaults in the factory contract
• Toggle a vault as whitelisted
• Set unipilot details to update addresses of Strategy and Index Fund contracts along with the Index Fund Fee Percentage 
• Set an operator address

The operator address has following activities it can be used for:

• Pull liquidity for a vault contract from its position on a Uniswap V3 Pool
• Call Readjust Liquidity to burn from and mint all liquidity back to a less volatile position on the Uniswap V3 Pool.

AUDIT REPORT

Executive Summary

The analysis indicates that some of the functionalities in the contracts audited are working properly.

Our team performed a technique called “Filtered Audit”, where the contract was separately audited by two individuals. After their thorough and rigorous process of manual testing, an automated review was carried out using Mythril, MythX, Surya and Slither. All the flags raised were manually reviewed and re-tested. 

Our team found: 

# of issues	Severity of the risk
1	Critical Risk issue(s)
2	High Risk issue(s)
2	Medium Risk issue(s)
7	Low Risk issue(s)
11	Informatory issue(s)

Findings

Critical risk issues

1. Deposit ether with zero value.

Description: 

If a deposit of tokens with ether as one is made, all the while the contract has pulled liquidity into the vault, the user making a deposit with 0 value can use the vault’s ether to execute a successful transaction.

Remedy:

Introduce a RefundETH() function that ensures a proper transfer of value either be in ethers or an ERC compatible version (WETH)

Status: 

Fixed

High risk issues

1. init() should have only once check

Description: 

Calling init() any time after the first time it has been called, can lead to permanent loss of position at uniswap V3.

function init() external onlyGovernance {
        int24 _tickSpacing = tickSpacing;
        int24 baseThreshold = _tickSpacing * getBaseThreshold();
        (, int24 currentTick, ) = pool.getSqrtRatioX96AndTick();

        int24 tickFloor = UniswapLiquidityManagement.floor(
            currentTick,
            _tickSpacing
        );

        ticksData.baseTickLower = tickFloor - baseThreshold;
        ticksData.baseTickUpper = tickFloor + baseThreshold;

        UniswapLiquidityManagement.checkRange(
            ticksData.baseTickLower,
            ticksData.baseTickUpper
        );
    }

Remedy:

There should be an onlyOnce modifier or a variable handling (as locks) that ensures init is never called again.

Status:

Fixed as per BlockApex recommendation.

2. Pulled liquidity rationale

Description: 

Considering the scenario where the vault has pulled Liquidity with the intention of depositing it back to Uniswap V3 when the pool is relatively less volatile; the smart contract code assumes a rational behavior to override checkDeviation modifier by manually modifying ticks through strategy using onlyGovernance functions. But the code does not guarantee any logic to push liquidity back to v3 in a safe manner

Remedy:

Debatable.

Status:

Acknowledge

Developer’s response:

Governance will manually update deviation in case of price volatility.

Medium risk issues

1. Unoptimized user liquidity is held in a vault.

Description: 

In Active vaults if a pool is created on 1-X ratio on Uniswap V3 and a user makes a deposit with 1-1 ratio through the vault, the vault is found to hold the remaining amount of the token initially at the price of X, giving users a complete share with proportion of the deposited amount while the remaining amount of user sits inactive within the vault.

Remedy: 

The vault must ensure that the amounts provided by a user are equal to the amounts of tokens actually deposited on a Uniswap pool.

Status:

Acknowledged

Developer’s response:

Added rearrange liquidity method.

2. Deposit should have non reentrant checks placed in both vaults

Description: 

The deposit() function in both vaults contract, that is, the UnipilotActiveVault and the UnipilotPassiveVault does not contain a non-reentrant modifier which is a standard practice to prevent any adversarial intent related to the reentrancy exploits.

Remedy:

A good industry practice requires that the deposit() function executes with a non-reentrant modifier; this modifier should be placed to ensure security as the deposit marks external calls through linked libraries to deposit values to the Uniswap V3.

Status:

Fixed as per BlockApex recommendation. 

Low-risk issue

1. Zero Address checks not placed in contract constructors()

Description: 

Constructor() does not contain checks for accepting params of address type whether an address is zero or not.

Remedy:

Since the constructor accepts an address from an argument, there should be a zero address check to ensure the functionality. These checks should be placed in almost every contract: Unipilot Factory , Unipilot Strategy , Unipilot Migration etc.

Status:

Acknowledged

2. Safecast in UnipilotPassiveVault.sol line 241 & 242 for `swapPercentage`

Description: 

In UnipilotPassiveVault.sol the readjustLiquidity() reads the swapPercentage variable in Line 238 of the contract to calculate the amountSpecified variable in Lines 241-242, this Math is unsafe as the calculation is executed with different types for each param. 

    if (amount0 == 0 || amount1 == 0) {
            bool zeroForOne = amount0 > 0 ? true : false;

            (, , , , uint8 swapPercentage) = getProtocolDetails();

            int256 amountSpecified = zeroForOne
                ? int256(FullMath.mulDiv(amount0, swapPercentage, 100))
                : int256(FullMath.mulDiv(amount1, swapPercentage, 100));

            pool.swapToken(address(this), zeroForOne, amountSpecified);
        }

Remedy:

Use safecasting for all type variables on lines 232-233 to ensure a seamless execution of the desired arithmetics.

Status:

Acknowledged

Developer’s response:

Percentage calculation is correct with this method. (Well tested)

3. Storage Layout is unoptimized.

Description:

Variable tight packing is strongly recommended for both vaults and factories in state variable declaration as the contracts are composed in order that is gas-consuming.

Remedy:

A solidity design pattern ‘Tight variable Packing’ ensures that the smart contract is optimized to execute efficiently within the EVM environment. 

Status:

Fixed as per BlockApex recommendation.

Status:

Slots are arranged bitwise now.

4. Check max cap for indexFundPercentage and swapPercentage.

Description: 

In setUnipilotDetails() the param indexFundPercentage is checked to receive a lowest value greater than zero.

  function setUnipilotDetails(
        address _strategy,
        address _indexFund,
        uint8 _indexFundPercentage
    ) external onlyGovernance {
        require(_strategy != address(0) && _indexFund != address(0));
        require(_indexFundPercentage > 0);
        strategy = _strategy;
        indexFund = _indexFund;
        indexFundPercentage = _indexFundPercentage;
    }

Remedy:

Ensure a check placed to bound the maximum value for the indexFundPercentage

Status:

Fixed

5. Deposit() function optimization.

Description: 

In UnipilotActiveVault.sol and UnipilotPassiveVault.sol, Users can call deposit() with zero amounts of both tokens and the function executes until the end.

function deposit(
        uint256 amount0Desired,
        uint256 amount1Desired,
        address recipient
    )
        external
        payable
        override
        returns (
            uint256 lpShares,
            uint256 amount0,
            uint256 amount1
        )
    {
        address sender = _msgSender();

        (lpShares, amount0, amount1) = pool.computeLpShares(
            true,
            amount0Desired,
            amount1Desired,
            _balance0(),
            _balance1(),
            totalSupply(),
            ticksData
        );

Remedy:

Function should check for zero value for both input args in the deposit() function in vaults contract.

Status:

Fixed

6. toggleWhitelistAccount() redundant.

Description:

toggleWhitelistAccount() can toggle the gov off in a redundant call of the same function to whitelist itself back. 

 function toggleWhitelistAccount(address _address) external onlyGovernance {
        require(_address != address(0));
        isWhitelist[_address] = !isWhitelist[_address];
    }

Remedy:

Ensure the address is checked to not allow governance to be toggled for whitelist.

Status:

Acknowledged.

7. _SortWeth() optimization

 function _sortWethAmount(
        address _token0,
        address _token1,
        uint256 _amount0,
        uint256 _amount1
    )
        private
        pure
        returns (
            address tokenAlt,
            uint256 altAmount,
            address tokenWeth,
            uint256 wethAmount
        )
    {
        // (
        //     address tokenA,
        //     address tokenB,
        //     uint256 amountA,
        //     uint256 amountB
        // ) = _token0 == WETH
        //         ? (_token0, _token1, _amount0, _amount1)
        //         : (_token0, _token1, _amount1, _amount0);

        (tokenAlt, altAmount, tokenWeth, wethAmount) = _token0 == WETH
            ? (_token1, _amount1, _token0, _amount0)
            : (_token0, _amount0, _token1, _amount1);
    }

Description: 

This function’s logic can be concise. The remedy, tested against the required logic, is mentioned as a code snippet in the screenshot above.

Status:

Acknowledged

Informatory issues

1. No NatSpec documentation

Description: 

NatSpec documentation is an essential part of smart contract readability; it is therefore advised that all contracts and following files contain proper explanatory commenting;

• UnipilotActiveVault.sol
• UnipilotPassiveVault.sol
• UnipilotMigrator.sol

Status:

Acknowledged

Developer’s response:

Completed netspec for all contracts.

2. Mark token0 and token1 as immutable in UnipilotActivevault.sol and UnipilotPassiveVault.sol

Description: 

State variables containing the address of tokens should be marked as immutable as the constructor locks the values for each after deployment.

Status:

Partial Fixed.

Developer’s response:

PassiveVault used immutables however active vaults don’t due to size issues.

3. onlyGovernance() modifier in passive vault contract

Description: 

The onlyGovernance modifier in the Passive Vault contract remains unused within the contract.

Status:

Fixed as per BlockApex recommendation.

4. _WETH address should be hardcoded before production wherever necessary

Description: 

Address of the WETH token contract is passed as a constructor param in both Factories which can be optimized by hardcoding the actual address of _WETH in the final deployment of the production environment.

constructor(
        address _pool,
        address _unipilotFactory,
        address _WETH,
        address governance,
        string memory _name,
        string memory _symbol
    ) ERC20Permit(_name) ERC20(_name, _symbol) {
        WETH = _WETH;
        unipilotFactory = IUnipilotFactory(_unipilotFactory);
        pool = IUniswapV3Pool(_pool);
        token0 = IERC20(pool.token0());
        token1 = IERC20(pool.token1());
        fee = pool.fee();
        tickSpacing = pool.tickSpacing();
        _operatorApproved[governance] = true;
    }

Status:

Acknowledged

5. Factory function createVault() optimized version

Description: 

createVault() is found to be optimized if it executes in the following recommended pattern: 

• First check pool on v3 - if returns true then check vault exists - if returns true then return from function
• If pool returns false - create & initialize pool then create vault

Current Implementation:

function createVault(
        address _tokenA,
        address _tokenB,
        uint24 _fee,
        uint160 _sqrtPriceX96,
        string memory _name,
        string memory _symbol
    ) external override onlyGovernance returns (address _vault) {
        require(_tokenA != _tokenB);
        (address token0, address token1) = _tokenA < _tokenB
            ? (_tokenA, _tokenB)
            : (_tokenB, _tokenA);
        require(vaults[token0][token1][_fee] == address(0));
        address pool = uniswapFactory.getPool(token0, token1, _fee);

        if (pool == address(0)) {
            pool = uniswapFactory.createPool(token0, token1, _fee);
            IUniswapV3Pool(pool).initialize(_sqrtPriceX96);
        }

        _vault = address(
            new UnipilotActiveVault{
                salt: keccak256(abi.encodePacked(_tokenA, _tokenB, _fee))
            }(pool, address(this), WETH, governance, _name, _symbol)
        );

        isWhitelist[_vault] = true;
        vaults[token0][token1][_fee] = _vault;
        vaults[token1][token0][_fee] = _vault; // populate mapping in the reverse direction
        emit VaultCreated(token0, token1, _fee, _vault);
    }

Status:

Acknowledged

6. Assignment of Params in order to receive the function signature.

Description: 

In all four contracts of vault and factory the constructor receives arguments in order which is out-of-sync to the one being assigned, reducing the code readability. Ensure param values and actual assignments are in sync for better code readability.

constructor(
        address _pool,
        address _unipilotFactory,
        address _WETH,
        address governance,
        string memory _name,
        string memory _symbol
    ) ERC20Permit(_name) ERC20(_name, _symbol) {
        WETH = _WETH;
        unipilotFactory = IUnipilotFactory(_unipilotFactory);
        pool = IUniswapV3Pool(_pool);
        token0 = IERC20(pool.token0());
        token1 = IERC20(pool.token1());
        fee = pool.fee();
        tickSpacing = pool.tickSpacing();
        _operatorApproved[governance] = true;
    }

Status:

Acknowledged

7. Order of functions as in solidity Style Guide

Description: 

Receive() and Fallback() should be moved on top, below constructor; following the solidity design patterns

Status:

Fixed

8. uint256 can be cheaper than uint8

Description: 

Uint8 is proved to be more costly than uint256 variables in a number of scenarios, where a better and optimized variable packing for uint8 variables is recommended or replaced with uint256/ uint64/ uint24 type vars.

Status:

Fixed

9. pullLiquidity() is vulnerable in its current execution

Description: 

The pullLiquidity(address _recipient) method is vulnerable to some extent, holding potential for mal-intent or permanent loss of value. Checking for the address argument as not another whitelisted vault can ensure no accidental and permanent loss of tokens happen.

Status:

Pending

Developer’s response:

Vaults will be whitelisted only for the execution of pull liquidity (when needed) soon after execution that vault will be blacklisted in order to avoid accidentally sending tokens to other active vaults.

10. Spelling mistakes in function signatures

Description: 

In the UnipilotMigrator.sol file,
migrateUnipilotLiquididty() and  _refundRemainingLiquidiy() are spelled wrong, causing readability issues as well as creating the wrong function signature.

Status:

Fixed 

11. Mark private functions as internal

Description: 

In the UnipilotMigrator.sol file,
_sortWethAmount() and _addLiquidityUnipilot() are private, which are gas costly.

Status:

Fixed as per BlockApex recommendation.

DISCLAIMER 

The smart contracts provided by the client for audit purposes have been thoroughly analyzed in compliance with the global best practices till date w.r.t cybersecurity vulnerabilities and issues in smart contract code, the details of which are enclosed in this report. 

This report is not an endorsement or indictment of the project or team, and they do not in any way guarantee the security of the particular object in context. This report is not considered, and should not be interpreted as an influence, on the potential economics of the token, its sale or any other aspect of the project. 

Crypto assets/tokens are results of the emerging blockchain technology in the domain of decentralized finance and they carry with them high levels of technical risk and uncertainty. No report provides any warranty or representation to any third-Party in any respect, including regarding the bug-free nature of code, the business model or proprietors of any such business model, and the legal compliance of any such business. No third-party should rely on the reports in any way, including for the purpose of making any decisions to buy or sell any token, product, service or other asset. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project.

Smart contracts are deployed and executed on a blockchain. The platform, its programming language, and other software related to the smart contract can have its vulnerabilities that can lead to hacks. The scope of our review is limited to a review of the Solidity code and only the Solidity code we note as being within the scope of our review within this report. The Solidity language itself remains under development and is subject to unknown risks and flaws. The review does not extend to the compiler layer, or any other areas beyond Solidity that could present security risks.

This audit cannot be considered as a sufficient assessment regarding the utility and safety of the code, bug-free status or any other statements of the contract. While we have done our best in conducting the analysis and producing this report, it is important to note that you should not rely on this report only - we recommend proceeding with several independent audits and a public bug bounty program to ensure security of smart contracts.