---

title: CertiK Auditor Note For GainsFarmV2&U8D

tags : audit-note

---

# CertiK Auditor Note For GainsFarmV2&U8D

tags : audit-note
===

<!-- Auditor note should be submitted daily. Use 10 mins per day to finish it and push to the project git repo through the hackmd embeded plugin: https://hackmd.io/c/tutorials/%2Fs%2Flink-with-github -->

###### tags: `Template` `Audit`

:::info
- **Project:** GainsFarmV2&U8D
- **Date:** Jan 4, 2021
- **Auditor:** [e-mail], [e-mail]
- **Reference:** [GainsFarmV2&U8D]

:::

:mag: New Findings
---
<!-- List all findings you think worthy note in the process of the audit. Not necessarily list here all vulnerabilities found in the project. List -->

| Finding Index | Finding Type | Finding Name | Link to Source | Comments |
| ------------- | ------------- | ------------ | ------------- | -------- |
| 1 | Bug | Re-entrancy Issue | LINK URL |See detail section |

:closed_book: Comments
--

Issue Analysis

If in function calls , state variables for balance are changed after transfers are done. It will lead to reentrancy issue.
Because the `payable` functions might have fallback function
, declared using fallback () external `payable` (without the function keyword).

fallback() external payable { x = 1; y = msg.value; }

The fallback function might do transfer again , since the balance is not updated yet.

1. if there is no ETH transfer(just erc20 tokens), it does not enable the `payable` functions of the `to` contract.
2. require `tx.origin == msg.sender`, then msg.sender cannot be a contract, will prevent re-entrancy attack.
3. anyway this is a best practice of coding style.

User case 1
### <a name="UNP-01" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> GNT-01: `Checks-effects-pattern` Not Used

| Type | Severity | Location |
| ------------ | ------------- | ---------------------------------- |
| Implementation | INFO | [GFarm.sol L571,L594 GFarmNFT.sol L1696](#) |

#### Description:

During `POOL1_unstake()`,`POOL2_unstake()`,`claim()` function calls state variables for balance are changed after transfers are done. This will lead to reentrancy issue.

#### Recommendation:
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.

    function claim(uint8 _leverage) external{
        require(tx.origin == msg.sender, "Contracts not allowed.");
        uint256 value = farm.NFT_CREDITS_amount(msg.sender);
        farm.spendCredits(msg.sender, requiredCredits(_leverage));
        mint(_leverage, value);
    }

User case 2
### <a name="UNP-01" style="display:none"> </a><img src="https://svgshare.com/i/Pp1.svg" width="40"/> CTR-02: `Checks-effects-pattern` Not Used

| Type | Severity | Location |
| ------------ | ------------- | ---------------------------------- |
| Implementation | Minor | [Bondings.sol L53 Comptroller.sol L38,L46 Pool.sol L60 ](#) |

#### Description:

During `deposit()`, `burnFromAccount()`, `redeemToAccount()` function calls state variables for balance are changed after transfers are done. This will lead to reentrancy issue. Although functions from Comptroller.sol contract are internal, it's always good to follow the said pattern.

#### Recommendation:
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 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.

#### Alleviation:
The development team think they do not use ETH in this function and reentrancy attack is not possible in their case.
(Certik -response) This is a recommendation for best practice.
ESD team has alleviated this issue. Refer to:
https://github.com/emptysetsquad/dollar/blob/2e4d49a24aa70993dfdb19ca4371fb53ed76e445/protocol/contracts/dao/Comptroller.sol