Skip links
Request an Audit
CoinFabrik

iCherry Smart Contract Audit

Author
Published on:
Published in: Blog

Introduction

CoinFabrik was asked to audit the contracts for the iCherry.Finance project. First we will provide a summary of our discoveries and then we will show the details of our findings.

Contracts

The audited contracts are:

  • MemberManager.sol: Data structure for managing members, which is already deployed on the tron network at address TRCKRkrUm9L2hz8DwgWoE2hpGNrzNez1cY
  • USDT.sol: The main contract is used for managing the rewards of each member.

The audited contracts and their md5sum are:

USDT.sol f7b0f29ab655d27bec38a6c889c11f75
MemberManager.sol 60ab2aeeda764e39f969f2e77ad404f8

Analyses

The following analyses were performed:

  • Misuse of the different call methods
  • Integer overflow errors
  • Division by zero errors
  • Outdated version of Solidity compiler
  • Front running attacks
  • Reentrancy attacks
  • Misuse of block timestamps
  • Softlock denial of service attacks
  • Functions with excessive gas cost
  • Missing or misused function qualifiers
  • Needlessly complex code and contract interactions
  • Poor or nonexistent error handling
  • Failure to use a withdrawal pattern
  • Insufficient validation of the input parameters
  • Incorrect handling of cryptographic signatures

Detailed findings

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 can 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.
  • Enhancement: These kinds of findings do not represent a security risk. They are best practices that we suggest to implement.

This classification is summarized in the following table:

SEVERITY EXPLOITABLE ROADBLOCK TO BE FIXED
Critical Yes Yes Immediately
Medium In the near future Yes As soon as possible
Minor Unlikely No Eventually
Enhancement No No Eventually

Issues Found by Severity

Critical severity

No issues of this type were found.

Medium severity

No issues of this type were found.

Minor severity

Inadequate check of return values

In the LPTokenWrapper contract, there are calls made to the USDT contract without checking the return value. According to ERC20, which TRC20 is fully compatible with, contracts must check return values for this. Using the following snippet this could be easily fixed: 

require(usdt.transfer(msg.sender, amount)), "Unable to transfer");

Not implementing this could make the transfers fail silently. Anyways, the stake and withdrawal codes will be executed successfully.

Enhancements

USDT interface is not defined properly

In the USDT.sol file, the interface for the USDT contract is defined as:

interface USDT {
	function totalSupply() external view returns (uint256);
	function transfer(address _to, uint256 _value) external;
	function transferFrom(address _from, address _to, uint256 _value) external;
	function balanceOf(address who) external view returns (uint256);
	function approve(address _spender, uint256 _value) external;
	function allowance(address _owner, address _spender) external view returns (uint256 remaining);
}

However, the USDT contract in the tron network has a different interface. Considering the pragma for the file (^0.5.0), it should be noted that the contracts compiled with newer solc versions will have returndatasize and returndatacopy opcodes enabled by default. For more information see here.  When using TRC20, the compliant contracts respond to the following interface:

contract TRC20 {
    function totalSupply() constant returns (uint theTotalSupply);
    function balanceOf(address _owner) constant returns (uint balance);
    function transfer(address _to, uint _value) returns (bool success);
    function transferFrom(address _from, address _to, uint _value) returns (bool success);
    function approve(address _spender, uint _value) returns (bool success);
    function allowance(address _owner, address _spender) constant returns (uint remaining);
    event Transfer(address indexed _from, address indexed _to, uint _value);
    event Approval(address indexed _owner, address indexed _spender, uint _value);
}

Also, the correct interface for the USDT contract is the following:

interface USDT {
   function transfer(address _to, uint _value) returns (bool);
   function transferFrom(address _from, address _to, uint _value) returns (bool);
   function balanceOf(address who) constant returns (uint);
   function oldBalanceOf(address who) constant returns (uint);
   function approve(address _spender, uint _value) returns (bool);
   function allowance(address _owner, address _spender) constant returns (uint remaining);
   function totalSupply() public constant returns (uint);
   //...
   event DestroyedBlackFunds(address indexed _blackListedUser, uint _balance);
   event Issue(uint amount);
   event Redeem(uint amount);
   event Deprecate(address newAddress);
}

Since the USDT token contract has a correct TRC20 interface implementation, this does not make it impossible for the contracts to interact, but this should taken into account, particularly because the contracts do not pay heed to the return values of the functions (in the LPTokenWrapper contract, for example, there are calls to usdt.transfer() and usdt.transferFrom() without checking if the transferences are successful).

Gas usage for functions

Gas usage could be optimized by declaring many of the functions in MemberManager.sol as external:

contract MemberManager is Basic {

        //...
	function getTotalMember() public view returns (uint256)

	function infoMember(address _member) external view returns (address parent, address[] memory refs);
	function addMember(address _member, address _parent) external;

	function getRef(address _member, uint256 _index) external view returns (address);
	function getParent(address _member) external view returns (address _parent);

	function getRefLength(address _member)external view returns (uint256 _length);

	function getParentTree(address _member, uint256 _deep) external view returns (address[] memory);
        //...
}

Conclusion

We found the contracts to be simple and straightforward. There were a few difficulties with following the standards and a small consideration with the gas usage. To sum up, the code is well-written and no important issues were found.

Disclaimer: This audit report is not a security warranty, investment advice, or an approval of the ICherry Finance since CoinFabrik has not reviewed its platform. Moreover, it does not provide a smart contract code faultlessness guarantee.

Appendix: Automated tool analysis

Mythril

  • Integer Arithmetic Bugs

These are in the context of a for that does i++ every cycle. Taking this into account, it can be seen that these do not generate issues.

  • An assertion violation was triggered.

This is triggered in the case of possible access to an array that is out of bounds, because the contract can be called with any index. Since in practice the index is not greater than 3 in one case (_index) and in another the set array is not accessed in the code. Therefore these are not considered issues.

Slither

MemberManager.sol:

  • Pragma version>=0.4.25<0.6.0 (MemberManager.sol#1) allows old versions
  • addMod(address), removeMod(address), changeOwner(address), getTotalMember() infoMember(address), addMember(address,address), getRef(address,uint256), getParent(address), getRefLength(address), getParentTree(address,uint256), isParent(address,address) should be declared external

This optimization would lead to reduced gas usage

USDT.sol:

  • CherryRewardsUSDT.notifyRewardAmount performs a multiplication on the result of a division

This is a mistake on the part of Slither, since it is taking the two branches of an if as the same block of code. In fact, in the false branch the correct order for avoiding the result flooring to 0 is being used.

  • USDT has incorrect ERC20 function interface

While these contracts will be deployed on the Tron network, which has the TRC-20 standard, slither is advising the contracts to follow the ERC20 standard. This is done so any usage of return values is taken correctly. However, the contracts audited don’t use the return values for USDT functions, but it is still advised to correct these warnings.

  • Reentrancy in CherryRewardsUSDT

In these cases, the calls are to known contracts, supposedly, but we could not audit them: USDT and Cherry, of which we do have the interface. Presumably these contracts are known by the team and will be set up properly when deploying the contract.

Slither also detected calls to the member manager contract such as in the stake function, but such is a known contract

  • Dangerous comparisons in CherryRewardsUSDT

Slither detects that these contracts use timestamps for comparisons. However, since the durations being considered are in the order of a week there is not much of a malicious miner.

  • Address.isContract(address) and Address._functionCallWithValue(address,bytes,uint256,string) use assembly

Slither detects the library Address as using assembly. While this is true, it does not necessarily mean there is a vulnerability, and further analysis lets us discard this.

  • Pragma version^0.5.0 (USDT.sol#1) allows old versions
  • Low level calls in Address.sendValue(address,uint256) (USDT.sol#33-45) and Address._functionCallWithValue(address,bytes,uint256,string) (USDT.sol#89-117)

Slither detects low level calls and outputs it as an interesting result. It doesn’t necessarily mean there are vulnerabilities

  • Parameters _rewardDistribution (USDT.sol#380) and _user (USDT.sol#481) are not in mixedCase

Slither detects problems with style, but further analysis lets us discard this, since it is just that there is an underscore as first character.

  • transferOwnership(address), getStats(address) should be declared external

Slither detects these functions could be declared external which would lead to less gas usage, as they are not used inside their own contracts. This is correct and should be considered.

You may also like