Throughout the code in scope, there are several instances where the files are imported but not used in a file:

In IInjector.sol:

import "./ISlashingIndicator.sol";
import "./ISystemReward.sol";
import "./IGovernance.sol";
import "./IStaking.sol";
import "./IDeployerProxy.sol";
import "./IStakingPool.sol";
import "./IChainConfig.sol";

In Injector.sol:

import "./interfaces/IValidatorSet.sol";

In Tokenomics.sol :

import "@openzeppelin/contracts/utils/Address.sol";

Remove the aforementioned unused imports.

Remediation Plan

ACKNOWLEDGED: The Chiliz team made a business decision to acknowledge this finding and not alter the contracts.

Throughout the codebase, there are multiple instances where full file imports are used; although this does not impact functionality, it is generally not considered a best practice.

It's considered best practice to follow named imports instead of importing full files.

import {MyContract} from "./MyContract.sol";

Remediation Plan

ACKNOWLEDGED: The Chiliz team made a business decision to acknowledge this finding and not alter the contracts.

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

Consider replacing all revert strings with custom errors. For more reference, see here.

Remediation Plan

ACKNOWLEDGED: The Chiliz team made a business decision to acknowledge this finding and not alter the contracts.

In Solidity development, it is a common convention that contracts, interfaces and library names should also match their filenames. However, the Injector.sol file does not follow this convention and contains a contract called InjectorContextHolder instead.

Rename the InjectorContextHolder contract to Injector, or rename the Injector.sol file to InjectorContextHolder.sol.

Remediation Plan

ACKNOWLEDGED: The Chiliz team made a business decision to acknowledge this finding and not alter the contracts.

The _deposit() function in the Tokenomics.sol file makes a low-level call to the _systemRewardContract address transferring systemAmount to it. However, the call function returns a data variable that is not used.

function _deposit(address validatorAddress, uint256 newTotalSupply, uint256 inflationPct) internal {
    require(msg.value > 0, "dizt"); // deposit is zero

    // Distribute
    uint256 stakingAmount = msg.value * _state.shareStaking / 10000;
    uint256 systemAmount = msg.value * _state.shareSystem / 10000;

    _stakingContract.deposit{value: stakingAmount}(validatorAddress);
    (bool sent, bytes memory data) = address(_systemRewardContract).call{value: systemAmount}("");
    require(sent, "sr"); // transfer to systemRewardsContract failed

    // Update state
    _state.inflationPct = inflationPct;
    _state.introducedSupply = msg.value;
    _state.totalIntroducedSupply += msg.value;
    _state.totalSupply = newTotalSupply;

    emit Deposit(msg.value, newTotalSupply, inflationPct, validatorAddress, stakingAmount, systemAmount);
}

Verify the return data from the external call if it needs validating. Alternatively, remove the unused data variable.

Remediation Plan

ACKNOWLEDGED: The Chiliz team made a business decision to acknowledge this finding and not alter the contracts.

The Injector.sol contract currently defines the initManually() and the setTokenomics() functions with public visibility, even though they are not called from within the smart contract or any contract that inherits from Injector.sol, resulting in higher gas costs than necessary.

Modify the aforementioned functions with the external visibility modifier.

Remediation Plan

ACKNOWLEDGED: The Chiliz team made a business decision to acknowledge this finding and not alter the contracts.

The INITIAL_TOTAL_SUPPLY variable in the Tokenomics.sol file is a large literal value set to 8888888888000000000000000000. Managing such large numbers directly can be cumbersome and error-prone. To enhance code readability and reduce potential mistakes, Solidity best practices recommend employing native units or employing powers of ten.

Replace the large literal value with power of ten or native units :

8_888_888_888e18

or

8_888_888_888 ether

Remediation Plan

ACKNOWLEDGED: The Chiliz team made a business decision to acknowledge this finding and not alter the contracts.

The _deposit() function within the Tokenomics smart contract is designed to facilitate the transfer of native assets to the contract and subsequently distribute these assets between the Staking contract and the SystemReward contract. However, this function does not adhere to the checks-effects-interactions pattern, as it updates the contract's state after making external calls.

While the reentrancy vulnerability may not directly affect the contract or funds in this specific instance, it is crucial to follow the check-effects-interaction pattern. This practice ensures that the contract state is updated prior to executing external calls, thus safeguarding against potential vulnerabilities.

Refactor the _deposit() function to follow the check-effects-interaction pattern and update the contract's state prior to making the external calls.

Additionally, ensure that each of the ds.account to which transfers are made in the _claimSystemFee in the SystemReward contract in not allowed to re-enter the system for any of that contract's functions and any other functions in the system beyond the receive function in the SystemReward contract.

Remediation Plan

ACKNOWLEDGED: The Chiliz team made a business decision to acknowledge this finding and not alter the contracts.

In Solidity development, emitting events is a recommended practice when state-changing functions are invoked. Currently, the Injector.sol file lacks an event emission in the setTokenomics() function. This absence may hamper effective state tracking in off-chain monitoring systems.

Additionally, the events declared in the Tokenomics contract are missing the indexed keyword, which make the data more quickly accessible to off-chain tools that parse events, and adds them to a special data structure known as "topics" instead of the data part of the log.

However, indexing more fields increases the gas cost for each event emitted. Therefore, extensive indexing is recommended when the advantages of easier data retrieval outweigh the higher gas expenses, particularly in cases where gas efficiency is less of a concern.

Introduce an event declaration and ensure its emission within the setTokenomics() function. This change will enhance transparency and facilitate accurate tracking of state changes.

If the value to emit in an event is fix-sized, it is recommended to add the indexed keyword when declaring events. For more reference, see the Solidity documentation.

Remediation Plan

ACKNOWLEDGED: The Chiliz team made a business decision to acknowledge this finding and not alter the contracts.