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.
Name: Rain Protocol |
Auditor: BlockApex |
Platform: EVM-based / Solidity |
Type of review: Manual Code Review | Automated Took Analysis |
Methods: Architecture Review | Functional Testing | Computer-Aided Verification | Manual Review |
Git repository/ Commit Hash: e2277e2253c60b04924f52b68e4ab6df4a68df6e |
White paper/ Documentation: Docs |
Document log: Initial Audit Completed: Nov 27th, 2022 Final Audit Completed: Nov 29th, 2022 |
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:
Code review | Functional review | |
Reentrancy | Unchecked external call | Business Logics Review |
Ownership Takeover | FT token API violation | Functionality Checks |
Timestamp Dependence | Unchecked math | Access Control & Authorization |
Gas Limit and Loops | Unsafe type inference | Escrow manipulation |
DoS with (Unexpected) Throw | Implicit visibility level | Token Supply manipulation |
DoS with Block Gas Limit | Deployment Consistency | Asset’s integrity |
Transaction-Ordering Dependence | Repository Consistency | User Balances manipulation |
Style guide violation | Data Consistency | Kill-Switch Mechanism |
Costly Loop | Inheritance Ordering | Operation Trails & Event Generation |
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.
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 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.
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.
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) |
# | Findings | Risk | Status |
1 | Potential Loss of Precision | Low | Acknowledged |
2 | Inexplicable inclusion of unused library | Informational | Fixed |
No critical issues were found.
No High-risk issues were found.
No Medium-risk issues were found.
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.
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.
// import "../math/FixedPointMath.sol"; // using FixedPointMath for uint256; |
Alleviation: This issue is fixed.
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.
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
Name: Phase Protocol |
Auditor: BlockApex |
Platform: Solana | Rust |
Type of review: Manual Code Review | Automated Took Analysis |
Methods: Architecture Review | Functional Testing | Computer-Aided Verification | Manual Review |
Git repository/ Commit Hash: https://github.com/dedmonkes/ded-social-programs |
White paper/ Documentation: https://www.ded.social |
Document log: Initial Audit Completed: July 30th, 2022 Final Audit Completed: August 19th, 2022 |
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:
Code review | Functional review | |
Reentrancy | Unchecked external call | Business Logics Review |
Ownership Takeover | ERC20 API violation | Functionality Checks |
Timestamp Dependence | Unchecked math | Access Control & Authorization |
Gas Limit and Loops | Unsafe type inference | Escrow manipulation |
DoS with (Unexpected) Throw | Implicit visibility level | Token Supply manipulation |
DoS with Block Gas Limit | Deployment Consistency | Asset’s integrity |
Transaction-Ordering Dependence | Repository Consistency | User Balances manipulation |
Style guide violation | Data Consistency | Kill-Switch Mechanism |
Costly Loop | Operation Trails & Event Generation |
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.
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.
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.
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.
# 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) |
# | Findings | Risk | Status |
1. | Lack of Proper Checks Could Potentially Lead To Bypass When Changing Phases | High | Fixed |
2. | Too Much Usage Of Helper Methods | Medium | Fixed |
3. | Use Of Insecure Math In Arithmetic | Low | Fixed |
4. | Use Of Outdated And Vulnerable Crates | Low | Fixed |
6. | Inexistent math Overflow Checks | Informatory | Fixed |
7. | Low Test Coverage | Informatory | Acknowledge |
No issues were found.
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
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
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
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
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
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
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.
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 .
Name: Spin.Finance Decentralized Exchange - Order Book Market Place | Swap Functionality |
Auditors: Moazzam Arif | Abdul Sami Jawed | Faizan Nehal | Muhammad Jariruddin |
Platform: NEAR Protocol | Rust |
Type of review: Manual Code Review | Automated Tools Analysis |
Methods: Architecture Review | Functional Testing | Computer-Aided Verification | Manual Review |
Git repository/ Commit Hash: 549bce99171d0fe5473075937f76310348b2dca2 |
White paper/ Documentation: Docs | Medium | Spin Intern Guides |
Document log: Initial Audit Completed: June 15th, 2022 Final Audit Completed: July 10th, 2022 |
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:
Code review | Functional review | |
Reentrancy | Unchecked external call | Business Logics Review |
Ownership Takeover | ERC20 API violation | Functionality Checks |
Timestamp Dependence | Unchecked math | Access Control & Authorization |
Gas Limit and Loops | Unsafe type inference | Escrow manipulation |
DoS with (Unexpected) Throw | Implicit visibility level | Token Supply manipulation |
DoS with Block Gas Limit | Deployment Consistency | Asset’s integrity |
Transaction-Ordering Dependence | Repository Consistency | User Balances manipulation |
Style guide violation | Data Consistency | Kill-Switch Mechanism |
Costly Loop | Operation Trails & Event Generation |
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.
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.
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.
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.
# 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) |
# | Findings | Risk | Status |
1. | Swap: Logic Flaw in set_availability() will lead wrong state of the market been set | High | Acknowledged |
2. | Order expiration time can be exploited | High | Fixed |
3. | Potential MultiSig Failure/ Phishable Deployment | High | Fixed |
4. | Potentially dangerous drop_state() method | High | Fixed |
5. | Inessential parameter optional visibility | High | Fixed |
6. | Unoptimized loop with redundant ops | Medium | Acknowledged |
7. | Impossible ownership transfer | Low | Acknowledged |
8. | Illegal Max Fee Possibility | Low | Fixed |
9. | Ineffectual availability of market | Low | Acknowledged |
10. | Inexistent math checks | Low | Acknowledged |
11. | Inconsistent asserts and panic | Informatory | Acknowledged |
12. | Unoptimized error message pattern | Informatory | Acknowledged |
13. | Inconsistent arguments type | Informatory | Acknowledged |
14. | Inexplicable balances variable | Informatory | Fixed |
15. | Inconsistent functions to handle errors | Informatory | Acknowledged |
16. | Unhandled whitelisting removal | Informatory | Acknowledged |
17. | Quality of Test Cases | Informatory | Partially Fixed |
18. | Incomplete condition evaluation | Informatory | Acknowledged |
19. | Misidentified default order behavior | Informatory | Acknowledged |
No issues were found.
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
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
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
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
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.
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.
Name: ChainpalsTransaction |
Auditor: Kaif Ahmed | Mohammad Memon |
Platform: Ethereum/Solidity/BCS |
Type of review: Manual Code Review | Automated Code Review |
Methods: Architecture Review, Functional Testing, Computer-Aided Verification, Manual Review |
Git repository: - |
White paper/ Documentation: https://dev.chainpals.io/assets/document/ChainpalsLightpaper.pdf |
Document log: Initial Audit: 16th June 2022 Final Audit: 22 June 2022 |
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:
Code review | Functional review | |
Reentrancy | Unchecked external call | Business Logics Review |
Ownership Takeover | ERC20 API violation | Functionality Checks |
Timestamp Dependence | Unchecked math | Access Control & Authorization |
Gas Limit and Loops | Unsafe type inference | Escrow manipulation |
DoS with (Unexpected) Throw | Implicit visibility level | Token Supply manipulation |
DoS with Block Gas Limit | Deployment Consistency | Asset’s integrity |
Transaction-Ordering Dependence | Repository Consistency | User Balances manipulation |
Style guide violation | Data Consistency | Kill-Switch Mechanism |
Costly Loop | Operation Trails & Event Generation |
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.
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.
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.
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.
# 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 | Risk | Status |
1. | Missing core functionality | Critical | Acknowledged |
2. | Misleading functionality | Critical | Acknowledged |
3. | Missing zero address checks | Low | Fixed |
4. | Unnecessary conversion | Low | Fixed |
5. | Anti-pattern check | Informatory | Fixed |
6. | Inconsistency in error messages | Informatory | Fixed |
7. | Spelling mistakes | Informatory | Fixed |
8. | Follow solidity design pattern | Informatory | Fixed |
9. | Inconsistent code writing | Informatory | Fixed |
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.
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:
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.
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:
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.
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.
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.
Name: Chainpals Token (BEP20) |
Auditor: Kaif Ahmed | Mohammad Memon |
Platform: Ethereum/Solidity/BSC |
Type of review: Manual Code Review | Automated Code Review |
Methods: Architecture Review, Functional Testing, Computer-Aided Verification, Manual Review |
Git repository: - |
White paper/ Documentation: https://dev.chainpals.io/assets/document/ChainpalsLightpaper.pdf |
Document log: Initial Audit: 1st June 2022 Final Audit: 17th June 2022 |
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:
Code review | Functional review | |
Reentrancy | Unchecked external call | Business Logics Review |
Ownership Takeover | ERC20 API violation | Functionality Checks |
Timestamp Dependence | Unchecked math | Access Control & Authorization |
Gas Limit and Loops | Unsafe type inference | Escrow manipulation |
DoS with (Unexpected) Throw | Implicit visibility level | Token Supply manipulation |
DoS with Block Gas Limit | Deployment Consistency | Asset’s integrity |
Transaction-Ordering Dependence | Repository Consistency | User Balances manipulation |
Style guide violation | Data Consistency | Kill-Switch Mechanism |
Costly Loop | Operation Trails & Event Generation |
Chainpals Token is a BEP20 token contract. It works slightly differently from the traditional BEP20 contract.
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.
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.
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.
# of issues | Severity of the risk |
0 | Critical Risk issue(s) |
0 | High-Risk issue(s) |
0 | Medium Risk issue(s) |
2 | Low-Risk issue(s) |
7 | Informatory issue(s) |
6 | Suggestion(s) |
# | Findings | Risk | Status |
1. | Potential Centralization risk | Low | acknowledged |
2. | No upper limit on fee percentage | Low | Fixed |
3. | Use struct to simplify readability | Informatory | acknowledged |
4. | Expensive uint comparison in require statements. | Informatory | Fixed |
5. | Use memory keyword instead of calldata on parameters inside external function | Informatory | acknowledged |
6. | V1 contract will hold both V1 and V2 tokens at the time of V2 token Migration. | Informatory | acknowledged |
7. | Follow the Solidity style guide | Informatory | acknowledged |
8. | Loop running till the end | Informatory | Fixed |
9. | Zero address checks in the constructor | Informatory | Fixed |
10. | setFeeExcludedAddress needs to contract only address | Suggestion | Fixed |
11. | There is no view function to view the ongoing stage | Suggestion | acknowledged |
12. | Known parameters should be hardcoded | Suggestion | acknowledged |
13. | Potential destruction of blacklisted address’s funds | Suggestion | acknowledged |
14. | Make sure that the error messages clearly explain the reason for the error | Suggestion | Fixed |
15. | transfer() should not deduct a 1% fee from whitelisted addresses | Suggestion | Fixed |
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.
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.
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.
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.
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.
Name: Chainpals Presale |
Auditor: Kaif Ahmed | Mohammad Memon |
Platform: Ethereum/Solidity/BSC |
Type of review: Manual Code Review | Automated Code Review |
Methods: Architecture Review, Functional Testing, Computer-Aided Verification, Manual Review |
Git repository: - |
White paper/ Documentation: https://dev.chainpals.io/assets/document/ChainpalsLightpaper.pdf |
Document log: Initial Audit: 7th June 2022 Final Audit: 17 June 2022 |
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:
Code review | Functional review | |
Reentrancy | Unchecked external call | Business Logics Review |
Ownership Takeover | ERC20 API violation | Functionality Checks |
Timestamp Dependence | Unchecked math | Access Control & Authorization |
Gas Limit and Loops | Unsafe type inference | Escrow manipulation |
DoS with (Unexpected) Throw | Implicit visibility level | Token Supply manipulation |
DoS with Block Gas Limit | Deployment Consistency | Asset’s integrity |
Transaction-Ordering Dependence | Repository Consistency | User Balances manipulation |
Style guide violation | Data Consistency | Kill-Switch Mechanism |
Costly Loop | Operation Trails & Event Generation |
Chainpals Token is a BEP20 token contract. Our contract at hand is the Chainpals Presale contract that uses the BEP20 token.
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.
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.
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.
# 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 | Risk | Status |
1. | Potential economic crash | Critical | Fixed |
2. | Unsupervised contract balance return | Medium | Fixed |
3. | Potential out-of-gas scenario | Medium | acknowledged |
4. | Check does not fulfill purpose of limit purchase | Medium | acknowledged |
5. | Zero address check missing in recoverWrongTokens() | Low | Fixed |
6. | Missing zero address checks in constructor | Low | Fixed |
7. | Missing zero value checks in constructor | Low | Fixed |
8. | updateWithdrawalRecord(): Condition satisfaction does not terminate loop | Low | Fixed |
9. | updateReferralWithdrawRecord(): Condition satisfaction does not terminate loop | Low | Fixed |
10. | withdrawReferralTokens() is gas-costly | Informatory | acknowledged |
11. | Hardcode the USDT and BUSD token addresses | Informatory | Fixed |
12. | Use struct to simplify readability | Informatory | Fixed |
13. | buyUsingBnbCoin() and buyUsingAltCoin() should be set to external. | Informatory | Fixed |
14. | Spelling mistakes in two functions | Informatory | Fixed |
15. | Unnecessary conversion | Informatory | Fixed |
16. | Put all external functions on the top, below the constructor. | Informatory | Fixed |
17. | TreasuryWallet address check | Informatory | Fixed |
18. | Make all necessary variables constant | Informatory | acknowledged |
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.
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.
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.
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.
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:
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.
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.
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.
Name: Unipilot Staking |
Auditors: Kaif Ahmed | Faizan Nehal |
Platform: EVM |
Type of review: Manual Code Review | Automated Tools Analysis |
Methods: Architecture Review, Functional Testing, Computer-Aided Verification, Manual Review |
Git repository/ Commit Hash: https://github.com/unipilot |
White paper/ Documentation: Docs | Medium |
Document log: Initial Audit Completed: June 25th, 2022 Final Audit Completed: June 27th, 2022 |
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:
Code review | Functional review | |
Reentrancy | Unchecked external call | Business Logics Review |
Ownership Takeover | ERC20 API violation | Functionality Checks |
Timestamp Dependence | Unchecked math | Access Control & Authorization |
Gas Limit and Loops | Unsafe type inference | Escrow manipulation |
DoS with (Unexpected) Throw | Implicit visibility level | Token Supply manipulation |
DoS with Block Gas Limit | Deployment Consistency | Asset’s integrity |
Transaction-Ordering Dependence | Repository Consistency | User Balances manipulation |
Style guide violation | Data Consistency | Kill-Switch Mechanism |
Costly Loop | Operation Trails & Event Generation |
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.
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.
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.
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.
# 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) |
# | Findings | Risk | Status |
1. | Pending rewards for the previous reward token are getting lost | High | Fixed |
2. | Incorrect behavior for pending reward tokens | High | Fixed |
3. | Incorrect behavior for staking when no one has staked their tokens | High | Fixed |
4. | Arithmetic overflow error on updating the reward token | Medium | Fixed |
5. | Inconsistent behavior if the reward token is other than 18 decimals | Medium | Fixed |
6. | Less optimized checks | Low | Acknowledged |
7. | Redundant event emission | Informatory | Fixed |
No issues were found.
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.
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.
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.
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.
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.
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.
Name: Flower Fam |
Auditor: Kaif Ahmed | Muhammad Jarir Uddin |
Platform: Ethereum/Solidity |
Type of review: Manual Code Review | Automated Code Review |
Methods: Architecture Review, Functional Testing, Computer-Aided Verification, Manual Review |
Git repository/SHA256 Checksum: 3c6fec450c5aa25e6dec9a0378d99dcbf44b4f113d096e581e01d86720b9cd1d |
White paper/ Documentation: https://docs.flowerfam.earth/welcome. |
Document log: Initial Audit: 19th May 2022 Final Audit: 23rd May 2022 |
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:
Code review | Functional review | |
Reentrancy | Unchecked external call | Business Logics Review |
Ownership Takeover | ERC20 API violation | Functionality Checks |
Timestamp Dependence | Unchecked math | Access Control & Authorization |
Gas Limit and Loops | Unsafe type inference | Escrow manipulation |
DoS with (Unexpected) Throw | Implicit visibility level | Token Supply manipulation |
DoS with Block Gas Limit | Deployment Consistency | Asset’s integrity |
Transaction-Ordering Dependence | Repository Consistency | User Balances manipulation |
Style guide violation | Data Consistency | Kill-Switch Mechanism |
Costly Loop | Operation Trails & Event Generation |
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.
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.
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.
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.
# 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 | Risk | Status |
1. | Centralization risk | Medium | Fixed |
2. | setMerkleRootOfRound() can overwrite mapping | Low | Acknowledged |
3. | Configuration inconsistency | Low | Acknowledged |
4. | Withdraw function argument validation check | Low | Fixed |
5. | Recommendation for missing function in the contract file as received by the client | Low | Acknowledged |
6. | Natspec missing | Informatory | Acknowledged |
7. | Order of Functions | Informatory | Fixed |
8. | Unchecked setter values | Informatory | Acknowledged |
9. | Interface Instead of Contract | Informatory | Fixed |
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
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.
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;
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.
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.
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.
Name: Unipilot-V2 |
Auditor: Moazzam Arif | Kaif Ahmed | Muhammad Jarir Uddin |
Platform: Ethereum/Solidity |
Type of review: Manual Code Review | Automated Code Review |
Methods: Architecture Review, Functional Testing, Computer-Aided Verification, Manual Review |
Git repository: https://github.com/VoirStudio/unipilot-v2/tree/revamp-structure |
White paper/ Documentation: https://unipilot.gitbook.io/unipilot/ |
Document log: Initial Audit: 14th Feb 2022 (complete) Quality Control: 14th - 22nd March 2022 Final Audit: 26th March 2022 (Complete) |
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:
Code review | Functional review | |
Reentrancy | Unchecked external call | Business Logics Review |
Ownership Takeover | ERC20 API violation | Functionality Checks |
Timestamp Dependence | Unchecked math | Access Control & Authorization |
Gas Limit and Loops | Unsafe type inference | Escrow manipulation |
DoS with (Unexpected) Throw | Implicit visibility level | Token Supply manipulation |
DoS with Block Gas Limit | Deployment Consistency | Asset’s integrity |
Transaction-Ordering Dependence | Repository Consistency | User Balances manipulation |
Style guide violation | Data Consistency | Kill-Switch Mechanism |
Costly Loop | Operation Trails & Event Generation |
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.
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.
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.
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:
The operator address has following activities it can be used for:
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.
# 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 | Risk | Status |
1. | Deposit ether with zero value | Critical | Fixed |
2. | init() should have only once check | High | Fixed |
3. | Pulled liquidity rationale | High | Acknowledged |
4. | Unoptimized user liquidity is held in a vault. | Medium | Acknowledged |
5. | Deposit should have non-reentrant checks placed in both vaults | Medium | Fixed |
6. | Zero Address checks not placed in contract constructors() | Low | Acknowledged |
7. | Safecast in UnipilotPassiveVault.sol line 232 & 233 for `swapPercentage` | Low | Acknowledged |
8. | The storage Layout is unoptimized. | Low | Acknowledged |
9. | Check max cap for indexFundPercentage and swapPercentage | Low | Fixed |
10. | Deposit() function optimization | Low | Fixed |
11. | toggleWhitelistAccount() redundant. | Low | Pending |
12. | SortWeth() optimization | Low | Acknowledged |
13. | No natspec documentation in UnipilotActiveVault.sol | Informatory | Acknowledged |
14. | Mark token0 and token1 as immutable in UnipilotActivevault.sol and UnipilotPassiveVault.sol | Informatory | Partially Fixed |
15. | onlyGovernance() modifier in passive vault contract | Informatory | Fixed |
16. | _WETH address should be hardcoded before production wherever necessary | Informatory | Acknowledged |
17. | Factory function createVault() optimization version. | Informatory | Acknowledged |
18. | Assignment of Params in order to receive the function signature. | Informatory | Acknowledged |
19. | Order of functions in solidity style guides. | Informatory | Acknowledged |
20. | uint256 can be cheaper than uint8 | Informatory | Fixed |
21. | pullLiquidity() is vulnerable in its current execution | Informatory | Fixed |
22. | Spelling mistakes in function signatures. | Informatory | Acknowledged |
23. | Mark private functions as internal | Informatory | Fixed |
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
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.
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.
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.
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
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
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;
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:
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.
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.