Protocol Oct 2023 - bitsCrunch

The owner of the protocol can create new schemes within the protocol for users to be able to use. There is no maximum amount of schemes that can be created. Thus, over time, or maliciously, the owner can create many schemes that would result in a denial of service due to reaching out of gas transaction, every time the owner needs to create a new one or update an existing one. Both functions (addScheme() and updateSchemeStatus()) call _isSchemeExists() to check if the scheme already exists, but this internal function iterates through the entire list of previously created schemes, consuming a lot of gas.

Code Location

Staking.sol

function addScheme(uint256 _discount, uint256 _stakeAmount) external onlyOwner {
    require((_discount>0 && _stakeAmount>0),"CS:ERR1");
    require(!_isSchemeExists(_discount, _stakeAmount),"CS:ERR2");

    schemeId_.increment();
    uint256 id = schemeId_.current();
    discountSchemes_[id] = Scheme(_discount, _stakeAmount,true);

    emit SchemeAdded(id, _discount, _stakeAmount);

}

Staking.sol

function updateSchemeStatus(uint256 _schemeId, bool _status) external onlyOwner {
    if(_status){
        require((_schemeId>0 && _schemeId<=schemeId_.current()),"CS:ERR3");
        require(!discountSchemes_[_schemeId].active,"CS:ERR4");
        require(!_isSchemeExists(
                discountSchemes_[_schemeId].discountPercent,
                discountSchemes_[_schemeId].stakeAmount),
                "CS:ERR2");

        discountSchemes_[_schemeId].active = true;
    }
    else{
        require((_schemeId>0 && _schemeId<=schemeId_.current()),"CS:ERR3");
        require(discountSchemes_[_schemeId].active,"CS:ERR5");

        discountSchemes_[_schemeId].active = false;
    }

    emit SchemeUpdated(_schemeId, _status);
}

Staking.sol

function _isSchemeExists(uint256 _discount, uint256 _stakeAmount) private view returns(bool){
    bool status = false;
    if(schemeId_.current()>0){
        for(uint256 i=1; i<= schemeId_.current(); i++){
            if(discountSchemes_[i].active && 
                ((discountSchemes_[i].discountPercent == _discount)||
                (discountSchemes_[i].stakeAmount == _stakeAmount)))
                    {
                        status = true;
                        break;
                    }
        }
    }
    return status;
}

To avoid this type of issue, setting a cap for the amount of created schemes within the protocol is recommended.

RISK ACCEPTED: The bitsCrunch team accepted the risk of the finding.

The code does not implement a two-step ownership transfer pattern. This practice is recommended when admin users have heavy responsibilities, such as the ability to mint tokens, freeze or unfreeze user accounts and set system configurations. It may happen that when transferring ownership of a contract, an error is made in the address. If the request were submitted, the contract would be lost forever. With this pattern, contract owners can submit a transfer request; however, this is not final until accepted by the new owner. If they realize they have made a mistake, they can stop it at any time before accepting it by calling cancelRequest.

Code Location

Controller.sol

function transferOwnership(address newOwner) public onlyOwner {
    _setupRole(DEFAULT_ADMIN_ROLE, newOwner);
    renounceRole(DEFAULT_ADMIN_ROLE, _msgSender());
}

It is recommended to implement a two-step process where the owner nominates an account, and the nominated account needs to call an acceptOwnership() function for the transfer of the ownership to succeed fully. This ensures the nominated EOA account is valid and active.

SOLVED: The bitsCrunch team solved the issue with the following commit id.

Commit ID: fcf43995a8ecf77586e3814914483469ab6b0f37

Failed operations in this contract are reverted with an accompanying message selected from a set of hard-coded strings.

In EVM, emitting a hard-coded string in an error message costs ~50 more gas than emitting a custom error. Additionally, hard-coded strings increase the gas required to deploy the contract.

Code Location

TokenUtils.sol

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

Custom errors are available from Solidity version 0.8.4 up. Consider replacing all revert strings with custom errors.

SOLVED: The bitsCrunch team solved the issue with the following commit id.

Commit ID: fcf43995a8ecf77586e3814914483469ab6b0f37

The file name and the contract name within the Solidity file are inconsistent. This can lead to confusion and potential errors when trying to interact with or deploy the contract. In Solidity, it is a best practice to keep the contract name and the file name the same for clarity and ease of management.

Code Location

Staking.sol

contract CustomerStaking is Billing {

This is just an example within the code, but many other contracts have the same issue. We strongly recommend renaming either the file or the contract so that they match. If the contract name is CustomerStaking, then the file name should ideally be CustomerStaking.sol.

SOLVED: The bitsCrunch team solved the issue with the following commit id.

Commit ID: fcf43995a8ecf77586e3814914483469ab6b0f37

In a for loop, the length of an array can be put in a temporary variable to save some gas. This has been done already in several other locations in the code.

In the above case, the solidity compiler will always read the length of the array during each iteration. That is,

• if it is a storage array, this is an extra sload operation (100 additional extra gas (EIP-2929) for each iteration except for the first),

• if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first),

• if it is a calldata array, this is an extra calldataload operation (3 additional gas for each iteration except for the first)

Code Location

Staking.sol

function _isSchemeExists(uint256 _discount, uint256 _stakeAmount) private view returns(bool){
    bool status = false;
    if(schemeId_.current()>0){
        for(uint256 i=1; i<= schemeId_.current(); i++){
            if(discountSchemes_[i].active && 
                ((discountSchemes_[i].discountPercent == _discount)||
                (discountSchemes_[i].stakeAmount == _stakeAmount)))
                    {
                        status = true;
                        break;
                    }
        }
    }
    return status;
}

In a for loop, store the length of an array or the current counter in a temporary variable.

SOLVED: The bitsCrunch team solved the issue with the following commit id.

Commit ID: fcf43995a8ecf77586e3814914483469ab6b0f37