CertiK Preliminary Comments For TLC

--- title: CertiK Preliminary Comments For TLC 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 4th, 2020 Preliminary Report For : TLC Team @ TLC 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://etherscan.io/address/0x9dafec62c3ef9cf8add8532ff339b991a30f421d">TLC Minter Protocol</a></td> </tr> <tr> <td width="50%" valign="top">Description</td> <td width="50%" valign="top">Contract of TLC TristersLightMinter, provide staking 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">tlc-20201029.zip</a></td> </tr> <tr> <td width="50%" valign="top">Checksum</td> <td width="50%" valign="top"> 7773619f4241de112526ff97d60e4c3d472b8971cfb8789f9da4d95677b964d2 </td> </tr> </table> #### Audit Summary <table> <tr> <td width="50%" valign="top">Delivery Date</td> <td width="50%" valign="top">Nov. 4, 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. 3, 2020 - Nov. 4, 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 TLC 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| Proper Usage of "public" and "external" type | Coding Style | Informational | | EXH-03| Unchecked constructor parameter | Optimization | Informational | | EXH-04| Typo in error message | Coding Style | Informational | | EXH-05| Missing Definitions of Events | Optimization | Minor | | EXH-06| Redundant codes | Coding Style | 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 | [Address.sol, Ownable.sol, SafeMath.sol, SafeERC20.sol, TristersLightMinterV2.sol, IERC20.sol](#) | #### Description: The compiler version utilized in several files uses the "^" prefix specifier, denoting that a compiler version which is greater than the version will be used to compile the contracts. The compiler version utilized in other files uses the "<=" prefix specifier, denoting that a compiler version which is smaller 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: Proper Usage of "public" and "external" type | Type | Severity | Location | |-|-|-| | Coding Style | Informational | [TristersLightMinterV2.sol L32,L56,L86,L90,L96,L107](#) | #### 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 : `deposit()` , `withdraw()`, `getBalance()`, `addToken()`, `removeToken()`, `setFeeAddress()` #### Recommendation: Consider using the "external" attribute for functions never called from the contract. ### <a name="UNP-03" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-03: Unchecked constructor parameter | Type | Severity | Location | |-|-|-| | Optimization | Informational | [TristersLightMinterV2.sol L27](#) | #### Description: The parameter `tlcAddress` is not verified before usage. #### Recommendation: We recommend to add: `require(tlcAddress != address(0),"tlcAddress is zero");` ### <a name="UNP-04" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-04: Typo in error message | Type | Severity | Location | |-|-|-| | Coding Style | Informational | [TristersLightMinterV2.sol L38](#) | #### Description: The error message `"TristersLightMinterV2: value must be greater than amoutn + gas"` should be `"TristersLightMinterV2: value must be greater than amount + gas"` ### <a name="UNP-05" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-05: Missing Definitions of Events | Type | Severity | Location | |-|-|-| | Optimization | Informational | [TristersLightMinterV2.sol L90,L96,L107](#) | #### Description: Better to add an event to track the change of sensitive information. Examples: `addToken()`,`removeToken()`,`setFeeAddress()` #### Recommendation: We recommend declaring events for most of sensitive information changes. ### <a name="UNP-06" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-06: Redundant codes | Type | Severity | Location | |-|-|-| | Coding Style | Informational | [TristersLightMinterV2.sol L66](#) | #### Description: The below codes are reduntant: `if (_stake < stake) stake = _stake;` Since `stake = _stake.mul(amount).div(balance)`, and `balance >= amount`, so the condition `_stake < stake` will never happen. #### Recommendation: We recommend removing the redundant codes.