CertiK Preliminary Comments For Deerfi

--- title: CertiK Preliminary Comments For Deerfi 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 21th, 2020 Preliminary Report For : Deerfi Team @ Deerfi 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/Deerfi/deerfi-protocol">Deerfi Protocol</a></td> </tr> <tr> <td width="50%" valign="top">Description</td> <td width="50%" valign="top">a decentralized money market that offers users access to permissionless lending and borrowing</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/Deerfi/deerfi-protocol">GitHub Repository</a></td> </tr> <tr> <td width="50%" valign="top">Commit</td> <td width="50%" valign="top"><a href="https://github.com/farmland-finance/contracts/commit/3a715b074990b6e8f7310f449a19655eca03b9ee"> 60f2284cf11bd5eb9df8205aed37e3344b166f0e</a> <a href="https://github.com/farmland-finance/contracts/commit/cb4dd660b54280ee9f3670c99ce281fc41ed9c1f"> cb4dd660b54280ee9f3670c99ce281fc41ed9c1f</a> <a href="https://github.com/farmland-finance/contracts/commit/dcb84ff6302acfc8e1666d0faf2f826d704d0e6e"> dcb84ff6302acfc8e1666d0faf2f826d704d0e6e</a> </td> </tr> </table> #### Audit Summary <table> <tr> <td width="50%" valign="top">Delivery Date</td> <td width="50%" valign="top">Nov. 21, 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. 18, 2020 - Nov. 20, 2020</td> </tr> </table> #### Vulnerability Summary <table> <tr> <td width="50%" valign="top">Total Issues</td> <td width="50%" valign="top">7</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">0</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 Deerfi Protocol 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. The Deerfi Protocol is a fork of Compound Finance that is powered by Chainlink oracles. The audited commits are<a href="https://github.com/farmland-finance/contracts/commit/3a715b074990b6e8f7310f449a19655eca03b9ee"> 60f2284cf11bd5eb9df8205aed37e3344b166f0e,</a><a href="https://github.com/farmland-finance/contracts/commit/cb4dd660b54280ee9f3670c99ce281fc41ed9c1f"> cb4dd660b54280ee9f3670c99ce281fc41ed9c1f,</a><a href="https://github.com/farmland-finance/contracts/commit/dcb84ff6302acfc8e1666d0faf2f826d704d0e6e"> dcb84ff6302acfc8e1666d0faf2f826d704d0e6e</a> and the files included in the scope were [ChainlinkPriceOracleProxy.sol, IUniswapV2Pair.sol, Math.sol](#). Compound Finance is audited by <a href="https://blog.openzeppelin.com/compound-audit/">OpenZeppelin</a>. --- ## <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| Gas Consumption | Optimization | Informational | | EXH-05| State Variable Shadowing from Abstract Contracts | Optimization | Informational | | EXH-06| Events Should Add Indexed Keyword | Optimization | Informational | | EXH-07| Missing Emit Events | Optimization | Minor | --- ### <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 | [ChainlinkPriceOracleProxy.sol, IUniswapV2Pair.sol](#) | #### Description: Different versions of Solidity is used in the project. The compiler version utilized in most files uses the “^0.5.16” prefix specifier, but “>=0.5.0” is used in `IUniswapV2Pair.sol`. 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 | [ChainlinkPriceOracleProxy.sol, IUniswapV2Pair.sol](#) | #### 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 Constants should use UPPER_CASE_WITH_UNDERSCORES. Examples: Constants like:`isPriceOracle` Functions shoud use mixedCase. Examples: Functions like:`DOMAIN_SEPARATOR`,`PERMIT_TYPEHASH`,`MINIMUM_LIQUIDITY` Inside each contract, library or interface, use the following order: Type declarations State variables Events Functions Events definition should be in front of function definitions: ```Solidity event TokenConfigUpdated( address cTokenAddress, address chainlinkAggregatorAddress, uint256 chainlinkPriceBase, uint256 underlyingTokenDecimals ); ``` #### 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 | [ChainlinkPriceOracleProxy.sol L64,L83,L92,L221](#) | #### 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 : `owner()` ,`renounceOwnership`,`transferOwnership()`,`getUnderlyingPrice` #### 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: Gas Consumption | Type | Severity | Location | |-|-|-| | Optimization | Informational | [ChainlinkPriceOracleProxy.sol L221](#) | #### Description: The function `getUnderlyingPrice()` does not verified the parameter `cToken` before usage. #### Recommendation: We recommend adding below code: ```Solidity address cTokenAddress = address(cToken); TokenConfig memory config = tokenConfig[cTokenAddress]; require(config.chainlinkPriceBase != 0, "Invalid config"); ``` ### <a name="UNP-05" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-05: State Variable Shadowing from Abstract Contracts | Type | Severity | Location | |-|-|-| | Optimization | Informational | [ChainlinkPriceOracleProxy.sol L138,L157](#) | #### Description: State variable `isPriceOracle` in contract `ChainlinkPriceOracleProxy` is shadowed from abstract contract `PriceOracle`. ```Solidity bool public constant isPriceOracle = true; ``` #### Recommendation: We recommend to remove the state variable shadowing. ### <a name="UNP-06" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-06: Events Should Add Indexed Keyword | Type | Severity | Location | |-|-|-| | Optimization | Informational | [ChainlinkPriceOracleProxy.sol L138,L157](#) | #### Description: Event definittions in contract `ChainlinkPriceOracleProxy` do not have `indexed` keyword. The indexed parameters for logged events will allow you to search for these events using the indexed parameters as filters. ```Solidity event TokenConfigUpdated( address cTokenAddress, address chainlinkAggregatorAddress, uint256 chainlinkPriceBase, uint256 underlyingTokenDecimals ); ``` #### Recommendation: We recommend to add the `indexed` keyword. ```Solidity event TokenConfigUpdated( address indexed cTokenAddress, address indexed chainlinkAggregatorAddress, uint256 indexed chainlinkPriceBase, uint256 indexed underlyingTokenDecimals ); ``` ### <a name="UNP-07" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-07: Missing Emit Events | Type | Severity | Location | |-|-|-| | Optimization | Minor | [ChainlinkPriceOracleProxysol L248](#) | #### Description: Several sensitive actions are defined without event declarations. Fucntion `setEthUsdChainlinkAggregatorAddress()` can change the chainlink price oracle address. #### Recommendation: Consider adding events for sensitive actions, and emit it in the function.