CertiK Preliminary Comments For DMM

--- title: CertiK Preliminary Comments For DMM 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 October 24th, 2020 Preliminary Report For : [DMM Team] @ [DMM] By : [Guilong 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://dao.defimoneymarket.com/farm">DMM Farming Protocol</a></td> </tr> <tr> <td width="50%" valign="top">Description</td> <td width="50%" valign="top">Contract of DMM farming, provide yield farming function.</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/defi-money-market-ecosystem/protocol/tree/master/contracts/external/farming">GitHub Repository</a></td> </tr> <tr> <td width="50%" valign="top">Commits</td> <td width="50%" valign="top"> 1. <a href="https://github.com/defi-money-market-ecosystem/protocol/commit/0d425d46e32befda1c7df0431802887a4edbf6c0#diff-92b73d60ce84dc27477311c0b54ce812083c2c662dd346d4685ffe23da87e7a9">0d425d46e32befda1c7df0431802887a4edbf6c0</a> </td> </tr> </table> #### Audit Summary <table> <tr> <td width="50%" valign="top">Delivery Date</td> <td width="50%" valign="top">Oct. 24, 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">Oct. 16, 2020 - Oct. 24 2020</td> </tr> </table> #### Vulnerability Summary <table> <tr> <td width="50%" valign="top">Total Issues</td> <td width="50%" valign="top">6</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">1</td> </tr> <tr> <td width="50%" valign="top">Total Informational</td> <td width="50%" valign="top">5</td> </tr> </table> --- ## <img src="https://svgshare.com/i/Pp1.svg" width="40"/> Executive Summary This report has been prepared for DMM 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| No setter for several variables | Coding Style | Informational | | EXH-04| Tautology | Comparison | Informational | | EXH-05| Security risk of transferring assets | Security | Minor | | EXH-06| Unused modifier | Optimization | Informational | --- ### <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 | [DMGYieldFarmingV2.sol,DMGYieldFarmingV2Lib.sol,IDMGYieldFarmingV2.sol,DMGYieldFarmingProxy.sol,DMGYieldFarmingRouter.sol,DMGYieldFarmingData.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 | [DMGYieldFarmingV2Lib.sol L66,L106,L168](#) | #### 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.13/style-guide.html#naming-conventions Functions other than constructors should use mixedCase. Examples: Functions like: `_payHarvestFee, _payFeesWithUniswapToken,_swapTokensForDmgViaUniswap` Function arguments should use mixedCase. Examples: Parameters like:`__user,__token,__tokenAmount` Keep consistency in one project/module. In `DMGYieldFarmingV2` the sequence of the state modifiers of functions are different in below examples: ```Solidity function setDmgGrowthCoefficient( uint __dmgGrowthCoefficient ) public nonReentrant onlyOwnerOrGuardian {... function setUnderlyingTokenValuator( address __underlyingTokenValuator ) onlyOwnerOrGuardian nonReentrant public { ``` #### 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: Important contract named as test | Type | Severity | Location | |-|-|-| | Coding Style | Informational | [DMGYieldFarmingData.sol L34](#) | #### Description: `TestDMGYieldFarmingV2` super inherits `DMGYieldFarmingData`, and initialises several important state varibales. ```Solidity address internal _dmgToken; address internal _guardian; address internal _dmmController; ``` `DMGYieldFarmingV2` inherits `DMGYieldFarmingData`, and then `TestDMGYieldFarmingV2` inherits `DMGYieldFarmingV2`. `TestDMGYieldFarmingV2` looks like an important formal contract. Plus the address arguments were not checked to be non-zero. #### Recommendation: Consider renaming `TestDMGYieldFarmingV2` as a formal contract. And the `initialize` function should check that addresses are non-zero. ### <a name="UNP-04" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-04: Tautology | Type | Severity | Location | |-|-|-| | Comparison | Informational | [DMGYieldFarmingV2.sol L720](#) | #### Description: Below expressions are tautologies. ```Solidity require( __fee >= 0 && __fee < FEE_AMOUNT_FACTOR, "DMGYieldFarmingV2::_verifyTokenFee: INVALID_FEES" ); ``` `__fee` is an uint256, so `__fee >= 0` will be always true. #### Recommendation: Fix the incorrect comparison by changing the value type or the comparison. ### <a name="UNP-05" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-05: Security risk of transferring assets | Type | Severity | Location | |-|-|-| | Security | Minor | [DMGYieldFarmingRouter.sol L105](#) | #### Description: Current implementation requires trusting that the `DMGYieldFarmingRouter` owners behave well. A malicious co-owner (or when their account gets hacked) could: 1) call `enableTokens` function to apporove an unlimited allowance to any spender. 2) transfer the assets from `addLiquidity` before `beginFarming`. It is possible because the function `enableTokens` has no access control. ```Solidity function enableTokens( address[] calldata __tokens, address[] calldata __spenders ) external nonReentrant { require( __tokens.length == __spenders.length, "DMGYieldFarmingFundingProxy::enableTokens: INVALID_LENGTH" ); for (uint i = 0; i < __tokens.length; i++) { IERC20(__tokens[i]).safeApprove(__spenders[i], uint(- 1)); } } ``` #### Recommendation: Consider adding `onlyOwner` modifier on this function and declaring events to emit the `__tokens` and the `__spenders`. ### <a name="UNP-06" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-06: Unused modifier | Type | Severity | Location | |-|-|-| | Optimization | Informational | [DMGYieldFarmingRouter.sol L63](#) | #### Description: Modifier `ensurePairIsSupported` is never used. ```Solidity modifier ensurePairIsSupported(address __tokenA, address __tokenB) { require( IDMGYieldFarmingV1(dmgYieldFarming).isSupportedToken(UniswapV2Library.pairFor(uniswapV2Factory, __tokenA, __tokenB, initCodeHash)), "DMGYieldFarmingFundingProxy: TOKEN_UNSUPPORTED" ); _; } ``` It could be duplicated code for `DMGYieldFarmingRouter._verifyTokensAreSupported` #### Recommendation: Consider removing unused modifiers.