Several functions in the CoreVault contract, namely withdrawDirect() and claimReward(), implement fee transfers by directly pushing native tokens to the owner via calls such as:

Address.sendValue(payable(owner()), fee);

This mechanism assumes that the owner is a payable address. However, if the owner is updated to a non-payable address (for instance, a contract lacking a payable fallback or receive function), the fee transfer will fail. Since these fee transfers are integral to the respective functions’ execution, the failure will cause the entire transaction to revert. As a result, users may be unable to withdraw funds or claim rewards, effectively locking funds and creating a denial-of-service situation.

Consider adopting a pull-over-push pattern for fee distribution. Instead of automatically transferring fees to the owner during each transaction, accumulate the fees within the contract (or another collector contract) and provide a dedicated withdrawal function that allows the owner to pull the accumulated fees. This approach decouples fee distribution from critical user operations, thereby preventing transaction reversion due to non-payable recipient issues.

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

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"));

It is recommended the following:

• Include initialization call in the proxy setup

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

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

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.

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

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

The CoreVault contract uses strict less than (<) comparisons when checking if unbond records are ready for withdrawal or unlocked. This means users cannot withdraw or see their funds as unlocked exactly at the unlock time, which is inconsistent with common DeFi practices and standards like EIP-2612.

• In the withdraw() function:

if (record.unlockTime < block.timestamp) {
  ...
}

• In the getUnbondAmount():

if (records[i].unlockTime < block.timestamp) {
    unlockedAmount += records[i].coreAmount;
} else {
    lockedAmount += records[i].coreAmount;
}

Consider changing all timestamp comparisons to be inclusive by using <= or >= as appropriate.

SOLVED: The B14G team fixed this finding in commit d91991a by adjusting the timestamp comparison as recommended.

In the CoreVault contract, the withdrawDirect() function attempts to ensure that a user’s remaining dualCore balance either meets a minimum threshold of 1 ether (in CORE equivalent) or is zero. However, this validation can be bypassed under certain conditions, making the check ineffective in preventing improper withdrawals:

function withdrawDirect(uint256 dualCoreAmount, bool claim) external payable whenNotPaused nonReentrant {
  if (claim) claimReward();
  uint256 coreAmount = exchangeCore(dualCoreAmount);
  require(coreAmount >= 1 ether, "Amount too low");
  IDualCore(dualCore).burn(msg.sender, dualCoreAmount);
  uint256 remainDualCore = IDualCore(dualCore).balanceOf(msg.sender);
  require(remainDualCore == 0 || exchangeCore(remainDualCore) >= 1 ether, "Remain amount too low");
...

In this function, a user can bypass the remainDualCore balance validation by transferring a small amount of dualCore tokens to another account before calling withdrawDirect(). This circumvents the intent of the check and may allow withdrawals that do not fully meet the specified threshold.

The following test function shows how it is not possible to call withdrawDirect() if the remaining is going to be too low. However, a user can just transfer out some tokens to successfully bypass the check.

it("withdrawDirect", async () => {
    // Get required contracts and accounts from fixture
    const { coreVault, accounts, orderStrategy, coreAgent, candidate, dualCore } = await loadFixture(deployContracts);

    // Get initial total staked and supply amounts
    let totalStaked = await coreVault.callStatic.totalStaked();
    let totalSupply = await dualCore.totalSupply();

    // Verify exchange rate calculation
    expect(await coreVault.callStatic.exchangeDualCore(BigNumber.from("10"))).to.eq(BigNumber.from("10").mul(totalSupply).div(totalStaked));

    // Update totals
    totalStaked = await coreVault.callStatic.totalStaked();
    totalSupply = await dualCore.totalSupply();

    // Test validation checks
    await expect(coreVault.withdrawDirect(parseEther("0.5"), false)).to.revertedWith("Amount too low");
    await expect(coreVault.withdrawDirect(parseEther("19.5"), false)).to.revertedWith("Remain amount too low");

    // Bypass validation check
    await dualCore.transfer(accounts[1].address, parseEther("0.5"));

    //Successfully withdraw
    await expect(coreVault.withdrawDirect(parseEther("19.5"), false)).not.to.be.reverted;
});

Consider redesigning the validation logic for the remaining dualCore balance to either improve or remove the verification.

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

The MergeMarketplaceStrategy contract is supposed to be designed to be an upgradeable contract. It inherits 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.

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

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

In CoreVault, the _exchangeDualCore() function performs a division without validating that denominators is non-zero. This unchecked divisions could lead to unexpected reverts and denial of service in edge cases.

function _exchangeDualCore(uint256 core) private returns (uint256) {
    uint256 totalSupply = IDualCore(dualCore).totalSupply();
    if (totalSupply == 0) {
        return core;
    }
    uint256 totalCore = totalStaked() - core;
    return core.mulDiv(totalSupply, totalCore);
}

The vulnerability can occur when totalStaked() equals core and it makes totalCore = 0. The mulDiv() operation will then attempt to divide by zero, causing the transaction to revert.

Consider adding a check for zero division:

function _exchangeDualCore(uint256 core) private returns (uint256) {
    uint256 totalSupply = IDualCore(dualCore).totalSupply();
    if (totalSupply == 0) {
        return core;
    }
    uint256 totalCore = totalStaked() - core;
    require(totalCore > 0, "CoreVault: division by zero");  // Added check
    return core.mulDiv(totalSupply, totalCore);
}

NOT APPLICABLE: After a discussion with the B14G team, it was determined that this was a false positive, so it is not applicable.

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.

• updateDualCore() and updateMarketplace() in MergeMarketplaceStrategy.

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.

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.

One example would be: Given that stake() does not allow the total supply of dualCORE to be greater than maxCap, if the owner ever updates maxCap, it could be reasonable to not be able to make it lower than the total supply of dualCORE:

function setMaxCap(uint256 _maxCap) external onlyOwner {
    require(_maxCap >= IDualCore(dualCore).totalSupply(), "New max cap must be >= current total supply");
    maxCap = _maxCap;
    emit SetMaxCap(_maxCap);
}

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

In the MergeMarketplaceStrategy contract, the reInvest() function accepts three arrays (withdrawData, stakeData, and indexReceivers) through encoded input data but does not validate their lengths relative to each other. The function uses indexReceivers to validate both withdrawal and stake operations. Specifically, indexReceivers[i] is used for withdrawals, and indexReceivers[i + withdrawData.length] is used for stakes.

Code snippet:

function reInvest(bytes calldata data) external payable nonReentrant onlyDualCore {
    (bytes memory changeData, uint totalAmount) = abi.decode(data, (bytes, uint));
    {
        IMarketplace.WithdrawParam[] memory withdrawData;
        IMarketplace.StakeParam[] memory stakeData;
        uint[] memory indexReceivers;
        (withdrawData, stakeData, indexReceivers) = abi.decode(changeData, (IMarketplace.WithdrawParam[], IMarketplace.StakeParam[], uint[]));
        if (withdrawData.length > 0) {
            for (uint i = 0; i < withdrawData.length; i++) {
                require(
                    IMarketplace(marketplace).getRewardReceiverBatch(indexReceivers[i], indexReceivers[i] + 1)[0] == withdrawData[i].receiver,
                    "invalid receiver"
                );
            }
            IMarketplace(marketplace).withdrawCoreProxy(withdrawData);
        }
        if (stakeData.length > 0) {
            uint stakeValue;
            for (uint i = 0; i < stakeData.length; i++) {
                require(
                    IMarketplace(marketplace).getRewardReceiverBatch(indexReceivers[i + withdrawData.length], indexReceivers[i + withdrawData.length] + 1)[

Without explicit length validation, this can lead to several issues:

1. Array Out-of-Bounds Access: If indexReceivers.length is less than withdrawData.length + stakeData.length, the function may access invalid array indices, potentially causing a transaction revert.

2. Invalid Receiver Validation: Incorrect or incomplete input data may cause the require statements to fail during validation checks.

Add explicit length validation after decoding changeData to ensure that the lengths of the arrays are consistent. For example:

require(
    indexReceivers.length == withdrawData.length + stakeData.length,
    "Invalid input array lengths"
);

SOLVED: The B14G team fixed this finding in commit 09483cc by implementing the recommended array length validation.

The CoreVault contract calculates fees and adjusts rewards using integer division, which inherently truncates any fractional values. This approach may introduce rounding errors in fee and reward computations:

• In claimReward():

function claimReward() public whenNotPaused returns (uint256 reward) {
    reward = IStrategy(strategy).claimReward();
    if (reward > 0) {
        uint fee = (reward * rewardFee) / 10000;
        reward = reward - fee;
        emit ClaimReward(reward, fee);
        Address.sendValue(payable(owner()), fee);
    }
}

Here, the fee is computed as (reward * rewardFee) / 10000. For small reward values, the truncation due to integer division can lead to:

• Minor discrepancies in the net reward allocated to the user.

• Situations where the fee might be rounded down to zero, thus not reflecting the intended deduction.

To mitigate precision loss and ensure fair fee and reward calculations, it is advisable to:

1. Implement Scaling Techniques:

   • Apply a scaling factor to the numerator before performing the division, preserving the fractional component.

   • Adjust the result accordingly after the division by reversing the scaling factor.

2. Consistent Calculation Updates:

   • Ensure that all fee and reward calculations in the contract are updated to use the scaling technique, thereby maintaining consistency.

3. Comprehensive Testing:

   • Thoroughly test the modified calculations with a range of edge cases, including very small and large values, to verify that the scaling approach reliably maintains accuracy without introducing unintended behavior.

Implementing these changes will enhance the precision of fee deductions and reward distributions, ensuring that users receive fair and predictable outcomes regardless of the transaction size.

RISK ACCEPTED: The B14G team accepted the risk of this finding because "the rounding differences are extremely small, so any discrepancies in fee and reward computations are negligible and well within acceptable limits".

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.

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.

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

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.

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.

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

The DualCore contract allows setting the coreVault address post-deployment via setCoreVaultAddress(), even though this can only be called once. This pattern is less efficient than making the address immutable and setting it in the constructor:

function setCoreVaultAddress(address _coreVault) external onlyOwner calledOnce {
    if (_coreVault == address(0)) {
        revert("Not zero address");
    }
    coreVault = _coreVault;
    setCoreVaultCalled = true;
    emit SetCoreVaultAddress(msg.sender, _coreVault);
}

The coreVault address could be declared as immutable and set in the constructor. If the recommendation is applied, it would also be possible to remove the setCoreVaultAddress() function, the calledOnce modifier and the setCoreVaultCalled variable.

ACKNOWLEDGED: The B14G team acknowledged this finding.

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

• ReentrancyGuardProxy inside contracts/marketplace/CoreVault.sol

• ReentrancyGuardProxy inside contracts/marketplace/MergeMarketplaceStrategy.sol

  
  

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.

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

It was identified that the CoreVault and MergeMarketplaceStrategy contracts indirectly inherit 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.

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.

ACKNOWLEDGED: The B14G team acknowledged this finding.

The receivers global state variable in MergeMarketplaceStrategy is of type EnumerableSetUpgradeable. If the order of an EnumerableSet is required, removing items in a loop using at() and remove() corrupts this order. This finding has been reported as informational because the developer team indicated that order is not currently important, but it is important to have this in mind for future reference.

Consider using a different data structure or removing items by collecting them during the loop, then removing after the loop.

ACKNOWLEDGED: The B14G team acknowledged this finding.

In MergeMarketplaceStrategy.sol, the getReceivers() function has a logical error in its bounds checking:

function getReceivers(uint256 start, uint256 end) public view returns (address[] memory) {
    require(start < end && end <= receivers.length(), "Invalid range");
    address[] memory result = new address[](end - start);
    for (uint256 i = start; i < end; i++) {
        result[i - start] = receivers.at(i);
    }
    return result;
}

• When receivers.length() == 0, the function will always revert because:

  • If end > 0: reverts because end <= receivers.length() is false

  • If end == 0: reverts because start < end is false

  • Therefore, no valid inputs exist when the list is empty

Modify the bounds checking to handle empty lists correctly.

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

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.

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.

ACKNOWLEDGED: The B14G team acknowledged this finding.

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.

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.

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

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

• updateDualCore() and updateMarketplace() in MergeMarketplaceStrategy.

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.

All functions updating important parameters should emit events.

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

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.

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

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

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 CoreVault.sol:

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

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

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

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).

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

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.

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

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

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.

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.

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

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.

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 MergeMarketplaceStrategy.sol, instead of declaring:

mapping(address => StakeInfo) public stakedOnReceivers;

It could be declared as:

mapping(address receiver => StakeInfo stakeInfo) public stakedOnReceivers;

ACKNOWLEDGED: The B14G team acknowledged this finding.

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.

• 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.

• 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.

• 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";

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

ACKNOWLEDGED: The B14G team acknowledged this finding.

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 CoreVault.sol:

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

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--) {
...

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

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.

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);

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

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.

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.

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

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.

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.

ACKNOWLEDGED: The B14G team acknowledged this finding.

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.

• An unused global state variable in CoreVault.sol:

uint256 public minAmount;

• Unused internal functions in CoreVault.sol:

function initialize_ReentrancyGuard() internal {
...
function _reentrancyGuardEntered() internal view returns (bool) {

• Unused internal functions in MergeMarketplaceStrategy.sol:

function initialize_ReentrancyGuard() internal {
...
function _reentrancyGuardEntered() internal view returns (bool) {

For better clarity, consider removing all unused code.

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

The project’s ERC20 token is named dualCore, yet the code uses the same identifier in two different contexts:

• In CoreVault, the dualCore variable represents the ERC20 token.

• In MergeMarketplaceStrategy, the dualCore variable is used to refer to the CoreVault contract.

This dual usage of the term dualCore can cause confusion for developers and auditors, potentially leading to integration or upgrade errors due to unclear semantics.

Standardize naming conventions to improve clarity. For example, consider renaming:

• The dualCore variable in CoreVault to dualCoreToken (or a similar descriptive name), and

• The dualCore variable in MergeMarketplaceStrategy to coreVault or another name that clearly indicates its purpose.

Clear and distinct naming will reduce ambiguity, ease future audits, and prevent potential integration mistakes.

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

In Solidity, return values often provide critical information about the success or failure of an operation. Disregarding these values can result in missed error handling.

The following instances were identified:

• In CoreVault.sol:

function stake() external payable whenNotPaused nonReentrant {0
  claimReward();
  ...

...

function withdrawDirect(uint256 dualCoreAmount, bool claim) external payable whenNotPaused nonReentrant {
  if (claim) claimReward();
  ...

• In MergeMarketplaceStrategy.sol:

function claimReward() public returns (uint reward) {
  ...
  IMarketplace(marketplace).claimCoreRewardProxy(claimParams);
...

It is recommended to:

1. Handle All Return Values: Review all instances of ignored return values and ensure they are either explicitly checked or deemed unnecessary with proper documentation.

2. Update Affected Functions: Refactor the relevant functions to utilize or log return values for additional validation and error handling.

By addressing this issue, the contract's robustness and reliability will be improved, ensuring that critical operations are validated and potential errors are caught effectively.

ACKNOWLEDGED: The B14G team acknowledged this finding.