Executive Summary
CoinFabrik was asked to audit the contracts for the KannaCoin project. The audited files are from the git repository. The audit is based on the commit 4d00e8dd08ce7532f4697fdd0a177110713eedec.
During this audit, we found a critical issue and two minor issues. Also, several enhancements were proposed.
Scope
The scope for this audit includes and is limited to the following files:
contracts/KannaPreSale.sol: Pre-sale contract for Kanna.contracts/KannaToken.sol: ERC20 token.contracts/KannaYield.sol: Kanna farming contract.
No other files in this repository were audited. Its dependencies are assumed to work according to their documentation. Also, no tests were reviewed for this audit.
Methodology
CoinFabrik was provided with the source code, including automated tests that define the expected behavior, and general documentation about the project. Our auditors spent one week auditing the source code provided, which includes understanding the context of use, analyzing the boundaries of the expected behavior of each contract and function, understanding the implementation by the development team (including dependencies beyond the scope to be audited) and identifying possible situations in which the code allows the caller to reach a state that exposes some vulnerability. Without being limited to them, the audit process included the following analyses:
- Arithmetic errors
- Outdated version of Solidity compiler
- Race conditions
- Reentrancy attacks
- Misuse of block timestamps
- Denial of service attacks
- Excessive gas usage
- Missing or misused function qualifiers
- Needlessly complex code and contract interactions
- Poor or nonexistent error handling
- Insufficient validation of the input parameters
- Incorrect handling of cryptographic signatures
- Centralization and upgradeability
After delivering a report with our findings, the development team had the opportunity to comment on every finding and fix the issues they considered convenient. Once fixed and/or commented, our team ran a second review process to verify that the changes to the code effectively solve the issues found and do not unintentionally add new ones. This report includes the final status after the second review.
Findings
In the following table we summarize the security issues we found in this audit. The severity classification criteria and the status meaning are explained below. This table does not include the enhancements we suggest to implement, which are described in a specific section after the security issues.
| ID | Title | Severity | Status |
| CR-01 | Inconsistent State And Denial Of Service After Claiming | Critical | Resolved |
| MI-01 | Contract Initialized With Zero-address | Minor | Resolved |
| MI-02 | Overflow Because Of Casting | Minor | Resolved |
Severity Classification
Security risks are classified as follows:
- Critical: These are issues that we manage to exploit. They compromise the system seriously. They must be fixed immediately.
- Medium: These are potentially exploitable issues. Even though we did not manage to exploit them or their impact is not clear, they might represent a security risk in the near future. We suggest fixing them as soon as possible.
- Minor: These issues represent problems that are relatively small or difficult to take advantage of, but might be exploited in combination with other issues. These kinds of issues do not block deployments in production environments. They should be taken into account and be fixed when possible.
Issues Status
An issue detected by this audit has one of the following statuses:
- Unresolved: The issue has not been resolved.
- Acknowledged: The issue remains in the code, but is a result of an intentional decision.
- Resolved: Adjusted program implementation to eliminate the risk.
- Partially resolved: Adjusted program implementation to eliminate part of the risk. The other part remains in the code, but is a result of an intentional decision.
- Mitigated: Implemented actions to minimize the impact or likelihood of the risk.
Critical Severity Issues
CR-01 Inconsistent State And Denial Of Service After Claiming
Location:
contracts/KannaPreSale.sol:145-161
When claim() is called with unlock=false, it is not supposed to use locked funds. However, the function does not verify if the contract has enough tokens in the unlocked balance in order to transfer to the claimer. Therefore, it will use locked funds and not update the internal balance related to the locked tokens, resulting in an erroneous state.
Exploiting it, besides using locked funds when explicitly expressing the opposite, will result in availableSupply() reverting because of an overflow. Since most of the functions depend on it, it will cause a denial of service.
Recommendation
Add a require statement to check if the available supply is greater than the claimed amount when unlock=false.
Status
Resolved. Two wrappers were created for the claiming function. One for locked and one for available tokens. Proper checks were added on those functions.
Medium Severity Issues
No issues found.
Minor Severity Issues
MI-01 Contract Initialized With Zero-address
Location:
contracts/KannaPreSale.sol:51-61contracts/KannaYield.sol:55-63
In KannaPreSale, the constructor function does not check whether knnToken address is different from zero, what will result in functions reverting because of failed external calls. Moreover, knnToken is an immutable variable, so it cannot be changed afterwards.
In KannaYield, the same happens to feeRecipient, which also is an immutable variable. In this case, this results in transferring collected fees to the zero-address.
Recommendation
Add a check to verify the set addresses are not zero.
Status
Resolved. Checks added to the constructor.
MI-02 Overflow Because Of Casting
Location:
contracts/KannaPreSale.sol:203
In the convertToWEI() function, answer, an int256 variable, is cast to uint256. If the variable has a value lower than zero, ethPriceInUSD will overflow and wrap, resulting in a large value.
Since the price aggregator is out of the scope of the audit, the likelihood of this issue is unknown.
Recommendation
Use the safeCast library included in OpenZeppelin’s module.
Status
Resolved. Fixed according to the recommendation.
Enhancements
These items do not represent a security risk. They are best practices that we suggest implementing.
| ID | Title | Status |
| EN-01 | Declare Compile-Time-Known Values As Constants | Implemented |
| EN-02 | Unnecessary Cast | Implemented |
| EN-03 | Unnecessary Zero-address Check | Implemented |
EN-01 Declare Compile-Time-Known Values As Constants
Location:
contracts/KannaToken.sol:24,25contracts/KannaYield.sol:53
In KannaToken, INITIAL_SUPPLY and MAX_SUPPLY are immutable variables set inline. However, they can be declared as constants, what will save gas.
Also, in KannaYield, the subscriptionFee variable will be set when deployed and will not be changed afterwards.
Recommendation
Declare those values as constants.
Status
Implemented.
EN-02 Unnecessary Cast
Location:
contracts/KannaToken.sol:57
The parameter newTreasury is an address, and it is cast to address, which is redundant and increases runtime cost.
Recommendation
Remove the unnecessary cast.
Status
Implemented.
EN-03 Unnecessary Zero-address Check
Location:
contracts/KannaToken.sol:72
The mint() function verifies if the treasury address is equal to zero. However, _mint() also verifies it.
Recommendation
Remove the unnecessary check in order to reduce runtime cost.
Status
Implemented.
Other Considerations
The considerations stated in this section are not right or wrong. We do not suggest any action to fix them. But we consider that they may be of interest to other stakeholders of the project, including users of the audited contracts, token holders or project investors.
Centralization
For KannaPreSale, the owner can withdraw the funds, assign roles to other addresses and stop the presale.
For KannaToken, the owner can update the treasury, add or remove minters,
For KannaYield, the owner should transfer the tokens used to reward users. Otherwise, no rewards will be distributed.
Upgrades
Contracts do not provide mechanisms for upgrades.
Privileged Roles
These are the privileged roles that we identified on each of the audited contracts.
Owner Can Set Any Price For Presale
The function updateQuotation() allows the contract owner to set the price at which KannaCoin is bought.
At the second revision, the function was removed. The owner cannot set a new price.
Use Of Transfer Method For Sending Ether
KannaPreSale::withdraw() sends ether through the transfer() method, which is limited in the gas which can be used by the recipient. Therefore, if the recipient is a contract with a fallback()/receive() function that has a high runtime cost, it will revert.
Tier List Should Be Ordered
KannaYield::feeOf() iterates over the tier list until subscriptionDuration is lower than one of the values and returns the corresponding fee. However, if the list is unordered, a greater incorrect state might be returned.
Treasury
Initially, the treasury was a custom smart contract with an owner and where tokens were stored.
The contract was removed and the development team informed us that the address that is going to be set as treasurer will be a multisig wallet. In order to verify it, the address can be inspected on the blockchain explorer.
Address to be set: 0x51F9298d8C9DAD2105c99cee8429430a15381C3E .
Changelog
- 2022-11-11 – Initial report based on commit 4d00e8dd08ce7532f4697fdd0a177110713eedec.
- 2022-11-25 – Fixes reviewed based on commit eef5801b875662248a70804709e80f28e1e085ad.
