In the updatePlan function of the ContributorMultiTierStaking contract, the NATSPEC comment is misleading. The comment states that the function is for deactivating a plan, but the function actually updates the plan, from an active to an inactive state and vice versa.

/**
* @dev Deactivate the plan.
* @param _planId is the id of the plan to deactivate.
*/

Update the NATSPEC comment to reflect the actual functionality of the function.

SOLVED: The BitsCrunch team solved this finding in commit 64e161677096dedd26cf25b65f4fa2ce166c8c6d by following the mentioned recommendation.

In Solidity, if a function has multiple modifiers, they are executed in the order specified. If checks or logic of modifiers depend on other modifiers, this has to be considered in their ordering.

Several functions of the contracts in scope have multiple modifiers, with one of them being nonReentrant which prevents reentrancy behavior on the functions. Ideally, the nonReentrant modifier should be the first one to prevent even the execution of other modifiers in case of reentrancy behavior.

While there is currently no obvious vulnerability with nonReentrant being the last modifier in the list, placing it first ensures that all other modifiers are executed only if the call is non-reentrant. This is a safer practice and can prevent potential issues in future updates or unforeseen scenarios.

Switch modifier order to consistently place the nonReentrant modifier as the first one to run so that all other modifiers are executed only if the call is non-reentrant.

SOLVED: The BitsCrunch team solved this finding in commit fa7db913cc3a9d01e535e5e3c530d7b8848b9ab9 by following the mentioned recommendation.

Throughout the files in scope, there are several instances of use of revert strings over custom errors.

In Solidity development, replacing hard-coded revert message strings with the Error() syntax is an optimization strategy that can significantly reduce gas costs. Hard-coded strings, stored on the blockchain, increase the size and cost of deploying and executing contracts.

The Error() syntax allows for the definition of reusable, parameterized custom errors, leading to a more efficient use of storage and reduced gas consumption. This approach not only optimizes gas usage during deployment and interaction with the contract but also enhances code maintainability and readability by providing clearer, context-specific error information.

Consider replacing all revert strings with custom errors. For example:

error ConditionNotMet();

if (!condition) revert ConditionNotMet();

For more reference, see here.

ACKNOWLEDGED: The BitsCrunch team made a business decision to acknowledge this finding and not alter the contracts. It will be planned for future upgrades.

Throughout the code in scope, there are several instances of unoptimized for loop declarations that may incur in higher gas costs than necessary.

Optimize the for loop declarations to reduce gas costs. Best practices include:

• The non-redundant initialization of the iterator with a default value (declaring simply i is equivalent to i = 0 but more gas efficient),

• The use of the pre-increment operator (inside an unchecked block if using Solidity >=0.8.0 and <= 0.8.21 :unchecked {++i} , or simply ++i if compiling with Solidity >=0.8.22).

Additionally, when reading from storage variables, it is recommended to reduce gas costs significantly by caching the array to read locally and iterate over it to avoid reading from storage on every iteration. Moreover, if there are several loops in the same function, the i variable can be re-used, to be able to set the value from non-zero to zero and reduce gas costs without additional variable declaration. For example:

uint256[] memory arrayInMemory = arrayInStorage;

uint256 i;
for (; i < arrayInMemory.length ;) {
  // code logic
  unchecked { ++i; }
}

delete i;

uint256[] memory arrayInMemory2 = arrayInStorage2;

for (; i < arrayInMemory2.length ;) {
  // code logic
  unchecked { ++i; }
}

ACKNOWLEDGED: The BitsCrunch team made a business decision to acknowledge this finding and not alter the contracts.

Throughout the code in scope, there are several instances where the contracts do not check for the address(0) when initializing the upgradeable contracts in scope. This can lead to an incorrect deployment state of contracts, where an improper initialization may require redeployment of the contracts.

Add a check for the address(0) when initializing variables to ensure proper deployment and initialization of the contracts.

SOLVED: The BitsCrunch team solved this finding in commit 64e161677096dedd26cf25b65f4fa2ce166c8c6d by following the mentioned recommendation.

Throughout the code in scope, there are several instances of unused imports that can be removed to improve code readability and maintainability. The following unused imports were identified:

import "@openzeppelin/contracts-upgradeable/token/ERC20/IERC20Upgradeable.sol"; (src/Bitscrunch/Payments/Storage.sol#4)

import {MathUtils} from "../../Utils/MathUtils.sol"; (src/Contributor/RewardManager/RewardManager.sol#8)

import { IOperator } from "../../Operator/IOperator.sol"; (src/Operator/RewardManager/Storage.sol#4)

import "@openzeppelin/contracts-upgradeable/token/ERC20/IERC20Upgradeable.sol"; (src/Operator/RewardManager/Storage.sol#3)

import "@openzeppelin/contracts-upgradeable/token/ERC20/IERC20Upgradeable.sol"; (src/Operator/Staking/Staking.sol#4)

import {IEpochManager} from "../../Epochs/EpochManager.sol"; (src/Operator/Staking/Staking.sol#6)

import "@openzeppelin/contracts-upgradeable/token/ERC20/IERC20Upgradeable.sol"; (src/Operator/Staking/Storage.sol#3)

Remove the unused imports from the contract.

SOLVED: The BitsCrunch team solved this finding in commit 64e161677096dedd26cf25b65f4fa2ce166c8c6d by following the mentioned recommendation.

In ContributorMultiTierStaking contract, the addPlan() and updatePlanLockPeriod() functions do not have a check for the _lockPeriod input parameter. This can lead to a situation where the _lockPeriod is set to an excessively high value, which can affect the functionality of the contract.

Add a check for the _lockPeriod input parameter in the addPlan() and updatePlanLockPeriod() functions to ensure that the lock period is within a reasonable threshold.

ACKNOWLEDGED: The BitsCrunch team made a business decision to acknowledge this finding and not alter the contracts. It will be planned for future upgrades.

Throughout the code in scope, it has been noted that there is inconsistent use of uint and uint256 variable declarations, where some variables are declared as uint and others as uint256. While this does not affect the functionality of the code, it is recommended to use explicit size declaration for integers to ensure consistency and readability.

Use explicit size declaration for integers. For example, use uint256 instead of uint.

SOLVED: The BitsCrunch team solved this finding in commit 64e161677096dedd26cf25b65f4fa2ce166c8c6d by following the mentioned recommendation.

The EpochManager contracts inherit from OpenZeppelin's OwnableUpgradeable contract and allows for single-step ownership transfer via the transferOwnership() function. In this aspect, it is crucial that the address to which ownership is transferred is verified to be active and willing to assume ownership responsibilities. Otherwise, the contract could be locked in a situation where it is no longer possible to make administrative changes to it.

Additionally, it is worth mentioning that in the OwnableUpgradeable contract, the renounceOwnership() function allows renouncing to the owner permission. Renouncing ownership before transferring it would result in the contract having no owner, eliminating the ability to call privileged functions.

Consider using OpenZeppelin's Ownable2StepUpgradeable contract over the Ownable contract, or implementing similar two-step ownership transfer logic into the contracts, so that rather than directly transferring to the new owner, the transfer only completes when the new owner accepts ownership.

Additionally, it is recommended that the owner cannot call the renounceOwnership() function without first transferring ownership to another address. In addition, if a multi-signature wallet is used, the call to the renounceOwnership() function should be confirmed by most signers.

ACKNOWLEDGED: The BitsCrunch team made a business decision to acknowledge this finding and not alter the contracts. It will be planned for future upgrades.

The claimReward function in the RewardManager contract is responsible for claiming rewards for a specific operator. However, the function does not follow the CEI pattern, which is a common pattern in Solidity development to prevent reentrancy attacks.

According to this pattern, any modifications to the contract's state should precede calls to external contracts or addresses. While the current usage of the ERC20 transfer method is not expected to make additional external calls, it is important to adhere to this pattern to ensure that the contract is secure against potential reentrancy behavior.

Follow the checks-effects-interactions pattern to execute the state modifications prior to making external calls .

ACKNOWLEDGED: The BitsCrunch team made a business decision to acknowledge this finding and not alter the contracts. It will be planned for future upgrades.