Cronus Audit Report

Our auditing service includes the following two stages:

1. Pre-Audit Consulting Service
   • As a part of the pre-audit service, the Verilog team worked closely with the project development team to discuss potential vulnerability and smart contract development best practices. Verilog team is very appreciative of establishing an efficient and effective communication channel with the project team, as new findings are often exchanged promptly and fixes were deployed quickly, during the preliminary report stage.

2. Smart Contract Auditing Service
   • Verilog Solutions team analyzed the entire project using a detailed-oriented approach to capture the fundamental logic and suggested improvements to the existing code. Details can be found under Findings & Improvement Suggestions

• Code Assessment
  • We evaluate the overall quality of the code and comments as well as the architecture of the repository.
  • We help the project dev team improve the overall quality of the repository by providing suggestions on refactorization to follow the best practice of Web3 software engineering.

• Code Logic Analysis
  • We dive into the data structures and algorithms in the repository and provide suggestions to improve the data structures and algorithms for the lower time and space complexities.
  • We analyze the hierarchy among multiple modules and the relations among the source code files in the repository and provide suggestions to improve the code architecture with better readability, reusability, and extensibility.

• Business Logic Analysis
  • We study the technical whitepaper and other documents of the project and compare its specification with the functionality implemented in the code for any potential mismatch between them.
  • We analyze the risks and potential vulnerabilities in the business logic and make suggestions to improve the robustness of the project.

• Access Control Analysis
  • We perform a comprehensive assessment of the special roles of the project, including their authorities and privileges.
  • We provide suggestions regarding the best practice of privilege role management according to the standard operating procedures (SOP).

Our auditing for Cronus Finance covered the following components:

• AMM DEX

• Liquidity Mining Farm

• $CRN Token

• Stable Cronus Staking

1. Critical State variables are not updated after critical operations

   Severity	High
   Source	contracts/MasterChefCronus.sol#L429;
   Status	Resolved in commit 5508803e0acbf40688557a8e68342581810c2a30;

   • Description

     UserInfo.rewardDebt is not updated in MasterChefCronus.collectAllPoolRewards(). Attackers can call this function to drain the Cronus tokens in pool.

     MasterChefCronus.collectAllPoolRewards() loops through all pools and transfers rewards of each pool to the user. However, UserInfo.rewardDebtis not updated as expected. Because memory instead of storage is used when fetching UserInfo. So the userInfo that was updated inside the function is just a local copy of the original variable and thus the update has no impact on the state variable reward debt. UserInfo.rewardDebtstill remains unchanged after MasterChefCronus.collectAllPoolRewards().

     The user’s reward debt is not updated and remains unchanged after claiming rewards. The calculated claimable rewards will also stay unchanged. Users can get the same Cronus token rewards again even if they have claimed. The attacker can just call this function multiple times to drain the Cronus token in the pool.

   • Exploit Scenario

     Malloy stakes some tokens in the pool and accumulates some rewards. He then calls MasterChefCronus.collectAllPoolRewards() many times to drain all the Cronus tokens in the pool.

   • Recommendations

     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);
               }
           }
       }

     In this way, UserInfo gets updated correctly and the function works as expected.

   • Results

     Resolved in commit 5508803e0acbf40688557a8e68342581810c2a30. The issue was fixed by changing the memory to storage for UserInfo.

2. Inter-contract state variables inconsistency

   Severity	High
   Source	contracts/MasterChefCronus.sol#L354;
   Status	Resolved in commit 84e287b3512fee5fb308ab15c9603ce63ff9651e;

   • Description

     Reward amount of rewardercontract is not reset in emergencyWithdraw(). Rewards on the rewarder contract can still be claimed after the user withdraws all the staked tokens.

     MasterChefCronus.emergencyWithdraw() is a function for users to withdraw all the staked tokens and give up all the unclaimed rewards. All the rewards that the user has earned should be reset to zero and staked tokens are transferred back to the user. However, the reward amount on the rewarder contract is not reset to zero. Users are still able to claim rewards from the rewarder contract after they emergency withdraw all the staked tokens.

   • Exploit Scenario

     • Attacker Mallory takes a flash loan.

     • Deposits x LP tokens into any double reward farm. He then

     • Emergency withdraws his LP tokens.

     • Deposits a single LP token back into the same farm and waits n number of days.

     • Harvests the bonus reward

     • Mallory now has rewards as he has x number of LP tokens instead of 1 LP token.
   • Recommendations

     Fix the mistake in emergencyWithddraw()function, clear user data in rewardercontract.

     /// @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

     Resolved in commit 84e287b3512fee5fb308ab15c9603ce63ff9651e. The user’s amount in the rewarder contract is reset to zero inside this function.

3. Critical State variables are not updated after critical operations

   Severity	High
   Source	contracts/StableCronusStaking.sol#L287;
   Status	resolved in commit 4909443408e4c01ef17684455f8dd021de4547ec;

   • Description

     A critical variable used to calculate rewards is not updated in StableCronusStaking.emergencyWithdraw(). It will result in incorrect calculation of rewards and may cause the calls on critical functions to fail.

     internalCronusBalanceis a variable used to track the balance of Cronus tokens. It should be updated every time the Cronus token is transferred to/from the current contract. In emergencyWithdraw(), the amount is not deducted from the internalCronusBalance when the Cronus token is transferred from this contract to users. The internalCronusBalance will be bigger than it should be.

     internalCronusBalanceis used to calculate rewards in function StableCronusStaking.pendingReward(). The inaccurate internalCronusBalance will result in incorrect rewards calculation as well. The reward will be smaller than it should be. Besides, this may also cause the reward calculation to revert on subtraction underflow (it deducts internalCronusBalance from the token balance), which will make calls to deposit(), addRewardToken(),removeRewardToken(), withdraw()fail because of the revert on updateReward().

   • Exploit Scenario

     Because of the Incorrect internal Cronus token balance tracking, the amount of Cronus token rewards Alice can claim is smaller than it should be.

   • Recommendations

     update the internalCronusBalance inside StableCronusStaking.emergencyWithdraw().

     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

     Resolved in commit 4909443408e4c01ef17684455f8dd021de4547ec. internalCronusBalance is updated when Cronus token gets transferred out through function StableCronusStaking.emergencyWithdraw().

None ; )

1. Gas-inefficient array operations

   Severity	Low
   Source	contracts/StableCronusStaking.sol#L198;
   Status	resolved in commit 9fe1b898da869d3c22fdb9757becd062b50219ba;

   • Description

     In order to remove one reward token, removeRewardToken() function loops through all the reward tokens. Loop can be avoided by using more optimized data structures. It would cost extra unneeded time.

     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));
       }

   • Exploit Scenario

     N/A

   • Recommendations

     Use EnumerableSet (See code below). tokenIndexcan also serve as isRewardTokento 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));
       }

   • Results

     Resolved in commit 9fe1b898da869d3c22fdb9757becd062b50219ba. The Cronus Finance team fixed this issue by using EnumerableSet

1. Lack of Re-entrancy Guard for critical function with external dependency

   Severity	Informational
   Source	contracts/MasterChefCronus.sol(#L294, #L321, #L350, #L358);
   Status	Unresolved;

   • Description

     The snippets are following a proper check-effects-interaction pattern. However, attention should be paid to the external lptoken.safeTransfer and lptoken.safeTransferFrom that is interacted. Be cautious when adding support for tokens like ERC777 that have before after transfer hooks. It brings in reentrancy vulnerabilities even when following a check-effects-interaction pattern.

   • Exploit Scenario

     N/A

   • Recommendations

     Be cautious when adding support for new tokens. ReentrancyGaurd can be added to functions that contain interactions with the external token transfer.

   • Results

     The suggestion is not adopted

2. Magic numbers

   Severity	Informational
   Source	contracts/StableCronusStaking.sol(#L106, #L187, #L218);
   Status	Unresolved;

   • Description

     There are several magic numbers (5e17, 25) used in the contracts.

     It’s a better practice to make those magic numbers constant variables and have sufficient comments on their usage, which can greatly improve code readability.

   • Exploit Scenario

     N/A

   • Recommendations

     Use constant state variables to represent those magic numbers.

   • Results

     Unresolved. The suggestion is not adopted.

3. Naming issues

   Severity	Informational
   Source	contracts/StableCronusStaking.sol#L78;
   Status	Resolved in commit 68790c5370be9411d2ac181a33088bf206c2b427;

   • Description

     event ClaimReward()does not follow the “noun + verb past participle” naming scheme.

   • Exploit Scenario

     N/A

   • Recommendations

     Events and indexed values of events can be used to monitor contract operations. Thus, event names similar to contract function names must keep best practice by using the “noun + verb past participle” naming scheme. Therefore, we suggest changing the event name to RewardClaimed.

   • Results

     Resolved in commit 68790c5370be9411d2ac181a33088bf206c2b427. The event name has been changed to RewardClaimed.

4. Missing indexed variables in event

   Severity	Informational
   Source	contracts/StableCronusStaking.sol#L84;
   Status	Partially resolved in commit 68790c5370be9411d2ac181a33088bf206c2b427;

   • Description

     event RewardTokenAdded(address token)and event RewardTokenRemoved(address token)do not index the added/removed token address.

     Events and indexed values of events can be used to monitor contract operations. It’s recommended to have events emitted on important function calls and index those critical variables.

   • Exploit Scenario

     N/A

   • Recommendations

     The indexed parameters for logged events will allow users/developers to search for these events using the indexed parameters as filters. We suggest adding indexed to keep the best practice of smart contract programming

   • Results

     Partially resolved in commit 68790c5370be9411d2ac181a33088bf206c2b427. indexedwas added to event RewardTokenAdded(address token), but not added to event RewardTokenRemoved.

5. Unbounded gas usage in recursive function

   Severity	Informational
   Source	contracts/MoneyMaker.sol#L253;
   Status	Unresolved;

   • 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 a 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.

   • Exploit Scenario

     As described above, under certain edge cases, convert()will be in a dead loop

   • Recommendations

     Check Directed Acyclic Graph(DAG) compliance before calling setBridge().

   • Results

     Unresolved. The suggestion is not adopted.

6. Inconsistency in variable name and actual implementation

   Severity	Informational
   Source	contracts/MoneyMaker.sol;
   Status	Resolved in commit 68790c5370be9411d2ac181a33088bf206c2b427;

   • Description

     wavaxis from Trader Joe XYZ’s implementation which is deployed on Avalanche. Using wavaxis a bit confusing in the code since Cronus is deployed on Evmos. The name is misleading and confusing to future developers.

   • Exploit Scenario

     N/A

   • Recommendations

     Use another name like wevmos

   • Results

     Resolved in commit 68790c5370be9411d2ac181a33088bf206c2b427. wavax has been changed to wevmos.

7. Missing variables in events

   Severity	Informational
   Source	contracts/MoneyMaker.sol;
   Status	Resolved in commit 68790c5370be9411d2ac181a33088bf206c2b427;

   • Description

     Events on updating state variables (SetDevAddr SetDevCut SetTokenTo) do not include the old values.

     Events are usually used to monitor contracts. Including old values in events emitted when there is a critical variable set to a new value is good practice. It can help monitor the state variable changing and have a track of the value change.

   • Exploit Scenario

     N/A

   • Recommendations

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

   • Results

     Resolved in commit 68790c5370be9411d2ac181a33088bf206c2b427. The old variable value has been added to the events.

8. Missing error message

   Severity	Informational
   Source	contracts/CronusRouter02.sol;
   Status	Unresolved;

   • Description

     The majority of the require statement in the CronusRouer02.solhas no error message, which is not a good practice in smart contract programming. Missing require error messages may result in users do not understand why their transaction failed.

   • Exploit Scenario

     N/A

   • Recommendations

     Add error string to require statements in the CronusRouer02.sol

   • Results

     Unresolved. The suggestion is not adopted.