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.

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.

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.

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

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

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.

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.

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.

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

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

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.

ACKNOWLEDGED: The B14G team acknowledged this finding.

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.

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.

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

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.

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

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

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.

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

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

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.

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

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

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

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

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.

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;

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;

ACKNOWLEDGED: The B14G team acknowledged this finding.

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

mapping(address => address[]) rewardReceiversPerAddress;

It could be declared as:

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

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.

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

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

function stakeCoreProxy(StakeParam[] memory stakeParam) public payable whenNotPaused {
  uint totalStake = msg.value;
  for (uint i = 0; i < stakeParam.length; 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.

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.

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.

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.

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

For better clarity, consider removing all unused code.

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

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

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

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