Token Earn Contract - Halo

The HALO token is a governance token, and is the token which is staked in the HaloStakeVault.sol contract. The total supply of the token is 2_100_000_000e18, depending on the quorum requirements if a big portion of the token is staked, quorum won't be reached in the upcoming governance proposals, as the staked tokens don't delegate the voting rights to anyone, and the HALO tokens that are deposited in the HaloStakeVault.sol contract are not utilized in any way.

Consider delegating the voting rights associated with the HALO tokens to the address provided as the to parameter in the stake() function.

Risk Accepted: The protocol team has chosen not to add voting-related logic yet

If the owner address is compromised, core functionality of the protocol will be affected. For example, in the HaloAirdrop.sol contract, a compromised owner can pause the contract indefinitely, or withdraw the tokens that are deposited and should be claimed by the users via the pause() and emergencyWithdraw() / emergencyWithdrawERC20() functions respectively.

In the HaloSocialMining.sol contract, a compromised owner can set a new merkle root via the setSessionMerkleRoot() function, and drain all of the rewards, or simply call the emergencyWithdraw() / emergencyWithdrawERC20() in order to drain the tokens within the contract.

In the HaloStakeVault.sol contract, a compromised owner can either increase drastically the reward per block, or make it 0 via the updateRewardRate() function, so that stakers don't receive any rewards. It is also possible to set a rewardVault address that doesn't have any rewards in it via the updateRewardVault() function.

The rewardVault contract / EOA is a single point of failure, if the reward tokens contained within it are drained, the users who staked their tokens won't receive any rewards.

Consider using multisig wallet address as the owner address.

Solved: The protocol team will set the owner to a 2/3 multiSig wallet.

The HaloStakeVault.sol contract does not include a pause mechanism that would allow the contract owner to temporarily disable certain functions in the contract. A pause mechanism is a critical feature that can be used to prevent potential issues in the event of a bug or vulnerability being discovered in the contract. It can also be used to prevent malicious actors from exploiting the contract while a fix is being implemented.

Implement a pause mechanism in the HaloStakeVault.sol contract that allows the contract owner to disable certain functions in the contract in case of an emergency or bug discovery.

Risk Accepted: The protocol team has accepted the risk and chosen not to implement an additional pausing mechanism.

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. As currently is not possible to use custom errors in combination with require statements, the standard syntax is:

if (!condition) revert ConditionNotMet();

More information about this topic in [Official Solidity Documentation](https://docs.soliditylang.org/en/v0.8.24/control-structures.html#panic-via-assert-and-error-via-require).

Acknowledged: The protocol team has acknowledged the issue, and chosen not to make changes.

The protocol allows the current owner to renounce ownership, which will disable important functionality of the protocol such as pausing and unpausing the claiming and unlocking of user rewards within the HaloAirdrop.sol contract, and updating the rewards per block in the HaloStakeVault.sol contract.

Consider disallowing the owner address to be set to address(0).

Acknowledged: The protocol team has acknowledged the issue, and chosen not to make changes.

In the HaloSocialMining.sol and HaloAirdrop.sol contracts when an emergency withdraw of the native tokens is performed, the solidity build in transfer() function is used to handle the native token transfer, it does this by forwarding a fixed amount of 2300 gas. This is dangerous for two reasons:

1. Gas costs of EVM instructions may change significantly during hard forks, which may previously assumed fixed gas costs. EIP 1884 as an example, broke several existing smart contracts due to a cost increase of the SLOAD instruction.

2. If the recipient is a contract or a multisig safe, with a receive/fallback function which requires >2300 gas, e.g safes that execute extra logic in the receive/fallback function, the transfer function will always fail for them due to out of gas errors.

Consider using the call() method for transferring funds, as it is more flexible and handles the increase in gas cost associated with the transfer.

Acknowledged: The protocol team has acknowledged the issue, and chosen not to make changes.

The project lacks unit and E2E tests. It is always a good practice to implement unit tests and E2E tests, as it helps discovering vulnerabilities in the early stages of development, and later on it helps with maintaining and updating the code base.

Consider adding unit and E2E tests to the project.

Future Release: The protocol has informed us that there are tests, however they are not considered opensource as of the date of the audit.

All contracts in scope currently use floating pragma versions ^0.8.27 which means that the code can be compiled by any compiler version that is greater than or equal to 0.8.27, and less than 0.9.0.

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

In this aspect, it is crucial to select the appropriate EVM version when it's intended to deploy the contracts on networks other than the Ethereum mainnet, which may not support these opcodes. Failure to do so could lead to unsuccessful contract deployments or transaction execution issues.

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

Additionally, make sure to specify the target EVM version when using Solidity versions from 0.8.20 and above if deploying to chains that may not support newly introduced opcodes. Additionally, it is crucial to stay informed about the opcode support of different chains to ensure smooth deployment and compatibility.

Acknowledged: The protocol team has acknowledged the issue, and chosen not to make changes.

The project is using a 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 contracts/HaloStakeVault.sol, instead of declaring:

mapping(address => uint256) public rewardsToClaim;

It could be declared as:

mapping(address userAddress => uint256 amount) public rewardsToClaim;

Acknowledged: The protocol team has acknowledged the issue, and chosen not to make changes.

The HGPBurn.sol contract imports the import "@openzeppelin/contracts/token/ERC721/IERC721.sol"; however it doesn't use it.

Consider removing the unused import, the import "@openzeppelin/contracts/token/ERC721/IERC721.sol"; is imported in the IHaloWalletGenesisPass.sol.

Solved: The protocol has removed the unused import.

In the HaloAirdop.sol contract in the isValid() function, the memory keyword is used for a parameter which is not modified instead of the calldata keyword.

Consider using the calldata keyword instead of the memory for function arguments which are not modified.

Acknowledged: The protocol team has acknowledged the issue, and chosen not to make changes.