CertiK Final Report For DMM

--- title: CertiK Final Report For DMM tags: final-report --- {%hackmd XdIXzOf5Ty2M3Uj1taFHIg %} <center> <img src="https://www.wing.vc/uploads/images/companies/certik-logo04.png" height="232" /> </center> DMM farming Security Assessment October 28th, 2020 For : DMM Team @ DMM By : Guilong Li @ CertiK guilong.li@certik.org Bryan Xu @ CertiK bryan.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> 2. <a href="https://github.com/defi-money-market-ecosystem/protocol/commit/f367a59b59e182e4ea5cc8e04c130a395d151b3a">f367a59b59e182e4ea5cc8e04c130a395d151b3a</a> </td> </tr> </table> #### Audit Summary <table> <tr> <td width="50%" valign="top">Delivery Date</td> <td width="50%" valign="top">Oct. 28, 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. 28 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"/> Documentation The sources of truth regarding the operation of the contracts in scope were lackluster and are something we advise to be enriched to aid in the legibility of the codebase as well as project. To help aid our understanding of each contract’s functionality we referred to in-line comments and naming conventions. These were considered the specification, and when discrepancies arose with the actual code behaviour, we consulted with the DMM team or reported an issue. --- ## <img src="https://svgshare.com/i/Pp1.svg" width="40"/> System Overview DMM Farming is a decentralized platform for DeFi farming and profit distribution. The core component is the DMGYieldFarmingV2, which allows users to deposit their mToken from DMM money market, into the system. These tokens and its underlying assets are transferred to a third party service Uniswap. In exchange, users are issued LP tokens that represent their claim on their deposits. Users will be incentivized for earning DMG in farming campaigns, to outweigh the costs of the fees. The external dependencies of this protocol is , all user deposits are immediately transferred to a third-party service (like Uniswap). The system should only be used if the service is appropriately trusted. --- ## <img src="https://svgshare.com/i/Pp1.svg" width="40"/> Review Notes The scope for the DMM Farming protocol contains the yield farming contracts and its interactions with the DMGToken, which is the governace token of DMM project. Certain optimization steps that we pinpointed in the source code mostly referred to coding standards and inefficiencies, however 1 minor vulnerability was identified during our audit that solely concerns the specification. Certain discrepancies between the expected specification and the implementation of it were identified and were relayed to the team, however they pose no type of vulnerability and concern an optional code path that was unaccounted for. The code has adequate comment coverage, but the project doumentation and specification outside of the source files was minimal. --- ## <img src="https://svgshare.com/i/Pp1.svg" width="40"/> Recommendations Overall, the codebase of the contracts should be refactored to assimilate the findings of this report, enforce linters and / or coding styles as well as correct any spelling errors and mistakes that appear throughout the code to achieve a high standard of code quality and security. --- ## <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. (DMM - confirmed) We strictly use Solidity 5.13 for compilation and stick with version 5 for compilation. ### <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. (DMM - confirmed) Our naming convention puts an underscore before internal functions. ### <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. (DMM - confirmed) The test file is only used in tests and not in production. ### <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. (DMM - resolved) The recommendations were applied in commit f367a59b59e182e4ea5cc8e04c130a395d151b3a. ### <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. (DMM - confirmed) For security risk of transferring assets - the spenders of the contract are hard coded so even if an unauthorized person inputted a nefarious spender, it wouldn't matter because there is no way for the router to call a non-hard coded address for spending the funds. Consider declaring events to emit the `__tokens` and the `__spenders`. (DMM - resolved) The recommendations were applied in commit f367a59b59e182e4ea5cc8e04c130a395d151b3a. ### <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. (DMM - resolved) The recommendations were applied in commit f367a59b59e182e4ea5cc8e04c130a395d151b3a.