Gems & Goblins Token Smart Contract Security Audit Report
Are Your Funds Safe?
Our expert team at Callisto Network has conducted an in-depth security audit of the Gems & Goblins Token smart contract. This audit aims to ensure the security of your funds by identifying and assessing any potential vulnerabilities. Here, we present our findings:
Executive Summary
This report presents the results of the security audit conducted by the Callisto Network Security Department on the Gems And Goblins ICO smart contract in February 2023. It analyzes the contract’s security posture in-depth and highlights any identified vulnerabilities.
1. Scope of the Audit
Commit f50556c44a6971462d122b0494311398556f3502
2. Audit Findings
Our audit reported a total of 1 finding(s), categorized as follows:
- 0 high-severity issue(s).
- 0 medium severity issue(s).
- 1 low-severity issue(s).
In addition to these findings, our audit identified 7 additional points, detailed in the following sections:
- 2 note(s).
- 5 owner privilege(s).
2.1 The Minimal Deposit Value Is Set In 1 Token
This requirement has no logical sense and should be removed because we already have a limitation of minimum purchase amount.[/bsf-info-box][bsf-info-box icon_type=”custom” icon_img=”id^11679|url^https://callisto.network/wp-content/uploads/2023/04/star-svgrepo-com.png|caption^null|alt^null|title^star-svgrepo-com|description^null” img_width=”20″ icon_animation=”fadeIn” title=”Code Snippets:”]
- https://github.com/Dexaran/GnG_ICO/blob/f50556c44a6971462d122b0494311398556f3502/ICO_vesting.sol#L822.
- https://github.com/Dexaran/GnG_ICO/blob/f50556c44a6971462d122b0494311398556f3502/ICO_vesting.sol#L776.
- https://github.com/Dexaran/GnG_ICO/blob/f50556c44a6971462d122b0494311398556f3502/ICO_vesting.sol#L861.
[/bsf-info-box][bsf-info-box icon_type=”custom” icon_img=”id^11679|url^https://callisto.network/wp-content/uploads/2023/04/star-svgrepo-com.png|caption^null|alt^null|title^star-svgrepo-com|description^null” img_width=”20″ icon_animation=”fadeIn” title=”Recommendation”]Remove the requirement to deposit at least 1 token/coin.[/bsf-info-box]
2.2 Use SafeTransfer Functions Instead of ERC20/223
transfer and transferFrom functions. So it’s highly recommended to use safeTransfer and safeTranferFrom functions instead of them. For example, in the function buy if transferFrom does not return any value transaction will fail, and the user can’t participate in ICO. But if it returns false when tokens can’t be transferred, the user will get GNG tokens for free because the returns value is not checked in this contract.
Also, in the function ERC20Rescue, it is better to use safeTransfer to rescue non-standard ERC20 tokens. Although the function delegateCall can help rescue such tokens, it’s required to deploy a particular contract with the correct transfer function (it will require more work).[/bsf-info-box][bsf-info-box icon_type=”custom” icon_img=”id^11679|url^https://callisto.network/wp-content/uploads/2023/04/star-svgrepo-com.png|caption^null|alt^null|title^star-svgrepo-com|description^null” img_width=”20″ icon_animation=”fadeIn” title=”Code Snippets:”]
- https://github.com/Dexaran/GnG_ICO/blob/f50556c44a6971462d122b0494311398556f3502/ICO_vesting.sol#L843.
- https://github.com/Dexaran/GnG_ICO/blob/f50556c44a6971462d122b0494311398556f3502/ICO_vesting.sol#L938.
[/bsf-info-box][bsf-info-box icon_type=”custom” icon_img=”id^11679|url^https://callisto.network/wp-content/uploads/2023/04/star-svgrepo-com.png|caption^null|alt^null|title^star-svgrepo-com|description^null” img_width=”20″ icon_animation=”fadeIn” title=”Recommendation”]Replace transfer and transferFrom for tokens transferring by safeTransfer() and safeTransferFrom().
function safeTransfer(address token, address to, uint value) internal {
// bytes4(keccak256(bytes(‘transfer(address,uint256)’)));
(bool success, bytes memory data) = token.call(abi.encodeWithSelector(0xa9059cbb, to, value));
require(success && (data.length == 0 || abi.decode(data, (bool))), ‘TransferHelper: TRANSFER_FAILED’);
}
function safeTransferFrom(address token, address from, address to, uint value) internal {
// bytes4(keccak256(bytes(‘transferFrom(address,address,uint256)’)));
(bool success, bytes memory data) = token.call(abi.encodeWithSelector(0x23b872dd, from, to, value));
require(success && (data.length == 0 || abi.decode(data, (bool))), ‘TransferHelper: TRANSFER_FROM_FAILED’);
}
[/bsf-info-box]
2.3 Owner Privileges
modify_asset allows the owner to modify the list of assets allowed to be traded in. The function is missing a check to ensure that the Token asset data for a certain id already exists, which could lead to overwriting an existing state.
Consider adding a boolean check to ensure that the modification is intentional and that overwriting existing assets in the list does not happen due to an error.
Note: Consider using EnumerableSet from OpenZeppelin to manage and keep track of the asset list.
function modify_asset(uint256 _id, address _token_contract, string memory _name, bool _modify) external // onlyOwner
{
require(msg.sender == owner() || msg.sender == admin, “ICO: asset access restriction error”);
// We are setting up the price for TOKEN that will be accepted as payment during ICO.
require (_token_contract != address(0));
if (asset_index[_token_contract] !=0)
{
require(_modify == true, “Overwriting existing asset!”);
}
assets[_id].contract_address = _token_contract;
assets[_id].name = _name;
asset_index[_token_contract] = _id;
}
2.withdraw allows the owner to withdraw native CLO from the contract before users have claimed all the GNG tokens.
3.withdrawGNG functions can be used to withdraw GNG tokens in the contract during the vesting period before the user can claim, resulting in losses for users.
As per the developer notes, excess tokens must be burned after the ICO has ended (should not be withdrawn).
If this function intends to withdraw tokens that were not bought, it should be used a counter for tokens bought against the total deposited by the owner to get the number of tokens not bought.
4.The functions setup_vesting() and setup_contract() allow the contract owner to modify ICO contracts parameters which could lead to multiple incorrect computations leading to the user being unable to claim all GNG tokes purchased or additional tokens might be available for claim.
For instance, if end_timestamp is modified to extend the ICO period, the user has already made one or more claims and decides to purchase additional GNG tokens after the vesting period. The user would not be able to claim all the GNG tokens purchased.
In another instance where vesting_periods_total is modified user might be able to claim additional tokens if increased and lose the amount of GNG tokens if the value is decreased.
Consider adding checks to verify modifications to the ICO parameters by the functions setup_vesting and setup_contract() are possible only before the ICO has started.
5.delegateCall function can be used at any moment to change any storage slot of the contract making it unusable or do everything the owner wants.[/bsf-info-box][bsf-info-box icon_type=”custom” icon_img=”id^11679|url^https://callisto.network/wp-content/uploads/2023/04/star-svgrepo-com.png|caption^null|alt^null|title^star-svgrepo-com|description^null” img_width=”20″ icon_animation=”fadeIn” title=”Recommendation”]Since the owner has unlimited rights to do everything, the ownership must be transferred to a multi-sig contract.[/bsf-info-box]
2.4 Follow Good Coding Practice
Use camel case for all functions, parameters, and variables and snake case for constants.
2. Functions not used internally could be marked external
It’s a good coding practice to mark a function external when it’s not called within the contract but only from outside the contract.
3. Missing docstrings
Many functions in the code base lack documentation. This hinders reviewers’ understanding of the code’s intention, which is fundamental to correctly assess not only security, but also correctness. Additionally, docstrings improve readability and ease maintenance. They should explicitly explain the purpose or intention of the functions, the scenarios under which they can fail, the roles allowed to call them, the values returned, and the events emitted.
Consider thoroughly documenting all functions (and their parameters) that are part of the contracts’ public API. Functions implementing sensitive functionality, even if not public, should be clearly documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).
4. Missing test suite.
The contract is missing a test suite to validate and verify the behavior of the contract functionalities. Add tests are recommended to ensure the contract functions and behaves as expected.[/bsf-info-box]
3. Security Practices
4. Conclusion
The audited smart contract can be deployed. Only low severity issues were found during the audit.
Users should be aware of unlimited owner’s rights that can destroy the entire ICO process.
It is recommended to adhere to the security practices described in pt. 4 of this report to ensure the contract’s operability and prevent any issues that are not directly related to the code of this smart contract.
About Callisto Network
Founded by Dexaran, co-founder of Ethereum Classic, Callisto Network is a blockchain platform that prioritizes security. We’ve conducted over 330 smart contract audits across platforms like Ethereum, Ethereum Classic, and EOS. In addition to our audits, we’ve developed the ERC 223 token standard and CallistoNFT standard, enhancements over existing standards that address flaws and offer new capabilities, further establishing us as industry leaders in crypto-security.
