Prepared by:
HALBORN
Last Updated 03/07/2024
Date of Engagement by: February 5th, 2024 - February 16th, 2024
100% of all REPORTED Findings have been addressed
All findings
6
Critical
0
High
0
Medium
1
Low
2
Informational
3
The bitsCrunch Protocol
Smart Contracts are a set of Solidity contracts that exist on the Polygon Blockchain. The contracts enable a decentralized network for the users to query data. Node Operators then provide queries to users. Users pay for queries with the selected stable coins.
\client engaged Halborn
to conduct a security assessment on their smart contracts beginning on 2024-02-05 and ending on 2024-02-16. The security assessment was scoped to the smart contracts provided in the bitscrunch-protocol/smartcontracts GitHub repository. Commit hashes and further details can be found in the Scope section of this report.
Halborn was provided 2 weeks for the engagement and assigned a full-time security engineer to review the security of the smart contracts in scope. The security team consists of a blockchain and smart contract security experts with advanced penetration testing and smart contract hacking skills, and deep knowledge of multiple blockchain protocols.
The purpose of the assessment is to:
Identify potential security issues within the smart contracts.
Ensure that smart contract functionality operates as intended.
In summary, Halborn identified some security risks that were partially addressed by the bitsCrunch team. The main one was the following:
Calculate the amountOutMinimum
parameter off-chain and pass it to the swapStableToBCUT()
function of the Payments
contract instead of using estimatedAmountOut
calculated in the function.
Halborn performed a combination of manual and automated security testing to balance efficiency, timeliness, practicality, and accuracy in regard to the scope of this assessment. While manual testing is recommended to uncover flaws in logic, process, and implementation; automated testing techniques help enhance coverage of the code and can quickly identify items that do not follow the security best practices. The following phases and associated tools were used during the assessment:
Research into architecture and purpose.
Smart contract manual code review and walkthrough.
Graphing out functionality and contract logic/connectivity/functions (solgraph
).
Manual assessment of use and safety for the critical Solidity variables and functions in scope to identify any arithmetic related vulnerability classes.
Manual testing by custom scripts.
Static Analysis of security for scoped contract, and imported functions (Slither
).
Testnet deployment (Foundry
, Brownie
).
Code repositories:
bitsCrunch Protocol Contracts
Repository: bitscrunch-protocol/smartcontracts
Commit ID: 54b65bc2433a1c1a63e610d3e6a9164e4d1cb382
Smart contracts in scope:
contracts/Bitscrunch/*
contracts/Contributor/*
contracts/Epochs/*
contracts/GovernanceController/*
contracts/Operator/*
contracts/Token/*
contracts/Utils/*
External libraries and financial-related attacks.
New features/implementations after/with the remediation commit IDs.
EXPLOITABILIY METRIC () | METRIC VALUE | NUMERICAL VALUE |
---|---|---|
Attack Origin (AO) | Arbitrary (AO:A) Specific (AO:S) | 1 0.2 |
Attack Cost (AC) | Low (AC:L) Medium (AC:M) High (AC:H) | 1 0.67 0.33 |
Attack Complexity (AX) | Low (AX:L) Medium (AX:M) High (AX:H) | 1 0.67 0.33 |
IMPACT METRIC () | METRIC VALUE | NUMERICAL VALUE |
---|---|---|
Confidentiality (C) | None (I:N) Low (I:L) Medium (I:M) High (I:H) Critical (I:C) | 0 0.25 0.5 0.75 1 |
Integrity (I) | None (I:N) Low (I:L) Medium (I:M) High (I:H) Critical (I:C) | 0 0.25 0.5 0.75 1 |
Availability (A) | None (A:N) Low (A:L) Medium (A:M) High (A:H) Critical (A:C) | 0 0.25 0.5 0.75 1 |
Deposit (D) | None (D:N) Low (D:L) Medium (D:M) High (D:H) Critical (D:C) | 0 0.25 0.5 0.75 1 |
Yield (Y) | None (Y:N) Low (Y:L) Medium (Y:M) High (Y:H) Critical (Y:C) | 0 0.25 0.5 0.75 1 |
SEVERITY COEFFICIENT () | COEFFICIENT VALUE | NUMERICAL VALUE |
---|---|---|
Reversibility () | None (R:N) Partial (R:P) Full (R:F) | 1 0.5 0.25 |
Scope () | Changed (S:C) Unchanged (S:U) | 1.25 1 |
Severity | Score Value Range |
---|---|
Critical | 9 - 10 |
High | 7 - 8.9 |
Medium | 4.5 - 6.9 |
Low | 2 - 4.4 |
Informational | 0 - 1.9 |
Critical
0
High
0
Medium
1
Low
2
Informational
3
Security analysis | Risk level | Remediation Date |
---|---|---|
HAL-01 - INAPPROPRIATE SLIPPAGE CONTROL | Medium | Risk Accepted - 03/06/2024 |
HAL-06 - IMPROPER OPERATOR STAKING AMOUNT CALCULATION | Low | Solved - 02/17/2024 |
HAL-04 - USING TRANSFER INSTEAD OF SAFETRANSFER | Low | Risk Accepted - 02/16/2024 |
HAL-05 - ARBITRARY SENDER USED IN PULLTOKENS | Informational | Acknowledged - 02/16/2024 |
HAL-02 - GAS INEFFICIENT ERROR HANDLING | Informational | Acknowledged - 02/16/2024 |
HAL-03 - GAS INEFFICENT INCREMENTATION | Informational | Acknowledged - 02/16/2024 |
// Medium
The swapStableToBCUT()
function of the Payments
contract uses the _swapExactInputSingle()
function to exchange tokens.
Exchanging tokens requires the user to specify the amountOutMinimum
parameter. If the swap results in less, the transaction reverts. This parameter is by the _getAmountOut()
function, which calculates the value based on the price just before the transaction.
Exchanging large amounts of tokens at once may encourage malicious users to perform a sandwich attack, causing loss to the protocol.
Note that the effectiveness of sandwich attacks largely depends on the available liquidity in the swap or liquidity pools or the number of tokens exchanged at once. Attackers usually only carry out such an attack if it is profitable for them. If they can predict the time of the swaps, they can increase the price seconds before the swap transaction is executed.
Input
contracts/Bitscrunch/Payments/Payments.sol
function swapStableToBCUT(uint256 _epoch, IERC20Upgradeable _stableCoin) external onlyManager nonReentrant {
_checkAndRevert(coins_[_stableCoin].active, "P_1");
_checkAndRevert(_epoch <= epochManager_.currentEpoch(), "P_2");
uint256 toSwap = epochBilledBalance_[_epoch][_stableCoin];
_checkAndRevert(toSwap > 0, "P_3");
uint256 toTreasury = _getShareAmount(toSwap, shares_.treasury, percentage_);
uint256 tokensLeft = toSwap - toTreasury;
uint256 toBcut = _getShareAmount(tokensLeft, shares_.bcut, percentage_);
uint256 stableCoinLeft = tokensLeft - toBcut;
uint256 estimatedAmountOut = _getAmountOut(address(_stableCoin),address(bcutToken_),uint128(toBcut));
uint256 bcutReceived = _swapExactInputSingle(address(_stableCoin), address(bcutToken_), toBcut, estimatedAmountOut);
balances_.treasury[address(_stableCoin)] += toTreasury;
balances_.coinBalance[_epoch][address(bcutToken_)] += bcutReceived;
balances_.coinBalance[_epoch][address(_stableCoin)] += stableCoinLeft;
It is recommended to allow the manager to configure the average price time frame for the calculation of the amountOutMinimum
value. The timeframe should be set so that malicious users can only raise and maintain the manipulated price more expensively than their expected profit from the sandwich attack.
Alternatively, the amountOutMinimum
parameter can be calculated off-chain and pass it to the swapStableToBCUT()
function. This approach is more gas efficient and allows the manual control of the minimum amount of tokens that the protocol expects to receive.
RISK ACCEPTED: The bitsCrunch team
accepted the risk of the identified issue regarding the swap function's operation within irregular intervals and the calculation of amountOutMinimum
.
// Low
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.
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_;
}
It is recommended to only increase the staked balance up to minimumStakeAmount_
during staking and withdraw the full staked balance during withdrawals.
SOLVED: The bitsCrunch team
solved the issue in commit 749b7f3.
// Low
It was identified that the pullTokens()
and pushTokens()
functions in the TokenUtils
contract use the IERC20Upgradeable
interface to interact with the _token
parameter 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.
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");
}
}
}
It is recommended to use OpenZeppelin's SafeERC20
wrapper with the IERC20Upgradeable
interface to make the contracts compatible with currencies that return no value.
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.
// Informational
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.
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);
}
It is recommended to only pull tokens from the msg.sender
or from a predefined address.
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.
// Informational
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
Consider implementing custom errors instead of reverting strings or using shorter error messages.
ACKNOWLEDGED: The bitsCrunch team
made a business decision to acknowledge this finding and not alter the contracts.
// Informational
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
Consider using prefix operation instead of postfix to increment the values of the uint
loop iterator variables.
ACKNOWLEDGED: The bitsCrunch team
made a business decision to acknowledge this finding and not alter the contracts.
Halborn used automated testing techniques to enhance the coverage of certain areas of the smart contracts in scope. Among the tools used was Slither, a Solidity static analysis framework. After Halborn verified the smart contracts in the repository and was able to compile them correctly into their abis and binary format, Slither was run against the contracts. This tool can statically verify mathematical relationships between Solidity variables to detect invalid or inconsistent usage of the contracts' APIs across the entire code-base. The security team assessed all findings identified by the Slither software, however, findings with severity Information
and Optimization
are not included in the below results for the sake of report readability.
contracts/Bitscrunch/*
contracts/Contributor/*
contracts/Epochs/*
contracts/GovernanceController/*
Slither did not identify any vulnerabilities.
contracts/Operator/*
contracts/Token/*
contracts/Utils/*
The findings obtained as a result of the Slither scan were reviewed.
The reentrancy, dangerous strict equality and local variable never initialized vulnerabilities were determined false-positives.
The arbitrary from in transferFrom vulnerability was added to the report.
Halborn strongly recommends conducting a follow-up assessment of the project either within six months or immediately following any material changes to the codebase, whichever comes first. This approach is crucial for maintaining the project’s integrity and addressing potential vulnerabilities introduced by code modifications.
// Download the full report
* Use Google Chrome for best results
** Check "Background Graphics" in the print settings if needed