Governance - Onyx DAO

One proposal can contain multiple operations in different contracts that have to be executed. Some governance proposals may require a sequence of operations to be executed in a specific order in order for the proposal to have an effect. Before the operations from the proposal can be executed they have to be queued first via the queue() function:

    function queue(uint proposalId) public {
        require(state(proposalId) == ProposalState.Succeeded, "GovernorAlpha::queue: proposal can only be queued if it is succeeded");
        Proposal storage proposal = proposals[proposalId];
        uint eta = add256(block.timestamp, timelock.delay());
        for (uint i = 0; i < proposal.targets.length; i++) {
            _queueOrRevert(proposal.targets[i], proposal.values[i], proposal.signatures[i], proposal.calldatas[i], eta);
        }
        proposal.eta = eta;
        emit ProposalQueued(proposalId, eta);
    }

  function _queueOrRevert(address target, uint value, string memory signature, bytes memory data, uint eta) internal {
        require(!timelock.queuedTransactions(keccak256(abi.encode(target, value, signature, data, eta))), "GovernorAlpha::_queueOrRevert: proposal action already queued at eta");
        timelock.queueTransaction(target, value, signature, data, eta);
  }

As can be seen from the _queueOrRevert() function the operation id is calculated with the provided target, value, signature, data, and eta parameters. In a scenario where one proposal has multiple operations that have to be executed, a malicious user with enough voting power can create a proposal with just one of the operations, and use the same target, value, signature and data parameters, right after the first proposal is created. The protocol is deployed on Ethereum so frontrunning and backrunning of transactions is possible. This way if both of the proposals succeed, they will have the same eta parameters. Now the malicious user has to queue his proposal first, and when someone tries to queue the non malicious proposal it will revert as one of the operations in the non malicious proposal will already be queued. This way a malicious user or a group of people with enough voting power can dos proposals that they are strictly against but they don't have enough voting power to outvote the other users in the non malicious proposal.

Consider adding the proposal id into the calculation of the operation id in the _queueOrRevert() function, also modify the queueTransaction() function in the CHNTimelock contract:

_queueOrRevert(proposal.targets[i], proposal.values[i], proposal.signatures[i], proposal.calldatas[i], eta, proposal.id);

RISK ACCEPTED: The Onyx team has accepted the risk.

The MasterChef contract has a maxXcnPerSecond parameter that defines the maximum amount of XCN tokens that should be distributed per second. However this restriction is not enforced in the constructor, and the actual xcnPerSecond parameter is set to 2.5e18, as can be seen on chain: https://etherscan.io/address/0x3fa642c0bbad64569eb8424af35f518347249216#readContract#F12

This results in the protocol distributing more rewards per second than the max amount of rewards per second that are defined as a constant.

Consider enforcing the maxXcnPerSecond parameter in the constructor, or remove it entirely.

RISK ACCEPTED: The Onyx team has accepted the risk.

If the guardian address in the CHNGovernance contract is compromised, all the critical functionalities of the CHNTimelock contract will be compromised. The guardian address can directly change the admin address in the CHNTimelock contract via the __queueSetTimelockPendingAdmin() and __executeSetTimelockPendingAdmin() functions. And the queueTransaction() and the executeTransaction() functions in the CHNTimelock contract can be called by the admin(which is originally set to the CHNGovernance contract). Additionally the guardian address can cancel all proposals that are not yet executed, no matter whether they have succeeded or not.

Consider using multisig wallet address for the guardian address. Additionally consider adding functionality to change the guardian address in the CHNGovernance contract.

RISK ACCEPTED: The Onyx team has accepted the risk.

In the MasterChef contract the inCaseETHGetStuck() function uses transfer() instead of safeTransfer(), and in a case where a weird ERC20 token have to be rescued, the function may revert.

In the inCaseETHGetStuck() function use safeTransfer() instead of transfer()

ACKNOWLEDGED: The Onyx team has acknowledged the risk.

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 Onyx team has acknowledged the risk.

In the CHNGovernance contract the queue() function iterates over all of the contract targets of the proposal and then calls the _queueOrRevert() function in order to queue all of the functions that have to be executed later. However the length of the proposal.targets is not cached before all of the targets are looped trough. This increases the gas consumed by the queue() function. There is a similar scenario in the execute() function.

Consider caching the length of the proposal.targets array before you loop over them.

ACKNOWLEDGED: The Onyx team has acknowledged the risk.

In the CHNGovernance contract in the propose(), and _queueOrRevert() functions, the memory keyword is used for parameters which are not modified instead of the calldata keyword. There is a similar scenario in the queueTransaction() function in the CHNTimelock contract.

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

ACKNOWLEDGED: The Onyx team has acknowledged the risk.

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.

ACKNOWLEDGED: The Onyx team has acknowledged the risk.

The Solidity version of the different contracts in scope is not fixed, some contracts use a Solidity version from 6.12.0 up to 7.0.0, others use a Solidity version greater than 0.8.1. Consider using a Solidity version grater than 0.8.18, which supports named mapping, and utilize the 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, in the CHNTimelock contract, instead of declaring:

mapping (bytes32 => bool) public queuedTransactions;

It could be declared as:

mapping (bytes32 transactionId => bool queued) public queuedTransactions;

ACKNOWLEDGED: The Onyx team has acknowledged the risk.

The Solidity version of the different contracts in scope is not fixed, some contracts use a Solidity version from 6.12.0 up to 7.0.0, others use a Solidity version greater than 0.8.1. 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 Onyx team has acknowledged the risk.

The owner of the CHNStaking contract is the CHNTimelock contract, that means if a proposal passes all kind of tokens can be added via the add() function.