---
# System prepended metadata

title: CertiK Final Report For Seascape
tags: [final-report]

---

---
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>
<p style="font-size: 28px">Seascape Protocol</p>
<p style="font-size: 22px">Security Assessment</p>
<p style="font-size: 18px">November 30th, 2020</p>

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"><b>Project Name</b></td>
<td width="50%" valign="top"><a href="https://github.com/blocklords/seascape-smartcontracts">Seascape</a></td>
</tr>
<tr>
<td width="50%" valign="top"><b>Description</b></td>
<td width="50%" valign="top">a platform for DEFI staking games</td>
</tr>
<tr>
<td width="50%" valign="top"><b>Platform</b></td>
<td width="50%" valign="top">Ethereum; Solidity</td>
</tr>
<tr>
<td width="50%" valign="top"><b>Codebase</b></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"><b>Commit</b></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><br/>
</td>
</tr>
</table>

#### Audit Summary

<table>
<tr>
<td width="50%" valign="top"><b>Delivery Date</b></td>
<td width="50%" valign="top">Nov. 27, 2020</td>
</tr>
<tr>
<td width="50%" valign="top"><b>Method of Audit</b></td>
<td width="50%" valign="top">Static Analysis, Manual Review</td>
</tr>
<tr>
<td width="50%" valign="top"><b>Consultants Engaged</b></td>
<td width="50%" valign="top">2</td>
</tr>
<tr>
<td width="50%" valign="top"><b>Timeline</b></td>
<td width="50%" valign="top">Nov. 25, 2020 - Nov. 27, 2020</td>
</tr>
</table>

#### Vulnerability Summary

<table>
<tr>
<td width="50%" valign="top"><b>Total Issues</b></td>
<td width="50%" valign="top">7</td>
</tr>
<tr>
<td width="50%" valign="top"><b>Total Critical</b></td>
<td width="50%" valign="top">0</td>
</tr>
<tr>
<td width="50%" valign="top"><b>Total Major</b></td>
<td width="50%" valign="top">0</td>
</tr>
<tr>
<td width="50%" valign="top"><b>Total Minor</b></td>
<td width="50%" valign="top">3</td>
</tr>
<tr>
<td width="50%" valign="top"><b>Total Informational</b></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");
```
 






