bitsCrunch Protocol Contracts - bitsCrunch

Operators cannot stake more than the minimumStakeAmount_ defined in the Staking contract. However, it was identified that the stakeBCUT(), unstakeBCUT() and _isStaked() functions assume that this fixed staking amount does not change. If the Owner increases the stake amount using the setMinimumStakeAmount() function, then the Operators might be able to stake more than the amount defined in the variable, but they will not be able to withdraw the full amount. If the Owner lowers the stake amount, Operators can only withdraw part of their staked funds.

Code Location

Input

contracts/Operator/Staking/Staking.sol

  function stakeBCUT() external {

    address staker = _msgSender();

    _checkAndRevert(operator_.isOperatorActive(staker), "Staking: Operator is not active");
    _checkAndRevert(!_isStaked(staker), "Staking: Operator Already staked");

    Operator storage currentOperator = operatorInfo_[staker];
    uint256 currentEpoch = epochManager_.currentEpoch();

    if(currentOperator.lastStakedEpoch == 0){
        operatorInfo_[staker] = Operator(minimumStakeAmount_, currentEpoch, 0, lockPeriod_);
    }
    else {

        currentOperator.balance += minimumStakeAmount_;
        currentOperator.lastStakedEpoch = currentEpoch;
        currentOperator.lockPeriod = lockPeriod_;

        operatorInfo_[staker] = currentOperator;
    }

    currentStaking_ += minimumStakeAmount_;
    TokenUtils.pullTokens(bcutToken_,staker,minimumStakeAmount_);

    emit StakerBalance(staker, operatorInfo_[staker].balance, currentEpoch, minimumStakeAmount_, "add");

  }

Input

contracts/Operator/Staking/Staking.sol

  function unstakeBCUT() external {

    address staker = _msgSender();
    _checkAndRevert(_isStaked(staker), "Staking: Operator Not Staked");

    Operator storage currentOperator = operatorInfo_[staker];
    uint256 epochPassed = epochManager_.epochsSince(currentOperator.lastStakedEpoch);

    _checkAndRevert(epochPassed >= currentOperator.lockPeriod, "Staking: Tokens Locked");

    currentOperator.balance -= minimumStakeAmount_;
    operatorInfo_[staker] = currentOperator;

    currentStaking_ -= minimumStakeAmount_;

    TokenUtils.pushTokens(bcutToken_,staker,minimumStakeAmount_);
    emit StakerBalance(staker, operatorInfo_[staker].balance, epochManager_.currentEpoch(), minimumStakeAmount_, "remove");
  }

Input

contracts/Operator/Staking/Staking.sol

    function _isStaked(address _operator) internal view returns(bool) {
      return operatorInfo_[_operator].balance >= minimumStakeAmount_;
    }

Recommendation

It is recommended to only increase the staked balance up to minimumStakeAmount_ during staking and withdraw the full staked balance during withdrawals.

Remediation Plan

SOLVED: The bitsCrunch team solved the issue in commit 749b7f3.

It was identified that the pullTokens() and pushTokens() functions in the TokenUtils contract use the IERC20Upgradeable interface to interact with the _tokenparameter to transfer currencies between the contract and the target wallet. The IERC20Upgradeable interface expects the transfer functions to have a return value on success. It is important to note that the transfer functions of some tokens (e.g., USDT, BNB on the Ethereum network) do not return any values, so these tokens are incompatible with the current version of the contracts using the functions from the TokenUtils contract.

Code Location

Input

contracts/Utils/TokenUtils.sol

    function pullTokens(
        IERC20Upgradeable _token,
        address _from,
        uint256 _amount
    ) internal {
        if (_amount > 0) {
            bool success = _token.transferFrom(_from, address(this), _amount);
            if(!success){
                revert TokenTransferError("Tokens: Couldn't transfer");
            }
        }
    }

Input

contracts/Utils/TokenUtils.sol

    function pushTokens(
        IERC20Upgradeable _token,
        address _to,
        uint256 _amount
    ) internal {
        if (_amount > 0) {
            bool success = _token.transfer(_to, _amount);
            if(!success){
                revert TokenTransferError("Tokens: Couldn't transfer");
            }
        }
    }

Recommendation

It is recommended to use OpenZeppelin's SafeERC20 wrapper with the IERC20Upgradeableinterface to make the contracts compatible with currencies that return no value.

Remediation Plan

RISK ACCEPTED: The bitsCrunch team made a business decision to accept the risk of this finding and not alter the contracts until they plan to support such tokens. The Protocol intended to be used with USDC as the stable coin.

It was identified that the receiveRewards() functions in the ContributorRewardManager and OperatorRewardManagercontracts can pull tokens from arbitrary addresses. These functions are used by the Managers to transfer rewards the contracts for the source addresses that previously approved token transfers.

Code Location

Input

contracts/Contributor/RewardManager/RewardManager.sol

   function receiveRewards(uint256 _epoch, address _from, uint256 _amount) external onlyManager nonReentrant {
                
        _checkAndRevert(_epoch < epochManager_.currentEpoch(), "Invalid Epoch Passed");
        _checkAndRevert(epochRewards_[_epoch].settledRewards == 0, "Settlement Already Started");

        TokenUtils.pullTokens(bcutToken_,_from,_amount);
        epochRewards_[_epoch].totalRewards += _amount;

        emit ReceivedRewards(_epoch,_amount,epochRewards_[_epoch].totalRewards);
    }

Input

contracts/Operator/RewardManager/RewardManager.sol

   function receiveRewards(uint256 _epoch, address _from, uint256 _amount) external onlyManager nonReentrant {
                
        _checkAndRevert(_epoch < epochManager_.currentEpoch(), "Invalid Epoch Passed");
        _checkAndRevert(epochRewards_[_epoch].settledRewards == 0, "Settlement Already Started");

        TokenUtils.pullTokens(bcutToken_,_from,_amount);
        epochRewards_[_epoch].totalRewards += _amount;

        emit ReceivedRewards(_epoch,_amount,epochRewards_[_epoch].totalRewards);
    }

Recommendation

It is recommended to only pull tokens from the msg.sender or from a predefined address.

Remediation Plan

ACKNOWLEDGED: The bitsCrunch team made a business decision to acknowledge this finding and not alter the contracts. The manager of the contract will be a timelock controller, and the manager's wallet will be managed by AWS KMS. This reward-receiving model is the intended design in that the token-transferring user will first approve the amount of tokens, and the manager will initiate the function to receive rewards for the contract.

It was identified that long revert strings are used in the several contracts. Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. If the revert string uses strings to provide additional information about failures, but they are rather expensive, especially when it comes to deploying cost, and it is difficult to use dynamic information in them.

Long revert strings were identified in the following contracts:

• contracts/Contributor/RewardManager/RewardManager.sol

• contracts/Contributor/Staking/ContributorStaking.sol

• contracts/Epochs/EpochManager.sol

• contracts/GovernanceController/Governance/Controller.sol

• contracts/Operator/Operator.sol

• contracts/Operator/RewardManager/RewardManager.sol

• contracts/Operator/Staking/Staking.sol

• contracts/Token/BCUTToken.sol

Recommendation

Consider implementing custom errors instead of reverting strings or using shorter error messages.

Remediation Plan

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

It was identified that postfix (e.g. i++) operators were used in several contracts to increment loop iterator variables. It is known that, in loops, using prefix operators (e.g. ++i) costs less gas per iteration than postfix operators.

Postfix operators were used in the following contracts:

• contracts/Bitscrunch/Customer/CustomerStaking.sol

• contracts/Bitscrunch/Payments/CustomerPayout.sol

• contracts/Bitscrunch/Payments/OperatorPayout.sol

• contracts/Contributor/RewardManager/RewardManager.sol

• contracts/Operator/RewardManager/RewardManager.sol

Recommendation

Consider using prefix operation instead of postfix to increment the values of the uint loop iterator variables.

Remediation Plan

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