Rover Protocol - Hydrogen Labs

The StakeManager.sol contract relies on the address(this).balance method for fetching the protocol's current TVL. While this represents a straight-forward strategy for Total Value Locked (TVL) calculation, it can lead to inaccuracies, considering that in some edge cases.

For example, in cases where the StakeManager.sol contract receives ether from self-destruction, the native balance in the contract will not reflect the actual Total Value Locked into the protocol through user deposits.

- contracts/StakeManager/StakeManager.sol [Lines: 95-97]

    function calculateTVL() public view returns (uint256 totalTVL) {
        totalTVL = address(this).balance;
    }

Also, ensuring a more sophisticated approach for mapping the users' positions across the protocol would benefit the ecosystem in the long term, providing a more efficient and reliable TVL tracking mechanism.

It is recommended to use an internal accounting mechanism instead of relying on methods like balance or balanceOf when performing operations that calculate the Total Value Locked (TVL).

Remediation Plan

SOLVED: The Hydrogen Labs team has solved this issue by creating an internal accounting mechanism for deposits.

Solc compiler version 0.8.20 switches the default target EVM version to Shanghai. The generated bytecode will include PUSH0 opcodes. The recently released Solc compiler version 0.8.25 switches the default target EVM version to Cancun, so it is also important to note that it also adds-up new opcodes such as TSTORE, TLOAD and MCOPY.

Be sure to select the appropriate EVM version in case you intend to deploy on a chain apart from mainnet, like L2 chains that may not support PUSH0, TSTORE, TLOAD and/or MCOPY, otherwise deployment of your contracts will fail.

It is important to consider the targeted deployment chains before writing smart contracts because, in the future, there might exist a need for deploying the contracts in a network that could not support new opcodes from Shanghai or Cancun EVM versions.

Remediation Plan

ACKNOWLEDGED: The Hydrogen Labs team has acknowledged the risk related to this issue.

The assessment of the smart contracts in-scope revealed instances where input validation is missing when assigning an address to a state variable. Failing to validate user-provided addresses can lead to unintended consequences and potential security vulnerabilities.

- contracts/RoverBtcToken/RovBtcToken.sol [Line: 39]

	        roleManager = _roleManager;

- contracts/StakeManager/StakeManager.sol [Line: 53]

	        roleManager = _roleManager;

- contracts/StakeManager/StakeManager.sol [Line: 54]

	        rovBTC = _rovBTC;

When an address is not properly validated, it may be assigned an incorrect or malicious value, such as the zero address (address(0)). This can result in broken contract logic, lost funds, or other unintended behavior.

To prevent unintended behavior and potential security vulnerabilities, it is essential to include checks for address(0) when assigning values to address state variables. This can be achieved by adding a simple check to ensure that the assigned address is not equal to address(0) before proceeding with the assignment.

Remediation Plan

SOLVED: The Hydrogen Labs team has solved this issue as recommended.

Functions that are not called internally can be marked as external instead of public. By doing so, it is possible to optimize gas consumption and improve contract efficiency.

External functions are more gas-efficient than public functions, as they do not create an additional internal function call within the contract.

- contracts/AccessControl/RoleManager.sol [Line: 22]

	    function initialize(address roleManagerAdmin) public initializer {

- contracts/RoverBtcToken/RovBtcToken.sol [Line: 35]

	    function initialize(IRoleManager _roleManager) public initializer {

- contracts/RoverBtcToken/RovBtcToken.sol [Line: 74]

	    function name() public view virtual override returns (string memory) {

- contracts/RoverBtcToken/RovBtcToken.sol [Line: 79]

	    function symbol() public view virtual override returns (string memory) {

- contracts/StakeManager/StakeManager.sol [Line: 51]

	    function initialize(IRoleManager roleManager, IRovBtcToken rovBTC) public initializer {

For functions that are only called externally, and never internally, it is recommended to change the function visibility modifier from public to external.

This small change can lead to gas savings, especially in contracts with high transaction volumes or complex functionality. By marking functions as external when appropriate, developers can enhance the overall performance and cost-effectiveness of their smart contracts.

Remediation Plan

ACKNOWLEDGED: The Hydrogen Labs team has acknowledged this issue.

Indexed event fields facilitate faster access to off-chain tools that analyze events. However, it is important to note that each indexed field consumes additional gas during event emission. Therefore, it is not always advisable to index the maximum number of fields allowed per event (three fields).

If an event has three or more fields and gas usage is not a significant concern, it is recommended to index three fields. On the other hand, if an event has fewer than three fields, it is best to index all of them. By making a well-informed decision on which fields to index, developers can balance the need for quick access to event data with the gas costs associated with event emission.

- contracts/StakeManager/StakeManager.sol [Line: 22]

 	    event Deposit(address indexed depositor, uint256 amount, uint256 rovBTCMinted, uint256 referralId); 	

It is recommended to add the indexed keyword when declaring events, considering the following example:

    event Indexed(
        address indexed from,
        bytes32 indexed id,
        uint indexed value
    );

Remediation Plan

SOLVED: The Hydrogen Labs team has solved this issue by adding the indexed attribute to the field.