BnbX - Stader Labs

Prepared by:

HALBORN

Last Updated 04/26/2024

Date of Engagement by: June 29th, 2022 - July 5th, 2022

Summary

100% of all REPORTED Findings have been addressed

All findings

Critical

High

Medium

Low

Informational

1. Introduction
2. Audit summary
3. Test approach & methodology
4. Scope
5. Risk methodology
6. Scope
7. Assessment summary & findings overview
8. Findings & Tech Details

8.1 Integer underflow
8.2 Calling increasetotalredelegated before user deposits may cause user getting 0 bnbx
8.3 Contracts are not using disableinitializers function
8.4 Missing address check

9. Automated Testing

1. INTRODUCTION

\client engaged Halborn to conduct a security audit on their smart contracts beginning on 2022-06-29 and ending on 2022-07-05. The security assessment was scoped to the smart contracts provided to the Halborn team.

2. AUDIT SUMMARY

The team at Halborn was provided one week for the engagement and assigned a full-time security engineer to audit the security of the smart contract. The security engineer is a blockchain and smart-contract security expert with advanced penetration testing, smart-contract hacking, and deep knowledge of multiple blockchain protocols.

The purpose of this audit is to:

Ensure that smart contract functions operate as intended
Identify potential security issues with the smart contracts

In summary, Halborn identified some security risks that were addressed by the \client team. The main ones are the following:

Ensure that increasing staked amount via increaseTotalRedelegated does not cause arithmetic issues in other functions.
Verify BNB/BNBX conversion logic, to ensure users are getting a share after deposit.

3. TEST APPROACH & METHODOLOGY

Halborn performed a combination of manual and automated security testing to balance efficiency, timeliness, practicality, and accuracy in regard to the scope of this audit. While manual testing is recommended to uncover flaws in logic, process, and implementation; automated testing techniques help enhance coverage of the bridge code and can quickly identify items that do not follow security best practices. The following phases and associated tools were used throughout the term of the audit:

Research into architecture and purpose
Smart contract manual code review and walk-through
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)
Local deployment (Brownie, Hardhat, Remix IDE)

4. SCOPE

The security assessment was scoped to the following smart contracts:

BnbX.sol
StakeManager.sol
IBnbX.sol
IStakeManager.sol

Commit ID: 2ddf3e2c30321587742630de90a1414434ff256f Remediation commit ID d56ab580231c56531edbb780387e1c711236c85d

5. RISK METHODOLOGY

Vulnerabilities or issues observed by Halborn are ranked based on the risk assessment methodology by measuring the LIKELIHOOD of a security incident and the IMPACT should an incident occur. This framework works for communicating the characteristics and impacts of technology vulnerabilities. The quantitative model ensures repeatable and accurate measurement while enabling users to see the underlying vulnerability characteristics that were used to generate the Risk scores. For every vulnerability, a risk level will be calculated on a scale of 5 to 1 with 5 being the highest likelihood or impact.

RISK SCALE - LIKELIHOOD

5 - Almost certain an incident will occur.
4 - High probability of an incident occurring.
3 - Potential of a security incident in the long term.
2 - Low probability of an incident occurring.
1 - Very unlikely issue will cause an incident.

RISK SCALE - IMPACT

5 - May cause devastating and unrecoverable impact or loss.
4 - May cause a significant level of impact or loss.
3 - May cause a partial impact or loss to many.
2 - May cause temporary impact or loss.
1 - May cause minimal or un-noticeable impact.

The risk level is then calculated using a sum of these two values, creating a value of 10 to 1 with 10 being the highest level of security risk.

Critical

High

Medium

Low

Informational

10 - CRITICAL
9 - 8 - HIGH
7 - 6 - MEDIUM
5 - 4 - LOW
3 - 1 - VERY LOW AND INFORMATIONAL

6. SCOPE

Out-of-Scope: New features/implementations after the remediation commit IDs.

7. Assessment Summary & Findings Overview

Critical

High

Medium

Low

Informational

Impact x Likelihood

Likelihood

Impact

HAL-01

HAL-02

HAL-03

HAL-04

Security analysis	Risk level	Remediation Date
INTEGER UNDERFLOW	High	Solved - 07/12/2022
CALLING increaseTotalRedelegated BEFORE USER DEPOSITS MAY CAUSE USER GETTING 0 BNBX	Medium	Solved - 07/12/2022
CONTRACTS ARE NOT USING disableInitializers FUNCTION	Low	Solved - 07/12/2022
MISSING ADDRESS CHECK	Informational	Solved - 07/12/2022

8. Findings & Tech Details

8.1 INTEGER UNDERFLOW

// High

Description

Calling the increaseTotalRedelegated function is causing an integer underflow in the startUndelegation function.

If the totalRedelegated amount is increased by calling increaseTotalRedelegated, the startUndelegation transaction will revert with: Arithmetic operation underflowed or overflowed outside an unchecked block error (in Solidity > 0.8). Because the undelegation process will fail, the user will not be able to withdraw the deposited BNB.

Code Location

The startUndelegation function performs a subtraction:

StakeManager.sol

function startUndelegation()
    external
    override
    whenNotPaused
    onlyRole(BOT)
    returns (uint256 _uuid, uint256 _amount)
{
    require(totalBnbToWithdraw > 0, "No Request to withdraw");

    _uuid = undelegateUUID++;
    _amount = totalBnbToWithdraw;
    uuidToBotUndelegateRequestMap[_uuid] = BotUndelegateRequest(
        block.timestamp,
        0,
        _amount
    );

    totalDeposited -= _amount;
    uint256 bnbXToBurn = totalBnbXToBurn; // To avoid Reentrancy attack
    totalBnbXToBurn = 0;
    totalBnbToWithdraw = 0;

    IBnbX(bnbX).burn(address(this), bnbXToBurn);
}

Where totalDeposited is an amount of BNB deposited and _amount is the value of totalBnbToWithdraw that is calculated in the requestWithdraw function:

StakeManager.sol

function requestWithdraw(uint256 _amount) external override whenNotPaused {
    require(_amount > 0, "Invalid Amount");
    uint256 amountInBnb = convertBnbXToBnb(_amount);

    IERC20Upgradeable(bnbX).safeTransferFrom(
        msg.sender,
        address(this),
        _amount
    );
    uint256 totalStakedBnb = getTotalStakedBnb();
    require(
        amountInBnb <= (totalStakedBnb - totalBnbToWithdraw),
        "Not enough BNB to withdraw"
    );

    totalBnbToWithdraw += amountInBnb;
    totalBnbXToBurn += _amount;
    userWithdrawalRequests[msg.sender].push(
        WithdrawalRequest(undelegateUUID, amountInBnb, block.timestamp)
    );

    emit RequestWithdraw(msg.sender, _amount, amountInBnb);
}

The value taken from convertBnbXToBnb is added to totalBnbToWithdraw. The convertBnbXToBnb function calculates its value based on the output of getTotalPooledBnb:

StakeManager.sol

uint256 totalPooledBnb = getTotalPooledBnb();

Which is using totalRedelegated set by the increaseTotalRedelegated function:

StakeManager.sol

function getTotalPooledBnb() public view override returns (uint256) {
    return (totalDeposited + totalRedelegated);
}

Score

Impact: 5
Likelihood: 3

Recommendation

SOLVED: The addRestakingRewards function (previously called increaseTotalRedelegated), now contains a check: the amount delegated must be greater than 0 to increase. Also, the startUndelegation function recalculates the BnbX/BNB ratio, instead of using a previously calculated one.

8.2 CALLING increaseTotalRedelegated BEFORE USER DEPOSITS MAY CAUSE USER GETTING 0 BNBX

// Medium

Description

If the increaseTotalRedelegated function is called when totalDeposited is 0 and the increased amount is greater than the amount of assets deposited by the first user, the depositor will get 0 BnbX tokens.

Deposited amount is 0 (none deposited yet)
increaseTotalRedelegated function is called with 10 BNB as parameter
User deposits 1 BNB

Result: The user gets 0 BnbX in return for a deposit of 1 BNB.

Code Location

StakeManager.sol

function convertBnbToBnbX(uint256 _amount)
    public
    view
    override
    returns (uint256)
{
    uint256 totalShares = IBnbX(bnbX).totalSupply();
    totalShares = totalShares == 0 ? 1 : totalShares;

    uint256 totalPooledBnb = getTotalPooledBnb();
    totalPooledBnb = totalPooledBnb == 0 ? 1 : totalPooledBnb;

    uint256 amountInBnbX = (_amount * totalShares) / totalPooledBnb;

    return amountInBnbX;
}

The getTotalPooledBnb calculation:

StakeManager.sol

function getTotalPooledBnb() public view override returns (uint256) {
    return (totalDeposited + totalRedelegated);
}

Score

Impact: 3
Likelihood: 3

Recommendation

SOLVED: The addRestakingRewards (previously called as increaseTotalRedelegated) function, now contains a check: the amount delegated must be greater than 0 to increase. This solves the problem of first depositor getting 0 BnbX tokens: Reference

8.3 CONTRACTS ARE NOT USING disableInitializers FUNCTION

// Low

Description

The StakeManager and BnbX contracts use Open Zeppelin Initializable module. According to the Open Zeppelin guidelines the _disableInitializers function call should be added to the constructor to lock contracts after deployment.

Score

Impact: 2
Likelihood: 2

Recommendation

SOLVED: The constructor with call to _disableInitializers() was added to StakeManager and BnbX contracts.

8.4 MISSING ADDRESS CHECK

// Informational

Description

The lack of zero address validation has been found in many instances when assigning user-supplied address values to state variables directly.

Code Location

BnbX.sol, #37-49 The setStakeManager function allows you to set a stakeManager address to 0x0.

StakeManager.sol, #70-73 The StakeManager's contract initialization function does not check that the passed addresses are non-0.

StakeManager.sol, #326-338 The setBotAddress function allows a bot's address to be set to 0x0.

Score

Impact: 1
Likelihood: 1

Recommendation

SOLVED: Added zero address checks in commit 4e04e46729153b6cb50d2ce4da2f807611fcc4d8

9. Automated Testing

STATIC ANALYSIS REPORT

Description

Halborn used automated testing techniques to enhance the coverage of certain areas of the scoped contracts. Among the tools used was Slither, a Solidity static analysis framework. After Halborn verified all the contracts in the repository and was able to compile them correctly into their ABI and binary formats, Slither was run on the all-scoped 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.

Slither results

StakeManager.sol

StakeManager.startDelegation() (contracts/StakeManager.sol#103-142) ignores return value by ITokenHub(tokenHub).transferOut{value: (amount + relayFeeReceived)}(address(0),bcDepositWallet,amount,expireTime) (contracts/StakeManager.sol#131-136)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#unused-return

StakeManager.initialize(address,address,address,address,address)._bnbX (contracts/StakeManager.sol#58) lacks a zero-check on :
        - bnbX = _bnbX (contracts/StakeManager.sol#70)
StakeManager.initialize(address,address,address,address,address)._tokenHub (contracts/StakeManager.sol#60) lacks a zero-check on :
        - tokenHub = _tokenHub (contracts/StakeManager.sol#71)
StakeManager.initialize(address,address,address,address,address)._bcDepositWallet (contracts/StakeManager.sol#61) lacks a zero-check on :
        - bcDepositWallet = _bcDepositWallet (contracts/StakeManager.sol#72)
StakeManager.initialize(address,address,address,address,address)._bot (contracts/StakeManager.sol#62) lacks a zero-check on :
        - bot = _bot (contracts/StakeManager.sol#73)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#missing-zero-address-validation

Reentrancy in StakeManager.requestWithdraw(uint256) (contracts/StakeManager.sol#191-213):
    External calls:
    - IERC20Upgradeable(bnbX).safeTransferFrom(msg.sender,address(this),_amount) (contracts/StakeManager.sol#195-199)
    State variables written after the call(s):
    - totalBnbToWithdraw += amountInBnb (contracts/StakeManager.sol#206)
    - totalBnbXToBurn += _amount (contracts/StakeManager.sol#207)
    - userWithdrawalRequests[msg.sender].push(WithdrawalRequest(undelegateUUID,amountInBnb,block.timestamp)) (contracts/StakeManager.sol#208-210)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#reentrancy-vulnerabilities-2

Reentrancy in StakeManager.claimWithdraw(uint256) (contracts/StakeManager.sol#215-235):
    External calls:
    - AddressUpgradeable.sendValue(address(user),amount) (contracts/StakeManager.sol#232)
    Event emitted after the call(s):
    - ClaimWithdrawal(user,_idx,amount) (contracts/StakeManager.sol#234)
Reentrancy in StakeManager.requestWithdraw(uint256) (contracts/StakeManager.sol#191-213):
    External calls:
    - IERC20Upgradeable(bnbX).safeTransferFrom(msg.sender,address(this),_amount) (contracts/StakeManager.sol#195-199)
    Event emitted after the call(s):
    - RequestWithdraw(msg.sender,_amount,amountInBnb) (contracts/StakeManager.sol#212)
Reentrancy in StakeManager.startDelegation() (contracts/StakeManager.sol#103-142):
    External calls:
    - ITokenHub(tokenHub).transferOut{value: (amount + relayFeeReceived)}(address(0),bcDepositWallet,amount,expireTime) (contracts/StakeManager.sol#131-136)
    Event emitted after the call(s):
    - TransferOut(amount) (contracts/StakeManager.sol#138)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#reentrancy-vulnerabilities-3

StakeManager.completeDelegation(uint256) (contracts/StakeManager.sol#149-166) uses timestamp for comparisons
    Dangerous comparisons:
    - require(bool,string)((uuidToBotDelegateRequestMap[_uuid].amount > 0) && (uuidToBotDelegateRequestMap[_uuid].endTime == 0),Invalid UUID) (contracts/StakeManager.sol#155-159)
StakeManager.claimWithdraw(uint256) (contracts/StakeManager.sol#215-235) uses timestamp for comparisons
    Dangerous comparisons:
    - require(bool,string)(uuidToBotUndelegateRequestMap[uuid].endTime != 0,Not able to claim yet) (contracts/StakeManager.sol#225-228)
StakeManager.isClaimable(address,uint256) (contracts/StakeManager.sol#243-257) uses timestamp for comparisons
    Dangerous comparisons:
    - _isClaimable = (uuidToBotUndelegateRequestMap[uuid].endTime != 0) (contracts/StakeManager.sol#256)
StakeManager.completeUndelegation(uint256) (contracts/StakeManager.sol#297-318) uses timestamp for comparisons
    Dangerous comparisons:
    - require(bool,string)((uuidToBotUndelegateRequestMap[_uuid].amount > 0) && (uuidToBotUndelegateRequestMap[_uuid].endTime == 0),Invalid UUID) (contracts/StakeManager.sol#304-308)
    - require(bool,string)(amount == uuidToBotUndelegateRequestMap[_uuid].amount,Incorrect Amount of Fund) (contracts/StakeManager.sol#311-314)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#block-timestamp
contracts/StakeManager.sol analyzed (15 contracts with 57 detectors), 13 result(s) found

BnbX.sol:

contracts/BnbX.sol analyzed (13 contracts with 57 detectors), 0 result(s) found

As a result of the tests carried out with the Slither tool, some results were obtained and reviewed by Halborn. Based on the results reviewed, some vulnerabilities were determined to be false positives.

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.

1. Introduction
2. Audit summary
3. Test approach & methodology
4. Scope
5. Risk methodology
6. Scope
7. Assessment summary & findings overview
8. Findings & Tech Details

8.1 Integer underflow
8.2 Calling increasetotalredelegated before user deposits may cause user getting 0 bnbx
8.3 Contracts are not using disableinitializers function
8.4 Missing address check

9. Automated Testing

// Download the full report

* Use Google Chrome for best results

** Check "Background Graphics" in the print settings if needed

BnbX

Stader Labs

Table of Contents

1. INTRODUCTION

2. AUDIT SUMMARY

3. TEST APPROACH & METHODOLOGY

4. SCOPE

5. RISK METHODOLOGY

6. SCOPE

7. Assessment Summary & Findings Overview

8. Findings & Tech Details

8.1 INTEGER UNDERFLOW

Description

Code Location

StakeManager.sol

StakeManager.sol

StakeManager.sol

StakeManager.sol

Score

Recommendation

8.2 CALLING increaseTotalRedelegated BEFORE USER DEPOSITS MAY CAUSE USER GETTING 0 BNBX

Description

Code Location

StakeManager.sol

StakeManager.sol

Score

Recommendation

8.3 CONTRACTS ARE NOT USING disableInitializers FUNCTION

Description

Score

Recommendation

8.4 MISSING ADDRESS CHECK

Description

Code Location

Score

Recommendation

9. Automated Testing

STATIC ANALYSIS REPORT

Description

Slither results

Table of Contents