![](/style/images/good.png)
![](/style/images/bad.png)
Feyorra Smart Contract Audit
source link: https://blog.coinfabrik.com/feyorra-smart-contract-audit/
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
- 1
- Share
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:
FileSha256token.solcb9760235e8f5e3cec51d44b763a88e965ace5175073e6f4e539d8a4b3036326
The audit was update with a new version of the contract:
FileSha256feyorra.sol35521f20347de3a0a9820377e9d8b09867607def4ee76f528c87c4562c8b9a73
Graph representing audited contracts structure
Note: Solid lines represent inheritance and dashed lines represent a library dependency.
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 FIXEDCriticalYesYesImmediatelyMediumIn the near futureYesAs soon as possibleMinorUnlikelyNoEventuallyEnhancementNoNoEventually
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.
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.
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.
Similarly, when closing a stake the user’s balance changes without generating an event notification.
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:
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.
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.
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.
- 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.
- In function transferFrom at line 253 the require is redundant since safeSub returns a uint256 which is always greater than zero.
- In function approve the require at line 299 is redundant since _amount is always a positive value.
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.
In the functions closeStake and getStaking, there is a variable with a similar name _stakingId representing the index of the stake.
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 1027 , and to cause an arithmetic overflow it will require more than 1050 transfers, which will cause an out of gas with current mainnet gas limit of 107 gas.
We recommend using SafeMath to accumulate the total amount sent.
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.
- 1
- Share
Related Posts
-
Stasis Token Smart Contract Audit
Coinfabrik has been hired to audit the smart contracts for Stasis sale, the Stable Euro…
-
DMToken Token Sale Smart Contract Audit
Coinfabrik was hired to audit the contract in terms of its security. First of all,…
-
EtherParty Smart Contract Security Audit
Coinfabrik team has been hired to audit Etherparty smart contracts. Firstly, we will provide a…
-
Smart Contract Auditing News
Every month several important smart contract audits are performed by blockchain security companies like us.…
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK