CertiK Final Report For Gainswap

--- title: CertiK Final Report For Gainswap tags: final-report --- {%hackmd XdIXzOf5Ty2M3Uj1taFHIg %} <center> <img src="https://www.wing.vc/uploads/images/companies/certik-logo04.png" height="232" /> </center> Gainswap Protocol Security Assessment December 11th, 2020 For : Gainswap team @ Gainswap 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/gainswap/gainswap">Gainswap Protocol</a></td> </tr> <tr> <td width="50%" valign="top">Description</td> <td width="50%" valign="top">a defi platform with swap and staking functionalities</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/gainswap/gainswap">GitHub Repository</a></td> </tr> <tr> <td width="50%" valign="top">Commit</td> <td width="50%" valign="top"> <a href="https://github.com/gainswap/gainswap/commit/866ccbe7e1ed9007f5e32c784288537a01d1d29f"> 866ccbe7e1ed9007f5e32c784288537a01d1d29f</a> </td> </tr> </table> #### Audit Summary <table> <tr> <td width="50%" valign="top">Delivery Date</td> <td width="50%" valign="top">Dec. 11, 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">Dec. 4, 2020 - Dec. 9, 2020</td> </tr> </table> #### Vulnerability Summary <table> <tr> <td width="50%" valign="top">Total Issues</td> <td width="50%" valign="top">11</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">4</td> </tr> <tr> <td width="50%" valign="top">Total Informational</td> <td width="50%" valign="top">7</td> </tr> </table> --- ## <img src="https://svgshare.com/i/Pp1.svg" width="40"/> Executive Summary This report has been prepared for **Gainswap** 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 **Gainswap** team or reported an issue. --- ## <img src="https://svgshare.com/i/Pp1.svg" width="40"/> System Overview The **Gainswap** protocol is a decentralized platform for token swap and staking. It's main advatage is increasing virtual liquidity and improving capital utilization. The core components are the GainswapFactory, GainswapPair and GainswapRouter, which allow users to swap tokens and deposit their digital assets. These assets are transferred to a third party service YFI. Liquidity providers will be incentivized for earning staking rewards in YFI. This protocol has an external dependency. All user deposits could be transferred to a third-party service (like YFI). 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 audited commits are <a href="https://github.com/gainswap/gainswap/commit/866ccbe7e1ed9007f5e32c784288537a01d1d29f">866ccbe7e1ed9007f5e32c784288537a01d1d29f</a> and the files included in the scope were <a href="https://github.com/gainswap/gainswap/blob/main/contract/GainswapFactory">GainswapFactory</a> and file <a href="https://github.com/gainswap/gainswap/blob/main/contract/GainswapRouter02">GainswapRouter02</a>. Source Code SHA-256 Checksum - GainswapFactory fbc6090a2d47d78b15d87b2333719e132a044c488c11ca0fd3b4af2c97109fa6 - GainswapRouter02 14fbb9c0cc88854e06a86fd79aa6698cadf648ff0cdcb8052724b7209f2e2644 Certain optimization steps that we pinpointed in the source code mostly referred to coding standards and inefficiencies, however 4 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. --- ## <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 | Incorrect File Name | Optimization | Informational | | EXH-02 | Compilation Issues | Compilation | Minor | | EXH-03 | Incorrect Naming Convention Utilization | Coding Style | Informational | | EXH-04 | Proper Usage of "public" and "external" type | Optimization | Informational | | EXH-05 | Controversial specifications in whitepaper | Optimization | Informational | | EXH-06 | Security risk of transferring assets | Security | Minor | | EXH-07 | Incorrect logic for `_withdraw0` | Optimization | Informational | | EXH-08 | Math Overflow | Optimization | Minor | | EXH-09 | Missing Emit Events | Optimization | Minor | | EXH-10 | Gas Consumption | Optimization | Informational | | EXH-11 | Check Zero Address | Optimization | Informational | --- ### <a name="UNP-02" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-01: Incorrect File Name | Type | Severity | Location | | ------------ | ------------- | --------------------------------------------- | | Optimization | Informational | [GainswapFactory.sol,GainswapRouter02.sol](#) | #### Description: There are no extension file names for file <a href="https://github.com/gainswap/gainswap/blob/main/contract/GainswapFactory">GainswapFactory</a> and file <a href="https://github.com/gainswap/gainswap/blob/main/contract/GainswapRouter02"> GainswapRouter02</a>. #### Recommendation: We recommend to add the extension file name as below: **GainswapFactory.sol** **GainswapRouter02.sol** ### <a name="UNP-02" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-02: Compilation Issues | Type | Severity | Location | | ----------- | -------- | --------------------------------------------- | | Compilation | Minor | [GainswapFactory.sol,GainswapRouter02.sol](#) | #### Description: There several compilation issues 1. Contract code size exceeds 24576 bytes (a limit introduced in Spurious Dragon). This contract may not be deployable on mainnet. Consider enabling the optimizer (with a low "runs" value!), turning off revert strings, or using libraries. 2. Duplicate contract names found for IERC20, IGainswapFactory, SafeMathGainswap. This can cause errors and unknown behavior. Please rename one of your contracts. #### Recommendation: We recommend to split the GainswapFactory contract and GainswapPair contract into different files. ### <a name="UNP-02" style="display:none"> </a> Exhibit-03: Incorrect Naming Convention Utilization | Type | Severity | Location | | ------------ | ------------- | ------------------------------------------------------------ | | Coding Style | Informational | [GainswapFactory.sol L440,L449,L575,L581 GainswapRouter02.sol L20,L21,L38](#) | #### Description: Solidity defines a naming convention that should be followed. In general, parameters should use mixedCase, refer to: https://solidity.readthedocs.io/en/v0.6.12/style-guide.html#naming-conventions Functions other than constructors should use mixedCase. Examples: Functions like: `dummy_mint()`, `dummy_burn()` , `set_redepositRatio0()`, `set_redepositRatio1()`, `DOMAIN_SEPARATOR()`,`PERMIT_TYPEHASH()`,`MINIMUM_LIQUIDITY()` Parameter shoud use mixedCase. Examples: Parameter like:`_redpositRatio0`, `_redpositRatio1` Inside each contract, library or interface, use the following order: Type declarations State variables Events Functions Events definition should be in front of function definitions: ```Solidity event Sync(uint112 reserve0, uint112 reserve1); event FeeUpdated(uint8 fee); event Y0Updated(address indexed token); event Y1Updated(address indexed token); event Deposited0Updated(uint deposited); event Deposited1Updated(uint deposited); event RedepositRatio0Updated(uint16 ratio); event RedepositRatio1Updated(uint16 ratio); ``` #### 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-04: Proper Usage of "public" and "external" type | Type | Severity | Location | | ------------ | ------------- | ------------------------------------------------------ | | Optimization | Informational | [GainswapFactory.sol L305,L310,L522,L525,L528,L533](#) | #### Description: "public" functions that are never called by the contract could be declared "external" . When the inputs are arrays "external" functions are more efficient than "public" functions. Examples Functions like : `getDeposited()` ,`getDummy()`, `unapprove0()`, `unapprove1()`, `setY0()`, `setY1()` #### Recommendation: Consider using the "external" attribute for functions never called from the contract. ### <a name="UNP-04" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-05: Controversial specifications in whitepaper | Type | Severity | Location | | ------------ | ------------- | ----------------------------- | | Optimization | Informational | [GainswapFactory.sol L501](#) | #### Description: According to the chapter 2.3 in doc <a href="https://github.com/gainswap/gainswap/blob/main/gainswap.pdf"> gainswap.pdf</a>, the fee is 0.30%. `This is effectively the same as letting anyone flash-borrow any of assets stored in a Gainswap pool (for the same 0.30% fee as Uniswap charges for trading).` But in chapter 3.3 in the doc : `The 30-base-point fee fixed on Uniswap can be negotiated and formulated by the community in Gainswap.That can be changed according to user needs when new situations appear.` This is controversial with chapter 2.3. ### <a name="UNP-05" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-06: Security risk of transferring assets | Type | Severity | Location | | -------- | -------- | ----------------------------- | | Security | Minor | [GainswapFactory.sol L528](#) | #### Description: This protocol has an external dependency. User's digital currencies can be deposited to a third-party service (like YFI) via IyToken contract. The system should only be used if the service is appropriately trusted. IyToken contract is not in the scope of audit. Additionally, the governace privilege should be controlled. `yToken0` and `yToken1` can be easily set by governace. This will change the address where user's digital currencies to be deposited. ```Solidity function setY0(address y) public onlyOwner() { yToken0 = y; emit Y0Updated(y); approve0(); } function setY1(address y) public onlyOwner() { yToken1 = y; emit Y1Updated(y); approve1(); } ``` #### Recommendation: We recommend to move the governace to Timelock or community after the protocol deployed. (Gainswap - response) We have deployed a multi-sig wallet with timelock. All sensitive actions will need multi-sig confirmations. The address is: https://etherscan.io/address/0x02c7d121e7f176ef5ddc1459e1ec05468fbc4ff2 users reminder: use set fee to to enable multi-sig ### <a name="UNP-05" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-07: Incorrect logic for `_withdraw0()` | Type | Severity | Location | | ------------ | ------------- | ----------------------------- | | Optimization | Informational | [GainswapFactory.sol L528](#) | #### Description: There are some corner cases not included by the logic in function `_withdraw0()`. 1. `delta` could be greater than `deposited0` even not withdraw all, since there is staking rewards. 2. This function will transfer the staking rewards gained from `IyToken` to address `feeTo`. Before using function `setFeeTo()` to set the destination, its initial value is the `GainswapFactory` contract. Then liquidity providers could not share the rewards. ```Solidity function _withdraw0(uint s) internal { ... if (delta <= deposited0) { deposited0 -= delta; } else { delta -= deposited0; deposited0 = 0; _safeTransfer(token0, owner(), delta); } ... } ``` #### ### <a name="UNP-05" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-08: Math Overflow | Type | Severity | Location | | ------------ | -------- | ----------------------------- | | Optimization | Minor | [GainswapFactory.sol L451](#) | #### Description: Function `dummy_burn()` did not use safe math. This can cause sub overflow issue. ```Solidity function dummy_burn(uint amount0, uint amount1) external onlyOwner() lock { (uint112 _reserve0, uint112 _reserve1,) = getReserves(); // gas savings dummy0 -= uint112(amount0); dummy1 -= uint112(amount1); emit DummyBurn(amount0, amount1); _update(b0(), b1(), _reserve0, _reserve1); } ``` #### Recommendation: We recommend to use safe math instead of arithmetic operators. ### <a name="UNP-07" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-09: Missing Emit Events | Type | Severity | Location | | ------------ | -------- | ---------------------------------- | | Optimization | Minor | [GainswapFactory.sol L688,L693](#) | #### Description: Several sensitive actions are defined without event declarations. Examples: Fucntions like `setFeeTo()` , `setFeeToSetter()`. #### Recommendation: Consider adding events for sensitive actions, and emit it in the function. ### <a name="UNP-04" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-10: Gas Consumption | Type | Severity | Location | | ------------ | ------------- | ---------------------------------- | | Optimization | Informational | [[GainswapFactory.sol L322](#)](#) | #### Description: In function `_safeTransfer()`, require statements shoud be in front of the `if (redepositRatio0 > 0){}` code snipit. ```Solidity if (token == token0) { _withdrawAll0(); (bool success, bytes memory data) = token.call(abi.encodeWithSelector(SELECTOR, to, value)); if (redepositRatio0 > 0) { redeposit0(); } require(success && (data.length == 0 || abi.decode(data, (bool))), 'Gainswap: TRANSFER_FAILED'); } else { _withdrawAll1(); (bool success, bytes memory data) = token.call(abi.encodeWithSelector(SELECTOR, to, value)); if (redepositRatio1 > 0) { redeposit1(); } require(success && (data.length == 0 || abi.decode(data, (bool))), 'Gainswap: TRANSFER_FAILED'); } ``` #### Recommendation: We recommend to put the `if (redepositRatio0 > 0){}` code snipit in front of require statement. ```Solidity if (token == token0) { _withdrawAll0(); (bool success, bytes memory data) = token.call(abi.encodeWithSelector(SELECTOR, to, value)); require(success && (data.length == 0 || abi.decode(data, (bool))), 'Gainswap: TRANSFER_FAILED'); if (redepositRatio0 > 0) { redeposit0(); } } else { _withdrawAll1(); (bool success, bytes memory data) = token.call(abi.encodeWithSelector(SELECTOR, to, value)); require(success && (data.length == 0 || abi.decode(data, (bool))), 'Gainswap: TRANSFER_FAILED'); if (redepositRatio1 > 0) { redeposit1(); } } ``` ### <a name="UNP-07" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> Exhibit-11: Check Zero Address | Type | Severity | Location | | ------------ | ------------- | --------------------------------------- | | Optimization | Informational | [[GainswapFactory.sol L528,L533](#)](#) | #### Description: The parameter of address ` y` is not checked for zero address in function `setY0` and `setY1`. ```Solidity function setY0(address y) public onlyOwner() { yToken0 = y; emit Y0Updated(y); approve0(); } ``` #### Recommendation: We recommend to add below checks. ```Solidity function setY0(address y) public onlyOwner() { require(y != address(0), "Address zero is forbbiden"); yToken0 = y; emit Y0Updated(y); approve0(); } ```