Gems & Goblins Token smart contract security audit, conducted by the Callisto Network Security Department in February 2023.
Gems & Goblins ICO Security Audit Report
Are Your Funds Safe?
Summary
- GNG ICO smart contract security audit report performed by Callisto Security Audit Department.
1. In scope
-
Commit
f50556c44a6971462d122b0494311398556f3502
2. Findings
In total, 1 issue were reported, including:
-
0 high severity issues.
-
0 medium severity issues.
-
1 low severity issue.
In total, 7 notes were reported, including:
-
2 notes.
-
5 owner privileges.
2.1 The minimal deposit value is set in 1 token
Severity: Note.
Description:
Since users can pay with different tokens (with different prices), the requirement to deposit at least 1 token puts them in an unequal position. Because users who will pay with ETH should deposit a minimum 1 ETH (about $1600), but users who pay with CLO can buy just for $0.003.
This requirement has no logical sense and should be removed because we already have a limitation of minimum purchase amount.
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
Recommendation:
Remove the requirement to deposit at least 1 token/coin.
2.2 Use safeTransfer functions instead of ERC20/223
Severity: Low
Description:
Since GNG ICO contract transfers different third-party tokens, not all of them may strictly follow the ERC20 standard in 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).
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
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');
}
2.3 Owner privileges
Severity: Owner privileges
Description:
- The function
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;
}
-
withdraw
allows the owner to withdraw native CLO from the contract before users have claimed all the GNG tokens. -
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. -
The functions
setup_vesting()
andsetup_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 and 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.
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.
Recommendation:
Since the owner has unlimited rights to do everything, the ownership must be transferred to a multi-sig contract.
2.4 Follow good coding practice
Severity: Note
Description:
1. Functions, parameters, and variables in snake case.
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.
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.
Appendix
Smart Contract Audits by Callisto Network.
Miscellaneous
Our Most Popular Audit Reports.
Trust the Blockchain, Audit the Smart Contracts.
Follow Callisto’s Security Department on Twitter to get our latest news and updates!
[/fusion_text][/fusion_builder_column][/fusion_builder_row][/fusion_builder_container]