Ecosystem - Merge Marketplace b14g - CoreDAO


Prepared by:

Halborn Logo

HALBORN

Last Updated 02/17/2025

Date of Engagement: January 28th, 2025 - February 5th, 2025

Summary

100% of all REPORTED Findings have been addressed

All findings

30

Critical

0

High

0

Medium

0

Low

7

Informational

23


1. Introduction

CoreDAO engaged Halborn to conduct a security assessment of the new B14G Contracts project by B14G beginning on January 28th and ending on February 05th. The security assessment was scoped to the smart contracts in the contracts GitHub repository of b14glabs provided to the Halborn team. Commit hashes and further details can be found in the Scope section of this report.

2. Assessment Summary

Halborn was provided 3.5 days for the engagement and assigned one full-time security engineer to review the security of the smart contract in scope. The engineer is a blockchain and smart contract security expert 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 improvements to reduce the likelihood and impact of risks, which were partially addressed by the B14G team. The main ones are the following:

    • Consider implementing restrictions or mitigations to prevent abuse of the createRewardReceiver() function.

    • Ensure initializers cannot be frontrun by including the initialization in the proxy creation or by using a factory.

    • For the StakeCore and WithdrawCore event, align the parameter names in both the event definitions and their emissions.

3. Test Approach and 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 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 (Hardhat and Foundry).

4. RISK METHODOLOGY

Every vulnerability and issue observed by Halborn is ranked based on two sets of Metrics and a Severity Coefficient. This system is inspired by the industry standard Common Vulnerability Scoring System.
The two Metric sets are: Exploitability and Impact. Exploitability captures the ease and technical means by which vulnerabilities can be exploited and Impact describes the consequences of a successful exploit.
The Severity Coefficients is designed to further refine the accuracy of the ranking with two factors: Reversibility and Scope. These capture the impact of the vulnerability on the environment as well as the number of users and smart contracts affected.
The final score is a value between 0-10 rounded up to 1 decimal place and 10 corresponding to the highest security risk. This provides an objective and accurate rating of the severity of security vulnerabilities in smart contracts.
The system is designed to assist in identifying and prioritizing vulnerabilities based on their level of risk to address the most critical issues in a timely manner.

4.1 EXPLOITABILITY

Attack Origin (AO):
Captures whether the attack requires compromising a specific account.
Attack Cost (AC):
Captures the cost of exploiting the vulnerability incurred by the attacker relative to sending a single transaction on the relevant blockchain. Includes but is not limited to financial and computational cost.
Attack Complexity (AX):
Describes the conditions beyond the attacker’s control that must exist in order to exploit the vulnerability. Includes but is not limited to macro situation, available third-party liquidity and regulatory challenges.
Metrics:
EXPLOITABILITY METRIC (mem_e)METRIC VALUENUMERICAL 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
Exploitability EE is calculated using the following formula:

E=meE = \prod m_e

4.2 IMPACT

Confidentiality (C):
Measures the impact to the confidentiality of the information resources managed by the contract due to a successfully exploited vulnerability. Confidentiality refers to limiting access to authorized users only.
Integrity (I):
Measures the impact to integrity of a successfully exploited vulnerability. Integrity refers to the trustworthiness and veracity of data stored and/or processed on-chain. Integrity impact directly affecting Deposit or Yield records is excluded.
Availability (A):
Measures the impact to the availability of the impacted component resulting from a successfully exploited vulnerability. This metric refers to smart contract features and functionality, not state. Availability impact directly affecting Deposit or Yield is excluded.
Deposit (D):
Measures the impact to the deposits made to the contract by either users or owners.
Yield (Y):
Measures the impact to the yield generated by the contract for either users or owners.
Metrics:
IMPACT METRIC (mIm_I)METRIC VALUENUMERICAL 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
Impact II is calculated using the following formula:

I=max(mI)+mImax(mI)4I = max(m_I) + \frac{\sum{m_I} - max(m_I)}{4}

4.3 SEVERITY COEFFICIENT

Reversibility (R):
Describes the share of the exploited vulnerability effects that can be reversed. For upgradeable contracts, assume the contract private key is available.
Scope (S):
Captures whether a vulnerability in one vulnerable contract impacts resources in other contracts.
Metrics:
SEVERITY COEFFICIENT (CC)COEFFICIENT VALUENUMERICAL VALUE
Reversibility (rr)None (R:N)
Partial (R:P)
Full (R:F)
1
0.5
0.25
Scope (ss)Changed (S:C)
Unchanged (S:U)
1.25
1
Severity Coefficient CC is obtained by the following product:

C=rsC = rs

The Vulnerability Severity Score SS is obtained by:

S=min(10,EIC10)S = min(10, EIC * 10)

The score is rounded up to 1 decimal places.
SeverityScore Value Range
Critical9 - 10
High7 - 8.9
Medium4.5 - 6.9
Low2 - 4.4
Informational0 - 1.9

5. SCOPE

Files and Repository
(a) Repository: contracts
(b) Assessed Commit ID: 5638d2e
(c) Items in scope:
  • contracts/utils/Address.sol
  • contracts/utils/Context.sol
  • contracts/utils/OnwnableWithPausable.sol
↓ Expand ↓
Out-of-Scope: Third party dependencies and economic attacks.
Remediation Commit ID:
Out-of-Scope: New features/implementations after the remediation commit IDs.

6. Assessment Summary & Findings Overview

Critical

0

High

0

Medium

0

Low

7

Informational

23

Security analysisRisk levelRemediation Date
Unrestricted Creation of Reward ReceiversLowRisk Accepted - 02/13/2025
Initializers Could Be Front-runLowRisk Accepted - 02/13/2025
Incorrect Event Parameter NamingLowSolved - 02/11/2025
Implementation Contracts Can Be InitializedLowFuture Release - 02/13/2025
Lack Of Storage Gap In Upgradeable ContractLowRisk Accepted - 02/13/2025
Missing Input ValidationLowFuture Release - 02/13/2025
Centralization RiskLowFuture Release - 02/13/2025
Direct Inclusion and Modification of OpenZeppelin Modules Leading to InconsistenciesInformationalFuture Release - 02/13/2025
Contract Name Reused in Different FilesInformationalAcknowledged - 02/13/2025
Potential Improvements to The Update Receiver ImplementationInformationalFuture Release - 02/13/2025
Owner Can Renounce OwnershipInformationalAcknowledged - 02/13/2025
Incomplete NatSpec DocumentationInformationalAcknowledged - 02/13/2025
Typos in InterfacesInformationalSolved - 02/11/2025
Commented-Out CodeInformationalFuture Release - 02/13/2025
Potential License IncompatibilitiesInformationalFuture Release - 02/13/2025
Lack of Event EmissionInformationalFuture Release - 02/13/2025
Debugging Import Included in Production CodeInformationalFuture Release - 02/13/2025
Magic Numbers in UseInformationalFuture Release - 02/13/2025
Unlocked Pragma CompilerInformationalFuture Release - 02/13/2025
Missing Visibility AttributeInformationalAcknowledged - 02/13/2025
Use of Revert Strings Instead of Custom ErrorInformationalFuture Release - 02/13/2025
Consider Using Named MappingsInformationalAcknowledged - 02/13/2025
Style Guide OptimizationsInformationalAcknowledged - 02/13/2025
Cache Array Length Outside of LoopInformationalFuture Release - 02/13/2025
Events Missing Indexed FieldsInformationalFuture Release - 02/13/2025
Use Calldata For Function Arguments Not MutatedInformationalFuture Release - 02/13/2025
Mixed msg.sender and _msgSenderInformationalFuture Release - 02/13/2025
Inconsistency Declaring uint256/uintInformationalAcknowledged - 02/13/2025
Unused or Dead CodeInformationalFuture Release - 02/13/2025
Empty Require StatementsInformationalSolved - 02/11/2025

7. Findings & Tech Details

7.1 Unrestricted Creation of Reward Receivers

//

Low

Description

In the Marketplace contract, the createRewardReceiver() function allows any user to create a new reward receiver without any access control or restrictions. Each new reward receiver is deployed as a proxy via the ProxyAdmin contract and added to the rewardReceivers array.

function createRewardReceiver(uint _portion) public whenNotPaused {
    address rewardReceiver = address(new ProxyAdmin(rewardReceiversImpl, address(this)));
    IRewardReceiver(rewardReceiver).initialize(msg.sender, _portion, fee, address(this));
    rewardReceivers.push(rewardReceiver);
    rewardReceiversPerAddress[msg.sender].push(rewardReceiver);
    emit CreateRewardReceiver(msg.sender, rewardReceiver, _portion, block.timestamp);
}

In the MergeMarketplaceStrategy contract, the reInvest() function uses the getRewardReceiverBatch() function of the Marketplace to fetch reward receivers for staking and withdrawing operations. If many reward receivers are created, this could result in high gas costs for operations that iterate through these receivers (e.g., in reInvest()).


Without proper limitations, malicious actors could flood the system with reward receivers, degrading performance and increasing transaction costs for valid users.

BVSS
Recommendation

Consider implementing restrictions or mitigations to prevent abuse of the createRewardReceiver() function.

Remediation

RISK ACCEPTED: The B14G team accepted the risk of this finding after analysing the actual impact. The risk was reduced from Medium to Low.

7.2 Initializers Could Be Front-run

//

Low

Description

According to the documentation, CoreVault, Marketplace and MergeMarketplaceStrategy contracts are meant to be upgradeable contracts. Initializers could be front-run, allowing an attacker to either set their own values, take ownership of the contract, and in the best case forcing a re-deployment.


Analyzing the deployment scripts, it can be observed that implementations are first deployed, then the proxies (using ProxyAdmin) and, finally, the proxies are initialized:

const CoreVault = await ethers.getContractFactory("CoreVault");
const coreVaultImpl = await CoreVault.deploy();
await coreVaultImpl.deployed();
const coreVaultProxy = await ProxyAdmin.deploy(coreVaultImpl.address, accounts[0].address);
await coreVaultProxy.deployed();
console.log("CoreVault deployed to:", coreVaultProxy.address);
const coreVault = CoreVault.attach(coreVaultProxy.address);
let tx = await coreVault.initialize(accounts[0].address, accounts[0].address, 1000, 100, 1800, parseEther("100000"));
await tx.wait();

And testing too:

const CoreVault = await ethers.getContractFactory("CoreVault");
const coreVaultImpl = await CoreVault.deploy();
const coreVaultProxy = await ProxyAdmin.deploy(coreVaultImpl.address, accounts[0].address);
const coreVault = CoreVault.attach(coreVaultProxy.address);
await coreVault.initialize(accounts[0].address, accounts[0].address, 1000, 100, 86400, parseEther("100"));
BVSS
Recommendation

It is recommended the following:

  • Include initialization call in the proxy setup

  • Use a factory to deploy and intializer in the same transaction.

Remediation

RISK ACCEPTED: The B14G team accepted the risk of this finding.

7.3 Incorrect Event Parameter Naming

//

Low

Description

In the RewardReceiver contract, the StakeCore and WithdrawCore events are emitted with parameters named to, candidate, and msg.value (or amount):

function stakeCore(bytes32 _txHash, address candidate, address to) payable external nonReentrant _finish(_txHash) onlyMarketplace {
  ...
  emit StakeCore(to, candidate, msg.value);
}

function withdrawCore(address candidate, uint amount, address to) external nonReentrant onlyMarketplace {
  ...
  emit WithdrawCore(to, candidate, amount);
}

However, in the IRewardReceiver interface, these events are defined with parameters named from, candidate, and amount:

event StakeCore(address indexed from, address indexed candidate, uint amount);
event WithdrawCore(address indexed from, address indexed candidate, uint amount);

This discrepancy in parameter naming (specifically, the use of from in the interface and to in the implementation) can cause confusion. Developers and integrators might misinterpret the event data, leading to potential errors in event handling and data analysis.

BVSS
Recommendation

Align the parameter names in both the event definitions and their emissions to ensure consistency. For instance, if the parameter represents the sender of the tokens, use from; if it represents the receiver, use to. Consistent naming enhances code readability and reduces the likelihood of misinterpretation by developers and integrators.

Remediation

SOLVED: The B14G team fixed this finding in commit 61db39e by correcting the event discrepancies.

Remediation Hash

7.4 Implementation Contracts Can Be Initialized

//

Low

Description

According to the documentation, CoreVault, MergeMarketplaceStrategy and Marketplace contracts are upgradeable contracts. In order to prevent leaving the contracts uninitialized, OpenZeppelin's documentation recommends adding the _disableInitializers() function in the constructor to automatically lock the contracts when they are deployed. However, all upgradeable contracts in scope are missing this protection.


This omission can lead to potential security vulnerabilities, as an uninitialized implementation contract can be taken over by an attacker, which may impact the proxy.

BVSS
Recommendation

Consider adding a constructor and calling the _disableinitializers() method from the OpenZeppelin module Initializable to prevent the implementation from being initialized.

Remediation

PENDING: The B14G team indicated that they will be addressing this finding in the future.

7.5 Lack Of Storage Gap In Upgradeable Contract

//

Low

Description

The MergeMarketplaceStrategy, Marketplace and RewardReceiver contracts are supposed to be designed to be upgradeable contracts. They inherit from OnwnableWithPausable, but it lacks storage gaps. Storage gaps are essential for ensuring that new state variables can be added in future upgrades without affecting the storage layout of inheriting child contracts. Without it, any addition of new state variables in future contract versions can lead to storage collisions.

BVSS
Recommendation

Consider adding a storage gap as the last storage variable to the implementation contracts, specially those meant to be inherited like OnwnableWithPausable.

Remediation

RISK ACCEPTED: The B14G team accepted the risk of this finding.

7.6 Missing Input Validation

//

Low

Description

During the security assessment, it was identified that some functions across the smart contracts lack proper input validation, allowing critical parameters to be set to undesired or unrealistic values. This can lead to potential vulnerabilities, unexpected behavior, or erroneous states within the contract.


Examples include, but are not limited to:

  • All initialize() functions of the project are missing input validation.

  • Most of the setters also lack a proper input validation. Therefore, for instance, it is possible to set an unlock period of 0 or unrealistically large.


This list is not exhaustive. It is recommended to conduct a comprehensive review of the codebase to identify and assess other functions that may require additional input validation. Ensuring appropriate checks are in place for critical parameters will enhance the overall reliability, security, and predictability of the contracts.

BVSS
Recommendation

To mitigate these issues, implement input validation in all constructor functions and other critical functions to ensure that inputs meet expected criteria. This can prevent unexpected behaviors and potential vulnerabilities.

Remediation

PENDING: The B14G team indicated that they will be addressing this finding in the future.

7.7 Centralization Risk

//

Low

Description

The contracts implement extensive owner privileges through Ownable2Step inheritance, granting significant control over critical protocol parameters and functionality. Furthermore, in the RewardReceiver contract, the owner address cannot be changed.


This centralization of power creates significant risks:

  • Protocol functionality could be compromised if owner keys are lost or stolen.

  • Users funds could be affected through malicious fee manipulation.

  • Single point of failure in critical protocol operations.

BVSS
Recommendation

Consider the following recommendations:

  • Implement Multi-Signature Requirements: Require multi-signature approval for critical functions. This distributes control and mitigates the risk of unilateral actions by a single party.


  • Introduce Governance Mechanisms: Consider integrating on-chain governance to manage changes to depositories and transfer permissions. This would enable the community to have a say in significant decisions and enhance decentralization.


  • Add Role-Based Restrictions: Limit the owner role's power by creating additional roles or access levels for specific functions.


  • Time Locks: Introduce time locks for critical functions, such as modifying depositories, changing transfer permissions, or altering the minter role. This allows stakeholders to review proposed changes and act if necessary before the changes are finalized. Time locks enhance transparency and provide a safeguard against unilateral and immediate decisions.

Remediation

PENDING: The B14G team indicated that they will be addressing this finding in the future.

7.8 Direct Inclusion and Modification of OpenZeppelin Modules Leading to Inconsistencies

//

Informational

Description

The review identified that the project directly includes copies of OpenZeppelin modules within its repository (e.g., some files under contracts/erc20/, contracts/utils/ and contracts/library/) and applies modifications to these files. Notably, the project contains:

  • Two separate copies of the Address.sol file (one under contracts/library/ and another under contracts/utils/), each marked with "OpenZeppelin Contracts (last updated v4.8.0) (utils/Address.sol)".

  • A custom interface IDualCore.sol that is derived from the OpenZeppelin IERC20.sol interface with modifications.


Additionally, contracts like CoreVault and MergeMarketplaceStrategy use a custom OnwnableWithPausable module, which is a combination of Ownable2Step and Pausable. However, Marketplace is also using Ownable2Step, but implementing the pausable logic inside. Furthermore, notice the OnwnableWithPausable module has a typo and should be OwnableWithPausable instead.


Finally, a further issue was observed with the Math library. In version v4.8.0 of the included OpenZeppelin contracts, the function log256 is documented incorrectly. The outdated documentation comment reads:

/**
 * @dev Return the log in base 10, following the selected rounding direction, of a positive value.
 * Returns 0 if given 0.
 */

Whereas the correct documentation (as updated in later commits, see commit 3a3c87b1a676f277c17a4601de56ddfc432d427d) should specify that the function returns the log in base 256. Retaining an outdated and incorrect version not only causes confusion for auditors and developers but also raises the risk of misinterpretation or misimplementation of mathematical operations.


This approach poses several risks:

  • Security and Maintenance Drift: OpenZeppelin contracts are thoroughly audited and maintained. By copying and modifying these files, the project bypasses the continuous improvements and security patches provided by the official library. This may inadvertently introduce vulnerabilities or inconsistencies with the audited versions.

  • Upgrade and Compatibility Issues: Custom modifications mean that future updates or bug fixes released by OpenZeppelin will not be automatically integrated. This increases the risk of the project using outdated or insecure implementations.

  • Code Duplication and Divergence: Maintaining multiple copies of similar code (as seen with Address.sol) can lead to inconsistencies and maintenance challenges. This duplication increases the attack surface and complicates the auditing and review process.

BVSS
Recommendation

It is recommended the following:

  • Utilize Dependency Management: Instead of copying OpenZeppelin contracts directly into the repository, consider using a package manager (such as npm or yarn) to include the OpenZeppelin libraries as external dependencies. This practice ensures access to the latest security updates and improvements.

  • Minimize Unnecessary Modifications: When modifications are necessary, it is recommended to fork the OpenZeppelin repository and document the changes along with their justifications.

  • Eliminate Redundant Code: Remove duplicate copies of modules (for instance, consolidate the Address.sol file into a single location) to prevent inconsistencies and simplify future updates.


By adhering to these recommendations, the project can benefit from the robust security and maintenance practices provided by the official OpenZeppelin libraries while minimizing the risks associated with direct modifications and code duplication.

Remediation

PENDING: The B14G team indicated that they will be addressing this finding in the future.

7.9 Contract Name Reused in Different Files

//

Informational

Description

The name Address and ReentrancyGuardProxy is used more than once to declare a library or contract inside the project. Specifically:

  • Address inside contracts/library/Address.sol

  • Address inside contracts/utils/Address.sol

  • ReentrancyGuardProxy inside contracts/marketplace/RewardReceiver.sol

BVSS
Recommendation

As indicated in the finding "Direct Inclusion and Modification of OpenZeppelin Modules Leading to Inconsistencies", consider using the well-known OpenZeppelin modules. If not, consider unifying all repeated contracts in a single file.

Remediation

ACKNOWLEDGED: The B14G team acknowledged this finding.

7.10 Potential Improvements to The Update Receiver Implementation

//

Informational

Description

The upgradeSingleReceiverImplementation() function in Marketplace has some architectural and security concerns regarding its proxy use and upgrade implementation:


1. Lack of Receiver Validation: The function does not validate that the provided receiver address is a legitimate reward receiver created by the Marketplace contract. This omission could allow the owner to inadvertently or maliciously upgrade contracts that are not part of the intended reward receiver set:

function upgradeSingleReceiverImplementation(address payable receiver) public onlyOwner {
    ProxyAdmin(receiver).upgradeTo(rewardReceiversImpl);
}

2. Inefficient Upgrade Process: If the design intent is for all reward receivers to run the same logic, individually upgrading each proxy is inefficient. A Beacon proxy pattern would ensure that all reward receivers automatically point to the same implementation, simplifying upgrades and reducing gas costs.


3. Custom Proxy Implementation Risks: As highlighted by the "Direct Inclusion and Modification of OpenZeppelin Modules Leading to Inconsistencies" finding, using a custom proxy solution increases the risk of subtle bugs. Adopting battle-tested upgrade modules from reputable libraries like OpenZeppelin can mitigate these risks.


4. Limited Administrative Flexibility: With Marketplace set as the owner of the receiver proxies, administrative functions such as changeAdmin() become ineffective. This inflexibility may hinder future maintenance or administrative interventions.


5. Potential Front-Running Vulnerabilities: The use of upgradeTo() (without an atomic initialization step) could allow an attacker to front-run the upgrade process. Switching to upgradeToAndCall() would combine the upgrade and initialization steps into a single atomic transaction, reducing this risk.

BVSS
Recommendation

It is recommended the following:


  1. Add Receiver Validation: Implement checks to ensure the receiver address is a valid reward receiver managed by the Marketplace contract.

  2. Adopt Beacon Proxy Pattern: Consider migrating to Beacon proxies for reward receivers to ensure consistent logic upgrades and improved efficiency.

  3. Utilize Battle-Tested Modules: Replace the custom proxy solution with a reputable, battle-tested upgradeability library (e.g., OpenZeppelin’s proxy contracts).

  4. Improve Administrative Flexibility: Revisit the admin model to allow essential administrative functions (e.g., changeAdmin()) if necessary.

  5. Use Atomic Upgrade Methods: Replace upgradeTo() with upgradeToAndCall() to perform atomic upgrades, mitigating front-running risks during the upgrade process.

Remediation

PENDING: The B14G team indicated that they will be addressing this finding in the future.

7.11 Owner Can Renounce Ownership

//

Informational

Description

It was identified that the Marketplace contract indirectly inherits from Ownable2Step through OnwnableWithPausable. In the Ownable2Step contract, the renounceOwnership() function is used to renounce the Owner permission. Renouncing ownership before transferring would result in the contract having no owner, eliminating the ability to call privileged functions.

/**
 * @dev Leaves the contract without owner. It will not be possible to call
 * `onlyOwner` functions. Can only be called by the current owner.
 *
 * NOTE: Renouncing ownership will leave the contract without an owner,
 * thereby disabling any functionality that is only available to the owner.
 */
function renounceOwnership() public virtual onlyOwner {
    _transferOwnership(address(0));
}

Furthermore, the contract owner or single user with a role is not prevented from renouncing the role/ownership while the contract is paused, which would cause any user assets stored in the protocol, to be locked indefinitely.

BVSS
Recommendation

It is recommended that the Owner cannot call renounceOwnership() 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 for two or more users.

Remediation

ACKNOWLEDGED: The B14G team acknowledged this finding.

7.12 Incomplete NatSpec Documentation

//

Informational

Description

The B14G project lacks comprehensive NatSpec comments for functions, state variables, and contracts. NatSpec is a widely adopted standard for documenting Solidity contracts, providing clear and structured explanations for developers, auditors, and end-users.

BVSS
Recommendation

Adopt and implement NatSpec comments across all contracts, functions, and state variables. Use the following structure as a guideline:


  1. Contract-Level Documentation: Include a brief overview of the contract's purpose, scope, and high-level details. For example:

  2. Function-Level Documentation: Document each function using NatSpec annotations, detailing:

    • @param descriptions for function parameters.

    • @return descriptions for return values.

    • @dev notes for developers about implementation details or caveats.

    • @notice for user-facing descriptions.

  3. State Variable Documentation: Document each variable with a concise explanation of its purpose and usage.

  4. Global Guidelines:

    • Use tools like solhint to enforce NatSpec standards.

    • Regularly review documentation to ensure alignment with code updates.


By adding NatSpec documentation, the project can improve code clarity, facilitate audits, and enhance developer and user trust in the protocol.

Remediation

ACKNOWLEDGED: The B14G team acknowledged this finding.

7.13 Typos in Interfaces

//

Informational

Description

Two typographical errors were found in the interfaces used by the project. While they may not be introduced by the team, they have been listed for the sake of completeness:


  • In IEarn.sol:

// Amount of CORE the user recieves
...
// Amount of CORE the protocol recieves

recieves should be recieves.


  • In IBitcoinStake.sol:

function accuredRewardPerBTCMap(address, uint256) external view returns (uint256);

accuredRewardPerBTCMap() should be accruedRewardPerBTCMap().

BVSS
Recommendation

To maintain clarity and trustworthiness, it is essential to rectify any typographical errors present within the contracts. Correcting such errors minimizes the likelihood of confusion and reinforces confidence in the accuracy and integrity of the documentation.

Remediation

SOLVED: The B14G team fixed this finding in commit 3545f22 by correcting the typographical errors.

Remediation Hash

7.14 Commented-Out Code

//

Informational

Description

In ProxyAdmin.sol, the constructor function includes commented-out lines of code. While these comments may have been added during development or testing, leaving them in production code creates ambiguity and potential risks:

constructor(address _logicAddress) payable {
    assert(IMPLEMENTATION_SLOT == bytes32(uint256(keccak256('eip1967.proxy.implementation')) - 1));
    _setImplementation(_logicAddress);
    // if (_data.length > 0) {
    //     (bool success,) = _logic.delegatecall(_data);
    //     require(success);
    // }
}
BVSS
Recommendation

Remove irrelevant commented-out code. Leaving commented-out code in production contracts introduces unnecessary ambiguity and risks. By removing or clarifying these lines, the code becomes more maintainable, transparent, and less prone to errors in future updates.

Remediation

PENDING: The B14G team indicated that they will be addressing this finding in the future.

7.15 Potential License Incompatibilities

//

Informational

Description

The project contains contracts with varying SPDX license identifiers:

  • Some contracts specify // SPDX-License-Identifier: Apache-2.0.

  • Others specify // SPDX-License-Identifier: MIT.

  • Some use // SPDX-License-Identifier: UNLICENSED.


This inconsistency can result in potential license incompatibilities or legal ambiguities regarding the use, distribution, and modification of the codebase. Specifically:

  • The MIT License is permissive, allowing free use, modification, and redistribution with attribution.

  • The Apache 2.0 License is also permissive but includes explicit patent grants and requires a more detailed notice, including a copy of the license and a notice of any modifications.

  • The UNLICENSED identifier indicates that the code is not licensed under any open-source terms, which may default to "all rights reserved" in some jurisdictions, limiting its usability.


While the MIT and Apache 2.0 licenses are generally compatible with each other, mixing them without clear documentation can create confusion. The presence of UNLICENSED files further complicates the legal standing of the codebase.

BVSS
Recommendation

To mitigate potential legal risks:

  1. Standardize Licensing:

    • Choose a single open-source license that aligns with the project's goals and ensure all contracts uniformly declare this license.

    • If multiple licenses are necessary, clearly document which parts of the codebase are under which licenses and ensure they are compatible.

  2. Avoid UNLICENSED Identifier:

    • Refrain from using the UNLICENSED identifier unless the intention is to restrict usage. If the code is meant to be open-source, apply an appropriate open-source license.

  3. Review License Compatibility:

    • If retaining multiple licenses, consult with legal counsel to ensure compatibility and compliance with all applicable licensing terms.


By standardizing the licensing across the codebase and ensuring clarity in licensing terms, the project can prevent legal ambiguities and facilitate easier use, distribution, and contribution.

Remediation

PENDING: The B14G team indicated that they will be addressing this finding in the future.

7.16 Lack of Event Emission

//

Informational

Description

It has been observed that some functionalities are missing event emissions. Events are essential for notifying external observers about critical state changes within a smart contract. They allow transaction initiators and external monitoring tools to track actions, enhancing transparency and usability.


Failing to emit events can lead to:

  • Reduced transparency of critical state changes.

  • Challenges for users and developers in debugging and auditing.

  • Missed opportunities for off-chain systems to react to state changes effectively.


Examples:

  • All initialize() functions of the project are missing an event emission

  • updateRewardReceiversImpl() in Marketplace.

  • The functions finish() and finishAndDelegate() in RewardReceiver.


This list is not exhaustive, and a thorough review of the entire codebase is recommended to identify additional instances where event emissions can improve transparency and traceability.

BVSS
Recommendation

All functions updating important parameters should emit events.

Remediation

PENDING: The B14G team indicated that they will be addressing this finding in the future.

7.17 Debugging Import Included in Production Code

//

Informational

Description

Most contracts in scope include the following import:

import "hardhat/console.sol";

The console.sol library is a debugging utility from Foundry, typically used for logging during development and testing. Including it in production contracts increases gas costs and can unintentionally expose sensitive information or internal logic.

BVSS
Recommendation

Remove the hardhat/console.sol import and any associated debugging statements from production code before deployment.

Remediation

PENDING: The B14G team indicated that they will be addressing this finding in the future.

7.18 Magic Numbers in Use

//

Informational

Description

In programming, "magic numbers" refer to the use of hard-coded numerical or string values directly in the code without clear indication of their purpose or origin. The use of magic numbers can lead to confusion and make the codebase more difficult to understand, maintain, and update.


Several instances of magic numbers were found in the provided contracts. These values lack meaningful context, making it unclear whether they are arbitrary, derived from a specific standard, or require future modification. Their presence increases the likelihood of errors during maintenance or updates.


  • In Marketplace.sol:

require(_fee < 1000, "Invalid fee");

The maximum fee in CoreVault is 10_000, but 1_000 in Marketplace.


  • In RewardReceiver.sol:

uint feeAmount = (rewards[2] - baseReward) * fee / 1000;

This list is not exhaustive, and it is recommended to review the entire codebase to identify additional instances where magic numbers are used.

BVSS
Recommendation

To improve code maintainability, readability, and reduce the risk of potential errors, it is recommended to replace magic numbers with well-defined constants. By using constants, developers can provide clear and descriptive names for specific values, making the code easier to understand and maintain. Additionally, updating the values becomes more straightforward, as changes can be made in a single location, reducing the risk of errors and inconsistencies. For large numbers, consider using scientific notation (e.g., 1e4).

Remediation

PENDING: The B14G team indicated that they will be addressing this finding in the future.

7.19 Unlocked Pragma Compiler

//

Informational

Description

The files in scope currently use floating pragma version ^0.8.0, which means that the code can be compiled by any compiler version that is greater than or equal to 0.8.0, and less than 0.9.0. It is recommended that contracts should be deployed with the same compiler version and flags used during development and testing. Locking the pragma helps to ensure that contracts do not accidentally get deployed using another pragma. For example, an outdated pragma version might introduce bugs that affect the contract system negatively.

BVSS
Recommendation

Lock the pragma version to the same version used during development and testing.

Remediation

PENDING: The B14G team indicated that they will be addressing this finding in the future.

7.20 Missing Visibility Attribute

//

Informational

Description

It is best practice to set the visibility of state variables and constants explicitly. In the MarketPlace contract, the rewardReceivers and initialized global state variables do not have the visibility set:

address[] rewardReceivers;
...
bool initialized;
BVSS
Recommendation

Consider explicitly setting the visibility of all state variables and constants. More specifically, the code could be updated to:

address[] internal rewardReceivers;
...
bool internal initialized;
Remediation

ACKNOWLEDGED: The B14G team acknowledged this finding.

7.21 Use of Revert Strings Instead of Custom Error

//

Informational

Description

In Solidity smart contract 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.

BVSS
Recommendation

It is recommended to replace hard-coded revert strings in require statements for custom errors, which can be done following the logic below.

1. Standard require statement (to be replaced):

require(condition, "Condition not met");

2. Declare the error definition to state

error ConditionNotMet();

3. Use the Custom Error in require or if statements.


Using custom errors inside require statements is only available from Solidity versions 0.8.26 and above. In case of older compiler version, consider using if statements + revert with Custom Error.

Remediation

PENDING: The B14G team indicated that they will be addressing this finding in the future.

7.22 Consider Using Named Mappings

//

Informational

Description

The project could be upgraded to a more recent Solidity version (greater than 0.8.18), which supports named mappings. Using named mappings can improve the readability and maintainability of the code by making the purpose of each mapping clearer. This practice helps developers and auditors understand the mappings' intent more easily.

BVSS
Recommendation

Consider refactoring the mappings to use named arguments, which will enhance code readability and make the purpose of each mapping more explicit.


For example, on Marketplace.sol, instead of declaring:

mapping(address => address[]) rewardReceiversPerAddress;

It could be declared as:

mapping(address account => address[] rewardReceivers) rewardReceiversPerAddress;
Remediation

ACKNOWLEDGED: The B14G team acknowledged this finding.

7.23 Style Guide Optimizations

//

Informational

Description

The project codebase contains several stylistic inconsistencies and deviations from Solidity best practices, which, while not directly impacting functionality, reduce code readability, maintainability, and adherence to standard conventions. Addressing these inconsistencies can enhance the clarity and professionalism of the code.


  • Constants not in uppercase: Some constants of the project are not declared in uppercase. See the following example in RewardReceiver:

ICoreAgent public constant coreAgent = ICoreAgent(0x0000000000000000000000000000000000001011);
IStakeHub public constant stakeHub = IStakeHub(0x0000000000000000000000000000000000001010);
IBitcoinStake public constant bitcoinStake = IBitcoinStake(0x0000000000000000000000000000000000001014);
IBitcoinAgent public constant bitcoinAgent = IBitcoinAgent(0x0000000000000000000000000000000000001013);

  • Whitespace issues: The first example below has two spaces between public and userStake. The second example is missing a whitespace between (uint256) and {.

mapping(address => Stake) public  userStake;
...
function getStakeOnCandidate(address user, address candidate) public view returns (uint256){

  • Use of public Where external Could Be Used: Certain public functions could be declared as external to potentially save gas and adhere to best practices. Example in Marketplace.sol:

function updateFee(uint _fee) public onlyOwner {

  • Redundant Use of block.timestamp in Event Parameters: This use of block.timestamp as an event parameter is redundant because the timestamp of when the event is emitted is already included in the transaction metadata and can be accessed directly from the event logs. Including it as an explicit parameter increases the size of the emitted event, leading to slightly higher gas costs without adding meaningful value.

event Withdraw(uint indexed timestamp, uint amount, uint receiveAmount);
event ClaimReward(uint indexed timestamp, uint rewardAmount);

  • Not Fully Leverage Named Returns: While some functions do use named returns, others do not. See the following example in Marketplace not using named returns:

function claimReward(bytes32 _txHash, address to) external nonReentrant onlyMarketplace _finish(_txHash) returns (uint) {
 [The code was removed per client request]
}

  • Use of Whole File Imports Instead of Named Imports: Full file imports are used across the codebase, which may include unnecessary code and reduce clarity. Example from MergeMarketplaceStrategy.sol:

import "../interfaces/IStrategy.sol";
import "../utils/OnwnableWithPausable.sol";
import "../library/Address.sol";
import "../interfaces/IMarketplace.sol";
import "../interfaces/IRewardReceiver.sol";
BVSS
Recommendation

Consider implementing style improvements highlighted above to improve the readability and consistency of the project.

Remediation

ACKNOWLEDGED: The B14G team acknowledged this finding.

7.24 Cache Array Length Outside of Loop

//

Informational

Description

When the length of an array is not cached outside of a loop, the Solidity compiler reads the length of the array during each iteration. For storage arrays, this results in an extra sload operation (100 additional gas for each iteration except the first). For memory arrays, this results in an extra mload operation (3 additional gas for each iteration except the first).


Detecting loops that use the length member of a storage array in their loop condition without modifying it can reveal opportunities for optimization. See the following example in Marketplace.sol:

function stakeCoreProxy(StakeParam[] memory stakeParam) public payable whenNotPaused {
  uint totalStake = msg.value;
  for (uint i = 0; i < stakeParam.length; i++) {
...
BVSS
Recommendation

Cache the length of the (storage and memory) arrays in a local variable outside the loop to optimize gas usage. This reduces the number of read operations required during each iteration of the loop. See the example fixes below:

function withdraw() external payable whenNotPaused nonReentrant {
    UnbondRecord[] storage records = unbondRecords[msg.sender];
    uint256 _recordsLength = records.length;
    if (_recordsLength == 0) revert("Empty record");
    uint totalAmount;
    for (uint256 i = _recordsLength; i != 0; i--) {
...
Remediation

PENDING: The B14G team indicated that they will be addressing this finding in the future.

7.25 Events Missing Indexed Fields

//

Informational

Description

Indexed event fields make the data more quickly accessible to off-chain tools that parse events, and add them to a special data structure known as "topics" instead of the data part of the log. A topic can only hold a single word (32 bytes) so if you use a reference type for an indexed argument, the Keccak-256 hash of the value is stored as a topic instead.


Each event can use up to three indexed fields. If there are fewer than three fields, all the fields can be indexed. It is important to note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed fields per event (three indexed fields).


This is specially recommended when gas usage is not particularly of concern for the emission of the events in question, and the benefits of querying those fields in an easier and straight-forward manner surpasses the downsides of gas usage increase.

BVSS
Recommendation

Consider adding the indexed keyword when declaring events, considering the following example in Marketplace.sol:

event Paused(address indexed account);
event Unpaused(address indexed account);
Remediation

PENDING: The B14G team indicated that they will be addressing this finding in the future.

7.26 Use Calldata For Function Arguments Not Mutated

//

Informational

Description

Some function in the contract in scope use memory arrays as arguments, even though the array is not mutated in the external function itself. This results in unnecessary gas overhead when copying data from calldata to memory during the abi.decode() process.


Using calldata directly for such arguments bypasses the copying loop, reducing gas costs, especially for larger arrays. Optimizing arrays as calldata instead of memory can reduce gas costs for external calls. The savings grow with the size of the input array.


Examples: The claimParam arguments of claimBTCRewardProxy() in Marketplace.

BVSS
Recommendation

Consider updating the function signatures as follows:

function claimBTCRewardProxy(ClaimParam[] calldata claimParam) public whenNotPaused returns (uint[] memory amounts) {

By switching the argument type to calldata, the claimParam argument is read directly from the transaction's calldata, eliminating unnecessary memory allocations and reducing gas costs.

Remediation

PENDING: The B14G team indicated that they will be addressing this finding in the future.

7.27 Mixed msg.sender and _msgSender

//

Informational

Description

The contracts in scope use msg.sender in some places and _msgSender() in others, despite inheriting context logic from OpenZeppelin. Even though meta-transactions may not be utilized, this inconsistency can introduce confusion and potential issues if meta-transactions or proxies are adopted in the future. In particular, _msgSender() properly reflects the original sender in scenarios involving relayers or proxies, whereas msg.sender might not.

BVSS
Recommendation

To maintain consistency and avoid potential issues with mixed usage, it is recommended to use _msgSender() exclusively throughout the codebase. This ensures a uniform approach to retrieving the caller's address and provides better compatibility with meta-transactions or proxies where _msgSender() may be overridden to return a different value than msg.sender.

Remediation

PENDING: The B14G team indicated that they will be addressing this finding in the future.

7.28 Inconsistency Declaring uint256/uint

//

Informational

Description

During the security review, it was observed that the contract uses both uint and uint256 types inconsistently across functions, events, and variable declarations. Although uint is an alias for uint256 in Solidity, inconsistency in type usage can reduce code readability and may lead to confusion for developers maintaining the contract.


Examples:

  • In CoreVault.sol:

function initialize(address _owner, address _operator, uint _withdrawFee, uint _rewardFee, uint256 _unlockPeriod, uint256 _maxCap) public { 

Notice _withdrawFee is uint and _unlockPeriod is of type uint256.


event Unbond(address indexed user, uint256 coreAmount, uint256 dualCoreAmount, uint unlockTime);

Notice coreAmount and dualCoreAmount are uint256, but unlockTime is of type uint.


Such inconsistencies can make it difficult to maintain code clarity and may lead to unnecessary type casting or confusion during contract upgrades.

BVSS
Recommendation

For consistency across the contract, it is recommended to declare all integer variables using a single type. Since uint256 is the preferred standard in Solidity, you can refactor the code by changing the few instances of uint to uint256.


  • In CoreVault.sol:

function initialize(address _owner, address _operator, uint256 _withdrawFee, uint256 _rewardFee, uint256 _unlockPeriod, uint256 _maxCap) public { 

Alternatively, if you prefer brevity and are not concerned about explicitness, you may change all uint256 types to uint consistently throughout the contract. This will improve maintainability and ensure clarity for future developers.

Remediation

ACKNOWLEDGED: The B14G team acknowledged this finding.

7.29 Unused or Dead Code

//

Informational

Description

During the security assessment of the smart contracts, several instances of unused code were identified. These unnecessary components can clutter the codebase, reduce readability, and potentially lead to confusion during development or auditing. Additionally, unused imports may slightly increase the compiled contract's bytecode size, affecting deployment and execution costs.


  • Unused internal function in RewardReceiver.sol:

function _reentrancyGuardEntered() internal view returns (bool) {

Multiple interface files of the projects were not used (e.g.: contracts/interfaces/IPledAgent.sol).

BVSS
Recommendation

For better clarity, consider removing all unused code.

Remediation

PENDING: The B14G team indicated that they will be addressing this finding in the future.

7.30 Empty Require Statements

//

Informational

Description

During the security assessment, two empty require() statements were found to be missing the error message.


  • In contracts/library/Math.sol:

require(denominator > prod1);

  • In contracts/proxy/ProxyAdmin.sol:

require(success);
BVSS
Recommendation

It is recommended to always use descriptive reason strings or custom errors for revert paths.

Remediation

SOLVED: The B14G team fixed this finding in commit 091cd35 by adding error messages to the highlighted require statements.

Remediation Hash

8. Automated Testing

Static Analysis Report

Description

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.

All issues identified by Slither were proved to be false positives or have been added to the issue list in this report.

Output

Slither output

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.

© Halborn 2025. All rights reserved.