iCherry Smart Contract Audit

Reading Time: 5 minutes

CoinFabrik specializes in auditing and developing Dapps.

  •  
  •  
  •  
  • 1
  •  
  •  
    1
    Share

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.solf7b0f29ab655d27bec38a6c889c11f75
MemberManager.sol60ab2aeeda764e39f969f2e77ad404f8

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:

SEVERITYEXPLOITABLEROADBLOCKTO BE FIXED
CriticalYesYesImmediately
MediumIn the near futureYesAs soon as possible
MinorUnlikelyNoEventually
EnhancementNoNoEventually

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: 

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:

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:

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

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:

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.


  •  
  •  
  •  
  • 1
  •  
  •  
    1
    Share