Vidma is pleased to present this audit report outlining our assessment of code, smart contracts, and other important audit insights and suggestions for management, developers, and users.
An audited contract is a vesting smart contract that can distribute any ERC20 tokens to a limited amount of addresses. The contract implements the AccesControl functionality with two roles, Default Admin and Vester. The Default Admin is actually an owner of the contract that has the ability to grant roles and set a signer, which is used to require that the creation of vesting pools will be made only through the Ferrum platform. The Vestor is allowed to create vesting pools, either Cliff or non-Cliff type.
The creation of the vesting pool and allocations of tokens itself happened in one function. It caused high gas usage and led to the limit of participants for that pool. The max amount of participants depends on the used network and gas price.
The auditor team found a bunch of issues, most of which did not significantly impact the contract’s ability to operate. After a second review there are a few unresolved issues that can affect the contract's ability to operate. The most important is a critical issue "The Lack of check for compliance of arrays". Since the contract does not allow edit allocations after the creation, it can lead to the incorrect distribution of tokens, between participants, eg. some participants will have the ability to claim more tokens that had to be allocated for them, and others - less. This issue can be handled by a specific check on FrontEnd but otherwise, it will be possible to reproduce.
After the third review, Vidma audit team confirm that all known issues are resolved and the contracts are secure and operational. The changes are reflected in this version of the report accordingly.
During the audit process, the Vidma team found several issues, including those with critical severity. A detailed summary and the current state are displayed in the table below.
After evaluating the findings in this report and the final state after fixes, the Vidma auditors can state that the contracts are fully operational and secure. Under the given circumstances, we set the following risk level:
To set the codebase quality mark, our auditors are evaluating the initial commit given for the scope of the audit and the last commit with the fixes. This approach helps us adequately and sequentially evaluate the quality of the code. Code style, optimization of the contracts, the number of issues, and risk level of the issues are all taken into consideration. The Vidma team has developed a transparent evaluation codebase quality system presented below.
Evaluating the initial commit and the last commit with the fixes, Vidma audit team set the following codebase quality mark.
Score
Based on the overall result of the audit and the state of the final reviewed commit, the Vidma audit team grants the following score:
In addition to manual check and static analysis, the auditing team has conducted a number of integrated autotests to ensure the given codebase has an adequate performance and security level. The test results and coverage can be found in the accompanying section of this audit report.
Please be aware that this audit does not certify the definitive reliability and security level of the contract. This document describes all vulnerabilities, typos, performance issues, and security issues found by the Vidma audit team.
If the code is still under development, we highly recommend running one more audit once the code is finalized.
With the mission of breaking down barriers to mass adoption, Ferrum empowers the industry by reducing friction and bringing startups and established networks closer together.
Within the scope of this audit, two independent auditors thoroughly investigated the given codebase and analyzed the overall security and performance of the smart contracts.
The audit was conducted from September 16, 2022 to September 23, 2022. The outcome is disclosed in this document.
The review of the fixes was conducted from October 3, 2022 to October 6, 2022.
The second review of fixes was done on October 13, 2022.
The third review of the fixes was done on October 24, 2022.
The final review of the latest commit and release tag was conducted on November 1, 2022.
The scope of work for the given audit consists of the following contracts:
The source code was taken from the following source:
https://github.com/ferrumnet/linear-release-engine/releases/tag/v.0.5.0
Initial commit submitted for the audit (Merge branch 'release/0.5.0'):
https://github.com/ferrumnet/linear-release-engine/commit/bb255308053b2f09e403237ea8c1b277496f9dc6
Final Release tag reviewed by Vidma auditors:
https://github.com/ferrumnet/linear-release-engine/releases/tag/v1.0.0
Final commit hash audited:
https://github.com/ferrumnet/linear-release-engine/commit/5825dc3485395f795cbeb8e18731fd63d8a4b11e
As a reference to the contracts logic, business concept, and the expected behavior of the codebase, the Ferrum Network team has provided the following documentation:
https://docs.ferrumnetwork.io/ferrum-network-ecosystem/v/iron-vest/
Vidma audit team uses the most sophisticated and contemporary methods and well-developed techniques to ensure contracts are free of vulnerabilities and security risks. The overall workflow consists of the following phases:
After the Audit kick-off, our security team conducts research on the contract’s logic and expected behavior of the audited contract.
Vidma auditors do a deep dive into your tech documentation with the aim of discovering all the behavior patterns of your codebase and analyzing the potential audit and testing scenarios.
At this point, the Vidma auditors are ready to kick off the process. We set the auditing strategies and methods and are prepared to conduct the first audit part.
During the manual phase of the audit, the Vidma team manually looks through the code in order to find any security issues, typos, or discrepancies with the logic of the contract. The initial commit as stated in the agreement is taken into consideration.
Static analysis tools are used to find any other vulnerabilities in smart contracts that were missed after a manual check.
An interim report with the list of issues.
Within the testing part, Vidma auditors run integration tests using the Truffle or Hardhat testing framework. The test coverage and the test results are inserted in the accompanying section of this audit report.
Second interim report with the list of new issues found during the testing part of the audit process.
For simplicity in reviewing the findings in this report, Vidma auditors classify the findings in accordance with the severity level of the issues. (from most critical to least critical).
All issues are marked as “Resolved” or “Unresolved”, depending on if they have been fixed by project team or not. The issues with “Not Relevant” status are left on the list of findings but are not eligible for the score points deduction.
The latest commit with the fixes reviewed by the auditors is indicated in the “Scope of Work” section of the report.
The Vidma team always provides a detailed description of the issues and recommendations on how to fix them.
Classification of found issues is graded according to 6 levels of severity described below:
Critical MC – 01 | Resolved
In the contract VestingHarvestContarct functions addVesting and addCliffVesting don't require the same length for arrays _usersAddresses and _userAlloc. It can bring Critical Issues, and wrong token allocations between beneficiaries, in the case when Vester made a mistake preparing data for addVesting.
Consider adding important checks to prevent human mistakes when adding new vesting.
Critical MC – 02 | Not relevant
The functions addCliffVesting of contract VestingHarvestContarct has a local variable nonClifVestingTime with wrong calculation (line 158).
Where:
Based on the variable name it should be the same as _vestingTime, since it's used in the function nonCliffClaimable for checking the claimable amount.
Consider using _vestingTime instead of nonClifVestingTime.
High MC – 01 | Resolved
In the contract VestingHarvestContarct there is an event AddVesting which has parameter releaseRate, it is supposed to be a value equal _totalVesting / Vesting duration, but it is always zero since changes in line 83 are not visible after the loop.
Consider removing such parameters or fixing them, eg. _totalVesting / Vesting duration.
High MC – 02 | Resolved
In the functions addVesting and addCliffVesting of contract VestingHarvestContarct absent check of zero address for the signer of the message.
Consider adding a zero address check for the signer address, it will make your contract safer.
High MC – 03 | Resolved
The contract VestingHarvestContarct doesn't fit the requirements for upgradable contracts.
When working with upgradeable contracts, there are a few minor caveats to keep in mind when writing your Solidity code. It’s worth mentioning that these restrictions have their roots in how the Ethereum VM works, and apply to all projects that work with upgradeable contracts.
Consider applying all of the requirements for upgradable contracts it will prevent implicit issues in your contract.
High MC – 04 | Resolved
The contract VestingHarvestContarct is vulnerable to a signature replay attack. There is no sure what you are signed on BE side, the functions that use signature don't reproduce signed messages and the same signature can be used for multiple transactions. This issue can be reproduced even on different networks with the same signature.
Consider using the best market standards for signing messages, such as using domain separators, deadlines, and message type hash.
High ТH – 01 | Resolved
In the addCliffVesting() function there is a requirement that checks whether the cliff percentage is not less than 0.1% but in the error message this value should be not less than 0.5%:
It could lead to incorrect calculations by cliff-/nonCliff- claiming.
Fix the condition or change the error message.
Move require from 118 line below line 120 rewardAmount = payMe (from, rewardAmount, rewardTokenAddress);
Medium MM – 01 | Resolved
There is an unchecked return value of the token transfer and transferFrom.
Consider implementing a check of the return value for the transfer and transferFrom functions using safe transfer functions.
From OpenZeppelin library SafeERC20 in the contract VestingHarvestContarct.
Medium MM – 02 | Resolved
The functions addCliffVesting of contract VestingHarvestContarct, absent check cliffPeriod > block.timestamp, which allows creating of vesting with already distributed tokens.
Consider adding important checks to prevent human mistakes when adding new vesting.
Medium MM – 03 | Resolved
In the function addVesting of the contract VestingHarvestContarct initializing of a state variable poolInfo (line 84) is located in the loop which means that it will be rewritten in each iteration of the loop.
Same for function addCliffVesting where state variable cliffPoolInfo is also initialized in the loop.
Consider moving the initialization of it outside the loop, it will bring saving gas during function execution.
Medium MM – 04 | Resolved
In the functions cliffClaimable and nonCliffClaimable of contract VestingHarvestContarct defined incompleted condition (line 194 and 223). In case when block.timestamp == cliffVestingTime, the function will return zero, instead of remainingToBeClaimableNonCliff.
Also valid in function claimable when block.timestamp == vestingTime.
Consider changing the condition to less equal instead of less.
Not resolved for claimable.
Medium TM – 01 | Not relevant
There is a releaseRate calculation in the VestingHarvestContarct in cliffClaimable() and nonCliffClaimable functions (line 205, 234) but it should be calculated before if-else branching because there will be returned zero values before unlocking start and/or full unlock.
Consider moving releaseRate calculation before if-else.
This issue is not relevant anymore since the releaseRate was removed in the latest version.
Medium TM – 02 | Resolved
In addVesting() and addCliffVesting() functions vesting pool data stores with pool id “i” and vestingPoolSize increases by 1, so after it event will be emitted with pool id “i + 1” which may confuse further usage.
Move increasing vestingPoolSize variable after event emitting.
Low ML – 01 | Resolved
In contracts VestingHarvestContarct, state variable vestingPoolSize (line 12) and cliff (line 90) are initialized by default type value that can be avoided.
Consider removing initialization by the default type value in the described cases.
Is not resolved for cliff[] in addVesting function.
Low ML – 02 | Resolved
The current version of solc in contract VestingHarvestContarct and interface Vesting are ^0.8.12 and it is better to lock the pragma to a specific version.
Contracts should be deployed with the same compiler version and flag that they
have been tested thoroughly. Locking the pragma helps to ensure that contracts
do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.
Consider locking pragma to a specific version.
Pragma was changed to ^0.8.17 but should be 0.8.17.
Low ML – 03 | Resolved
Using SafeMath is useless since from version 8.0.0+ underflow/overflow are checked under the hood.
Consider using regular mathematical operators instead of library SafeMath, it will save gas and make your code more clear and readable.
Low ML – 04 | Resolved
All events from the contract VestingHarvestContarct didn't specify any indexing parameters. The indexed parameters for logged events will allow you to search for these events using the indexed parameters as filters.
Consider indexing at least addresses of beneficiary and pool index.
Low ML – 05 | Resolved
In function addCliffVesting are unnecessary check _vestingTime > _cliffPeriod (line 165), since in this function we have check _vestingTime > _cliffVestingTime, and _cliffVestingTime > _cliffPeriod it will automatically mean that _vestingTime > _cliffPeriod.
Consider removing useless requirements it will bring saving gas during function execution.
Low ML – 06 | Resolved
In the function addVesting of the contract VestingHarvestContarct are unused local variable releaseRate (line 83). It is supposed to be used in the initialization of userInfo (line 85), but there this value is calculated again.
Consider removing unused local variables or reusing them if it is needed, it will bring saving gas during function execution.
Low ML – 07 | Resolved
In the function claimable() of the contract VestingHarvestContarct one of the return parameters is not initialized and always will return zero.
Consider removing useless parameters, it will bring saving gas during function execution.
Low ML – 08 | Resolved
In the function claim, claimCliff of the contract, VestingHarvestContarct are defined local variables secDiff and releaseRate, but they are not used anywhere.
Consider removing unused local variables, it will bring saving gas during function execution.
Low ML – 09 | Resolved
Functions claim, claimCliff, and claimNonCliff of the contract VestingHarvestContarct, change the state variables userInfo, UserClifInfo, nonCliffClaimable, by rewriting the whole object instead of changing only some parameters.
Consider changing only parameters in the state variables, it will bring saving gas during function execution.
Low ML – 10 | Resolved
The functions claim of contract VestingHarvestContarct makes a check on zero claimable amount after transferring it to the user.
Consider using Checks Effects Interactions patterns for your functions, it will bring your contract more secure and save gas during function execution.
Low ML – 11 | Resolved
Functions claim, claimCliff, and claimNonCliff of the contract VestingHarvestContarct, have the repeated calculation of value for parameter remaining of events Claim, CliffClaim, and NonCliffClaim:
Functions addCliffVesting of the contract VestingHarvestContarct 6 times calculate a value for cliffAlloc and 2 times for remainingToBeClaimableNonCliff:
Functions cliffClaimable of the contract VestingHarvestContarct 2 times calculate a value for secondDiff:
Functions nonCliffClaimable of the contract VestingHarvestContarct 2 times calculate a value for secondDiff:
Consider creating local variables to not repeat calculations of repeated values, it will bring saving gas during function execution.
Low ML – 12 | Resolved
The functions addVesting, claim, addCliffVesting, claimCliff, and claimNonCliff of contract VestingHarvestContarct can be marked as external, it will save gas during function execution.
Consider changing visibility to external if possible, it will bring saving gas during function execution.
Low ML – 13 | Resolved
In the functions cliffClaimable and nonCliffClaimable of contract VestingHarvestContarct defined unnecessary condition(line 198 and 227). In case when condition `cliffVestingTime < block.timestamp` return false it automatically means that cliffVestingTime > block.timestamp.
Consider deleting unnecessary conditions, it will save gas during function execution.
Low ML – 14 | Resolved
In the function VerifyMessage of contract VestingHarvestContarct there is shadowing of the state variable signer.
Consider renaming the local variables that shadow another component.
Low ML – 15 | Resolved
In the file ironVest.sol imported draft-EIP712.sol from the openzeppelin library but it isn’t used in the contract VestingHarvestContarct .
Consider deleting, unused imports, it will save gas during deployment.
Low ML – 16 | Resolved
Usually, a lot of people accidentally send tokens to a contract, and if the contract doesn't have the functionality to withdraw them they are locked on the contract forever.
Consider adding functionality for emergency withdrawal of not allocated tokens from the contract, it will allow withdrawing tokens that were sent to the contract by mistake.
Resolved but owner has possibility to withdraw all tokens that could be allocated to users. Maybe that funds should be limited to withdraw?
Low TL – 01 | Resolved
There is an unused internal function poolInfoArrays in the contract VestingHarvestContarct (line 104). Since internal functions can only be used internally or by derived contracts, there is no need for this function.
Consider removing unused functionality, it will save gas during deployment.
Low TL – 02 | Resolved
Consider adding a gasleft() tracking that if it returns true it will break the execution so the ‘out of gas’ error message will be avoided.
Consider adding a gasleft() tracking that if it returns true it will break the execution so the ‘out of gas’ error message will be avoided.
Low TL – 03 | Resolved
There is an unnecessary checking in the VestingHarvestContarct in claim() function (line 104). startDate value is equal to timestamp value when vesting was created, a user will claim this vesting this requirement will be never reverted because the current timestamp will be always greater than the start date.
Consider removing this requirement.
Low TL – 04 | Not relevant
Add a calculation that will emit a value of how much time has passed between user withdrawals.
This issue is not relevant anymore since secondDiff was removed in latest version.
Low TL – 05 | Resolved
In the poolInformation() view function there is isCliff returned value but it is not initialized. It always returns the default false value.
Initialize isCliff variable before if-else branching and use it like condition instead of cliff[_poolId] repeat:
Resolved but added declaration that shadows existing variable. Remove `bool` type at line 603.
Low TL – 06 | Resolved
In the initialize() function there is requirement that checks whether the contract has not been initialized before (!initialized) but it was already checked in the initializer modifier.
Remove useless requirement. Also initialized = true; should be removed from the end of the constructor body.
Low TL – 07 | Resolved
In the claimable(), cliffClaimable() and nonCliffClaimable() functions there are the last branches that rewriting variables values from zero to zero:
Remove these branches to the gas economy, zero values were setted by default during variable declarations.
Low TL – 08 | Resolved
In function addCliffVesting are unnecessary check _vestingEndTime > block.timestamp (line 361), since in this function we have checks block.timestamp < _cliffPeriodEndTime < _cliffVestingEndTime < _vestingEndTime it will automatically mean that _vestingEndTime > block.timestamp.
Consider removing useless requirements it will bring saving gas during function execution.
Low TL – 09 | Resolved
Visibility of the initialize() and setSigner() functions could be optimized from public to external. It has no calls inside the contract code.
Consider changing visibility of described functions.
Is not resolved for poolInformation().
Low TL – 10 | Resolved
The default visibility for poolInfo, cliffPoolInfo mappings are internal. Other possible visibility settings are public and private.
Mark visibility for two mappings. If poolInfo should be internal, mark it internal, rename it with underscore to mapping(uint256 => PoolInfo) internal _poolInfo and move to the correct position (according to style guide). It is also relevant to private visibility.
If it is a public variable, just adding public will suffice.
Low TL – 11 | Resolved
In claimable() function the last condition of if-else branching could be removed because it will never return false value. It will save some gas.
Remove poolInfo[_poolId].vestingEndTime >= block.timestamp checking.
Informational MI – 01 | Resolved
The layout contract elements in VestingHarvestContarct are not logically grouped.
The contract elements should be grouped and ordered in the following way:
Inside each contract, library or interface, use the following order:
Ordering helps readers to navigate the code and find the elements more quickly.
Consider changing the order of layout according to solidity documentation: Order of Layout.
Informational MI – 02 | Resolved
The functions in contract VestingHarvestContarct are not grouped according to their visibility and order.
Functions should be grouped according to their visibility and ordered in the following way:
Ordering helps readers navigate across the code, identify which functions they can call and find the constructor and fallback definitions easier.
Consider changing functions order according to solidity documentation: Order of Functions.
Informational MI – 03 | Resolved
The code of the interface Vesting and the contract VestingHarvestContarct are not formatted, different spaces and tabs, and the chaotically located parts of code make your code confused and hard to navigate.
Consider formatting your code according to Solidity Code Style it will make your code more clear and readable.
Informational MI – 04 | Resolved
In the interface Vesting and in the contract VestingHarvestContarct absent any description of the functions and variables.
Solidity contracts can use a special form of comments to provide rich documentation for functions, return variables, and more.
Consider adding NatSpec documentation to your contracts it will make your code more clear and readable.
There were added simple comments, not natspec.
Informational MI – 05 | Not relevant
Functions from library SafeMath are not attached to type uint256 through directive using.
Consider using directive using to attach functions from library SafeMath, it will make your code clear and readable.
SafeMath was removed from the new version, so the issue is not relevant anymore.
Informational MI – 06 | Resolved
There is a few typos in parameter and variable names.
In interface Vesting:
In the contract VestingHarvestContarct:
Consider fixing all these typos in the contract parameters and variables.
Informational MI – 07 | Resolved
According to Solidity Style Guide, internal functions should begging from the underscore. In contract VestingHarvestContarct there is an internal function pastpoolInfoArrays which didn’t fit the naming convention.
Consider using directive using to attach functions from library SafeMath, it will make your code clear and readable.
Consider following the Solidity Style Guide, and naming conventions.
Informational MI – 08 | Not relevant
According to Solidity Style Guide, state and local variables should be in mixedCase style. In contract VestingHarvestContarct there is a local variable Info (line 100) in function poolInfoArrays which didn’t fit naming convention (initial character in uppercase).
Consider following the Solidity Style Guide, and naming conventions.
This issue is not relevant anymore since poolInfoArrays() function was removed.
Informational MI – 09 | Not relevant
Since vestingPoolSize is used as incrementing key for both types of vesting there will never exist vesting with the same index.
Eg. Two times add Vesting and one time add CliffVesting, poolInfo[0] - first Vesting, poolInfo[1] - second Vesting, cliffPoolInfo[0] - didn't exist, cliffPoolInfo[2] - first CliffVesting.
Make sure it is expected behavior.
Informational MI – 10 | Resolved
Since in the contract VestingHarvestContarct precision for cliffPercentage is not defined, the minimal cliff percentage will be 1%.
Consider adding precision in case you need a minimal cliff percentage of less than 1%.
This issue is resolved. However, the fix of it caused the new issue: % should be more than 0.5% but we have expression % > 0.1%. This issue is added to this report.
Informational MI – 11 | Not relevant
In the contract VestingHarvestContarct there are functions and variables that realize the functionality of additional distributing, a kind of "boost" for the main allocation, but most of them consist of cliff in the name. It makes your code confused since in classic Vesting cliff means that in that period tokens are not distributed, it kind of a pause.
Consider replacing cliff with the word "boost" etc., for this additional distribution period.
Informational TI - 01 | Resolved
The prefix variable is unused in the messageHash() internal view function.
Remove unused variable.
Informational TI - 02 | Resolved
The MIT License in the start of code is invisible for solidity compiled. It is ignored and marked like ‘No license’.
Change
line to
Informational TI - 03 | Resolved
In addCliffVesting() function there is a requirement that checks cliff percentage and has the following error message: "Percentage :Percentage Should Be less Than 50%" that style differs from styles in other error messages.
Add space after colon and remove extra space before percentage value: "Percentage : Percentage Should Be less Than 50%".
To verify the security of contracts and their performance, a number of integration tests were carried out using the Truffle testing framework.
Vidma Coverage – 95.2%
Industry Standard – 95%
It is important to note that Vidma auditors do not modify, edit or add tests to the existing tests provided in the Ferrum Network repository. We write totally separate tests with code coverage of a minimum of 95% to meet the industry standards.
We are delighted to have a chance to work with the Ferrum Network team and contribute to your company's success by reviewing and certifying the security of your smart contracts.
The statements made in this document should be interpreted neither as investment or legal advice, nor should its authors be held accountable for decisions made based on this document.