Feyorra Smart Contract Audit

Introduction

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

Summary

The contract implements an ERC20 token that allows the users to stake the tokens, earning interest while tokens are locked.

The project consist in a single file with the following characteristics:

File	Sha256
token.sol	cb9760235e8f5e3cec51d44b763a88e965ace5175073e6f4e539d8a4b3036326

The audit was update with a new version of the contract:

File	Sha256
feyorra.sol	35521f20347de3a0a9820377e9d8b09867607def4ee76f528c87c4562c8b9a73

Graph representing audited contracts structure

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 have been found.

Medium severity

No issues have been found.

Minor severity

Unaccounted penalties when closing a stake

If a stake is closed before 45 days have elapsed since it was created a penalty fee will be charged from the stake. This penalty fee is not considered when updating the total supply, only the rewards are added. This inconsistency will cause the total supply to be greater than available tokens.

We recommend subtracting penalties from the total amount to reflect the correct scenario.

_totalSupply = SafeMath.safeAdd(_totalSupply, rewardAmount);
 // -- CoinFabrik:
 // -- Subtract penalties from total supply
 _totalSupply = SafeMath.safeSub(_totalSupply, penaltyAmount);

Fixes were applied to contract with hash 35521f2..9a73.

Inconsistent total supply update when tokens are burned

In the functions transfer, transferFrom and multiTransfer one percent of transferred tokens are burned as a fee. A Transfer event is generated but the total supply is not updated to reflect this operation.

On the other hand, the function burn updates the total supply when tokens are explicitly burned.

In our analysis we did not find an exploit for this discrepancy but we suggest fixing it by always subtracting the burned amount from the total supply.

uint256 amountToBurn = _percentCalculator(_amount, 1);
 …
 emit Transfer(msg.sender, address(0), amountToBurn);

Fixes were applied to contract with hash 35521f2..9a73.

Missing Transfer events at newStake and closeStake

Staking tokens in the function newStake will decrement the user’s balance but no Transfer event is emitted. The missing event might cause a wallet that depends on them not to report the balance change.

uint256 amountToBurn = _percentCalculator(_amount, 1);
 …
 emit Transfer(msg.sender, address(0), amountToBurn);
 // -- CoinFabrik:
 // -- Subtract burned amount from total supply
 _totalSupply = SafeMath.safeSub(_totalSupply, amountToBurn);

Similarly, when closing a stake the user’s balance changes without generating an event notification.

userBalances[msg.sender] = SafeMath.safeAdd(userBalances[msg.sender], totalReturnAmount);
 // -- CoinFabrik:
 // -- Generate Transfer event
 emit Transfer(address(0), msg.sender, totalReturnAmount);

Fixes were applied to contract with hash 35521f2..9a73.

Enhancements

Use named constant instead of hardcoded values

It is a good engineering practice to use named constants instead of raw values making the contract easier to read and understand. We suggest naming only the more important constants since naming everything will make the code harder to read.

For example in the function newStake:

// -- CoinFabrik:
 // -- Add constant at contract scope
 uint constant MINIMUM_STAKE = 100000000000000000000;
 function newStake(uint256 _amount) external returns (bool status) {
 // -- CoinFabrik: // -- Replace number by consta, add error message require(_amount >= MINIMUM_STAKE, "Minimum stake required");

Fixes were applied to contract with hash 35521f2..9a73.

Missing error message in require statements

It is recommended to always include a message with require statements indicating the cause of failure. 

// -- CoinFabrik:
 // -- Add error message
 require(difference > 3, "Cannot close stake before three days");

Fixes were applied to contract with hash 35521f2..9a73.

Use of deprecated assert statement

In the library SafeMath every function uses the deprecated assert statement. After the Byzantium fork it is recommended to use require because it consumes less gas when its condition is false.

function safeMul(uint256 a, uint256 b) internal pure returns (uint256) {
     uint256 c = a * b;
 // -- CoinFabrik: // -- Replace assert by require, add error message require(a == 0 || c / a == b, "SafeMul: Overflow"); return c; }

Fixes were applied to contract with hash 35521f2..9a73.

Observations

Redundant checks

• In function burn at line 71 in file token.sol the require statement is unnecessary, since its condition is always true and the side effect is computed in another statement in line 75.

require(SafeMath.safeSub(userBalances[msg.sender], _amount) >= 0);

• In function _transferCheck at line 118 in file token.sol the require statement is redundant since the condition is always true and side effects by safeSub are already computed in line 117.

require(SafeMath.safeSub(userBalances[_sender], _amount) >= 0);

• In function transferFrom at line 253 the require is redundant since safeSub returns a uint256 which is always greater than zero.

 require(SafeMath.safeSub(userAllowances[_owner][msg.sender], _amount) >= 0); 

• In function approve the require at line 299 is redundant since _amount is always a positive value.

require(_amount >= 0);

Similar name used for different purposes

The name stakingId is used in two places with different purposes. In the function newStake it is a global unique number that identifies the stake being created.

 stakeList[msg.sender].push(
   StakeElement(
       stakingId,     // -- CoinFabrik: stake's ID
       _amount,
       0,
       now,
       false
   )); 

In the functions closeStake and getStaking, there is a variable with a similar name _stakingId representing the index of the stake.

function getStaking(address _address, uint256 _stakingId) external view returns (uint256 _stakedAmount, uint256 _returnAmount, uint256 _stakedAt, bool _released) {
// -- CoinFabrik:  // -- _stakingId is not the stake's ID  // -- but an index in the user's array StakeElement storage _stakeElement = stakeList[_address][_stakingId];

This observation no longer applies to the contract with hash 35521f2..9a73.

The contract was refactored and does not use an array to store StakeElements, it uses a mapping instead. 

Missing use of SafeMath in multiTransfer

In function multiTransfer individual amounts are validated with _transferCheck and the total is verified at the end. However, the intermediate accumulation steps are not verified with SafeMath.add.

An explicit exploit is unlikely since the token initial supply is 10²⁷ , and to cause an arithmetic overflow it will require more than 10⁵⁰ transfers, which will cause an out of gas with current mainnet gas limit of 10⁷ gas.

We recommend using SafeMath to accumulate the total amount sent.

// -- CoinFabrik:
 // -- Use SafeMath to accumulate transferred amounts
 totalSent = SafeMath.add(totalSent, _values[i]);

Conclusion

The contract has some problems that should be easy to fix. It does not have any comments nor documentation available. We recommend documenting the expected behavior of the main functions. 

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