1 Executive Summary

This report presents the results of our engagement with Rocket Pool to review their upcoming Rocket Pool Atlas release (v1.2).

The review was conducted over three weeks, from January 16, 2023 to February 03, 2023. A total of 2x15 person-days were spent.

A critical reentrancy issue in the node distributor has been found, allowing a node owner to drain funds from the respective distributor. Furthermore, several major severity issues have been found regarding the updated Minipool delegate contract and a node operator DAO contract.

1.1 Mitigations: 17 Mar 2023

Latest commit with changes: rocket-pool/rocketpool@77d7cca

Compared to audit commit: rocket-pool/rocketpool@7771afa…77d7cca

The client provided code changes and remediation information for the findings outlined in this report. Details and can be found in the Remediation Notes for the respective findings in Section: Findings.

We removed one recommendation noting that zero initialization is unnecessary agreeing with and following the clients remark that “[… ]Being explicit even if redundant is not a problem.”.
We downgraded the finding “Missing extcodesize check in dynamic proxy” from Major -> Medium in agreement with the client as funds are not at immediate risk and they can recover from this problem. Nevertheless, this finding should be addressed as per the recommendation.
We downgraded the finding “Duplicate check to avoid revert” from Minor -> Informational given that it more a technical debt than a concrete security issue and the client provided a the following out-of-band fix: rocket-pool/rocketpool@3ab7af1

2 Scope

Our review focused on the following repositories:

rocketpool@7771afa…fde3f8c

Furthermore, the following information was provided:

GitHub/Gist describing changes (accessed January 2023)

2.1 Objectives

Together with the client, we identified the following priorities for our review:

Ensure that the system is implemented consistently with the intended functionality and without unintended edge cases.
Identify known vulnerabilities particular to smart contract systems, as outlined in our Smart Contract Best Practices, and the Smart Contract Weakness Classification Registry.
Review key risks in regard to recent changes flagged by the development team:

The minipool delegate has changed, and the possible state transitions are more complex.

The deposit credit system used for the lower ETH bonded minipools and solo migration is new

The minipool distribution (reward distribution) logic has changed considerably to incorporate partial withdrawals (skimming), optimizing withdrawals, and solo migration. With the Shanghai hard fork, this will be the first time these features will be exercised in production.

3 Document Change Log

Version	Date	Description
1.0	2023-02-06	Initial report
1.1	2023-03-17	Updated Report: Mitigations
1.2	2023-03-24	Updated Report: Client provided a fix for 5.2

4 System Overview

This section describes the top-level/deployable contracts, their inheritance structure and interfaces, actors, permissions and important contract interactions of the system under review.

Contracts are depicted as boxes. Public reachable interface methods are outlined as rows in the box. The 🔍 icon indicates that a method is declared as non-state-changing (view/pure) while other methods may change state. A yellow dashed row at the top of the contract shows inherited contracts. A green dashed row at the top of the contract indicates that that contract is used in a usingFor declaration. Modifiers used as ACL are connected as yellow bubbles in front of methods.

RocketPool 1.2 - Architecture excluding ODAO

RocketPool 1.2 - MiniPool State Diagram

5 Findings

Each issue has an assigned severity:

Minor issues are subjective in nature. They are typically suggestions around best practices or readability. Code maintainers should use their own judgment as to whether to address such issues.
Medium issues are objective in nature but are not security vulnerabilities. These should be addressed unless there is a clear reason not to.
Major issues are security vulnerabilities that may not be directly exploitable or may require certain conditions in order to be exploited. All major issues should be addressed.
Critical issues are directly exploitable security vulnerabilities that need to be fixed.

5.1 RocketNodeDistributorDelegate - Reentrancy in `distribute()` allows node owner to drain distributor funds Critical ✓ Fixed

Resolution

Fixed in https://github.com/rocket-pool/rocketpool/tree/77d7cca65b7c0557cfda078a4fc45f9ac0cc6cc6 by implementing a custom reentrancy guard via a new state variable lock that is appended to the end of the storage layout. The reentrancy guard is functionally equivalent to the OpenZeppelin implementation. The method was not refactored to give user funds priority over the node share. Additionally, the client provided the following statement:

We acknowledge this as a critical issue and have solved with a reentrancy guard.

We followed OpenZeppelin’s design for a reentrancy guard. We were unable to use it directly as it is hardcoded to use storage slot 0 and because we already have deployment of this delegate in the wild already using storage slot 0 for another purpose, we had to append it to the end of the existing storage layout.

Description

The distribute() function distributes the contract’s balance between the node operator and the user. The node operator is returned their initial collateral, including a fee. The rest is returned to the RETH token contract as user collateral.

After determining the node owner’s share, the contract transfers ETH to the node withdrawal address, which can be the configured withdrawal address or the node address. Both addresses may potentially be a malicious contract that recursively calls back into the distribute() function to retrieve the node share multiple times until all funds are drained from the contract. The distribute() function is not protected against reentrancy:

code/contracts/contract/node/RocketNodeDistributorDelegate.sol:L59-L73

/// @notice Distributes the balance of this contract to its owners
function distribute() override external {
 // Calculate node share
 uint256 nodeShare = getNodeShare();
 // Transfer node share
 address withdrawalAddress = rocketStorage.getNodeWithdrawalAddress(nodeAddress);
 (bool success,) = withdrawalAddress.call{value : nodeShare}("");
 require(success);
 // Transfer user share
 uint256 userShare = address(this).balance;
 address rocketTokenRETH = rocketStorage.getAddress(rocketTokenRETHKey);
 payable(rocketTokenRETH).transfer(userShare);
 // Emit event
 emit FeesDistributed(nodeAddress, userShare, nodeShare, block.timestamp);
}

We also noticed that any address could set a withdrawal address as there is no check for the caller to be a registered node. In fact, the caller can be the withdrawal address or node operator.

code/contracts/contract/RocketStorage.sol:L118-L133

// Set a node's withdrawal address
function setWithdrawalAddress(address _nodeAddress, address _newWithdrawalAddress, bool _confirm) external override {
 // Check new withdrawal address
 require(_newWithdrawalAddress != address(0x0), "Invalid withdrawal address");
 // Confirm the transaction is from the node's current withdrawal address
 address withdrawalAddress = getNodeWithdrawalAddress(_nodeAddress);
 require(withdrawalAddress == msg.sender, "Only a tx from a node's withdrawal address can update it");
 // Update immediately if confirmed
 if (_confirm) {
 updateWithdrawalAddress(_nodeAddress, _newWithdrawalAddress);
 }
 // Set pending withdrawal address if not confirmed
 else {
 pendingWithdrawalAddresses[_nodeAddress] = _newWithdrawalAddress;
 }
}

Recommendation

Add a reentrancy guard to functions that interact with untrusted contracts. Adhere to the checks-effects pattern and send user funds to the ’trusted’ RETH contract first. Only then send funds to the node’s withdrawal address.

5.2 RocketMinipoolDelegateOld - Node operator may reenter `finalise()` to manipulate accounting Major ✓ Fixed

Resolution

The client acknowledges the finding and provided the following statement:

We are aware of this issue (it was reported via our Immunefi bug bounty). It is live but that code path is inaccessible. It requires the oDAO to mark a minipool as Withdrawable which we don’t do and have removed from the withdrawal process moving forward.

In a later revision, the development team fixed the issue in the following commit: 73d5792a671db5d2f4dcbd35737e729f9e01aa11

Description

In the old Minipool delegate contract, a node operator may call the finalise() function to finalize a Minipool. As part of this process, a call to _refund() may be performed if there is a node refund balance to be transferred. This will send an amount of nodeRefundBalance in ETH to the nodeWithdrawalAddress via a low-level call, handing over control flow to an - in terms of the system - untrusted external account that this node operator controls. The node operator, therefore, is granted to opportunity to call back into finalise(), which is not protected against reentrancy and violates the checks-effects-interactions pattern (finalised = true is only set at the very end), to manipulate the following system settings:

node.minipools.finalised.count<NodeAddress>: NodeAddress finalised count increased twice instead
minipools.finalised.count: global finalised count increased twice
eth.matched.node.amount<NodeAddress> - NodeAddress eth matched amount potentially reduced too many times; has an impact on getNodeETHCollateralisationRatio -> GetNodeShare, getNodeETHProvided -> getNodeEffectiveRPLStake and getNodeETHProvided->getNodeMaximumRPLStake->withdrawRPL and is the limiting factor when withdrawing RPL to ensure the pools stay collateralized.

Note: RocketMinipoolDelegateOld is assumed to be the currently deployed MiniPool implementation. Users may upgrade from this delegate to the new version and can roll back at any time and re-upgrade, even within the same transaction (see issue 5.3 ).

The following is an annotated call stack from a node operator calling minipool.finalise() reentering finalise() once more on their Minipool:

finalise() -->
status == MinipoolStatus.Withdrawable //<-- true
withdrawalBlock > 0 //<-- true
_finalise() -->
!finalised //<-- true
_refund()
nodeRefundBalance = 0 //<-- reset refund balance
---> extCall: nodeWithdrawalAddress
---> reenter: finalise()
status == MinipoolStatus.Withdrawable //<-- true
withdrawalBlock > 0 //<-- true
_finalise() -->
!finalised //<-- true
nodeRefundBalance > 0 //<-- false; no refund()
address(this).balance to RETH
RocketTokenRETHInterface(rocketTokenRETH).depositExcessCollateral()
rocketMinipoolManager.incrementNodeFinalisedMinipoolCount(nodeAddress) //<-- 1st time
eventually call rocketDAONodeTrusted.decrementMemberUnbondedValidatorCount(nodeAddress);
finalised = true;
<--- return from reentrant call
<--- return from _refund()
address(this).balance to RETH //<-- NOP as balance was sent to RETH already
RocketTokenRETHInterface(rocketTokenRETH).depositExcessCollateral(); //<-- does not revert
rocketMinipoolManager.incrementNodeFinalisedMinipoolCount(nodeAddress); //<-- no revert, increases
'node.minipools.finalised.count', 'minipools.finalised.count', reduces 'eth.matched.node.amount' one to
many times
eventually call rocketDAONodeTrusted.decrementMemberUnbondedValidatorCount(nodeAddress); //<-- manipulates
'member.validator.unbonded.count' by +1
finalised = true; //<-- is already 'true', gracefully continues
<--- returns

code/contracts/contract/old/minipool/RocketMinipoolDelegateOld.sol:L182-L191


// Called by node operator to finalise the pool and unlock their RPL stake
function finalise() external override onlyInitialised onlyMinipoolOwnerOrWithdrawalAddress(msg.sender) {
 // Can only call if withdrawable and can only be called once
 require(status == MinipoolStatus.Withdrawable, "Minipool must be withdrawable");
 // Node operator cannot finalise the pool unless distributeBalance has been called
 require(withdrawalBlock > 0, "Minipool balance must have been distributed at least once");
 // Finalise the pool
 _finalise();
}

_refund() handing over control flow to nodeWithdrawalAddress

code/contracts/contract/old/minipool/RocketMinipoolDelegateOld.sol:L311-L341

// Perform any slashings, refunds, and unlock NO's stake
function _finalise() private {
 // Get contracts
 RocketMinipoolManagerInterface rocketMinipoolManager = RocketMinipoolManagerInterface(getContractAddress("rocketMinipoolManager"));
 // Can only finalise the pool once
 require(!finalised, "Minipool has already been finalised");
 // If slash is required then perform it
 if (nodeSlashBalance > 0) {
 _slash();
 }
 // Refund node operator if required
 if (nodeRefundBalance > 0) {
 _refund();
 }
 // Send any left over ETH to rETH contract
 if (address(this).balance > 0) {
 // Send user amount to rETH contract
 payable(rocketTokenRETH).transfer(address(this).balance);
 }
 // Trigger a deposit of excess collateral from rETH contract to deposit pool
 RocketTokenRETHInterface(rocketTokenRETH).depositExcessCollateral();
 // Unlock node operator's RPL
 rocketMinipoolManager.incrementNodeFinalisedMinipoolCount(nodeAddress);
 // Update unbonded validator count if minipool is unbonded
 if (depositType == MinipoolDeposit.Empty) {
 RocketDAONodeTrustedInterface rocketDAONodeTrusted = RocketDAONodeTrustedInterface(getContractAddress("rocketDAONodeTrusted"));
 rocketDAONodeTrusted.decrementMemberUnbondedValidatorCount(nodeAddress);
 }
 // Set finalised flag
 finalised = true;
}

code/contracts/contract/old/minipool/RocketMinipoolDelegateOld.sol:L517-L528

function _refund() private {
 // Update refund balance
 uint256 refundAmount = nodeRefundBalance;
 nodeRefundBalance = 0;
 // Get node withdrawal address
 address nodeWithdrawalAddress = rocketStorage.getNodeWithdrawalAddress(nodeAddress);
 // Transfer refund amount
 (bool success,) = nodeWithdrawalAddress.call{value : refundAmount}("");
 require(success, "ETH refund amount was not successfully transferred to node operator");
 // Emit ether withdrawn event
 emit EtherWithdrawn(nodeWithdrawalAddress, refundAmount, block.timestamp);
}

Methods adjusting system settings called twice:

code/contracts/contract/old/minipool/RocketMinipoolManagerOld.sol:L265-L272

// Increments _nodeAddress' number of minipools that have been finalised
function incrementNodeFinalisedMinipoolCount(address _nodeAddress) override external onlyLatestContract("rocketMinipoolManager", address(this)) onlyRegisteredMinipool(msg.sender) {
 // Update the node specific count
 addUint(keccak256(abi.encodePacked("node.minipools.finalised.count", _nodeAddress)), 1);
 // Update the total count
 addUint(keccak256(bytes("minipools.finalised.count")), 1);
}

code/contracts/contract/dao/node/RocketDAONodeTrusted.sol:L139-L142

}
function decrementMemberUnbondedValidatorCount(address _nodeAddress) override external onlyLatestContract("rocketDAONodeTrusted", address(this)) onlyRegisteredMinipool(msg.sender) {
 subUint(keccak256(abi.encodePacked(daoNameSpace, "member.validator.unbonded.count", _nodeAddress)), 1);
}

Recommendation

We recommend setting the finalised = true flag immediately after checking for it. Additionally, the function flow should adhere to the checks-effects-interactions pattern whenever possible. We recommend adding generic reentrancy protection whenever the control flow is handed to an untrusted entity.

5.3 RocketMinipoolDelegate - Sandwiching of Minipool calls can have unintended side effects Major

Resolution

The client provided the following statement:

The slashed value is purely for NO informational purposes and not used in any logic in the contracts so this example is benign as you say. We have fixed this particular issue by moving the slashed boolean out of the delegate and into RocketMinipooLManager. It is now set on any call to rocketNodeStaking.slashRPL which covers both old delegate and new.

We appreciate that the finding was more a classification of potential issues with upgrades and rollbacks. At this stage, we cannot change this functionality as it is already deployed in a non-upgradable way to over 12,000 contracts.

As this is more of a guidance and there is no immediate threat, we don’t believe this should be considered a ‘major’ finding.

With https://github.com/rocket-pool/rocketpool/tree/77d7cca65b7c0557cfda078a4fc45f9ac0cc6cc6 the slashed flag was moved to RocketNodeStaking.slashRPL() (minipool.rpl.slashed|<msg.sender> = true).

The audit team acknowledges that this issue does not provide a concrete exploit that puts funds at risk. However, due to the sensitive nature and potential for issues regarding future updates, we stand by the initial severity rating as it stands for security vulnerabilities that may not be directly exploitable or require certain conditions to be exploited.

Description

The RocketMinipoolBase contract exposes the functions delegateUpgrade and delegateRollback, allowing the minipool owner to switch between delegate implementations. While giving the minipool owner a chance to roll back potentially malfunctioning upgrades, the fact that upgrades and rollback are instantaneous also gives them a chance to alternate between executing old and new code (e.g. by utilizing callbacks) and sandwich user calls to the minipool.

Examples

Assuming the latest minipool delegate implementation, any user can call RocketMinipoolDelegate.slash, which slashes the node operator’s RPL balance if a slashing has been recorded on their validator. To mark the minipool as having been slashed, the slashed contract variable is set to true. A minipool owner can avoid this flag from being set By sandwiching the user calls:

Minipool owner rolls back to the old implementation from RocketMinipoolDelegateOld.sol
User calls slash on the now old delegate implementation (where slashed is not set)
Minipool owner upgrades to the latest delegate implementation again

In detail, the new slash implementation:

code/contracts/contract/minipool/RocketMinipoolDelegate.sol:L687-L696

function _slash() private {
 // Get contracts
 RocketNodeStakingInterface rocketNodeStaking = RocketNodeStakingInterface(getContractAddress("rocketNodeStaking"));
 // Slash required amount and reset storage value
 uint256 slashAmount = nodeSlashBalance;
 nodeSlashBalance = 0;
 rocketNodeStaking.slashRPL(nodeAddress, slashAmount);
 // Record slashing
 slashed = true;
}

Compared to the old slash implementation:

code/contracts/contract/old/minipool/RocketMinipoolDelegateOld.sol:L531-L539

function _slash() private {
 // Get contracts
 RocketNodeStakingInterface rocketNodeStaking = RocketNodeStakingInterface(getContractAddress("rocketNodeStaking"));
 // Slash required amount and reset storage value
 uint256 slashAmount = nodeSlashBalance;
 nodeSlashBalance = 0;
 rocketNodeStaking.slashRPL(nodeAddress, slashAmount);
}

While the bypass of slashed being set is a benign example, the effects of this issue, in general, could result in a significant disruption of minipool operations and potentially affect the system’s funds. The impact highly depends on the changes introduced by future minipool upgrades.

Recommendation

We recommend limiting upgrades and rollbacks to prevent minipool owners from switching implementations with an immediate effect. A time lock can fulfill this purpose when a minipool owner announces an upgrade to be done at a specific block. A warning can precede user-made calls that an upgrade is pending, and their interaction can have unintended side effects.

5.4 RocketDAONodeTrustedActions - No way to access ETH provided by non-member votes Major Acknowledged

Resolution

According to the client, this is the intended behavior. The client provided the following statement:

This is by design.

Description

DAO members can challenge nodes to prove liveliness for free. Non-DAO members must provide members.challenge.cost = 1 eth to start a challenge. However, the provided challenge cost is locked within the contract instead of being returned or recycled as system collateral.

Examples

code/contracts/contract/dao/node/RocketDAONodeTrustedActions.sol:L181-L192

// In the event that the majority/all of members go offline permanently and no more proposals could be passed, a current member or a regular node can 'challenge' a DAO members node to respond
// If it does not respond in the given window, it can be removed as a member. The one who removes the member after the challenge isn't met, must be another node other than the proposer to provide some oversight
// This should only be used in an emergency situation to recover the DAO. Members that need removing when consensus is still viable, should be done via the 'kick' method.
function actionChallengeMake(address _nodeAddress) override external onlyTrustedNode(_nodeAddress) onlyRegisteredNode(msg.sender) onlyLatestContract("rocketDAONodeTrustedActions", address(this)) payable {
 // Load contracts
 RocketDAONodeTrustedInterface rocketDAONode = RocketDAONodeTrustedInterface(getContractAddress("rocketDAONodeTrusted"));
 RocketDAONodeTrustedSettingsMembersInterface rocketDAONodeTrustedSettingsMembers = RocketDAONodeTrustedSettingsMembersInterface(getContractAddress("rocketDAONodeTrustedSettingsMembers"));
 // Members can challenge other members for free, but for a regular bonded node to challenge a DAO member, requires non-refundable payment to prevent spamming
 if(rocketDAONode.getMemberIsValid(msg.sender) != true) require(msg.value == rocketDAONodeTrustedSettingsMembers.getChallengeCost(), "Non DAO members must pay ETH to challenge a members node");
 // Can't challenge yourself duh
 require(msg.sender != _nodeAddress, "You cannot challenge yourself");
 // Is this member already being challenged?

Recommendation

We recommend locking the ETH inside the contract during the challenge process. If a challenge is refuted, we recommend feeding the locked value back into the system as protocol collateral. If the challenge succeeds and the node is kicked, it is assumed that the challenger will be repaid the amount they had to lock up to prove non-liveliness.

5.5 Multiple checks-effects violations Major

Resolution

The client provided the following statement:

In many of the cited examples, the “external call” is a call to another network contract that has the same privileges as the caller. Preventing reentrancy against our own internal contracts provides no additional security. If a malicious contract is introduced via a malicious oDAO they already have full keys to the kingdom.

None of the examples provide an attack surface and so we don’t believe this to be a ‘major’ finding and should be downgraded.

This finding highlights our concerns about a dangerous pattern used throughout the codebase that may eventually lead to exploitable scenarios if continued to be followed, especially on codebases that do not employ protective measures against reentrant calls. This report also flagged one such exploitable instance, leading to a critical exploitable issue in one of the components.

This repeated occurrence led us to flag this as a major issue to highlight a general error and attack surface present in several places.

From our experience, there are predominantly positive side-effects of adhering to safe coding patterns, even for trusted contract interactions, as developers indirectly follow or pick up the coding style from existing code, reducing the likelihood of following a pattern that may be prone to be taken advantage of.

For example, to a developer, it might not always be directly evident that control flow is passed to potentially untrusted components/addresses from the code itself, especially when calling multiple ’trusted’ components in the system. Furthermore, individual components down the call stack may be updated at later times, introducing an untrusted external call (i.e., because funds are refunded) and exposing the initially calling contract to a reentrancy-type issue. Therefore, we highly recommend adhering to a safe checks-effects pattern even though the contracts mainly interact with other trusted components and build secure code based on defense-in-depth principles to contain potential damage in favor of assuming worst-case scenarios.

Description

Throughout the system, there are various violations of the checks-effects-interactions pattern where the contract state is updated after an external call. Since large parts of the Rocket Pool system’s smart contracts are not guarded against reentrancy, the external call’s recipient may reenter and potentially perform malicious actions that can impact the overall accounting and, thus, system funds.

Examples

distributeToOwner() sends the contract’s balance to the node or the withdrawal address before clearing the internal accounting:

code/contracts/contract/minipool/RocketMinipoolDelegate.sol:L564-L581

/// @notice Withdraw node balances from the minipool and close it. Only accepts calls from the owner
function close() override external onlyMinipoolOwner(msg.sender) onlyInitialised {
 // Check current status
 require(status == MinipoolStatus.Dissolved, "The minipool can only be closed while dissolved");
 // Distribute funds to owner
 distributeToOwner();
 // Destroy minipool
 RocketMinipoolManagerInterface rocketMinipoolManager = RocketMinipoolManagerInterface(getContractAddress("rocketMinipoolManager"));
 require(rocketMinipoolManager.getMinipoolExists(address(this)), "Minipool already closed");
 rocketMinipoolManager.destroyMinipool();
 // Clear state
 nodeDepositBalance = 0;
 nodeRefundBalance = 0;
 userDepositBalance = 0;
 userDepositBalanceLegacy = 0;
 userDepositAssignedTime = 0;
}

The withdrawal block should be set before any other contracts are called:

code/contracts/contract/minipool/RocketMinipoolDelegate.sol:L498-L499

// Save block to prevent multiple withdrawals within a few blocks
withdrawalBlock = block.number;

The slashed state should be set before any external calls are made:

code/contracts/contract/minipool/RocketMinipoolDelegate.sol:L686-L696

/// @dev Slash node operator's RPL balance based on nodeSlashBalance
function _slash() private {
 // Get contracts
 RocketNodeStakingInterface rocketNodeStaking = RocketNodeStakingInterface(getContractAddress("rocketNodeStaking"));
 // Slash required amount and reset storage value
 uint256 slashAmount = nodeSlashBalance;
 nodeSlashBalance = 0;
 rocketNodeStaking.slashRPL(nodeAddress, slashAmount);
 // Record slashing
 slashed = true;
}

In the bond reducer, the accounting values should be cleared before any external calls are made:

code/contracts/contract/minipool/RocketMinipoolBondReducer.sol:L120-L134

// Get desired to amount
uint256 newBondAmount = getUint(keccak256(abi.encodePacked("minipool.bond.reduction.value", msg.sender)));
require(rocketNodeDeposit.isValidDepositAmount(newBondAmount), "Invalid bond amount");
// Calculate difference
uint256 existingBondAmount = minipool.getNodeDepositBalance();
uint256 delta = existingBondAmount.sub(newBondAmount);
// Get node address
address nodeAddress = minipool.getNodeAddress();
// Increase ETH matched or revert if exceeds limit based on current RPL stake
rocketNodeDeposit.increaseEthMatched(nodeAddress, delta);
// Increase node operator's deposit credit
rocketNodeDeposit.increaseDepositCreditBalance(nodeAddress, delta);
// Clean up state
deleteUint(keccak256(abi.encodePacked("minipool.bond.reduction.time", msg.sender)));
deleteUint(keccak256(abi.encodePacked("minipool.bond.reduction.value", msg.sender)));

The counter for reward snapshot execution should be incremented before RPL gets minted:

code/contracts/contract/rewards/RocketRewardsPool.sol:L210-L213

// Execute inflation if required
rplContract.inflationMintTokens();
// Increment the reward index and update the claim interval timestamp
incrementRewardIndex();

Recommendation

We recommend following the checks-effects-interactions pattern and adjusting any contract state variables before making external calls. With the upgradeable nature of the system, we also recommend strictly adhering to this practice when all external calls are being made to trusted network contracts.

5.6 Minipool state machine design and pseudo-states Medium Acknowledged

Resolution

The client acknowledges the finding and provided the following statement.

We agree that the state machine is complicated. This is a symptom of technical debt and backwards compatibility.

There is no actionable response to this finding as we cannot make changes to the existing 12,000 contracts already deployed.

We want to emphasize that this finding strongly suggests that there are design deficits in the minipool state machine that, sooner or later, may impact the overall system’s security. We suggest refactoring a clean design with clear transitions and states for the current iteration removing technical debt from future versions. This may mean that it may be warranted to release a new major Rocketpool version as a standalone system with a clean migration path avoiding potential problems otherwise introduced by dealing with the current technical debt.

Description

The development team has provided the assessment team with a Minipool state machine diagram. In this document, the Destroyed and Finalised states are denoted as fully qualified Minipool states. However, these conditions are pseudo-states. Specifically, the Destroyed pseudo-state leaves the Minipool in the actual Dissolved state and removes it from the Minipool accounting components. The Finalised pseudo-state sets the finalised flag on the Minipool without changing its original state. Actors may still be able to execute functions on the Minipool while it should be in an end state.

Recommendation

We strongly discourage the use of pseudo-states in state machines as they make the state machine less intuitive and present challenges in mapping state transitions to the code base. Real states and transitions should be used where possible.

Generally, we recommend the following when designing state machines:

Using clear and descriptive transition names,
Avoiding having multiple transitions with the same trigger,
Modeling decisions in the form of state transitions rather than states themselves.

In any case, every Minipool should terminate in a clear end state.

5.7 RocketMinipoolDelegate - Redundant `refund()` call on forced finalization Medium ✓ Fixed

Resolution

Description

The RocketMinipoolDelegate.refund function will force finalization if a user previously distributed the pool. However, _finalise already calls _refund() if there is a node refund balance to transfer, making the additional call to _refund() in refund() obsolete.

Examples

code/contracts/contract/minipool/RocketMinipoolDelegate.sol:L200-L209

function refund() override external onlyMinipoolOwnerOrWithdrawalAddress(msg.sender) onlyInitialised {
 // Check refund balance
 require(nodeRefundBalance > 0, "No amount of the node deposit is available for refund");
 // If this minipool was distributed by a user, force finalisation on the node operator
 if (!finalised && userDistributed) {
 _finalise();
 }
 // Refund node
 _refund();
}

code/contracts/contract/minipool/RocketMinipoolDelegate.sol:L445-L459

function _finalise() private {
 // Get contracts
 RocketMinipoolManagerInterface rocketMinipoolManager = RocketMinipoolManagerInterface(getContractAddress("rocketMinipoolManager"));
 // Can only finalise the pool once
 require(!finalised, "Minipool has already been finalised");
 // Set finalised flag
 finalised = true;
 // If slash is required then perform it
 if (nodeSlashBalance > 0) {
 _slash();
 }
 // Refund node operator if required
 if (nodeRefundBalance > 0) {
 _refund();
 }

Recommendation

We recommend refactoring the if condition to contain _refund() in the else branch.

5.8 Sparse documentation and accounting complexity Medium Acknowledged

Resolution

The client acknowledges the finding and provided the following statement:

Acknowledged and agree.

Description

Throughout the project, inline documentation is either sparse or missing altogether. Furthermore, few technical documents about the system’s design rationale are available. The recent releases’ increased complexity makes it significantly harder to trace the flow of funds through the system as components change semantics, are split into separate contracts, etc.

It is essential that documentation not only outlines what is being done but also why and what a function’s role in the system’s “bigger picture” is. Many comments in the code base fail to fulfill this requirement and are thus redundant, e.g.

code/contracts/contract/minipool/RocketMinipoolDelegate.sol:L292-L293

// Sanity check that refund balance is zero
require(nodeRefundBalance == 0, "Refund balance not zero");

code/contracts/contract/minipool/RocketMinipoolDelegate.sol:L333-L334

// Remove from vacant set
rocketMinipoolManager.removeVacantMinipool();

code/contracts/contract/minipool/RocketMinipoolDelegate.sol:L381-L383

if (ownerCalling) {
 // Finalise the minipool if the owner is calling
 _finalise();

The increased complexity and lack of documentation can increase the likelihood of developer error. Furthermore, the time spent maintaining the code and introducing new developers to the code base will drastically increase. This effect can be especially problematic in the system’s accounting of funds as the various stages of a Minipool imply different flows of funds and interactions with external dependencies. Documentation should explain the rationale behind specific hardcoded values, such as the magic 8 ether boundary for withdrawal detection. An example of a lack of documentation and distribution across components is the calculation and influence of ethMatched as it plays a role in:

the minipool bond reducer,
the node deposit contract,
the node manager, and
the node staking contract.

Recommendation

As the Rocketpool system grows in complexity, we highly recommend significantly increasing the number of inline comments and general technical documentation and exploring ways to centralize the system’s accounting further to provide a clear picture of which funds move where and at what point in time. Where the flow of funds is obscured because multiple components or multi-step processes are involved, we recommend adding extensive inline documentation to give context.

5.9 RocketNodeDistributor - Missing `extcodesize` check in dynamic proxy Medium Won't Fix

Resolution

The client decided not to address the finding with the upcoming update. As per their assessment, the scenario outlined would require a series of misconfigurations/failures and hence is unlikely to happen. Following a defense-in-depth approach we, nevertheless, urge to implement safeguards on multiple layers as a condition like this can easily go undetected. However, after reviewing the feedback provided by the client we share the assessment that the finding should be downgraded from Major to Medium as funds are not at immediate risk and they can recover from this problem by fixing the delegate. For transparency, the client provided the following statement:

Agree that an extcodesize check here would add safety against a future mistake. But it does require a failure at many points for it to actually lead to an issue. Beacuse this contract is not getting upgraded in Atlas, we will leave it as is. We will make note to add a safety check on it in a future update of this contract.

We don’t believe this consitutes a ‘major’ finding given that it requires a future significant failure. If such a failure were to happen, the impact is also minimal as any calls to distribute() would simply do nothing. A contract upgrade would fix the problem and no funds would be at risk.

Description

RocketNodeDistributor dynamically retrieves the currently set delegate from the centralized RocketStorage contract. The target contract (delegate) is resolved inside the fallback function. It may return address(0). rocketStorage.getAddress() does not enforce that the requested settings key exists, which may lead to RocketNodeDistributor delegate-calling into address(0), which returns no error. This might stay undetected when calling RocketNodeDistributorDelegate.distribute() as the method does not return a value, which is consistent with calling a target address with no code.

Examples

code/contracts/contract/node/RocketNodeDistributor.sol:L23-L31

fallback() external payable {
 address _target = rocketStorage.getAddress(distributorStorageKey);
 assembly {
 calldatacopy(0x0, 0x0, calldatasize())
 let result := delegatecall(gas(), _target, 0x0, calldatasize(), 0x0, 0)
 returndatacopy(0x0, 0x0, returndatasize())
 switch result case 0 {revert(0, returndatasize())} default {return (0, returndatasize())}
 }
}

code/contracts/contract/RocketStorage.sol:L153-L155

function getAddress(bytes32 _key) override external view returns (address r) {
 return addressStorage[_key];
}

Recommendation

Before delegate-calling into the target contract, check if it exists.

assembly {
 codeSize := extcodesize(_target)
}
require(codeSize > 0);

5.10 Kicked oDAO members’ votes taken into account Medium Acknowledged

Resolution

The client acknowledges the finding and provided the following statement:

We are aware of this limitation but the additional changes required to implement a fix outweigh the concern in our opinion.

Description

oDAO members can vote on proposals or submit external data to the system, acting as an oracle. Data submission is based on a vote by itself, and multiple oDAO members must submit the same data until a configurable threshold (51% by default) is reached for the data to be confirmed.

When a member gets kicked or leaves the oDAO after voting, their vote is still accounted for while the total number of oDAO members decreases.

A (group of) malicious oDAO actors may exploit this fact to artificially lower the consensus threshold by voting for a proposal and then leaving the oDAO. This will leave excess votes with the proposal while the total member count decreases.

For example, let’s assume there are 17 oDAO members. 9 members must vote for the proposal for it to pass (52.9%). Let’s assume 8 members voted for, and the rest abstained and is against the proposal (47%, threshold not met). The proposal is unlikely to pass unless two malicious oDAO members leave the DAO, lowering the member count to 15 in an attempt to manipulate the vote, suddenly inflating vote power from 8/17 (47%; rejected) to 8/15 (53.3%; passed).

The crux is that the votes of ex-oDAO members still count, while the quorum is based on the current oDAO member number.

Here are some examples, however, this is a general pattern used for oDAO votes in the system.

Example: RocketNetworkPrices

Members submit votes via submitPrices(). If the threshold is reached, the proposal is executed. Quorum is based on the current oDAO member count, votes of ex-oDAO members are still accounted for. If a proposal is a near miss, malicious actors can force execute it by leaving the oDAO, lowering the threshold, and then calling executeUpdatePrices() to execute it.

code/contracts/contract/network/RocketNetworkPrices.sol:L75-L79

RocketDAONodeTrustedInterface rocketDAONodeTrusted = RocketDAONodeTrustedInterface(getContractAddress("rocketDAONodeTrusted"));
if (calcBase.mul(submissionCount).div(rocketDAONodeTrusted.getMemberCount()) >= rocketDAOProtocolSettingsNetwork.getNodeConsensusThreshold()) {
 // Update the price
 updatePrices(_block, _rplPrice);
}

code/contracts/contract/network/RocketNetworkPrices.sol:L85-L86

function executeUpdatePrices(uint256 _block, uint256 _rplPrice) override external onlyLatestContract("rocketNetworkPrices", address(this)) {
 // Check settings

RocketMinipoolBondReducer

The RocketMinipoolBondReducer contract’s voteCancelReduction function takes old votes of previously kicked oDAO members into account. This results in the vote being significantly higher and increases the potential for malicious actors, even after their removal, to sway the vote. Note that a canceled bond reduction cannot be undone.

code/contracts/contract/minipool/RocketMinipoolBondReducer.sol:L94-L98

RocketDAONodeTrustedSettingsMinipoolInterface rocketDAONodeTrustedSettingsMinipool = RocketDAONodeTrustedSettingsMinipoolInterface(getContractAddress("rocketDAONodeTrustedSettingsMinipool"));
uint256 quorum = rocketDAONode.getMemberCount().mul(rocketDAONodeTrustedSettingsMinipool.getCancelBondReductionQuorum()).div(calcBase);
bytes32 totalCancelVotesKey = keccak256(abi.encodePacked("minipool.bond.reduction.vote.count", _minipoolAddress));
uint256 totalCancelVotes = getUint(totalCancelVotesKey).add(1);
if (totalCancelVotes > quorum) {

RocketNetworkPenalties

code/contracts/contract/network/RocketNetworkPenalties.sol:L47-L51

RocketDAONodeTrustedInterface rocketDAONodeTrusted = RocketDAONodeTrustedInterface(getContractAddress("rocketDAONodeTrusted"));
if (calcBase.mul(submissionCount).div(rocketDAONodeTrusted.getMemberCount()) >= rocketDAOProtocolSettingsNetwork.getNodePenaltyThreshold()) {
 setBool(executedKey, true);
 incrementMinipoolPenaltyCount(_minipoolAddress);
}

code/contracts/contract/network/RocketNetworkPenalties.sol:L54-L58

// Executes incrementMinipoolPenaltyCount if consensus threshold is reached
function executeUpdatePenalty(address _minipoolAddress, uint256 _block) override external onlyLatestContract("rocketNetworkPenalties", address(this)) {
 // Get contracts
 RocketDAOProtocolSettingsNetworkInterface rocketDAOProtocolSettingsNetwork = RocketDAOProtocolSettingsNetworkInterface(getContractAddress("rocketDAOProtocolSettingsNetwork"));
 // Get submission keys

Recommendation

Track oDAO members’ votes and remove them from the tally when the removal from the oDAO is executed.

5.11 RocketDAOProtocolSettingsRewards - settings key collission Medium Acknowledged

Resolution

The client acknowledges the finding and provided the following statement:

We are aware of this limitation but making this change now with an existing deployment outweighs the concern in our opinion.

Description

A malicious user may craft a DAO protocol proposal to set a rewards claimer for a specific contract, thus overwriting another contract’s settings. This issue arises due to lax requirements when choosing safe settings keys.

code/contracts/contract/dao/protocol/settings/RocketDAOProtocolSettingsRewards.sol:L36-L49

function setSettingRewardsClaimer(string memory _contractName, uint256 _perc) override public onlyDAOProtocolProposal {
 // Get the total perc set, can't be more than 100
 uint256 percTotal = getRewardsClaimersPercTotal();
 // If this group already exists, it will update the perc
 uint256 percTotalUpdate = percTotal.add(_perc).sub(getRewardsClaimerPerc(_contractName));
 // Can't be more than a total claim amount of 100%
 require(percTotalUpdate <= 1 ether, "Claimers cannot total more than 100%");
 // Update the total
 setUint(keccak256(abi.encodePacked(settingNameSpace,"rewards.claims", "group.totalPerc")), percTotalUpdate);
 // Update/Add the claimer amount
 setUint(keccak256(abi.encodePacked(settingNameSpace, "rewards.claims", "group.amount", _contractName)), _perc);
 // Set the time it was updated at
 setUint(keccak256(abi.encodePacked(settingNameSpace, "rewards.claims", "group.amount.updated.time", _contractName)), block.timestamp);
}

The method updates the rewards claimer for a specific contract by writing to the following two setting keys:

settingNameSpace.rewards.claimsgroup.amount<_contractName>
settingNameSpace.rewards.claimsgroup.amount.updated.time<_contractName>

Due to the way the settings hierarchy was chosen in this case, a malicious proposal might define a <_contractName> = .updated.time<targetContract> that overwrites the settings of a different contract with an invalid value.

Note that the issue of delimiter consistency is also discussed in issue 5.12.

The severity rating is based on the fact that this should be detectable by DAO members. However, following a defense-in-depth approach means that such collisions should be avoided wherever possible.

Recommendation

We recommend enforcing a unique prefix and delimiter when concatenating user-provided input to setting keys. In this specific case, the settings could be renamed as follows:

settingNameSpace.rewards.claimsgroup.amount.value<_contractName>
settingNameSpace.rewards.claimsgroup.amount.updated.time<_contractName>

5.12 RocketDAOProtocolSettingsRewards - missing setting delimiters Medium Acknowledged

Resolution

The client acknowledges the finding and provided the following statement:

We are aware of this limitation but making this change now with an existing deployment outweighs the concern in our opinion.

Description

Settings in the Rocket Pool system are hierarchical, and namespaces are prefixed using dot delimiters.

Calling abi.encodePacked(<string>, <string>) on strings performs a simple concatenation. According to the settings’ naming scheme, it is suggested that the following example writes to a key named: <settingNameSpace>.rewards.claims.group.amount.<_contractName>. However, due to missing delimiters, the actual key written to is: <settingNameSpace>.rewards.claimsgroup.amount<_contractName>.

Note that there is no delimiter between claims|group and amount|<_contractName>.

code/contracts/contract/dao/protocol/settings/RocketDAOProtocolSettingsRewards.sol:L36-L49

function setSettingRewardsClaimer(string memory _contractName, uint256 _perc) override public onlyDAOProtocolProposal {
 // Get the total perc set, can't be more than 100
 uint256 percTotal = getRewardsClaimersPercTotal();
 // If this group already exists, it will update the perc
 uint256 percTotalUpdate = percTotal.add(_perc).sub(getRewardsClaimerPerc(_contractName));
 // Can't be more than a total claim amount of 100%
 require(percTotalUpdate <= 1 ether, "Claimers cannot total more than 100%");
 // Update the total
 setUint(keccak256(abi.encodePacked(settingNameSpace,"rewards.claims", "group.totalPerc")), percTotalUpdate);
 // Update/Add the claimer amount
 setUint(keccak256(abi.encodePacked(settingNameSpace, "rewards.claims", "group.amount", _contractName)), _perc);
 // Set the time it was updated at
 setUint(keccak256(abi.encodePacked(settingNameSpace, "rewards.claims", "group.amount.updated.time", _contractName)), block.timestamp);
}

Recommendation

We recommend adding the missing intermediate delimiters. The system should enforce delimiters after the last setting key before user input is concatenated to reduce the risk of accidental namespace collisions.

5.13 Use of `address` instead of specific contract types Minor Acknowledged

Resolution

Description

Rather than using a low-level address type and then casting to the safer contract type, it’s better to use the best type available by default so the compiler can eventually check for type safety and contract existence and only downcast to less secure low-level types (address) when necessary.

Examples

RocketStorageInterface _rocketStorage should be declared in the arguments, removing the need to cast the address explicitly.

code/contracts/contract/minipool/RocketMinipoolBase.sol:L39-L47

/// @notice Sets up starting delegate contract and then delegates initialisation to it
function initialise(address _rocketStorage, address _nodeAddress) external override notSelf {
 // Check input
 require(_nodeAddress != address(0), "Invalid node address");
 require(storageState == StorageState.Undefined, "Already initialised");
 // Set storage state to uninitialised
 storageState = StorageState.Uninitialised;
 // Set rocketStorage
 rocketStorage = RocketStorageInterface(_rocketStorage);

RocketMinipoolInterface _minipoolAddress should be declared in the arguments, removing the need to cast the address explicitly. Downcast to low-level address if needed. The event can be redeclared with the contract type.

code/contracts/contract/minipool/RocketMinipoolBondReducer.sol:L33-L34

function beginReduceBondAmount(address _minipoolAddress, uint256 _newBondAmount) override external onlyLatestContract("rocketMinipoolBondReducer", address(this)) {
 RocketMinipoolInterface minipool = RocketMinipoolInterface(_minipoolAddress);

code/contracts/contract/minipool/RocketMinipoolBondReducer.sol:L69-L76

/// @notice Returns whether owner of given minipool can reduce bond amount given the waiting period constraint
/// @param _minipoolAddress Address of the minipool
function canReduceBondAmount(address _minipoolAddress) override public view returns (bool) {
 RocketMinipoolInterface minipool = RocketMinipoolInterface(_minipoolAddress);
 RocketDAONodeTrustedSettingsMinipoolInterface rocketDAONodeTrustedSettingsMinipool = RocketDAONodeTrustedSettingsMinipoolInterface(getContractAddress("rocketDAONodeTrustedSettingsMinipool"));
 uint256 reduceBondTime = getUint(keccak256(abi.encodePacked("minipool.bond.reduction.time", _minipoolAddress)));
 return rocketDAONodeTrustedSettingsMinipool.isWithinBondReductionWindow(block.timestamp.sub(reduceBondTime));
}

code/contracts/contract/minipool/RocketMinipoolBondReducer.sol:L80-L84

function voteCancelReduction(address _minipoolAddress) override external onlyTrustedNode(msg.sender) onlyLatestContract("rocketMinipoolBondReducer", address(this)) {
 // Prevent calling if consensus has already been reached
 require(!getReduceBondCancelled(_minipoolAddress), "Already cancelled");
 // Get contracts
 RocketMinipoolInterface minipool = RocketMinipoolInterface(_minipoolAddress);

Note that abi.encode*(contractType) assumes address for contract types by default. An explicit downcast is not required.

 » Test example = Test(0x5B38Da6a701c568545dCfcB03FcB875f56beddC4)
» abi.encodePacked("hi", example)
0x68695b38da6a701c568545dcfcb03fcb875f56beddc4
» abi.encodePacked("hi", address(example))
0x68695b38da6a701c568545dcfcb03fcb875f56beddc4

More examples of address _minipool declarations:

code/contracts/contract/minipool/RocketMinipoolManager.sol:L449-L455

/// @dev Internal logic to set a minipool's pubkey
/// @param _pubkey The pubkey to set for the calling minipool
function _setMinipoolPubkey(address _minipool, bytes calldata _pubkey) private {
 // Load contracts
 AddressSetStorageInterface addressSetStorage = AddressSetStorageInterface(getContractAddress("addressSetStorage"));
 // Initialize minipool & get properties
 RocketMinipoolInterface minipool = RocketMinipoolInterface(_minipool);

code/contracts/contract/minipool/RocketMinipoolManager.sol:L474-L478

function getMinipoolDetails(address _minipoolAddress) override external view returns (MinipoolDetails memory) {
 // Get contracts
 RocketMinipoolInterface minipoolInterface = RocketMinipoolInterface(_minipoolAddress);
 RocketMinipoolBase minipool = RocketMinipoolBase(payable(_minipoolAddress));
 RocketNetworkPenaltiesInterface rocketNetworkPenalties = RocketNetworkPenaltiesInterface(getContractAddress("rocketNetworkPenalties"));

More examples of RocketStorageInterface _rocketStorage casts:

code/contracts/contract/node/RocketNodeDistributor.sol:L8-L13

contract RocketNodeDistributor is RocketNodeDistributorStorageLayout {
 bytes32 immutable distributorStorageKey;

 constructor(address _nodeAddress, address _rocketStorage) {
 rocketStorage = RocketStorageInterface(_rocketStorage);
 nodeAddress = _nodeAddress;

Recommendation

We recommend using more specific types instead of address where possible. Downcast if necessary. This goes for parameter types as well as state variable types.

5.14 Redundant double casts Minor Acknowledged

Resolution

The client acknowledges the finding and provided the following statement:

Acknowledged. These contracts are non-upgradable.

Description

_rocketStorageAddress is already of contract type RocketStorageInterface.

code/contracts/contract/RocketBase.sol:L78-L82

/// @dev Set the main Rocket Storage address
constructor(RocketStorageInterface _rocketStorageAddress) {
 // Update the contract address
 rocketStorage = RocketStorageInterface(_rocketStorageAddress);
}

_tokenAddress is already of contract type ERC20Burnable.

code/contracts/contract/RocketVault.sol:L132-L138

function burnToken(ERC20Burnable _tokenAddress, uint256 _amount) override external onlyLatestNetworkContract {
 // Get contract key
 bytes32 contractKey = keccak256(abi.encodePacked(getContractName(msg.sender), _tokenAddress));
 // Update balances
 tokenBalances[contractKey] = tokenBalances[contractKey].sub(_amount);
 // Get the token ERC20 instance
 ERC20Burnable tokenContract = ERC20Burnable(_tokenAddress);

_rocketTokenRPLFixedSupplyAddress is already of contract type IERC20.

code/contracts/contract/token/RocketTokenRPL.sol:L47-L51

constructor(RocketStorageInterface _rocketStorageAddress, IERC20 _rocketTokenRPLFixedSupplyAddress) RocketBase(_rocketStorageAddress) ERC20("Rocket Pool Protocol", "RPL") {
 // Version
 version = 1;
 // Set the mainnet RPL fixed supply token address
 rplFixedSupplyContract = IERC20(_rocketTokenRPLFixedSupplyAddress);

Recommendation

We recommend removing the unnecessary double casts and copies of local variables.

5.15 RocketMinipoolDelegate - Missing event in `prepareVacancy` Minor ✓ Fixed

Resolution

Description

The function prepareVacancy updates multiple contract state variables and should therefore emit an event.

Examples

code/contracts/contract/minipool/RocketMinipoolDelegate.sol:L286-L309

/// @dev Sets the bond value and vacancy flag on this minipool
/// @param _bondAmount The bond amount selected by the node operator
/// @param _currentBalance The current balance of the validator on the beaconchain (will be checked by oDAO and scrubbed if not correct)
function prepareVacancy(uint256 _bondAmount, uint256 _currentBalance) override external onlyLatestContract("rocketMinipoolManager", msg.sender) onlyInitialised {
 // Check status
 require(status == MinipoolStatus.Initialised, "Must be in initialised status");
 // Sanity check that refund balance is zero
 require(nodeRefundBalance == 0, "Refund balance not zero");
 // Check balance
 RocketDAOProtocolSettingsMinipoolInterface rocketDAOProtocolSettingsMinipool = RocketDAOProtocolSettingsMinipoolInterface(getContractAddress("rocketDAOProtocolSettingsMinipool"));
 uint256 launchAmount = rocketDAOProtocolSettingsMinipool.getLaunchBalance();
 require(_currentBalance >= launchAmount, "Balance is too low");
 // Store bond amount
 nodeDepositBalance = _bondAmount;
 // Calculate user amount from launch amount
 userDepositBalance = launchAmount.sub(nodeDepositBalance);
 // Flag as vacant
 vacant = true;
 preMigrationBalance = _currentBalance;
 // Refund the node whatever rewards they have accrued prior to becoming a RP validator
 nodeRefundBalance = _currentBalance.sub(launchAmount);
 // Set status to preLaunch
 setStatus(MinipoolStatus.Prelaunch);
}

Recommendation

Emit the missing event.

5.16 Compiler error due to missing `RocketMinipoolBaseInterface` Minor ✓ Fixed

Resolution

Description

The interface RocketMinipoolBaseInterface is missing from the code repository. Manually generating the interface and adding it to the repository fixes the error.

⇒ npx hardhat compile
Error HH404: File ../../interface/minipool/RocketMinipoolBaseInterface.sol, imported from contracts/contract/minipool/RocketMinipoolBase.sol, not found.
For more info go to https://hardhat.org/HH404 or run Hardhat with --show-stack-traces

Recommendation

Add the missing source unit to the repository.

5.17 Unused Imports Minor Partially Addressed

Resolution

Description

The following source units are imported but not referenced in the importing source unit:

code/contracts/contract/rewards/RocketMerkleDistributorMainnet.sol:L11

import "../../interface/rewards/RocketSmoothingPoolInterface.sol";

code/contracts/contract/minipool/RocketMinipoolFactory.sol:L12-L18

import "../../interface/minipool/RocketMinipoolManagerInterface.sol";
import "../../interface/minipool/RocketMinipoolQueueInterface.sol";
import "../../interface/node/RocketNodeStakingInterface.sol";
import "../../interface/util/AddressSetStorageInterface.sol";
import "../../interface/node/RocketNodeManagerInterface.sol";
import "../../interface/network/RocketNetworkPricesInterface.sol";
import "../../interface/dao/protocol/settings/RocketDAOProtocolSettingsMinipoolInterface.sol";

code/contracts/contract/minipool/RocketMinipoolFactory.sol:L8-L10

import "../../types/MinipoolStatus.sol";
import "../../types/MinipoolDeposit.sol";
import "../../interface/dao/node/RocketDAONodeTrustedInterface.sol";

code/contracts/contract/minipool/RocketMinipoolBase.sol:L7-L8

import "../../types/MinipoolDeposit.sol";
import "../../types/MinipoolStatus.sol";

code/contracts/contract/minipool/RocketMinipoolDelegate.sol:L13-L14

import "../../interface/network/RocketNetworkPricesInterface.sol";
import "../../interface/node/RocketNodeManagerInterface.sol";

code/contracts/contract/node/RocketNodeManager.sol:L13

import "../../interface/rewards/claims/RocketClaimNodeInterface.sol";

code/contracts/contract/rewards/RocketClaimDAO.sol:L7

import "../../interface/rewards/RocketRewardsPoolInterface.sol";

Duplicate Import:

code/contracts/contract/minipool/RocketMinipoolFactory.sol:L19-L20

import "../../interface/dao/protocol/settings/RocketDAOProtocolSettingsNodeInterface.sol";
import "../../interface/dao/protocol/settings/RocketDAOProtocolSettingsNodeInterface.sol";

The above list is exemplary, and there are likely more occurrences across the code base.

Recommendation

We recommend checking all imports and removing unused/unreferenced and unnecessary imports.

5.18 RocketMinipool - Inconsistent access control modifier declaration `onlyMinipoolOwner` Minor Acknowledged

Resolution

Acknowledged by the client. Not addressed within rocket-pool/rocketpool@77d7cca

Agreed. This would change a lot of contracts just for a minor improvement in readbility.

Description

The access control modifier onlyMinipoolOwner should be renamed to onlyMinipoolOwnerOrWithdrawalAddress to be consistent with the actual check permitting the owner or the withdrawal address to interact with the function. This would also be consistent with other declarations in the codebase.

Example

The onlyMinipoolOwner modifier in RocketMinipoolBase is the same as onlyMinipoolOwnerOrWithdrawalAddress in other modules.

code/contracts/contract/minipool/RocketMinipoolBase.sol:L31-L37

/// @dev Only allow access from the owning node address
modifier onlyMinipoolOwner() {
 // Only the node operator can upgrade
 address withdrawalAddress = rocketStorage.getNodeWithdrawalAddress(nodeAddress);
 require(msg.sender == nodeAddress || msg.sender == withdrawalAddress, "Only the node operator can access this method");
 _;
}

code/contracts/contract/old/minipool/RocketMinipoolOld.sol:L21-L27

// Only allow access from the owning node address
modifier onlyMinipoolOwner() {
 // Only the node operator can upgrade
 address withdrawalAddress = rocketStorage.getNodeWithdrawalAddress(nodeAddress);
 require(msg.sender == nodeAddress || msg.sender == withdrawalAddress, "Only the node operator can access this method");
 _;
}

Other declarations:

code/contracts/contract/minipool/RocketMinipoolDelegate.sol:L97-L107

/// @dev Only allow access from the owning node address
modifier onlyMinipoolOwner(address _nodeAddress) {
 require(_nodeAddress == nodeAddress, "Invalid minipool owner");
 _;
}

/// @dev Only allow access from the owning node address or their withdrawal address
modifier onlyMinipoolOwnerOrWithdrawalAddress(address _nodeAddress) {
 require(_nodeAddress == nodeAddress || _nodeAddress == rocketStorage.getNodeWithdrawalAddress(nodeAddress), "Invalid minipool owner");
 _;
}

code/contracts/contract/old/minipool/RocketMinipoolDelegateOld.sol:L82-L92

// Only allow access from the owning node address
modifier onlyMinipoolOwner(address _nodeAddress) {
 require(_nodeAddress == nodeAddress, "Invalid minipool owner");
 _;
}

// Only allow access from the owning node address or their withdrawal address
modifier onlyMinipoolOwnerOrWithdrawalAddress(address _nodeAddress) {
 require(_nodeAddress == nodeAddress || _nodeAddress == rocketStorage.getNodeWithdrawalAddress(nodeAddress), "Invalid minipool owner");
 _;
}

Recommendation

We recommend renaming RocketMinipoolBase.onlyMinipoolOwner to RocketMinipoolBase.onlyMinipoolOwnerOrWithdrawalAddress.

5.19 RocketDAO*Settings - `settingNameSpace` should be `immutable` Minor Acknowledged

Resolution

Description

The settingNameSpace in the abstract contract RocketDAONodeTrustedSettings is only set on contract deployment. Hence, the fields should be declared immutable to make clear that the settings namespace cannot change after construction.

Examples

RocketDAONodeTrustedSettings

code/contracts/contract/dao/node/settings/RocketDAONodeTrustedSettings.sol:L13-L16

// The namespace for a particular group of settings
bytes32 settingNameSpace;

code/contracts/contract/dao/node/settings/RocketDAONodeTrustedSettings.sol:L25-L30

// Construct
constructor(RocketStorageInterface _rocketStorageAddress, string memory _settingNameSpace) RocketBase(_rocketStorageAddress) {
 // Apply the setting namespace
 settingNameSpace = keccak256(abi.encodePacked("dao.trustednodes.setting.", _settingNameSpace));
}

RocketDAOProtocolSettings

code/contracts/contract/dao/protocol/settings/RocketDAOProtocolSettings.sol:L13-L14

// The namespace for a particular group of settings
bytes32 settingNameSpace;

code/contracts/contract/dao/protocol/settings/RocketDAOProtocolSettings.sol:L25-L29

// Construct
constructor(RocketStorageInterface _rocketStorageAddress, string memory _settingNameSpace) RocketBase(_rocketStorageAddress) {
 // Apply the setting namespace
 settingNameSpace = keccak256(abi.encodePacked("dao.protocol.setting.", _settingNameSpace));
}

code/contracts/contract/dao/protocol/settings/RocketDAOProtocolSettingsAuction.sol:L13-L15

constructor(RocketStorageInterface _rocketStorageAddress) RocketDAOProtocolSettings(_rocketStorageAddress, "auction") {
 // Set version
 version = 1;

Recommendation

We recommend using the immutable annotation in Solidity (see Immutable).

5.20 Inefficiencies with the `onlyMinipoolOwner` modifier Acknowledged

Resolution

Acknowledged by the client. No further actions.

Correct. This change would change every single contract we have and so the benefit does not outweigh the change.

Description

If a withdrawal address has not been set (or has been zeroed out), rocketStorage.getNodeWithdrawalAddress(nodeAddress) returns nodeAddress. This outcome leads to the modifier checking the same address twice (msg.sender == nodeAddress || msg.sender == nodeAddress):

code/contracts/contract/minipool/RocketMinipoolBase.sol:L31-L37

/// @dev Only allow access from the owning node address
modifier onlyMinipoolOwner() {
 // Only the node operator can upgrade
 address withdrawalAddress = rocketStorage.getNodeWithdrawalAddress(nodeAddress);
 require(msg.sender == nodeAddress || msg.sender == withdrawalAddress, "Only the node operator can access this method");
 _;
}

code/contracts/contract/RocketStorage.sol:L103-L111

// Get a node's withdrawal address
function getNodeWithdrawalAddress(address _nodeAddress) public override view returns (address) {
 // If no withdrawal address has been set, return the nodes address
 address withdrawalAddress = withdrawalAddresses[_nodeAddress];
 if (withdrawalAddress == address(0)) {
 return _nodeAddress;
 }
 return withdrawalAddress;
}

5.21 RocketNodeDeposit - Duplicate check to avoid revert ✓ Fixed

Resolution

Fixed with rocket-pool/rocketpool@3ab7af1 by introducing a new method maybeAssignDeposits() that does not revert by default but returns a boolean instead. This way, RocketNodeDeposit directly call the maybeAssignDeposits() function, avoiding the duplicate check.

This finding does not present a security-related problem in the code base, which is why we downgrade its severity to informational. However, we opted to keep this recommendation present in the report since it underlines a form of technical debt where old functionality is wrapped by new functionality using a workaround.

Description

When receiving and subsequently assigning deposits, the RocketNodeDeposit contract’s assignDeposits function calls RocketDAOProtocolSettingsDeposit.getAssignDepositsEnabled and skips the assignment of funds. This is done because the RocketDepositPool.assignDeposits function reverts if the setting is disabled:

code/contracts/contract/deposit/RocketDepositPool.sol:L207-L212

function assignDeposits() override external onlyThisLatestContract {
 // Load contracts
 RocketDAOProtocolSettingsDepositInterface rocketDAOProtocolSettingsDeposit = RocketDAOProtocolSettingsDepositInterface(getContractAddress("rocketDAOProtocolSettingsDeposit"));
 // Revert if assigning is disabled
 require(_assignDeposits(rocketDAOProtocolSettingsDeposit), "Deposit assignments are currently disabled");
}

However, the underlying _assignDeposits function already performs a check for the setting and returns prematurely to avoid assignment.

code/contracts/contract/deposit/RocketDepositPool.sol:L217-L219

if (!_rocketDAOProtocolSettingsDeposit.getAssignDepositsEnabled()) {
 return false;
}

The rocketDAOProtocolSettingsDeposit.getAssignDepositsEnabled() setting is checked twice. The first occurrence is in RocketNodeDeposit.assignDeposits and the second one in the same flow is contained in RocketDepositPool._assignDeposits. The second check is performed in a reverting fashion, thus requiring the top-level check in the RocketNodeDeposit contract to preemptively fetch and check the setting before continuing.

Recommendation

Since Rocketpool v1.2 already aims to perform an upgrade on the RocketDepositPool contract, we do recommend adding a separate, non-reverting version of the RocketDepositPool.assignDeposits function to the code base and removing the redundant preemptive check in RocketNodeDeposit.assignDeposits. This will improve readability and maintainability of future versions of the code, and save gas cost on deposit assignment operations.

5.22 Inconsistent Coding Style Acknowledged

Resolution

The client provided the following statement:

Acknolwedge your recommendation but we are dealing with an existing deployed codebase and if we change codestyle on only the contracts we update we will end up with a codebase with different code styles which is worse than one that is internally consistent but not consistent with best practice.

Description

Deviations from the Solidity Style Guide were identified throughout the codebase. Considering how much value a consistent coding style adds to the project’s readability, enforcing a standard coding style with the help of linter tools is recommended.

Inconsistent Function naming scheme for external and internal interfaces

Throughout the codebase, private/internal functions are generally prefixed with an underscore (_<name>). This allows for an easy way to see if an external party can interact with a function without having to scan the declaration line for the corresponding visibility keywords. However, this naming scheme is not enforced consistently. Many internal function names are indistinguishable from external function names. It is therefore highly recommended to implement a consistent naming scheme and prefix internal functions with an underscore (_<name>).

code/contracts/contract/node/RocketNodeDeposit.sol:L268-L283

/// @dev Reverts if vacant minipools are not enabled
function checkVacantMinipoolsEnabled() private view {
 // Get contracts
 RocketDAOProtocolSettingsNodeInterface rocketDAOProtocolSettingsNode = RocketDAOProtocolSettingsNodeInterface(getContractAddress("rocketDAOProtocolSettingsNode"));
 // Check node settings
 require(rocketDAOProtocolSettingsNode.getVacantMinipoolsEnabled(), "Vacant minipools are currently disabled");
}

/// @dev Executes an assignDeposits call on the deposit pool
function assignDeposits() private {
 RocketDAOProtocolSettingsDepositInterface rocketDAOProtocolSettingsDeposit = RocketDAOProtocolSettingsDepositInterface(getContractAddress("rocketDAOProtocolSettingsDeposit"));
 if (rocketDAOProtocolSettingsDeposit.getAssignDepositsEnabled()) {
 RocketDepositPoolInterface rocketDepositPool = RocketDepositPoolInterface(getContractAddress("rocketDepositPool"));
 rocketDepositPool.assignDeposits();
 }
}

code/contracts/contract/minipool/RocketMinipoolDelegate.sol:L339-L345

/// @dev Stakes the balance of this minipool into the deposit contract to set withdrawal credentials to this contract
/// @param _validatorSignature A signature over the deposit message object
/// @param _depositDataRoot The hash tree root of the deposit data object
function preStake(bytes calldata _validatorPubkey, bytes calldata _validatorSignature, bytes32 _depositDataRoot) internal {
 // Load contracts
 DepositInterface casperDeposit = DepositInterface(getContractAddress("casperDeposit"));
 RocketMinipoolManagerInterface rocketMinipoolManager = RocketMinipoolManagerInterface(getContractAddress("rocketMinipoolManager"));

code/contracts/contract/minipool/RocketMinipoolDelegate.sol:L651-L654

/// @dev Distributes the current contract balance based on capital ratio and node fee
function distributeSkimmedRewards() internal {
 uint256 rewards = address(this).balance.sub(nodeRefundBalance);
 uint256 nodeShare = calculateNodeRewards(nodeDepositBalance, getUserDepositBalance(), rewards);

code/contracts/contract/minipool/RocketMinipoolDelegate.sol:L661-L663

/// @dev Set the minipool's current status
/// @param _status The new status
function setStatus(MinipoolStatus _status) private {

code/contracts/contract/node/RocketNodeDeposit.sol:L202-L206

/// @dev Adds a minipool to the queue
function enqueueMinipool(address _minipoolAddress) private {
 // Add minipool to queue
 RocketMinipoolQueueInterface(getContractAddress("rocketMinipoolQueue")).enqueueMinipool(_minipoolAddress);
}

code/contracts/contract/node/RocketNodeDeposit.sol:L208-L213

/// @dev Reverts if node operator has not initialised their fee distributor
function checkDistributorInitialised() private view {
 // Check node has initialised their fee distributor
 RocketNodeManagerInterface rocketNodeManager = RocketNodeManagerInterface(getContractAddress("rocketNodeManager"));
 require(rocketNodeManager.getFeeDistributorInitialised(msg.sender), "Fee distributor not initialised");
}

code/contracts/contract/node/RocketNodeDeposit.sol:L215-L218

/// @dev Creates a minipool and returns an instance of it
/// @param _salt The salt used to determine the minipools address
/// @param _expectedMinipoolAddress The expected minipool address. Reverts if not correct
function createMinipool(uint256 _salt, address _expectedMinipoolAddress) private returns (RocketMinipoolInterface) {

code/contracts/contract/auction/RocketAuctionManager.sol:L58-L60

function setLotCount(uint256 _amount) private {
 setUint(keccak256("auction.lots.count"), _amount);
}

Appendix 1 - Files in Scope

This audit covered the following files:

SHA-1 Hash	File
`e0d054c08e868a73e78f29c5b80b6b2d9d31ac43`	`contracts/contract/node/RocketNodeDistributorStorageLayout.sol`
`ed07e8bd9a7309d3755ed76bce7c04e48628b9de`	`contracts/contract/node/RocketNodeDistributorFactory.sol`
`ef40c3420e4492bbd405d239c63b2dbb23d167e3`	`contracts/contract/node/RocketNodeDistributor.sol`
`493234d4c3fc24d598067d60e746f1257bf4bb5f`	`contracts/contract/node/RocketNodeStaking.sol`
`eb50e6e9cc2eb94cd01288521f9bc73890fc483c`	`contracts/contract/node/RocketNodeManager.sol`
`e73ed34c2c3baa463dac0947b70dbb1e5bcff99d`	`contracts/contract/node/RocketNodeDeposit.sol`
`77785b4a3196f4736e574ea5e87b018d3d3e7824`	`contracts/contract/node/RocketNodeDistributorDelegate.sol`
`8749d09f3e9e80f9676fd9d48622a850d3d3f71b`	`contracts/contract/upgrade/RocketUpgradeOneDotTwo.sol`
`cd1c38eb32ab318934d94b40531e32f1ac58261e`	`contracts/contract/rewards/RocketMerkleDistributorMainnet.sol`
`2d7fcb81de5fb404c26880d8015535c5423b8651`	`contracts/contract/rewards/RocketRewardsPool.sol`
`93d3616ebaa9053f8defdebf8568ebcdc7d6b7b6`	`contracts/contract/rewards/RocketSmoothingPool.sol`
`0242996368cabfed713cceccb2313f2a0f6fe586`	`contracts/contract/rewards/RocketClaimDAO.sol`
`b0bda868dad3c7c43ad740b24a37e2cb4f52994f`	`contracts/contract/RocketBase.sol`
`e17c6b07288ea458d101cd54bd2a230d67f8722d`	`contracts/contract/network/RocketNetworkBalances.sol`
`cdf2a398e2ff8f6a55ec7efd5c6c316e34ae4966`	`contracts/contract/network/RocketNetworkFees.sol`
`9b81afb407809332c2a3f605317cfadebf3289cb`	`contracts/contract/network/RocketNetworkPenalties.sol`
`5ce805aba1d1457a80b9c9d3b49fa2753771aeee`	`contracts/contract/network/RocketNetworkPrices.sol`
`bfb92f4e76b81ac6ed1c09e405a39394b93ec398`	`contracts/contract/RocketVault.sol`
`385c9f5e2e6eeac513c923e163e531fc18d89cbc`	`contracts/contract/dao/node/RocketDAONodeTrustedActions.sol`
`3072edef2f2e1f58be33b27d6dc36071529e1f56`	`contracts/contract/dao/node/settings/RocketDAONodeTrustedSettingsRewards.sol`
`ed47d92656f92c5b09a84a343bc071aa76d20bd2`	`contracts/contract/dao/node/settings/RocketDAONodeTrustedSettingsProposals.sol`
`a4876b026c0e03777e1121616856b57a0fe20f0c`	`contracts/contract/dao/node/settings/RocketDAONodeTrustedSettingsMinipool.sol`
`154ae3907234b7cadbd35841f8cba3582ff4e96c`	`contracts/contract/dao/node/settings/RocketDAONodeTrustedSettingsMembers.sol`
`cc57265a125d582c3b4e4e50f2ea3f2427df8133`	`contracts/contract/dao/node/settings/RocketDAONodeTrustedSettings.sol`
`0f8e23d4ad751551638956a5e4673723ab466634`	`contracts/contract/dao/node/RocketDAONodeTrusted.sol`
`2511ff09533cc83a7daf112121ed20dad88ee8a0`	`contracts/contract/dao/node/RocketDAONodeTrustedUpgrade.sol`
`6ca81e7796640015458083001b068fe2aa9cd56f`	`contracts/contract/dao/node/RocketDAONodeTrustedProposals.sol`
`ee9dcdc3cfcb1375bd7c0a18e602b603118baaba`	`contracts/contract/dao/protocol/RocketDAOProtocolProposals.sol`
`9eb042d1c047dd258ab3785458e14e34b93bc82e`	`contracts/contract/dao/protocol/RocketDAOProtocol.sol`
`75e11937311100a134d055c6d683b286825b88b5`	`contracts/contract/dao/protocol/settings/RocketDAOProtocolSettingsRewards.sol`
`76dd45aeebdade40cf3df12c3b6eaa4d60a31d44`	`contracts/contract/dao/protocol/settings/RocketDAOProtocolSettingsInflation.sol`
`05ef6bb54e501a6bdd3718a387d92fd6e0ca570a`	`contracts/contract/dao/protocol/settings/RocketDAOProtocolSettingsDeposit.sol`
`d8d177fbeb398efed9ae2cf46511e5614ab96872`	`contracts/contract/dao/protocol/settings/RocketDAOProtocolSettingsNetwork.sol`
`bfba6f800c374736cf64fa842a6e5001d28635eb`	`contracts/contract/dao/protocol/settings/RocketDAOProtocolSettings.sol`
`1c7ddd122b145bd100d5e25ce8486e69bb19d4a8`	`contracts/contract/dao/protocol/settings/RocketDAOProtocolSettingsAuction.sol`
`820f43fea15eff788f5f85896cb52ee2d8810775`	`contracts/contract/dao/protocol/settings/RocketDAOProtocolSettingsMinipool.sol`
`9145f779f88723787a5fe20a3228058e4fe9186a`	`contracts/contract/dao/protocol/settings/RocketDAOProtocolSettingsNode.sol`
`c692c30c4914517da9f87e0b2dccd7a9d4134b9b`	`contracts/contract/dao/protocol/RocketDAOProtocolActions.sol`
`c6b7566a67ed27afcbb4e3b77ca1877167fe94d7`	`contracts/contract/dao/RocketDAOProposal.sol`
`597f7b6f0eb5b05cfb9b74f4af750e2afa07f11c`	`contracts/contract/helper/PenaltyTest.sol`
`bf503ce2a597cc582c255f39e9ecfb6a5362ffc2`	`contracts/contract/helper/RevertOnTransfer.sol`
`5692e5148551021f7a529190338e0276f2cc7343`	`contracts/contract/minipool/RocketMinipoolBondReducer.sol`
`872b8563fef2dbe77a3301a205ff3c39182cf1cb`	`contracts/contract/minipool/RocketMinipoolManager.sol`
`2fa1e7d7e69703ae18759bf4afa82a674420c01b`	`contracts/contract/minipool/RocketMinipoolQueue.sol`
`f393a16e1f674ade2ef9f1ecb86b930d18ae8d45`	`contracts/contract/minipool/RocketMinipoolStorageLayout.sol`
`cb683564f47dac2a13ba87622bf4f69d264f1cc6`	`contracts/contract/minipool/RocketMinipoolBase.sol`
`3df5f46799f97b7a383bc3cc8661adfe5aaa913a`	`contracts/contract/minipool/RocketMinipoolStatus.sol`
`08b9c9d65188d4931b24865c86795602a48247a2`	`contracts/contract/minipool/RocketMinipoolPenalty.sol`
`a12e8f5594c70b9841097836dc3954ddb97bf02e`	`contracts/contract/minipool/RocketMinipoolDelegate.sol`
`9fbadfca1a9d012bafbf6fff127fe38d589826d8`	`contracts/contract/minipool/RocketMinipoolFactory.sol`
`d4c47c746ef9fbdee7bef9cab11ccc8811478ed2`	`contracts/contract/auction/RocketAuctionManager.sol`
`fa66f01a46cc020e8195ebe8da572ee941a82df8`	`contracts/contract/util/AddressSetStorage.sol`
`538e2b9b6b13d1479226a2fe9cdd8dc2495b2514`	`contracts/contract/util/AddressQueueStorage.sol`
`5883ae7125d52f6f2257e0513300897f9a783343`	`contracts/contract/old/node/RocketNodeStakingOld.sol`
`e99e576cbdfd38d8d6e3fbcdd5abd63362f5cc5c`	`contracts/contract/old/node/RocketNodeDistributorDelegateOld.sol`
`4163cf93214c71d46078ee8551863770069cc360`	`contracts/contract/old/node/RocketNodeManagerOld.sol`
`18be22f61a864272b7927d8015404a0a2cc7705c`	`contracts/contract/old/node/RocketNodeDepositOld.sol`
`c63733bf0da9f7a2750abc17c0399fd578c2f74e`	`contracts/contract/old/network/RocketNetworkPricesOld.sol`
`73249ffc350985908b54b498b5564d52613d7788`	`contracts/contract/old/dao/node/settings/RocketDAONodeTrustedSettingsMinipoolOld.sol`
`d52198de4d315e7e5f9277e3a6217540f098bb1b`	`contracts/contract/old/dao/protocol/settings/RocketDAOProtocolSettingsDepositOld.sol`
`22596ea8ae13053a6b71f25e0524c92694daf837`	`contracts/contract/old/dao/protocol/settings/RocketDAOProtocolSettingsNodeOld.sol`
`c824fb92cf1dd25326cf9f82048ed1ceca0d86df`	`contracts/contract/old/dao/protocol/settings/RocketDAOProtocolSettingsMinipoolOld.sol`
`731eed38ef605e4ab3f7e1b7f2885e3ebf5a55c8`	`contracts/contract/old/minipool/RocketMinipoolOld.sol`
`9be158bbbfa5a787847468b92067f24d9eab1c6c`	`contracts/contract/old/minipool/RocketMinipoolManagerOld.sol`
`b56e74b5e1fd7692e71a2b33c77f7db6cb09bea3`	`contracts/contract/old/minipool/RocketMinipoolFactoryOld.sol`
`3b517c55a54b5234c8e5c5a5497b3ead4020dd8b`	`contracts/contract/old/minipool/RocketMinipoolQueueOld.sol`
`9ca01d01c6c8e3c41573f273c9c9545d3866aae4`	`contracts/contract/old/minipool/RocketMinipoolDelegateOld.sol`
`bdc6324139800c351bb70a0e7f47d5586c95a56e`	`contracts/contract/old/minipool/RocketMinipoolStorageLayoutOld.sol`
`200d56ccab76cc739055597e0af551f4a09add1c`	`contracts/contract/old/deposit/RocketDepositPoolOld.sol`
`78b524187aec0479e15178ded2d4f183c7414e02`	`contracts/contract/old/RocketNetworkFeesOld.sol`
`04cdfab0f47dd76a900ad25ef7a1995790007cbd`	`contracts/contract/deposit/RocketDepositPool.sol`
`6d556731d640f8ff78f04b3b2ba54ccc52b9791a`	`contracts/contract/token/RocketTokenRPL.sol`
`1fa2dd47d3d999602b866db678db083d700e0799`	`contracts/contract/token/temp/RocketTokenDummyRPL.sol`
`b9cea77bf1178201cc8e4b839016165d73740137`	`contracts/contract/token/RocketTokenRETH.sol`
`0e17a838a7ae3a0d117bdb29dedf77b16bce3736`	`contracts/contract/RocketStorage.sol`
`2b123f1a01b8fc7664336af46038d5db2f0117f3`	`contracts/types/MinipoolDetails.sol`
`5bd86e815d047d6aab8b091a0b783f1cd10de4e7`	`contracts/types/MinipoolDeposit.sol`
`31e943e327367421baf300e9b820823fafb94311`	`contracts/types/NodeDetails.sol`
`131fabe8d6efbed51f9c7f24ed88356c03315c9f`	`contracts/types/SettingType.sol`
`555564772a803a032929cb3d50091999170ec93c`	`contracts/types/RewardSubmission.sol`
`d57614fab9b875413a5afa2fa28837f005af067a`	`contracts/types/MinipoolStatus.sol`
`93d2dc04d17a72fcd9f5b8a736b62f524ee87106`	`contracts/types/old/NodeDetailsOld.sol`
`cfff9e6406d398e3fe838d001ffd91cdeffa0b88`	`contracts/interface/node/RocketNodeDistributorFactoryInterface.sol`
`dfde8dac4ab81303f3787b8ce9ea628773a8428d`	`contracts/interface/node/RocketNodeStakingInterface.sol`
`5c9cb11249756bfeac769deaf0c8533e5d381ca2`	`contracts/interface/node/RocketNodeManagerInterface.sol`
`c07bc1a65cc9a9beb06eb311d5345a7040bfb778`	`contracts/interface/node/RocketNodeDepositInterface.sol`
`2a993cfb99c99df89628c77e9edefefe55d1483a`	`contracts/interface/node/RocketNodeDistributorInterface.sol`
`f97a5ced28a49d32180441d7379457896f6bb825`	`contracts/interface/rewards/claims/RocketClaimDAOInterface.sol`
`808f2c9875eb438edf944ba564adc5ea31213b44`	`contracts/interface/rewards/claims/RocketClaimNodeInterface.sol`
`f9b774da2203be3833acb225b2ed1a57b06d4a46`	`contracts/interface/rewards/claims/RocketClaimTrustedNodeInterface.sol`
`8eb877d619690538c64b84c45c15077d3439501b`	`contracts/interface/rewards/RocketSmoothingPoolInterface.sol`
`5943539f2bae9ee3b4cd49cfd1bcb9e3c033c094`	`contracts/interface/rewards/RocketRewardsRelayInterface.sol`
`b2e6aabaccf88e3ac9e3a846e7df41ad64a691de`	`contracts/interface/rewards/RocketRewardsPoolInterface.sol`
`1cc38d548b5051fef88fb1fb8b2c5715cfd93048`	`contracts/interface/RocketStorageInterface.sol`
`f1c07b7eff6755965f2f6d8d04f5f8a160e73fbe`	`contracts/interface/network/RocketNetworkBalancesInterface.sol`
`66a93db7577f75799403201e116bc98b39753e66`	`contracts/interface/network/RocketNetworkFeesInterface.sol`
`23a03945ca61fde02619754dab79738ebc699cde`	`contracts/interface/network/RocketNetworkPricesInterface.sol`
`de391100daede5612fb913f904b64c84e5b5564a`	`contracts/interface/network/RocketNetworkPenaltiesInterface.sol`
`05b0dc75a1c8871ec3ff17c9a68f68335e344786`	`contracts/interface/dao/node/RocketDAONodeTrustedInterface.sol`
`1e2fcfeba5a1cb02520328d0cbb82640f51a5440`	`contracts/interface/dao/node/settings/RocketDAONodeTrustedSettingsInterface.sol`
`452c28b2b5db2b487013ae27a4dafd3826cc7aad`	`contracts/interface/dao/node/settings/RocketDAONodeTrustedSettingsMembersInterface.sol`
`8bd46a6e1f6988a332f6c1c6921ee566b3128db2`	`contracts/interface/dao/node/settings/RocketDAONodeTrustedSettingsRewardsInterface.sol`
`d74e01d9b27e2e544c498d80a43a4ca9eb559f75`	`contracts/interface/dao/node/settings/RocketDAONodeTrustedSettingsMinipoolInterface.sol`
`e51bda8eaf5d0adbed8ce748d50bf394194020a3`	`contracts/interface/dao/node/settings/RocketDAONodeTrustedSettingsProposalsInterface.sol`
`2b403337eb717c363a8c0d43f734e8bb2a52a428`	`contracts/interface/dao/node/RocketDAONodeTrustedProposalsInterface.sol`
`a060b82998383560e546e4f3d979648db36f8b35`	`contracts/interface/dao/node/RocketDAONodeTrustedActionsInterface.sol`
`12bf1b71506bcaa85c905796e1a45bf32cf4cd20`	`contracts/interface/dao/node/RocketDAONodeTrustedUpgradeInterface.sol`
`9611ef6727bd4660835d3d63a20b870d83091c57`	`contracts/interface/dao/RocketDAOProposalInterface.sol`
`954bdc0c88c5a2cc1f48e0d36094497573d8316c`	`contracts/interface/dao/protocol/RocketDAOProtocolProposalsInterface.sol`
`de87bf180b6fb7df2ccd6235e5d65d41f5adee0d`	`contracts/interface/dao/protocol/RocketDAOProtocolActionsInterface.sol`
`1763f669eed40014cd6b9a5b6450ef8933d5b93d`	`contracts/interface/dao/protocol/settings/RocketDAOProtocolSettingsNodeInterface.sol`
`71bb735546a78b0d5655362900099b6a48be09b2`	`contracts/interface/dao/protocol/settings/RocketDAOProtocolSettingsDepositInterface.sol`
`239d0e9b16826d507e5969b576f0e9ae62c4c19b`	`contracts/interface/dao/protocol/settings/RocketDAOProtocolSettingsNetworkInterface.sol`
`d7f3796a17e351037c5b26d5cce8813732f35478`	`contracts/interface/dao/protocol/settings/RocketDAOProtocolSettingsInflationInterface.sol`
`f5a68583e26cbcecaae578e05a193b629a675971`	`contracts/interface/dao/protocol/settings/RocketDAOProtocolSettingsAuctionInterface.sol`
`c02d578e41414d241ab10b082cec3fa72e61ea55`	`contracts/interface/dao/protocol/settings/RocketDAOProtocolSettingsRewardsInterface.sol`
`d5fb57ee5e68482d1ef976d3f89c873d8b19c317`	`contracts/interface/dao/protocol/settings/RocketDAOProtocolSettingsMinipoolInterface.sol`
`af57eb613c63ce3d47f812c96a42d024430961ac`	`contracts/interface/dao/protocol/settings/RocketDAOProtocolSettingsInterface.sol`
`c774f46eca674eff940a8879986495871486ce29`	`contracts/interface/dao/protocol/RocketDAOProtocolInterface.sol`
`9cced1a86b7063603337ea26351a5eab6172eeb4`	`contracts/interface/minipool/RocketMinipoolStatusInterface.sol`
`5a21b810b85ec99f4449580d1cb308f0829c3132`	`contracts/interface/minipool/RocketMinipoolInterface.sol`
`9612f16185357104a28835121f9c276ac41c3ac0`	`contracts/interface/minipool/RocketMinipoolBondReducerInterface.sol`
`c7454f8bc5474382369bd03313abb378852a855d`	`contracts/interface/minipool/RocketMinipoolManagerInterface.sol`
`a5ab18a608c884c09ed8646fa637b154ed475d7f`	`contracts/interface/minipool/RocketMinipoolPenaltyInterface.sol`
`8b7e3d06e9a221b769359bf43d4a8bf714700b21`	`contracts/interface/minipool/RocketMinipoolFactoryInterface.sol`
`54d241f1e4bf58715fc0e3c57e8689aa2fcfd03b`	`contracts/interface/minipool/RocketMinipoolQueueInterface.sol`
`b4384136cd0b9a0ada9fbbf4677e93508aa2102d`	`contracts/interface/RocketVaultInterface.sol`
`97be385ba8163a4d6e090162d265fc524ca4b4ed`	`contracts/interface/auction/RocketAuctionManagerInterface.sol`
`5cbb81c3088e73dddd70e14625df8692c0992ab7`	`contracts/interface/util/AddressQueueStorageInterface.sol`
`896dacb8f86aadcb1bcfe1d4fab102b10d2bc4fa`	`contracts/interface/util/AddressSetStorageInterface.sol`
`1a0dc382251ed0c61e264a618dda4690f9c1038c`	`contracts/interface/RocketVaultWithdrawerInterface.sol`
`e15b25e42ced34804a3bb9d8b703f49ac4abcd91`	`contracts/interface/old/RocketNodeDepositInterfaceOld.sol`
`a3b41c4e7b1a364cf06b8a3ef212d7711f8d5b7d`	`contracts/interface/old/RocketNodeManagerInterfaceOld.sol`
`393060945891e037d826c9eb68245ab032c8facb`	`contracts/interface/old/RocketDAOProtocolSettingsDepositInterfaceOld.sol`
`199b624f7e297770a57f9194329d612c88450912`	`contracts/interface/old/RocketDAOProtocolSettingsMinipoolInterfaceOld.sol`
`443a05bb1d24b47f917fd82ccab11edf26275532`	`contracts/interface/old/RocketNodeStakingInterfaceOld.sol`
`fbb07f75c8cc09480dca7d2d77f31f393a30768c`	`contracts/interface/old/RocketNetworkPricesInterfaceOld.sol`
`641fec69187057f8a6b61af1cd319d16691e31e7`	`contracts/interface/old/RocketMinipoolQueueInterfaceOld.sol`
`06dbe261b1e5845f8372dcf38960586671536f66`	`contracts/interface/old/RocketMinipoolManagerInterfaceOld.sol`
`8474c41be9da63a43932cf664549e80cd328936b`	`contracts/interface/old/RocketDAOProtocolSettingsNodeInterfaceOld.sol`
`c482301168243ce9fb2fb30437e4ef80c70150c0`	`contracts/interface/old/RocketMinipoolFactoryInterfaceOld.sol`
`c91cd7e324ebb5816c5777a5439d97ff5facbfbb`	`contracts/interface/old/RocketMinipoolInterfaceOld.sol`
`9b80affcc3e20f394a8e8d6a51e2b50f7c2e04a3`	`contracts/interface/old/RocketDAONodeTrustedSettingsMinipoolInterfaceOld.sol`
`0f076ba9160da4e99fce2f74d05de49baddf78d7`	`contracts/interface/old/RocketDepositPoolInterfaceOld.sol`
`b6869515a10a2ac8092bef1769a6f1e34b907715`	`contracts/interface/old/RocketNodeDistributorInterfaceOld.sol`
`ec56e4455efe3514697104258c32df13f3372469`	`contracts/interface/deposit/RocketDepositPoolInterface.sol`
`af4be5e3610b15b9670e1af4b3976b45885f85d4`	`contracts/interface/token/RocketTokenRPLInterface.sol`
`5527b3f945c2650b7efccbf44eb5f781f3a2e4f7`	`contracts/interface/token/RocketTokenRETHInterface.sol`
`2705faf4605281dfeee78c56847d09cb3100bf77`	`contracts/interface/casper/DepositInterface.sol`

Appendix 2 - Disclosure

ConsenSys Diligence (“CD”) typically receives compensation from one or more clients (the “Clients”) for performing the analysis contained in these reports (the “Reports”). The Reports may be distributed through other means, including via ConsenSys publications and other distributions.

The Reports are not an endorsement or indictment of any particular project or team, and the Reports do not guarantee the security of any particular project. This Report does not consider, and should not be interpreted as considering or having any bearing on, the potential economics of a token, token sale or any other product, service or other asset. Cryptographic tokens are emergent technologies and carry with them high levels of technical risk and uncertainty. No Report provides any warranty or representation to any Third-Party in any respect, including regarding the bugfree nature of code, the business model or proprietors of any such business model, and the legal compliance of any such business. No third party should rely on the Reports in any way, including for the purpose of making any decisions to buy or sell any token, product, service or other asset. Specifically, for the avoidance of doubt, this Report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project. CD owes no duty to any Third-Party by virtue of publishing these Reports.

PURPOSE OF REPORTS The Reports and the analysis described therein are created solely for Clients and published with their consent. The scope of our review is limited to a review of code and only the code we note as being within the scope of our review within this report. Any Solidity code itself presents unique and unquantifiable risks as the Solidity language itself remains under development and is subject to unknown risks and flaws. The review does not extend to the compiler layer, or any other areas beyond specified code that could present security risks. Cryptographic tokens are emergent technologies and carry with them high levels of technical risk and uncertainty. In some instances, we may perform penetration testing or infrastructure assessments depending on the scope of the particular engagement.

CD makes the Reports available to parties other than the Clients (i.e., “third parties”) – on its website. CD hopes that by making these analyses publicly available, it can help the blockchain ecosystem develop technical best practices in this rapidly evolving area of innovation.

LINKS TO OTHER WEB SITES FROM THIS WEB SITE You may, through hypertext or other computer links, gain access to web sites operated by persons other than ConsenSys and CD. Such hyperlinks are provided for your reference and convenience only, and are the exclusive responsibility of such web sites’ owners. You agree that ConsenSys and CD are not responsible for the content or operation of such Web sites, and that ConsenSys and CD shall have no liability to you or any other person or entity for the use of third party Web sites. Except as described below, a hyperlink from this web Site to another web site does not imply or mean that ConsenSys and CD endorses the content on that Web site or the operator or operations of that site. You are solely responsible for determining the extent to which you may use any content at any other web sites to which you link from the Reports. ConsenSys and CD assumes no responsibility for the use of third party software on the Web Site and shall have no liability whatsoever to any person or entity for the accuracy or completeness of any outcome generated by such software.

TIMELINESS OF CONTENT The content contained in the Reports is current as of the date appearing on the Report and is subject to change without notice. Unless indicated otherwise, by ConsenSys and CD.

Audit Reports: Ethereum Improvement Proposals (EIPs)

Search Criteria

Enrichments

Find more results