###### tags: `Final Report`

Cronus Finance Audit
===

> Copyright © 2022 by Verilog Solutions. All rights reserved.
> May 23, 2022
> by **Verilog Solutions**


This report presents our engineering engagement with Cronus Finance, a decentralized exchange deployed on the EVMOS ecosystem. Cronus Finance is an AMM DEX with liquidity mining rewards. Cronus Finance has its own governance token, $CRN, which can be staked into $sCRN and earn staking rewards denominated in stablecoin.

---

## Table of Content

[TOC]

---
## Project Summary

Cronus Finance is an AMM DEX deployed on the EVMOS ecosystem. A portion of Cronus Finance's code is based on SushiSwap, which features liquidity mining rewards and governance token staking. It is worth noting that Cronus Finance also implemented new features such as a Stable Cronus Staking that converts LP fees into stablecoins and allows $sCRN holders to claim exchange fees denominated in stablecoin.
___

## Service Scope

Our review focused on the [**main** branch](https://github.com/cronus-finance/cronus-core/tree/main), specifically, commit hash [**06820480fcf5bfe6906d5ddae0ff52f3e0907d0f**](https://github.com/cronus-finance/cronus-core/commit/06820480fcf5bfe6906d5ddae0ff52f3e0907d0f).

Our auditing for Cronus Finance covered the following components:

- AMM DEX
- Liquidity Mining Farm
- $CRN Token
- Stable Cronus Staking

---

## Cronus Finance Architecture


1. **AMM DEX**
The AMM DEX part of Cronus Finance is based on SushiSwap, which is based on Uniswap V2. Users can add/remove liquidity and swap assets by interacting with the `CronusRouter02.sol` contract. For adding liquidity, `CronusRouter02.sol` will first check whether the pair exists. If the pair does not exist, A `CronusPair.sol` will be deployed by `CronusFactory.sol`. If the pair exists, `CronusRouter02.sol` will query the reserve of `tokenA` and `tokenB` of the pair by calling `CronusLibrary` contract. `CronusRouter02.sol` will then check whether the user is depositing the minimal amount of `tokenA` and `tokenB` to the pair. If the check passes, then `CronusRouter02.sol` will the `tokenA` and `tokenB` that the user is depositing to the `CronusPair.sol` contract of the pair. The `CronusPair.sol` contract of the pair will then mint LP tokens to the user, representing the liquidity provided by the user. \
\
For removing liquidity, `CronusRouter02.sol` will first query the address of the deployed `CronusPair.sol` for the pair by calling `CronusLibrary` contract. The `CronusPair.sol` contract of the pair will then transfer the LP token from the user to the `CronusPair.sol` contract of the pair. The `CronusPair.sol` contract of the pair will then burn the LP tokens and send the amount (corresponding to the amount of LP tokens and the reserves) of `tokenA` and `tokenB` to the user.\
\
For swapping tokens. `CronusRouter02.sol` will query the `CronusLibrary` contract for the number of tokens that the user should receive/send. `CronusRouter02.sol` will first query the address of the deployed `CronusPair.sol` for the first hop of pairs in the `path` by calling `CronusLibrary` contract. The `CronusPair.sol` contract of the pair will then transfer the token that needs to be swapped from the user to the `CronusPair.sol` contract of the first hop of pairs in the `path`. Then a low-level function `_swap` is called to swap tokens between the hops in the `path`.

2. **Liquidity Mining Farm**
The liquidity mining farm part of Cronus Finance is based on SushiSwap MasterChef contracts. The `MasterChefCronus.sol` mints new $CRN tokens at each call to the `updatePool` function. New $CRN tokens are minted to the developer address, treasury address, investor address, and the deployed `MasterChefCronus.sol` contract based on a proportion defined in the `MasterChefCronus.sol` contract. The proportions can be changed by the owner of the `MasterChefCronus.sol` contract.\
\
The owner of the `MasterChefCronus.sol` can add a new reward pool to the liquidity mining contract by calling `add` function. Each reward pool needs to be specified with three parameters: `_allocPoint`, `_lpToken`, `_rewarder`. `_allocPoint` determines the proportion of the $CRN liquidity mining rewards that the reward pool is allocated. `_lpToken` determines which LP token is accepted by the reward pool. `_rewarder` is optional and used when a reward pool has double rewards in $CRN and another token. The `owner` of `MasterChefCronus.sol` can also later adjust the `_allocPoint` and `_rewarder` parameters by calling `set`.\
\
The function `updatePool` will update the rewards metrics of a particular pool, specified by `_pid`. The `updatePool` function will calculate the amount of $CRN that should be minted based on the time of the last update for the particular pool, the amount of $CRN to be minted every second, and the allocation point of the particular pool. The function would then mint the $CRN tokens to the `devAddr`, `treasuryAddr`, `investorAddr`, and the liquidity mining contract itself. The amount of $CRN tokens minted is determined by `devPercent`, `treasuryPercent`, and `investorPercent`, which can be set by the owner of the liquidity mining contract. The number of $CRN that each share of LP token deposit is then calculated and stored in the reward pool data structure as `accCronusPerShare`.\
\
The function `deposit` handles the deposit of LP tokens into the liquidity mining pool. The `deposit` function will first call `updatePool` to update the rewards metrics for the reward pool to the latest state, corresponding to the latest timestamp. Then the `deposit` function will check whether the user has already deposited LP tokens into the reward pair. If the user has already deposited into the reward pair, the already accumulated rewards will be harvested and sent to the user. Then the `deposit` function will calculate and store `rewardDebt` for the user that is depositing LP tokens. `rewardDebt` is the amount of $CRN token accumulated to date, before the user's deposit, for the amount of LP token that the user is depositing. Therefore, when calculating the amount of $CRN that the user is earning, the smart contract only needs to multiply the number of $CRN per share by the number of reward pool shares that the user holds, minus the `rewardDebt` for the user. The `deposit` function will also call `onCronusReward` function of the `rewarder` contract associated with the reward pool, if available. Lastly, the `deposit` function will transfer the LP token from the user to the liquidity mining farm.\
\
The function `withdraw` handles the withdrawal of LP tokens from the liquidity mining pool. The `withdraw` function will first call `updatePool` to update the rewards metrics for the reward pool to the latest state, corresponding to the latest timestamp. Then the already accumulated rewards will be harvested and sent to the user. Then the `withdraw` function will calculate and store `rewardDebt` for the user that is depositing LP tokens. The `withdraw` function will also call `onCronusReward` function of the `rewarder` contract associated with the reward pool, if available. Lastly, the `withdraw` function will transfer the LP token to the user from the liquidity mining farm.

3. **\$CRN Token**
`CronusToken.sol` inherits `SafeERC20.sol` from OpenZepplin. `CronusToken.sol` has additoinal governance related features such as `delegateBySig` `delegates`.

---

## Priviliged Roles
1. `CronusToken.sol`
* `owner`: mint new tokens.
* `keeper`: update `maxSupply` limit.

1. `CronusFactory.sol`
* `feeTo` : the address that can receive mint fee when lp token minted.
* `feeToSetter`: set `feeTo` and `feeToSetter` address.

2. `MasterChefCronus.sol`
* `owner` :`add`and `set` pool.
* `admin`: deposit token for a user.

3. `MoneyMaker.sol`
* `auth` : authorized address. It can set bridge, dev cut, dev address, `tokenTo` address. It can also convert pairs of tokens to `tokenTo`
* `owner`: add/remove `auth` role.
4. `StableCronusStaking.sol`
* `owner`: add/remove reward token and set deposit fee percent.

## Findings & Improvement Suggestions
####
<html></html>
<style>
.info {
background-color:mediumseagreen;
font-size: 12px;
color: white;
border-radius:4px;
padding: 1px 4px;
font-weight: 500;
display: inline-block;
margin: 2px;
letter-spacing: 0.3px}
</style><style>
.minor {
background-color: #698999;
font-size: 12px;
color: white;
border-radius:4px;
padding: 1px 4px;
font-weight: 500;
display: inline-block;
margin: 2px;
letter-spacing: 0.3px}
</style><style>
.medium {
background-color: #FFCA0F;
color: #121212;
font-size: 12px;
border-radius:4px;
padding: 1px 4px;
font-weight: 500;
display: inline-block;
margin: 2px;
letter-spacing: 0.3px}
</style><style>
.major{
background-color: #FF6B4A;
color: white;
font-size: 12px;
border-radius:4px;
padding: 1px 4px;
font-weight: 500;
display: inline-block;
margin: 2px;
letter-spacing: 0.3px}
</style><style>
.critical{
background-color: #FF0000;
color: white;
font-size: 12px;
border-radius:4px;
padding: 1px 4px;
font-weight: 500;
display: inline-block;
margin: 2px;
letter-spacing: 0.3px}
</style>

<span class='info'>Informational</span><span class='minor'>Minor</span><span class='medium'>Medium</span><span class='major'>Major</span><span class='critical'>Critical</span>

| | Total | Acknowledged | Resolved |
| ------------- | ----- | ------------ | -------- |
| Critical | 2 | 2 | 2 |
| Major | 1 | 1 | 1 |
| Medium | 1 | 1 | 0 |
| Minor | 1 | 1 | 1 |
| Informational | 9 | 9 | 0 |

### Critical
1. `MasterChefCronus.sol`: `UserInfo.rewardDebt` is not updated in `collectAllPoolRewards()`. <span class='critical'>Critical</span>
**Description**: `collectAllPoolRewards()` loops through all pools and transfers rewards of each pool to the user. However, `UserInfo.rewardDebt` is not updated to the state variable because `UserInfo` is a local copy in `memory` given `memory` modifier is used when declaring the variable in Line 5. `UserInfo.rewardDebt` still remains unchange after calling `collectAllPoolRewards()`.
**Recommendation**: Use `storage` instead of `memory` for `UserInfo`.

    function collectAllPoolRewards() public {
        for (uint256 _pid = 0; _pid < poolInfo.length; _pid++) {
            PoolInfo memory pool = poolInfo[_pid];
            // * suggestion: use `storage` *
            UserInfo storage user = userInfo[_pid][msg.sender];
            updatePool(_pid);
            if (user.amount > 0) {
                // Harvest Cronus
                uint256 pending = user.amount.mul(pool.accCronusPerShare).div(1e12).sub(user.rewardDebt);
                safeCronusTransfer(msg.sender, pending);
                emit Harvest(msg.sender, _pid, pending);
            }
            user.rewardDebt = user.amount.mul(pool.accCronusPerShare).div(1e12);

            IRewarder rewarder = poolInfo[_pid].rewarder;
            if (address(rewarder) != address(0)) {
                rewarder.onCronusReward(msg.sender, user.amount);
            }
        }
    }

**Result**: Fixed in commit [5508803e0acbf40688557a8e68342581810c2a30](https://github.com/cronus-finance/cronus-core/commit/5508803e0acbf40688557a8e68342581810c2a30).

2. `MasterChefCronus.sol`: reward amount of `rewarder` contract is not reset in `emergencyWithdraw()`. <span class='critical'>Critical</span>

**Description**: Users call`emergencyWithdraw()` function to withdraw all staked tokens without caring about all the rewards. All reward amounts should be reset to zero. It does not reset the reward amount of `rewarder` contracts. An attacker can take a flash loan, deposit it into the double reward farm and drain the bonus rewards from `rewarder` contract.

**How The Attack Works**:
- Attacker takes a flash loan.
- Deposits x LP tokens into any double reward farm.
- Emergency withdraws its LP tokens.
- Deposits a single LP token back into the same farm and waits n number of days.
- Harvests the bonus reward as if it had x number of LP tokens instead of 1 LP token.

**Recommendation**: Fix the mistake in `emergencyWithddraw()` function, clear user data in `rewarder` contract.

    /// @notice Withdraw without caring about rewards. EMERGENCY ONLY.
    /// @param pid The index of the pool. See `poolInfo`.
    function emergencyWithdraw(uint256 pid) external nonReentrant {
        PoolInfo memory pool = poolInfo[pid];
        UserInfo storage user = userInfo[pid][msg.sender];
        uint256 amount = user.amount;
        user.amount = 0;
        user.rewardDebt = 0;
        // * suggestion: update amount of rewarder contract *
        IRewarder _rewarder = pool.rewarder;
        if (address(_rewarder) != address(0)) {
            _rewarder.onCronusReward(msg.sender, 0);
        }

        // Note: transfer can fail or succeed if `amount` is zero.
        pool.lpToken.safeTransfer(msg.sender, amount);
        emit EmergencyWithdraw(msg.sender, pid, amount);
    }

**Result**: Fixed in commit [84e287b3512fee5fb308ab15c9603ce63ff9651e](https://github.com/cronus-finance/cronus-core/commit/84e287b3512fee5fb308ab15c9603ce63ff9651e).

### Major
1. `StableCronusStaking.sol`: `internalCronusBalance` is not updated in `emergencyWithdraw()`. <span class='major'>Major</span>
**Description**: `internalCronusBalance` is a varible used in `updateReward` to calculate rewards. It should be updated when cronus token is transferred to/from current contract otherwise the reward calculation will be inaccurate.

In `emergencyWithdraw()`, the $CRN token is transferred from this contract to the user. It does not deduct the amount from `internalCronusBalance`. Besides making the reward calculation inaccurate, this may also cause the reward calculation to revert on subtraction underflow (it deducts `internalCronusBalance` from the token balance). Thus it may also make the functions calls to `deposit()`, `addRewardToken()`,`removeRewardToken()`, `withdraw()` fail because of the revert on `updateReward()`.
**Recommendation**:

    function emergencyWithdraw() external {
        UserInfo storage user = userInfo[_msgSender()];

        uint256 _amount = user.amount;
        user.amount = 0;
        uint256 _len = rewardTokens.length;
        for (uint256 i; i < _len; i++) {
            IERC20 _token = rewardTokens[i];
            user.rewardDebt[_token] = 0;
        }
        // *suggestion: update internalCronusBalance*
        internalCronusBalance = internalCronusBalance.sub(_amount);
        
        CRN.safeTransfer(_msgSender(), _amount);
        emit EmergencyWithdraw(_msgSender(), _amount);
    }

**Result**: Fixed in commit [4909443408e4c01ef17684455f8dd021de4547ec](https://github.com/cronus-finance/cronus-core/commit/4909443408e4c01ef17684455f8dd021de4547ec).

### Medium
1. `StableCronusStaking.sol`: removing reward token void unclaimed reward of the removed token. <span class='medium'>Medium</span>
**Description**: After calling `removeRewardToken()`, the removed token is removed from the `rewardTokens` list and thus no longer accessable when fetching rewards (through `withdraw()` and `deposit()`).
**Result**: Discussion needed with Cornus Finance Team.

### Minor

1. `StableCronusStaking.sol`: loop can be avoided through data structure. <span class='minor'>Minor</span>
**Description**:In order to remove one reward token, `removeRewardToken()` function loops through all the reward tokens. Loop can be avoid by using more optimized data structures.

    function removeRewardToken(IERC20 _rewardToken) external onlyOwner {
        require(isRewardToken[_rewardToken], "StableCronusStaking: token can't be removed");
        updateReward(_rewardToken);
        isRewardToken[_rewardToken] = false;
        uint256 _len = rewardTokens.length;
        for (uint256 i; i < _len; i++) {
            if (rewardTokens[i] == _rewardToken) {
                rewardTokens[i] = rewardTokens[_len - 1];
                rewardTokens.pop();
                break;
            }
        }
        emit RewardTokenRemoved(address(_rewardToken));
    }

**Recommendation**: Use EnumerableSet (See code below). `tokenIndex` can also serve as `isRewardToken` to further save gas. All `require(isRewardToken[_token])` should change to `require(tokenIndex[_token] != 0)`.

    mapping(IERC20 => uint256) tokenIndex;
    //mapping(IERC20 => bool) isRewardToken;  // removed
    function addRewardToken(IERC20 _rewardToken) external onlyOwner {
        uint256 valueIndex = tokenIndex[_rewardToken]; // 0 index is reserve as null identifier
        require(valueIndex, "StableCronusStaking: rewardToken already exists.");
        
        rewardTokens.push(_rewardToken);
        tokenIndex[_rewardToken] = rewardTokens.length;
        
        updateReward(_rewardToken);
        emit RewardTokenAdded(address(_rewardToken));
    }
    function removeRewardToken(IERC20 _rewardToken) external onlyOwner {
        uint256 valueIndex = tokenIndex[_rewardToken];
        require(valueIndex, "StableCronusStaking: rewardToken does not exist.");
        
        updateReward(_rewardToken);
        
        uint256 toDeleteIndex = tokenIndex[_rewardToken] - 1;
        uint256 lastIndex = rewardTokens.length - 1;
        
        if (lastIndex != toDeleteIndex) {
            address lastValue = rewardTokens[lastIndex]
            rewardTokens[toDeleteIndex] = lastValue;    // update last value to the removed value
            tokenIndex[lastValue] = valueIndex;    // update index
        }
        
        rewardTokens.pop();
        detele tokenIndex[_rewardToken];
        
        emit RewardTokenRemoved(address(_rewardToken));
    }

**Result**: Fixed in commit [9fe1b898da869d3c22fdb9757becd062b50219ba](https://github.com/cronus-finance/cronus-core/commit/9fe1b898da869d3c22fdb9757becd062b50219ba).

### Informational
1. `MasterChefCronus.sol:` Interaction with external contract (L294, L321, L350, L358) <span class='info'>Informational</span>
**Description**: The snippets are following proper check-effects-interaction pattern. However, attention should be paid to the external `lptoken.safeTransfer` and `lptoken.safeTransferFrom` that is interacted.
**Recommendation**: Ensure the external `lpToken.safeTransfer` and `lpToken.safeTransferFrom` have proper reentrancy guard.
**Result**: This suggestion is not adopted.

2. `CronusToken.sol:` a limitation on max inflation each year <span class='info'>Informational</span>
**Description**: Comparing with Uniswap goveranance token contract,
**Result**: This suggestion is not adopted.

3. `StableCronusStaking .sol` has serveral magic numbers (Rui). <span class='info'>Informational</span>
**Description**: In `constructor()` L106 `5e17`, in `addRewardToken()` L187 `25`, in `setDepositFeePercent()` L218 `5e17`
**Recommendation**: Consider using constant state varibale to represent fixed numbers.
**Result**: This suggestion is not adopted.

5. `StableCronusStaking.sol`: event names inconsistency. <span class='info'>Informational</span>
**Description**: `event ClaimReward()` does not follow the "noun + verb past participle" naming scheme.
**Recommendation**: `event RewardClaimed()`
**Result**: This suggestion is not adopted.

6. `StableCronusStaking.sol`: event important fields not indexed. <span class='info'>Informational</span>
**Description**: `event RewardTokenAdded(address token)` and `event RewardTokenRemoved(address token)` do not index the added/removed token address.
**Recommendation**: Add `indexed` modifier.
**Result**: This suggestion is not adopted.

7. `MoneyMaker.sol`: `setBridge(address token, address bridge)` does not check loops in existing bridge. <span class='info'>Informational</span>
**Description**: In `_convertStep()`, the recursive function will recuse all the input tokens until all of them are converted to `tokenTo`, however, if the `bridgeOf(token)` graph has a loop, the recursive function will be in dead loop and revert due to out-of-gas. Example: The path of token bridges must not contain any loop (Say, A - B - C - A), otherwise the `convert()` will be in a dead loop.
**Recommendation**: Check Directed Acyclic Graph (DAG) compliance before calling `setBridge()`.
**Consequence**: `convert()` will be in dead loop.
**Result**: This suggestion is not adopted.

8. `MoneyMaker.sol`: state variable `wavax` does not represent the correct meaning of the variable’s value. <span class='info'>Informational</span>
**Description**: `wavax` is from traderJoes implementation which is deployed on Avalanche. Using `wavax` is a bit confusing in the code since Cronos is deployed on Evmos.
**Recommendation**: Use another name like `wevmos` .
**Consequence**: Misleading and confusing to future developers.
**Result**: This suggestion is not adopted.

9. `MoneyMaker.sol`: Events on updating state variables should also include the old values. <span class='info'>Informational</span>
**Description**: Events on updating state variables (`SetDevAddr` `SetDevCut` `SetTokenTo`) do not include the old values.
**Recommendation**: See code below:

    event SetDevAddr(address indexed newDevAddr, address indexed oldDevAddr);
    event SetDevCut(uint256 indexed newDevCut, uint256 indexed oldDevCut);
    event SetTokenTo(address indexed newTokenTo, address indexed oldTokenTo);

**Result**: This suggestion is not adopted.

10. `CronusRouter02.sol`: missing `require` error message <span class='info'>Informational</span>
**Description**: majority of the `require` statement in the `CronusRouer02.sol` has no error message, which is not a good practice in smart contract programming. Missing `require` error message may result in users do not understand why their transaction failed.
**Recommendation**: add error messages.
**Result**: This suggestion is not adopted.

---
## Disclaimer

Verilog receives compensation from one or more clients for performing the smart contract and auditing analysis contained in these reports. The report created is solely for Clients and published with their consent. As such, the scope of our audit is limited to a review of code, and only the code we note as being within the scope of our audit detailed in this report. It is important to note that the Solidity code itself presents unique and unquantifiable risks since the Solidity language itself remains under current development and is subject to unknown risks and flaws. Our sole goal is to help reduce the attack vectors and the high level of variance associated with utilizing new and consistently changing technologies. Thus, Verilog in no way claims any guarantee of security or functionality of the technology we agree to analyze.

In addition, Verilog reports do not provide any indication of the technologies proprietors, business, business model, or legal compliance. As such, reports do not provide investment advice and should not be used to make decisions about investment or involvement with any particular project. Verilog has the right to distribute the Report through other means, including via Verilog publications and other distributions. Verilog makes the reports available to parties other than the Clients (i.e., “third parties”) – on its website in hopes that it can help the blockchain ecosystem develop technical best practices in this rapidly evolving area of innovation.