CertiK Final Report For Seascape

--- title: CertiK Final Report For Seascape tags: final-report --- {%hackmd XdIXzOf5Ty2M3Uj1taFHIg %} <center> <img src="https://www.wing.vc/uploads/images/companies/certik-logo04.png" height="232" /> </center> Seascape Protocol Security Assessment November 30th, 2020 For : Seascape team @ Seascape 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/blocklords/seascape-smartcontracts">Seascape</a></td> </tr> <tr> <td width="50%" valign="top">Description</td> <td width="50%" valign="top">a platform for DEFI staking games</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/blocklords/seascape-smartcontracts">GitHub Repository</a></td> </tr> <tr> <td width="50%" valign="top">Commit</td> <td width="50%" valign="top"> <a href="https://github.com/blocklords/seascape-smartcontracts/commit/36709463ae6d6a2266d7067c0c40f119e6e3cffb"> 36709463ae6d6a2266d7067c0c40f119e6e3cffb</a> <a href="https://github.com/blocklords/seascape-smartcontracts/commit/e218e18b45553ab7ead11ff4d32478f844b4b990"> e218e18b45553ab7ead11ff4d32478f844b4b990</a> <a href="https://github.com/blocklords/crowns/commit/5e8076ad8aecfd743ef5a788beba12427e6883f5"> 5e8076ad8aecfd743ef5a788beba12427e6883f5</a> <a href="https://github.com/blocklords/crowns/commit/bd1ad15f8935061952fe3207cf88fa3155b4fe3a"> bd1ad15f8935061952fe3207cf88fa3155b4fe3a</a> </td> </tr> </table> #### Audit Summary <table> <tr> <td width="50%" valign="top">Delivery Date</td> <td width="50%" valign="top">Nov. 27, 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. 25, 2020 - Nov. 27, 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">3</td> </tr> <tr> <td width="50%" valign="top">Total Informational</td> <td width="50%" valign="top">4</td> </tr> </table> --- ## <img src="https://svgshare.com/i/Pp1.svg" width="40"/> Executive Summary This report has been prepared for **Seascape** 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. --- ## <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 Farmland team or reported an issue. --- ## <img src="https://svgshare.com/i/Pp1.svg" width="40"/> System Overview Seascape is a decentralized platform for DeFi farming and profit distribution. The audited commits are 36709463ae6d6a2266d7067c0c40f119e6e3cffb, ad8d68269b6e30ae3a3f901f877a3dcf9fdce939, 5e8076ad8aecfd743ef5a788beba12427e6883f5 and the files included in the scope were CrownsToken.sol, VestingContract.sol, LpMining.sol. The core components are the Controll,Strategy and Vault, which allow users to deposit their cross-chain digital assets. These 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 staking rewards in Unipool, and saving gas by aggregation staking. This protocol has an external dependency. 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 Seascape protocol contains the vault,strategy and controll function. Certain optimization steps that we pinpointed in the source code mostly referred to coding standards and inefficiencies, however 3 minor vulnerabilities were 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 project has adequate doumentation and specification outside of the source files, but the code comment coverage 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| Gas consumption | Optimization | Informational | | EXH-02| Incorrect Naming Convention Utilization | Coding Style | Informational | | EXH-03| Missing Emit Events | Optimization | Minor | | EXH-04| Boolean Equality | Optimization | Informational | | EXH-05| Incorrect Logic of "isStartedFor" | Discussion | Minor | | EXH-06| Too Many Digits | Optimization | Informational | | EXH-07| Missing Checks | Optimization | Informational | | EXH-08| Redundant Codes | Coding Style | Informational | | EXH-09| Proper Usage of “public” and “external” type | Optimization | Informational | | EXH-10| Missing Emit Events | Optimization | Minor | | EXH-11| Missing Checks | Optimization | Informational | | EXH-12| Emit Fixed Value | Optimization | Informational | --- ### <a name="UNP-02" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-01: Gas consumption | Type | Severity | Location | |-|-|-| | Optimization | Informational | [LpMining.sol L18,L22](#) | #### Description: Below variables change only once, better to define it as immutable to avoid gas consumption. ```Solidity IERC20 public CWS; ``` Constant state variables should be declared constant to save gas. ```Solidity uint256 scaler = 10**18; ``` #### Recommendation: We recommend to change the codes as below: ```Solidity IERC20 public immutable CWS; uint256 private constant SCALER = 10**18; ``` ### <a name="UNP-03" 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 | [LpToken.sol L13, LpMining.sol L18, CrownsToken.sol L37, VestingContract.sol L26](#) | #### Description: Solidity defines a naming convention that should be followed. In general, parameters should use mixedCase, constants should use UPPER_CASE_WITH_UNDERSCORES. Refer to: https://solidity.readthedocs.io/en/v0.6.12/style-guide.html#naming-conventions Constants should use UPPER_CASE_WITH_UNDERSCORES. Examples: Constants like:`_million`,`scaler` #### 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-04" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-03: Missing Emit Events | Type | Severity | Location | |-|-|-| | Optimization | Informational | [LpMining.sol L114](#) | #### Description: Changing the `nftFactory` is a sensitive action without event declarations. ```Solidity function setNftFactory(address _address) external onlyOwner { nftFactory = NftFactory(_address); } ``` #### Recommendation: Consider adding events for sensitive actions, and emit it in the function. ```Solidity event SetNftFactory(address indexed _address); function setNftFactory(address _address) external onlyOwner { require (_address != address(0), "Seascape Staking: NFTFactory addess cannot be zero"); nftFactory = NftFactory(_address); emit SetNftFactory(_address); } ``` ### <a name="UNP-05" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-04: Boolean Equality | Type | Severity | Location | |-|-|-| | Optimization | Minor | [LpMining.sol L132,L171,L194 VestingContract.sol L47,L87](#) | #### Description: Boolean constants can be used directly and do not need to be compare to true or false. ```Solidity require(_token.transferFrom(msg.sender, address(this), _amount) == true, "Seascape Staking: Failed to transfer LP tokens into contract"); require(CWS.transfer(msg.sender, _interest) == true, "Seascape Staking: Failed to transfer reward CWS token"); require(_token.transfer(msg.sender, _amount) == true, "Seascape Staking: Failed to transfer token from contract to user"); ``` #### Recommendation: We recommend to change the codes as below: ```Solidity require(_token.transferFrom(msg.sender, address(this), _amount), "Seascape Staking: Failed to transfer LP tokens into contract"); require(CWS.transfer(msg.sender, _interest), "Seascape Staking: Failed to transfer reward CWS token"); require(_token.transfer(msg.sender, _amount), "Seascape Staking: Failed to transfer token from contract to user"); ``` ### <a name="UNP-06" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-05: Incorrect Calculation of Session Cap | Type | Severity | Location | |-|-|-| | Optimization | Minor | [LpMining.sol L269](#) | #### Description: ```Solidity uint256 _sessionCap = block.timestamp; if (isStartedFor(_sessionId) == false) { _sessionCap = _session.startTime.add(_session.period); } ``` If _sessionCap is less than session.starttime, isStartedFor will return false as well. ```Solidity isStartedFor if (sessions[_sessionId].totalReward == 0) { return false; } ``` #### Recommendation: We recommend to change the codes as below: ```Solidity uint256 _sessionCap = block.timestamp; if (_sessionCap > _session.startTime.add(_session.period)) { _sessionCap = _session.startTime.add(_session.period); } ``` 1.Adding this check is neccesary, otherwise the safemath will revert and users cannot withdraw. ```Solidity if (isActive(_sessionId) == false) { ... if (_balance.claimedTime > _sessionCap) { return 0; } } uint256 _earnPeriod = _sessionCap.sub(_balance.claimedTime); ``` 2.During `claim()`,`withdraw()` function calls state variables for balance are changed after transfers are done. This may lead to reentrancy issue. It is recommended to follow <a href="https://docs.soliditylang.org/en/develop/security-considerations.html?highlight=check-effects%23use-the-checks-effects-interactions-pattern">checks-effects-interactions pattern</a> for cases like this. It shields public/external functions from re-entrancy attacks. It's always a good practice to follow this pattern. `checks-effects-interactions` pattern also applies to ERC20 tokens as they can inform the recipient of a transfer in certain implementations. Example: ```Solidity function claim(uint256 _sessionId) public returns(bool) { ... _session.claimed = _session.claimed.add(_interest); _balance.claimed = _balance.claimed.add(_interest); _balance.claimedTime = block.timestamp; rewardSupply = rewardSupply.sub(_interest); require(CWS.transfer(msg.sender, _interest), "Seascape Staking: Failed to transfer reward CWS token"); ... } ``` ### <a name="UNP-07" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-06: Incorrect Logic of isStartedFor | Type | Severity | Location | |-|-|-| | Optimization | Minor | [LpMining.sol L247](#) | #### Description: Function `isStartedFor` has corner case to pass the check. ```Solidity function isStartedFor(uint256 _sessionId) internal view returns(bool) { if (sessions[_sessionId].totalReward == 0) { return false; } if (now > sessions[_sessionId].startTime + sessions[_sessionId].period) { return false; } return true; } ``` Once the function `startSession` is called, `totalReward` gets initialized, and the _startTime is in the future. Hence the checks can be bypassed but the current time is actually not in the session period. #### Recommendation: We recommend to change the codes as below: ```Solidity function isStartedFor(uint256 _sessionId) internal view returns(bool) { if (sessions[_sessionId].totalReward == 0) { return false; } if (now < sessions[_sessionId].startTime ) { return false; } if (now > sessions[_sessionId].startTime + sessions[_sessionId].period) { return false; } return true; ``` ### <a name="UNP-07" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-07: Too Many Digits | Type | Severity | Location | |-|-|-| | Optimization | Informational | [LpToken.sol L13,CrownsToken.sol L37](#) | #### Description: Literals with many digits are difficult to read and review. ```Solidity uint256 private constant _million = 1000000; ``` #### Recommendation: Consider to use Ether suffix. ```Solidity uint256 private constant _million = 1e6; ``` ### <a name="UNP-07" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-08: Missing Checks | Type | Severity | Location | |-|-|-| | Optimization | Informational | [LpToken.sol L209](#) | #### Description: Check for `minted` is missing in function `claimNft`. We understand the check is commented out for certain testing purpose. Anyway this check needs to be added. ```Solidity //require(_balance.minted == false, "Seascape Staking: Already minted"); ``` #### Recommendation: We recommend to uncomment the below codes. ```Solidity require(_balance.minted == false, "Seascape Staking: Already minted"); ```