Farmland CertiK Preliminary Comments For Farmland-Finance

--- title: Farmland CertiK Preliminary Comments For Farmland-Finance tags: pre-report --- {%hackmd XdIXzOf5Ty2M3Uj1taFHIg %} <center> <img src="https://www.wing.vc/uploads/images/companies/certik-logo04.png" height="232" /> </center> Preliminary Comments Security Assessment November 13th, 2020 Preliminary Report For : Farmland-Finance Team @ Farmland By : Owan Li @ CertiK guilong.li@certik.org Bryan Xu @ CertiK buyun.xu@certik.org --- {%hackmd vb2ypisZSneY5y8y5ou-nw %} --- ## <img src="https://svgshare.com/i/Pp1.svg" width="40"/> Overview #### Project Summary <table> <tr> <td width="50%" valign="top">Project Name</td> <td width="50%" valign="top"><a href="https://github.com/farmland-finance/contracts">Farmland Finance Protocol</a></td> </tr> <tr> <td width="50%" valign="top">Description</td> <td width="50%" valign="top">an ERC20 token implementation with defi functions such as staking and swapping.</td> </tr> <tr> <td width="50%" valign="top">Platform</td> <td width="50%" valign="top">Ethereum; Solidity</td> </tr> <tr> <td width="50%" valign="top">Codebase</td> <td width="50%" valign="top"><a href="https://github.com/farmland-finance/contracts">GitHub Repository</a></td> </tr> <tr> <td width="50%" valign="top">Checksum</td> <td width="50%" valign="top"><a href="https://github.com/farmland-finance/contracts/commit/3a715b074990b6e8f7310f449a19655eca03b9ee"> 3a715b074990b6e8f7310f449a19655eca03b9ee</a> </td> </tr> </table> #### Audit Summary <table> <tr> <td width="50%" valign="top">Delivery Date</td> <td width="50%" valign="top">Nov. 13, 2020</td> </tr> <tr> <td width="50%" valign="top">Method of Audit</td> <td width="50%" valign="top">Static Analysis, Manual Review</td> </tr> <tr> <td width="50%" valign="top">Consultants Engaged</td> <td width="50%" valign="top">2</td> </tr> <tr> <td width="50%" valign="top">Timeline</td> <td width="50%" valign="top">Nov. 10, 2020 - Nov. 13, 2020</td> </tr> </table> #### Vulnerability Summary <table> <tr> <td width="50%" valign="top">Total Issues</td> <td width="50%" valign="top">9</td> </tr> <tr> <td width="50%" valign="top">Total Critical</td> <td width="50%" valign="top">0</td> </tr> <tr> <td width="50%" valign="top">Total Major</td> <td width="50%" valign="top">0</td> </tr> <tr> <td width="50%" valign="top">Total Minor</td> <td width="50%" valign="top">2</td> </tr> <tr> <td width="50%" valign="top">Total Informational</td> <td width="50%" valign="top">7</td> </tr> </table> --- ## <img src="https://svgshare.com/i/Pp1.svg" width="40"/> Executive Summary This report has been prepared for Farmland Finance to discover issues and vulnerabilities in the source code of their Smart Contract as well as any contract dependencies that were not part of an officially recognized library. A comprehensive examination has been performed, utilizing Dynamic Analysis, Static Analysis, and Manual Review techniques. The auditing process pays special attention to the following considerations: * Testing the smart contracts against both common and uncommon attack vectors. * Assessing the codebase to ensure compliance with current best practices and industry standards. * Ensuring contract logic meets the specifications and intentions of the client. * Cross referencing contract structure and implementation against similar smart contracts produced by industry leaders. * Thorough line-by-line manual review of the entire codebase by industry experts. --- ## <img src="https://svgshare.com/i/Pp1.svg" width="40"/> Findings | ID | Title | Type | Severity | |-:|-|-|-| | EXH-01| Unlocked Compiler Version Declaration | Language Specific | Informational | | EXH-02| Incorrect Naming Convention Utilization | Coding Style | Informational | | EXH-03| Proper Usage of "public" and "external" type | Optimization | Informational | | EXH-04| Check Zero Address | Optimization | Informational | | EXH-05| Missing Emit Events | Optimization | Minor | | EXH-06| Simplifying Existing Code | Optimization | Informational | | EXH-07| Check Zero Address | Optimization | Minor | | EXH-08| Redundant Codes | Coding Style | Informational | | EXH-09| Duplicated Codes | Coding Style | Informational | | EXH-10| Several Discussions | Optimization | Discussion | --- ### <a name="UNP-01" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-01: Unlocked Compiler Version Declaration | Type | Severity | Location | |-|-|-| | Language Sepcific | Informational | [Controller.sol, StrategyDForceDAI.sol, VaultDai.sol, StrategyCRV.sol,VaultRenbtc.sol, StrategyFortubeUsdc.sol,VaultUsdc.sol, StrategyDForceDAFortube.sol，VaultUsdt.sol](#) | #### Description: The compiler version utilized throughout the project uses the “^” prefix specifier, denoting that a compiler version which is greater than the version will be used to compile the contracts. Recommend the compiler version should be consistent throughout the codebase. #### Recommendation: It is a general practice to instead lock the compiler at a specific version rather than allow a range of compiler versions to be utilized to avoid compiler-specific bugs and be able to identify ones more easily. We recommend locking the compiler at the lowest possible version that supports all the capabilities wished by the codebase. This will ensure that the project utilizes a compiler version that has been in use for the longest time and as such is less likely to contain yet-undiscovered bugs. ### <a name="UNP-02" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-02: Incorrect Naming Convention Utilization | Type | Severity | Location | |-|-|-| | Coding Style | Informational | [StrategyDForceDAI.sol L124,L132,L149,L151,L153 Controller.sol L188 StrategyCRV.sol L171,L176 StrategyFortubeUsdc.sol L161,L164 StrategyFortube.sol L161,L164](#) | #### Description: Solidity defines a naming convention that should be followed. In general, parameters should use mixedCase, refer to: https://solidity.readthedocs.io/en/v0.5.16/style-guide.html#naming-conventions Functions other than constructors should use mixedCase. Examples: Contract like: `dRewards`,`dERC20` Variables should use UPPER_CASE_WITH_UNDERSCORES. Examples: Constant like:`d`,`df`,`unirouter`,`curveminter`,`curvedeposit` Parameter shoud use mixedCase. Examples: Parameter like:`_onesplit` #### Recommendation: The recommendations outlined here are intended to improve the readability, and thus they are not rules, but rather guidelines to try and help convey the most information through the names of things. ### <a name="UNP-03" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-03: Proper Usage of "public" and "external" type | Type | Severity | Location | |-|-|-| | Optimization | Informational | [VaultDai.sol L465,Controller.sol L244,L256, VaultRenbtc.sol L391,L412,L456, VaultUsdc.sol L465,VaultUsdt.sol L465](#) | #### Description: "public" functions that are never called by the contract should be declared "external" . When the inputs are arrays "external" functions are more efficient than "public" functions. Examples Functions like : `getPricePerFullShare()` ,`inCaseTokensGetStuck`,`yearn()`,`batchDepositbtc`,`balanceOfBtc` #### Recommendation: Consider using the "external" attribute for functions never called from the contract. ### <a name="UNP-04" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-04: Check Zero Address | Type | Severity | Location | |-|-|-| | Optimization | Informational | [Controller.sol L193, StrategyDForceDAI.sol L340, VaultDai.sol L384,VaultRenbtc.sol L318, StrategyCRV.sol L340, StrategyFortubeUsdc.sol L320, VaultUsdc.sol L384, StrategyFortube.sol L319, VaultUsdt.sol L384](#) | #### Description: The function `setGovernance` does not verified the address before usage. #### Recommendation: We recommend adding below code: `require(_governance != address(0), "Address zero is forbbiden");` ### <a name="UNP-05" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-05: Missing Emit Events | Type | Severity | Location | |-|-|-| | Optimization | Minor | [Controller.sol, StrategyDForceDAI.sol, VaultDai.sol, StrategyCRV.sol,VaultRenbtc.sol, StrategyFortubeUsdc.sol,VaultUsdc.sol, StrategyDForceDAFortube.sol，VaultUsdt.sol](#) | #### Description: Several sensitive actions are defined without event declarations. 1. Fucntion `setGovernance()` can change the governance of the contracts. 2. Fucntions `setMin()`, `available()` will decide the proportion of assest to be borrowed. 3. Function `inCaseTokensGetStuck()` can transfer any amount of assets to governance in case the controller has. 4. Fucntions `setFee()`,`setCallFee()`,`setWithdrawalFee()` will decide the important metrics. #### Recommendation: Consider adding events for sensitive actions, and emit it in the function. ### <a name="UNP-06" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-06: Simplifying Existing Code | Type | Severity | Location | |-|-|-| | Optimization | Informational | [Controller.sol L178,L183,L188,L193,L198,L205,L210,L239,L244 StrategyDForceDAI.sol L340,L345,L349,L353,L358,L364 VaultDai.sol L379,L384,L389,L393 StrategyCRV.sol L349,L345,L350,L354,L358,VaultRenbtc.sol L313,L318,L323,L327, StrategyFortubeUsdc.sol L320,L325,L329,L333,L338,L344 VaultUsdc.sol L379, L384,L389,L393 StrategyDForceDAFortube.sol L319,L324,L328,L332,L337,L348，VaultUsdt.sol L379, L384,L389,L393](#) | #### Description: Consider using a modifier to replace the below same codes existing in many functions: require(msg.sender == governance, "!governance"); Example: Functions `setFactory()`, `setSplit()`, `setOneSplit()`,`setGovernance()`,`setVault()`,`setConverter()`,`setStrategy()`,`withdrawAll()`,`inCaseTokensGetStuck()` in Controller.sol. Functions `setGovernance()`,`setController()`,`setFee()`,`setCallFee()`,`setWithdrawalFee()`,`setLastHarvestTimestampSec()` in StrategyDForceDAI.sol. Functions `setMin()`,`setGovernance()`,`setController()`,`setEarnLowerlimit()` in VaultDai.sol #### Recommendation: Consider changing it as following example: ```Solidity modifier onlyGovernance() { require(msg.sender == governance, "!governance"); _; } ``` ### <a name="UNP-07" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-07: Check Zero Address | Type | Severity | Location | |-|-|-| | Optimization | Minor | [Controller.sol L66](#) | #### Description: Function `earn()` is public, anyone can call it with any parameters. The address of `strategies[_token]` is not checked. If it's a zero address, assets in `Controller` will be transferred to zero address. #### Recommendation: We recommend adding below code: `require(strategies[_token] != address(0), "Address zero is forbbiden");` ### <a name="UNP-08" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-08: Redundant Codes | Type | Severity | Location | |-|-|-| | Coding Style | Informational | [Controller.sol L160,L178](#) | #### Description: Variable `factory` and function `setFactory()` are defined but never used. #### Recommendation: We recommend removing the redundant codes. ### <a name="UNP-09" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-09: Duplicated codes | Type | Severity | Location | |-|-|-| | Optimization | Informational | [VaultRenbtc.sol L371,L395](#) | #### Description: There are below duplicated codes in both `withdraw` and `_checkBtcWithdrawal` : ```Solidity uint r = (balance().mul(_shares)).div(totalSupply()); _burn(msg.sender, _shares); // Check balance uint b = token.balanceOf(address(this)); if (b < r) { uint _withdraw = r.sub(b); Controller(controller).withdraw(address(token), _withdraw); uint _after = token.balanceOf(address(this)); uint _diff = _after.sub(b); if (_diff < _withdraw) { r = b.add(_diff); } } ``` #### Recommendation: We recommend to move the duplicated codes out of the code block and they can be replaced by one. ### <a name="UNP-10" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-10: Several Discussions | Type | Severity | Location | |-|-|-| | Optimization | Discussion | [StrategyDForceDAI.sol L369,L281 VaultDai.sol L404 ](#) | #### Discussion: 1.Function `_checkHarvest()` in StrategyDForceDAI will check whether the current time in the scope of the farm season. If there is still earnings in the vault after the harvest time, what will happen? 2.Function `dosplit` in StrategyDForceDAI will transfer `_callfee` to msg.sender. Anyone can call the function `harvest` to get this `_callfee`. 3.Function `earn()` in VaultDai is public, so anyone can call this function no matter it meets the `earnLowerlimit` or not. In terms of aggregation, better to add some access limit and condition limit for the calling frequency of this function?