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

Cronus Finance Audit

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, specifically, commit hash 06820480fcf5bfe6906d5ddae0ff52f3e0907d0f.

Our auditing for Cronus Finance covered the following components:

AMM DEX
Liquidity Mining Farm
$CRN Token
Stable Cronus Staking

Cronus Finance Architecture

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.
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.
$CRN Token
CronusToken.sol inherits SafeERC20.sol from OpenZepplin. CronusToken.sol has additoinal governance related features such as delegateBySig delegates.

Priviliged Roles

CronusToken.sol
- owner: mint new tokens.
- keeper: update maxSupply limit.
CronusFactory.sol
- feeTo : the address that can receive mint fee when lp token minted.
- feeToSetter: set feeTo and feeToSetter address.
MasterChefCronus.sol
- owner :addand set pool.
- admin: deposit token for a user.
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.
StableCronusStaking.sol
- owner: add/remove reward token and set deposit fee percent.

Findings & Improvement Suggestions

InformationalMinorMediumMajorCritical

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

Critical

MasterChefCronus.sol: UserInfo.rewardDebt is not updated in collectAllPoolRewards(). Critical
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.

MasterChefCronus.sol: reward amount of rewarder contract is not reset in emergencyWithdraw(). Critical

Description: Users callemergencyWithdraw() 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.

Major

StableCronusStaking.sol: internalCronusBalance is not updated in emergencyWithdraw(). Major
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.

Medium

StableCronusStaking.sol: removing reward token void unclaimed reward of the removed token. Medium
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

StableCronusStaking.sol: loop can be avoided through data structure. Minor
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.

Informational

MasterChefCronus.sol: Interaction with external contract (L294, L321, L350, L358) Informational
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.
CronusToken.sol: a limitation on max inflation each year Informational
Description: Comparing with Uniswap goveranance token contract,
Result: This suggestion is not adopted.
StableCronusStaking .sol has serveral magic numbers (Rui). Informational
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.
StableCronusStaking.sol: event names inconsistency. Informational
Description: event ClaimReward() does not follow the "noun + verb past participle" naming scheme.
Recommendation: event RewardClaimed()
Result: This suggestion is not adopted.
StableCronusStaking.sol: event important fields not indexed. Informational
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.
MoneyMaker.sol: setBridge(address token, address bridge) does not check loops in existing bridge. Informational
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.
MoneyMaker.sol: state variable wavax does not represent the correct meaning of the variable’s value. Informational
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.
MoneyMaker.sol: Events on updating state variables should also include the old values. Informational
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.
CronusRouter02.sol: missing require error message Informational
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.

tags: Final Report