Rocketpool

Description

Consider simplifying the logic in getWithdrawalCredentials to abi.encodePacked(byte(0x01), bytes11(0x0), address(this)).

Examples

rocketpool-2.5-Tokenomics-updates/contracts/contract/minipool/RocketMinipool.sol:L91-L105

function getWithdrawalCredentials() override external view returns (bytes memory) {
    // Parameters
    uint256 credentialsLength = 32;
    uint256 addressLength = 20;
    uint256 addressOffset = credentialsLength - addressLength;
    byte withdrawalPrefix = 0x01;
    // Calculate & return
    bytes memory ret = new bytes(credentialsLength);
    bytes20 addr = bytes20(address(this));
    ret[0] = withdrawalPrefix;
    for (uint256 i = 0; i < addressLength; i++) {
        ret[i + addressOffset] = addr[i];
    }
    return ret;
}

Description

Consider using the best type available in the function arguments and even declaration instead of accepting address and later casting it to the correct type.

Examples

This is only one of many examples. The method accepts address type arguments while this could already be declared as RocketStorageInterface _rocketStorageAddress.

rocketpool-2.5-Tokenomics-updates/contracts/contract/minipool/RocketMinipool.sol:L71-L77

constructor(address _rocketStorageAddress, address _nodeAddress, MinipoolDeposit _depositType) {
    // Check parameters
    require(_rocketStorageAddress != address(0x0), "Invalid storage address");
    require(_nodeAddress != address(0x0), "Invalid node address");
    require(_depositType != MinipoolDeposit.None, "Invalid deposit type");
    // Initialise RocketStorage
    rocketStorage = RocketStorageInterface(_rocketStorageAddress);

_tokenAddress can be declared as IERC20 in arguments and events.

rocketpool-2.5-Tokenomics-updates/contracts/contract/RocketVault.sol:L82-L82

IERC20 tokenContract = IERC20(_tokenAddress);

rocketpool-2.5-Tokenomics-updates/contracts/contract/RocketVault.sol:L26-L27

event TokenDeposited(bytes32 indexed by, address indexed tokenAddress, uint256 amount, uint256 time);
event TokenWithdrawn(bytes32 indexed by, address indexed tokenAddress, uint256 amount, uint256 time);

RocketStorageInterface can be declared in the argument list.

rocketpool-2.5-Tokenomics-updates/contracts/contract/RocketBase.sol:L76-L80

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

RocketMinipool cast to address while it is used as RocketMinipool contract by the caller creating the contract.

rocketpool-2.5-Tokenomics-updates/contracts/contract/minipool/RocketMinipoolFactory.sol:L20-L25

function createMinipool(address _nodeAddress, MinipoolDeposit _depositType) override external onlyLatestContract("rocketMinipoolFactory", address(this)) onlyLatestContract("rocketMinipoolManager", msg.sender) returns (address) {
    // Create RocketMinipool contract
    address contractAddress = address(new RocketMinipool(address(rocketStorage), _nodeAddress, _depositType));
    // Return
    return contractAddress;
}

rocketpool-2.5-Tokenomics-updates/contracts/contract/node/RocketNodeDeposit.sol:L65-L66

address minipoolAddress = rocketMinipoolManager.createMinipool(msg.sender, depositType);
RocketMinipoolInterface minipool = RocketMinipoolInterface(minipoolAddress);

Pass the minipool contract type instead of address to the subcall:

rocketpool-2.5-Tokenomics-updates/contracts/contract/minipool/RocketMinipoolStatus.sol:L64-L64

setMinipoolWithdrawable(_minipoolAddress, _stakingStartBalance, _stakingEndBalance);

Description

withdrawToken breaks checks-effects-interactions by calling token.transfer before updating the internal accounting.

This may only be problematic when calling out callback tokens (e.g., ERC-777) tokens or when withdrawing from an unsafe _tokenAddress (which should never happen).

It is recommended to update the internal account first (analog to withdrawEth) and then call out to the potentially untrusted token or token callback receiver.

Examples

rocketpool-2.5-Tokenomics-updates/contracts/contract/RocketVault.sol:L101-L116

function withdrawToken(address _withdrawalAddress, address _tokenAddress, uint256 _amount) override external onlyLatestNetworkContract returns (bool) {
    // Get contract key
    bytes32 contractKey = keccak256(abi.encodePacked(getContractName(msg.sender), _tokenAddress));
    // Get the token ERC20 instance
    IERC20 tokenContract = IERC20(_tokenAddress);
    // Verify this contract has that amount of tokens at a minimum
    require(tokenContract.balanceOf(address(this)) >= _amount, "Insufficient contract token balance");
    // Withdraw to the desired address
    require(tokenContract.transfer(_withdrawalAddress, _amount), "Rocket Vault token withdrawal unsuccessful");
    // Update balances
    tokenBalances[contractKey] = tokenBalances[contractKey].sub(_amount);
    // Emit token withdrawn event
    emit TokenWithdrawn(contractKey, _tokenAddress, _amount, block.timestamp);
    // Done
    return true;
}

RocketNodeStaking:

rocketpool-2.5-Tokenomics-updates/contracts/contract/node/RocketNodeStaking.sol:L172-L175

rocketVault.withdrawToken(msg.sender, getContractAddress("rocketTokenRPL"), _amount);
// Update RPL stake amounts
decreaseTotalRPLStake(_amount);
decreaseNodeRPLStake(msg.sender, _amount);

Description

SafeERC20 provides a wrapper around ERC20 standard function calls that handles common deviations from ERC20, like missing return values. For example, the vault implementation relies on the transfer* functions to return true or fail. Well-known but broken ERC20 tokens (USDT, BNB, OMG, …) might therefore not be safely used with the system.

Anyone can deposit any token right now. This shouldn’t be a problem as long as the system isn’t using them. If the vault is not meant to be used with unauthorized tokens, it should be considered to implement a token whitelist.

It should be noted that forcefully sent ETH or tokens cannot be reclaimed via the contract.

Examples

rocketpool-2.5-Tokenomics-updates/contracts/contract/RocketVault.sol:L90-L91

require(tokenContract.transferFrom(msg.sender, address(this), _amount), "Token transfer was not successful");
// Update contract balance

rocketpool-2.5-Tokenomics-updates/contracts/contract/RocketVault.sol:L108-L110

// Withdraw to the desired address
require(tokenContract.transfer(_withdrawalAddress, _amount), "Rocket Vault token withdrawal unsuccessful");
// Update balances

Description

This is a batch issue tracking some opportunities to reduce the gas footprint of the contract system. In general, the gas consumption is very high especially because almost all setting read/writes require an external call. For example, this can be problematic if the ethereum network is congested and gas prices rise unpredictably. Registered and Trusted nodes may not be able or willing to perform their duties. Nodes may race to not be the one that executes actions (proposals, setting oracle values) leading to stale prices/balances or minipools that cannot be progressed. Important contract upgrades may not go through because gas consumption in the DAO and upgrading mechanisms may be too high and this is only to express our concerns about the gas footprint.

The following section lists some opportunities for improvement. However, redundant code and excessive gas consumption was seen throughout the codebase.

Examples

RocketVault

• unnecessary balance/allowance checks before transfer. The transfer cannot succeed if balance or allowance is insufficient.

rocketpool-2.5-Tokenomics-updates/contracts/contract/RocketVault.sol:L83-L91

// Check they can cover the amount
require(tokenContract.balanceOf(msg.sender) >= _amount, "Not enough tokens to cover transfer");
// Check they have allowed this contract to send their tokens
require(tokenContract.allowance(msg.sender, address(this)) >= _amount, "Not enough allowance given for transfer of tokens");
// Get contract key
bytes32 contractKey = keccak256(abi.encodePacked(_networkContractName, _tokenAddress));
// Send the tokens to this contract now
require(tokenContract.transferFrom(msg.sender, address(this), _amount), "Token transfer was not successful");
// Update contract balance

rocketpool-2.5-Tokenomics-updates/contracts/contract/RocketVault.sol:L107-L110

require(tokenContract.balanceOf(address(this)) >= _amount, "Insufficient contract token balance");
// Withdraw to the desired address
require(tokenContract.transfer(_withdrawalAddress, _amount), "Rocket Vault token withdrawal unsuccessful");
// Update balances

RocketNodeManager

• duplicate “exists” check in manager and indirectly when adding to addressSetStorage

rocketpool-2.5-Tokenomics-updates/contracts/contract/node/RocketNodeManager.sol:L63-L63

require(!getBool(keccak256(abi.encodePacked("node.exists", msg.sender))), "The node is already registered in the Rocket Pool network");

rocketpool-2.5-Tokenomics-updates/contracts/contract/util/AddressSetStorage.sol:L39-L40

function addItem(bytes32 _key, address _value) override external onlyLatestContract("addressSetStorage", address(this)) onlyLatestNetworkContract {
    require(getUint(keccak256(abi.encodePacked(_key, ".index", _value))) == 0, "Item already exists in set");

RocketDAOProposal

• vote only needs to check for ProposalState.Active as this excludes ProposalState.Succeeded

rocketpool-2.5-Tokenomics-updates/contracts/contract/dao/RocketDAOProposal.sol:L203-L205

require(getState(_proposalID) != ProposalState.Succeeded, "Proposal has passed, voting is complete and the proposal can now be executed");
// Check the proposal is in a state that can be voted on
require(getState(_proposalID) == ProposalState.Active, "Voting is not active for this proposal");

RocketDAONodeTrustedUpgrade

• unnecessary check for _contractAddress != 0 because oldContractAddress !=0 && _contractAddress != oldContractAddressimplies_contractAddress !=0`

rocketpool-2.5-Tokenomics-updates/contracts/contract/dao/node/RocketDAONodeTrustedUpgrade.sol:L51-L54

require(oldContractAddress != address(0x0), "Contract does not exist");
// Check new contract address
require(_contractAddress != address(0x0), "Invalid contract address");
require(_contractAddress != oldContractAddress, "The contract address cannot be set to its current address");

• consider checking for _name.length instead of comparing with the hash of an empty string

rocketpool-2.5-Tokenomics-updates/contracts/contract/dao/node/RocketDAONodeTrustedUpgrade.sol:L69-L71

// Check contract name
bytes32 nameHash = keccak256(abi.encodePacked(_name));
require(nameHash != keccak256(abi.encodePacked("")), "Invalid contract name");

• type should be an enum instead of string

rocketpool-2.5-Tokenomics-updates/contracts/contract/dao/node/RocketDAONodeTrustedUpgrade.sol:L27-L36

function upgrade(string memory _type, string memory _name, string memory _contractAbi, address _contractAddress) override public onlyLatestContract("rocketDAONodeTrustedProposals", msg.sender) {
    // What action are we performing?
    bytes32 typeHash = keccak256(abi.encodePacked(_type));
    // Lets do it!
    if(typeHash == keccak256(abi.encodePacked("upgradeContract"))) _upgradeContract(_name, _contractAddress, _contractAbi);
    if(typeHash == keccak256(abi.encodePacked("addContract"))) _addContract(_name, _contractAddress, _contractAbi);
    if(typeHash == keccak256(abi.encodePacked("upgradeABI"))) _upgradeABI(_name, _contractAbi);
    if(typeHash == keccak256(abi.encodePacked("addABI"))) _addABI(_name, _contractAbi);
}

• use local var challengeBlock instead of wasting gas on a contract call to an external contract

rocketpool-2.5-Tokenomics-updates/contracts/contract/dao/node/RocketDAONodeTrustedActions.sol:L239-L239

uint256 challengeBlock = getUint(keccak256(abi.encodePacked(daoNameSpace, "member.challenged.block", _nodeAddress)));

rocketpool-2.5-Tokenomics-updates/contracts/contract/dao/node/RocketDAONodeTrustedActions.sol:L251-L251

if(getUint(keccak256(abi.encodePacked(daoNameSpace, "member.challenged.block", _nodeAddress))).add(rocketDAONodeTrustedSettingsMembers.getChallengeWindow()) < block.number) {

• order of checks is wasting gas. Checking depositType first may bypass the branch earlier and avoid reaching out to getMemberIsValid extcall in most cases.

rocketpool-2.5-Tokenomics-updates/contracts/contract/node/RocketNodeDeposit.sol:L54-L61

if (rocketDaoNodeTrusted.getMemberIsValid(msg.sender)) {
    // If creating an unbonded minipool, check current unbonded minipool count
    if (depositType == MinipoolDeposit.Empty) {
        require(rocketDaoNodeTrusted.getMemberUnbondedValidatorCount(msg.sender) < rocketDaoNodeTrustedSettingsMembers.getMinipoolUnbondedMax(), "Trusted node member would exceed the amount of unbonded minipools allowed");
    }
}
// Node is not trusted - it cannot create unbonded minipools
else { require(depositType != MinipoolDeposit.Empty, "Only members of the trusted node DAO may create unbonded minipools"); }

• duplicate checks minipoolDeposit.None is checked in deposit and RocketMinipool.constructor. also, putting the Invalid node deposit amount check into the else branch would save some gas.

rocketpool-2.5-Tokenomics-updates/contracts/contract/node/RocketNodeDeposit.sol:L47-L52

MinipoolDeposit depositType = MinipoolDeposit.None;
if (msg.value == rocketDAOProtocolSettingsMinipool.getFullDepositNodeAmount()) { depositType = MinipoolDeposit.Full; }
else if (msg.value == rocketDAOProtocolSettingsMinipool.getHalfDepositNodeAmount()) { depositType = MinipoolDeposit.Half; }
else if (msg.value == rocketDAOProtocolSettingsMinipool.getEmptyDepositNodeAmount()) { depositType = MinipoolDeposit.Empty; }
// Check deposit type is valid
require(depositType != MinipoolDeposit.None, "Invalid node deposit amount");

rocketpool-2.5-Tokenomics-updates/contracts/contract/minipool/RocketMinipool.sol:L75-L75

require(_depositType != MinipoolDeposit.None, "Invalid deposit type");

• RocketTokenRPL - duplicate external calls; unnecessary balance/allowance checks because transferFrom should bail by definition if the transfer fails. amount >0 && balance >0 checks could otherwise be combined as well.

rocketpool-2.5-Tokenomics-updates/contracts/contract/token/RocketTokenRPL.sol:L189-L199

function swapTokens(uint256 _amount) override external {
    // Valid amount?
    require(_amount > 0, "Please enter valid amount of RPL to swap");
    // Check they have a valid amount to swap from
    require(rplFixedSupplyContract.balanceOf(address(msg.sender)) > 0, "No existing RPL fixed supply tokens available to swap");
    // Check they can cover the amount
    require(rplFixedSupplyContract.balanceOf(address(msg.sender)) >= _amount, "Not enough RPL fixed supply tokens available to cover swap amount desired");
    // Check they have allowed this contract to send their tokens
    uint256 allowance = rplFixedSupplyContract.allowance(msg.sender, address(this));
    // Enough to cover it?
    require(allowance >= _amount, "Not enough allowance given for transfer of tokens");

• RocketTokenRPL - unnecessary initialization before assigning a new value

rocketpool-2.5-Tokenomics-updates/contracts/contract/token/RocketTokenRPL.sol:L172-L174

uint256 vaultAllowance = 0;
// Get the current allowance for Rocket Vault
vaultAllowance = rplFixedSupplyContract.allowance(rocketVaultAddress, address(this));

• RocketTokenRPL - secret of method invocation wastes gas: inflationRate could be calculated after intervalsSinceLastMint was checked.

rocketpool-2.5-Tokenomics-updates/contracts/contract/token/RocketTokenRPL.sol:L126-L134

function inflationCalculate() override public view returns (uint256) {
    // The inflation amount
    uint256 inflationTokenAmount = 0;
    // Optimisation
    uint256 inflationRate = getInflationIntervalRate();
    // Compute the number of inflation intervals elapsed since the last time we minted infation tokens
    uint256 intervalsSinceLastMint = getInlfationIntervalsPassed();
    // Only update  if last interval has passed and inflation rate is > 0
    if(intervalsSinceLastMint > 0 && inflationRate > 0) {

Description

For example, in RocketDAONodeTrusted the statevars calcBase, daoNameSpace, and daoMemberMinCount cannot be updated and should therefore be declared constant.

Besides making it obvious that the parameters are immutable, this will save some gas as access to const literals are resolved at compile time and do not require SLOAD’s.

Examples

rocketpool-2.5-Tokenomics-updates/contracts/contract/dao/node/RocketDAONodeTrusted.sol:L25-L32

uint256 private calcBase = 1 ether;

// The namespace for any data stored in the trusted node DAO (do not change)
string private daoNameSpace = 'dao.trustednodes';

// Min amount of trusted node members required in the DAO
uint256 private daoMemberMinCount = 3;

e.g. should be uint256 private constant calcBase = 1 ether.

rocketpool-2.5-Tokenomics-updates/contracts/contract/util/AddressQueueStorage.sol:L20-L20

uint256 public capacity = 2 ** 255; // max uint256 / 2

totalInitialSupply never changes.

rocketpool-2.5-Tokenomics-updates/contracts/contract/token/RocketTokenRPL.sol:L24-L24

uint256 public totalInitialSupply = 18000000000000000000000000;

Recommendation

Instead of casting to IERC20(self) to force an external call to a contract function from its own address (msg.sender) this could be changed to call this.transfer() as the this keyword forces an external call. Ideally, with a short explanation of why the call needs to come from the contract address (this is already the case with the current code revision)

rocketpool-2.5-Tokenomics-updates/contracts/contract/token/RocketTokenRPL.sol:L202-L205

// Initialise itself and send from it's own balance (cant just do a transfer as it's a user calling this so they are msg.sender)
IERC20 rplInflationContract = IERC20(address(this));
// Transfer from the contracts RPL balance to the user
require(rplInflationContract.transfer(msg.sender, _amount), "Token transfer from RPL inflation contract was not successful");

rocketpool-2.5-Tokenomics-updates/contracts/contract/token/RocketTokenRPL.sol:L169-L176

// Initialise itself and allow from it's own balance (cant just do an allow as it could be any user calling this so they are msg.sender)
IERC20 rplInflationContract = IERC20(address(this));
// This is to prevent an allowance reentry style attack
uint256 vaultAllowance = 0;
// Get the current allowance for Rocket Vault
vaultAllowance = rplFixedSupplyContract.allowance(rocketVaultAddress, address(this));
// Now allow Rocket Vault to move those tokens, we also need to account of any other allowances for this token from other contracts in the same block
require(rplInflationContract.approve(rocketVaultAddress, vaultAllowance.add(newTokens)), "Allowance for Rocket Vault could not be approved");

Recommendation

allowance shadows inherited name IERC20.allowance(). Consider renaming the local var allowance to not overlap with any inherited name.

rocketpool-2.5-Tokenomics-updates/contracts/contract/token/RocketTokenRPL.sol:L197-L199

uint256 allowance = rplFixedSupplyContract.allowance(msg.sender, address(this));
// Enough to cover it?
require(allowance >= _amount, "Not enough allowance given for transfer of tokens");

Description

For consistency, and user- and machine-readability, it is recommended to use the NatSpec format: https://docs.soliditylang.org/en/v0.7.6/natspec-format.html

Description

Low-level calls that are unnecessary for the system should be avoided whenever possible because low-level calls behave differently from a contract-type call. For example, address.call(abi.encodeWithSelector("fancy(bytes32)", mybytes) does not verify that a target is actually a contract, while ContractInterface(address).fancy(mybytes) does. Additionally, when calling out to functions declared view/pure, the solidity compiler would actually perform a staticcall providing additional security guarantees while a low-level call does not. Similarly, return values have to be decoded manually when performing low-level calls.

Note: if a low-level call needs to be performed, consider relying on Contract.function.selector instead of encoding using a hardcoded ABI string.

Examples

• cast the returned address to the correct interface class and perform the call

rocketpool-2.5-Tokenomics-updates/contracts/contract/dao/node/RocketDAONodeTrusted.sol:L161-L193

// Bootstrap mode - If there are less than the required min amount of node members, the owner can add some to bootstrap the DAO
function bootstrapMember(string memory _id, string memory _email, address _nodeAddress) override public onlyGuardian onlyBootstrapMode onlyRegisteredNode(_nodeAddress) onlyLatestContract("rocketDAONodeTrusted", address(this)) {
    // Ok good to go, lets add them
    (bool success, bytes memory response) = getContractAddress('rocketDAONodeTrustedProposals').call(abi.encodeWithSignature("proposalInvite(string,string,address)", _id, _email, _nodeAddress));
    // Was there an error?
    require(success, getRevertMsg(response));
}


// Bootstrap mode - Uint Setting
function bootstrapSettingUint(string memory _settingContractName, string memory _settingPath, uint256 _value) override public onlyGuardian onlyBootstrapMode onlyLatestContract("rocketDAONodeTrusted", address(this)) {
    // Ok good to go, lets update the settings
    (bool success, bytes memory response) = getContractAddress('rocketDAONodeTrustedProposals').call(abi.encodeWithSignature("proposalSettingUint(string,string,uint256)", _settingContractName, _settingPath, _value));
    // Was there an error?
    require(success, getRevertMsg(response));
}

// Bootstrap mode - Bool Setting
function bootstrapSettingBool(string memory _settingContractName, string memory _settingPath, bool _value) override public onlyGuardian onlyBootstrapMode onlyLatestContract("rocketDAONodeTrusted", address(this)) {
    // Ok good to go, lets update the settings
    (bool success, bytes memory response) = getContractAddress('rocketDAONodeTrustedProposals').call(abi.encodeWithSignature("proposalSettingBool(string,string,bool)", _settingContractName, _settingPath, _value));
    // Was there an error?
    require(success, getRevertMsg(response));
}


// Bootstrap mode - Upgrade contracts or their ABI
function bootstrapUpgrade(string memory _type, string memory _name, string memory _contractAbi, address _contractAddress) override public onlyGuardian onlyBootstrapMode onlyLatestContract("rocketDAONodeTrusted", address(this)) {
    // Ok good to go, lets update the settings
    (bool success, bytes memory response) = getContractAddress('rocketDAONodeTrustedProposals').call(abi.encodeWithSignature("proposalUpgrade(string,string,string,address)", _type, _name, _contractAbi, _contractAddress));
    // Was there an error?
    require(success, getRevertMsg(response));
}

rocketpool-2.5-Tokenomics-updates/contracts/contract/dao/protocol/RocketDAOProtocol.sol:L39-L77

// Bootstrap mode - Uint Setting
function bootstrapSettingUint(string memory _settingContractName, string memory _settingPath, uint256 _value) override public onlyGuardian onlyBootstrapMode onlyLatestContract("rocketDAOProtocol", address(this)) {
    // Ok good to go, lets update the settings
    (bool success, bytes memory response) = getContractAddress('rocketDAOProtocolProposals').call(abi.encodeWithSignature("proposalSettingUint(string,string,uint256)", _settingContractName, _settingPath, _value));
    // Was there an error?
    require(success, getRevertMsg(response));
}

// Bootstrap mode - Bool Setting
function bootstrapSettingBool(string memory _settingContractName, string memory _settingPath, bool _value) override public onlyGuardian onlyBootstrapMode onlyLatestContract("rocketDAOProtocol", address(this)) {
    // Ok good to go, lets update the settings
    (bool success, bytes memory response) = getContractAddress('rocketDAOProtocolProposals').call(abi.encodeWithSignature("proposalSettingBool(string,string,bool)", _settingContractName, _settingPath, _value));
    // Was there an error?
    require(success, getRevertMsg(response));
}

// Bootstrap mode - Address Setting
function bootstrapSettingAddress(string memory _settingContractName, string memory _settingPath, address _value) override public onlyGuardian onlyBootstrapMode onlyLatestContract("rocketDAOProtocol", address(this)) {
    // Ok good to go, lets update the settings
    (bool success, bytes memory response) = getContractAddress('rocketDAOProtocolProposals').call(abi.encodeWithSignature("proposalSettingAddress(string,string,address)", _settingContractName, _settingPath, _value));
    // Was there an error?
    require(success, getRevertMsg(response));
}

// Bootstrap mode - Set a claiming contract to receive a % of RPL inflation rewards
function bootstrapSettingClaimer(string memory _contractName, uint256 _perc) override public onlyGuardian onlyBootstrapMode onlyLatestContract("rocketDAOProtocol", address(this)) {
    // Ok good to go, lets update the rewards claiming contract amount
    (bool success, bytes memory response) = getContractAddress('rocketDAOProtocolProposals').call(abi.encodeWithSignature("proposalSettingRewardsClaimer(string,uint256)", _contractName, _perc));
    // Was there an error?
    require(success, getRevertMsg(response));
}

// Bootstrap mode -Spend DAO treasury
function bootstrapSpendTreasury(string memory _invoiceID, address _recipientAddress, uint256 _amount) override public onlyGuardian onlyBootstrapMode onlyLatestContract("rocketDAOProtocol", address(this)) {
    // Ok good to go, lets update the rewards claiming contract amount
    (bool success, bytes memory response) = getContractAddress('rocketDAOProtocolProposals').call(abi.encodeWithSignature("proposalSpendTreasury(string,address,uint256)", _invoiceID, _recipientAddress, _amount));
    // Was there an error?
    require(success, getRevertMsg(response));
}

Recommendation

When calling out to a contract known in the system, always prefer typed contract calls (interfaces/contract) instead of low-level calls. This is to avoid errors, potentially unchecked return values, have security guarantees provided by the compiler.

Description

The system heavily relies on a centralized hash indexed storage. The storage keys are formed with distinct namespace prefixes potentially concatenated with user tainted input. Here’s one example of this:

rocketpool-2.5-Tokenomics-updates/contracts/contract/auction/RocketAuctionManager.sol:L187-L192

setBool(keccak256(abi.encodePacked("auction.lot.exists", lotIndex)), true);
setUint(keccak256(abi.encodePacked("auction.lot.block.start", lotIndex)), block.number);
setUint(keccak256(abi.encodePacked("auction.lot.block.end", lotIndex)), block.number.add(rocketAuctionSettings.getLotDuration()));
setUint(keccak256(abi.encodePacked("auction.lot.price.start", lotIndex)), rplPrice.mul(rocketAuctionSettings.getStartingPriceRatio()).div(calcBase));
setUint(keccak256(abi.encodePacked("auction.lot.price.reserve", lotIndex)), rplPrice.mul(rocketAuctionSettings.getReservePriceRatio()).div(calcBase));
setUint(keccak256(abi.encodePacked("auction.lot.rpl.total", lotIndex)), lotRplAmount);

abi.encodePacked encodes dynamic types in-place without a length prefix and static types will not be padded if they are shorter than 32 bytes. If namespace prefixes are not chosen with care a user might provide a value that overlaps into another settings namespace (after the prefix). Special care should be taken if dynamic or short types are used with encodePacked as this might make it easier to force such situations.

Throughout the review, the assessment team has not found any signs of this issue. However, it should be noted that developers must be made aware of this potential problem and it is highly recommended to support the secure development process by tooling that checks for the potential of overlaps in the CI pipeline.

Description

Endusers rely on the information displayed by the application and they are basing their action upon it. A successful attempt in manipulating what is displayed to the user by an attacker can be security-critical. It is therefore recommended to use a safe logging framework for log messages and potentially user tainted information that is displayed in the console or used with another presentation layer (web/xss, db/injection, ..).

For example, the golang log package nor the fmt.print* family encode CRLF (and other control chars) before printing them on the console as they are meant to be used with trusted inputs. User tainted information must, therefore, be properly encoded before being passed to these routines.

Description

A certain amount of interaction is expected from a registered or trusted node operator. For example, the trusted node operator is supposed to provide oracle services, vote on proposals can be challenged by other nodes to prove liveness, or else they’ll be evicted from the DAO.

For example, if someone manages to force a golang exception in the trusted node application causing it to terminate, this node may not be able to defeat challenges or participate in the oracle services it is supposed to provide.

Similar to the ETH2 validator application or an ETH1 blockchain node liveness is important and failure to prove liveness may be severely punished or put the network at risk. The worst-case might probably be someone adding information to a smart contract that causes a golang panic in the client implementation. This might put the network in a stale state where oracle provided information becomes stale/outdated providing opportunities to malicious actors to profit from this.

It is, therefore, highly recommend to segment the application into recoverable and non-recoverable zones. If a recoverable exception is detected (e.g. parseInt trying to parse user tainted data and failing to do so, but the application would be able to continue by just skipping this entry) the application should print the stack-trace, log this event, make the user aware but continue providing its services to support the security of the network.

For example, in golang this can be achieved by catching panic conditions using the Defer, Panic, Recover pattern.

Description

The initial deployer of the RocketStorage contract is set as the Guardian/Bootstrapping role. This guardian can bootstrap the TrustedNode and Protocol DAO, add members, upgrade components, change settings.

Right after deploying the DAO contract the member count is zero. The Guardian can now begin calling any of the bootstrapping functions to add members, change settings, upgrade components, interact with the treasury, etc. The bootstrapping configuration by the Guardian is unlikely to all happen within one transaction which might allow other parties to interact with the system while it is being set up.

RocketDaoNodeTrusted also implements a recovery mode that allows any registered node to invite themselves directly into the DAO without requiring approval from the Guardian or potential other DAO members as long as the total member count is below daoMemberMinCount (3). The Guardian itself is not counted as a DAO member as it is a supervisory role.

rocketpool-2.5-Tokenomics-updates/contracts/contract/dao/node/RocketDAONodeTrusted.sol:L202-L215

/**** Recovery ***************/
   
// In an explicable black swan scenario where the DAO loses more than the min membership required (3), this method can be used by a regular node operator to join the DAO
// Must have their ID, email, current RPL bond amount available and must be called by their current registered node account
function memberJoinRequired(string memory _id, string memory _email) override public onlyLowMemberMode onlyRegisteredNode(msg.sender) onlyLatestContract("rocketDAONodeTrusted", address(this)) {
    // Ok good to go, lets add them
    (bool successPropose, bytes memory responsePropose) = getContractAddress('rocketDAONodeTrustedProposals').call(abi.encodeWithSignature("proposalInvite(string,string,address)", _id, _email, msg.sender));
    // Was there an error?
    require(successPropose, getRevertMsg(responsePropose));
    // Get the to automatically join as a member (by a regular proposal, they would have to manually accept, but this is no ordinary situation)
    (bool successJoin, bytes memory responseJoin) = getContractAddress("rocketDAONodeTrustedActions").call(abi.encodeWithSignature("actionJoinRequired(address)", msg.sender));
    // Was there an error?
    require(successJoin, getRevertMsg(responseJoin));
}

This opens up a window during the bootstrapping phase where any Ethereum Address might be able to register as a node (RocketNodeManager.registerNode) if node registration is enabled (default=true) rushing into RocketDAONodeTrusted.memberJoinRequired adding themselves (up to 3 nodes) as trusted nodes to the DAO. The new DAO members can now take over the DAO by issuing proposals, waiting 2 blocks to vote/execute them (upgrade, change settings while Guardian is changing settings, etc.). The Guardian role can kick the new DAO members, however, they can invite themselves back into the DAO.

rocketpool-2.5-Tokenomics-updates/contracts/contract/dao/protocol/settings/RocketDAOProtocolSettingsNode.sol:L19-L19

setSettingBool("node.registration.enabled", true);     

Recommendation

Disable the DAO recovery mode during bootstrapping. Disable node registration by default and require the guardian to enable it. Ensure that bootstrapDisable (in both DAO contracts) performs sanity checks as to whether the DAO bootstrapping finished and permissions can effectively be revoked without putting the DAO at risk or in an irrecoverable state (enough members bootstrapped, vital configurations like registration and other settings are configured, …).

Description

The rETH token price is not coupled to the amount of rETH tokens in circulation on the Ethereum chain. The price is reported by oracle nodes and committed to the system via a voting process. The price of rETH changes If 51% of nodes observe and submit the same price information. If nodes fail to find price consensus for a block, then the rETH price might be stale.

There is an opportunity for the user to front-run the price update right before it is committed. If the next price is higher than the previous (typical case), this gives an instant opportunity to perform a risk-free ETH -> rETH -> ETH exchange for profit. In the worst case, one could drain all the ETH held by the RocketTokenRETH contract + excess funds stored in the vault.

Note: there seems to be a "network.submit.balances.frequency" price and balance submission frequency of 24hrs. However, this frequency is not enforced, and it is questionable if it makes sense to pin the price for 24hrs.

Note: the total supply of the RocketTokenRETH contract may be completely disconnected from the reported total supply for RETH via oracle nodes.

Examples

A user observes a price update for rETH submitted to RocketNetworkPrices, resulting in an increased price for rETH. The user front-runs the effective price update (51% consensus reached on submission) by rETH at the current, discounted rate. Ideally, the user checks that none of the funds will be assigned to minipools in the queue (empty queue, disabled assignment, ..) and that the expected amount of ETH returned is available RocketTokenRETH and RocketDeposit (excess funds) contracts. The user then waits for the price update and back-runs it with a call burning all the rETH obtained at a discount for ETH, realizing an immediate profit.

The amount of ETH was only staked during this one process for the price update duration and unlikely to be useful to the system. This way, a whale (only limited by the max deposit amount set on deposit) can drain the RocketTokenRETH contract from all its ETH and excess eth funds.

mempool observed: submitPrice tx (an effective transaction that changes the price) wrapped with buying rETH and selling rETH for ETH:

• RocketDepositPool.deposit() at old price => mints rETH at current rate
• RocketNetworkPrices.submitPrices(newRate)
• RocketTokenRETH.burn(balanceOf(msg.sender) => burns rETH for ETH at new rate

• deposit (virtually no limit with 1000ETH being the limit right now)

rocketpool-2.5-Tokenomics-updates/contracts/contract/deposit/RocketDepositPool.sol:L63-L67

require(rocketDAOProtocolSettingsDeposit.getDepositEnabled(), "Deposits into Rocket Pool are currently disabled");
require(msg.value >= rocketDAOProtocolSettingsDeposit.getMinimumDeposit(), "The deposited amount is less than the minimum deposit size");
require(getBalance().add(msg.value) <= rocketDAOProtocolSettingsDeposit.getMaximumDepositPoolSize(), "The deposit pool size after depositing exceeds the maximum size");
// Mint rETH to user account
rocketTokenRETH.mint(msg.value, msg.sender);

• trustedNodes submitPrice (changes params for getEthValue and getRethValue)

rocketpool-2.5-Tokenomics-updates/contracts/contract/network/RocketNetworkPrices.sol:L69-L72

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

• immediately burn at new rate (as params for getEthValue changed)

rocketpool-2.5-Tokenomics-updates/contracts/contract/token/RocketTokenRETH.sol:L107-L124

function burn(uint256 _rethAmount) override external {
    // Check rETH amount
    require(_rethAmount > 0, "Invalid token burn amount");
    require(balanceOf(msg.sender) >= _rethAmount, "Insufficient rETH balance");
    // Get ETH amount
    uint256 ethAmount = getEthValue(_rethAmount);
    // Get & check ETH balance
    uint256 ethBalance = getTotalCollateral();
    require(ethBalance >= ethAmount, "Insufficient ETH balance for exchange");
    // Update balance & supply
    _burn(msg.sender, _rethAmount);
    // Withdraw ETH from deposit pool if required
    withdrawDepositCollateral(ethAmount);
    // Transfer ETH to sender
    msg.sender.transfer(ethAmount);
    // Emit tokens burned event
    emit TokensBurned(msg.sender, _rethAmount, ethAmount, block.timestamp);
}

Description

Any registered (even untrusted) node can challenge a trusted DAO node to respond. The challenge is initiated by calling actionChallengeMake. Trusted nodes can challenge for free, other nodes have to provide members.challenge.cost as a tribute to the Ethereum gods. The challenged node must call actionChallengeDecide before challengeStartBlock + members.challenge.window blocks are over (default approx 7 days). However, the Golang codebase does not actively monitor for the ActionChallengeMade event, nor does the node - regularly - check if it is being challenged. Means to respond to the challenge (calling actionChallengeDecide to stop the challenge) are not implemented.

• Nodes do not seem to monitor ActionChallengeMade events so that they could react to challenges
• Nodes do not implement actionChallengeDecide and, therefore, cannot successfully stop a challenge
• Funds/Tribute sent along with the challenge will be locked forever in the RocketDAONodeTrustedActions contract. There’s no means to recover the funds.
• It is questionable whether the incentives are aligned well enough for anyone to challenge stale nodes. The default of 1 eth compared to the risk of the “malicious” or “stale” node exiting themselves is quite high. The challenger is not incentivized to challenge someone other than for taking over the DAO. If the tribute is too low, this might incentivize users to grief trusted nodes and force them to close a challenge.
• Requiring that the challenge initiator is a different registered node than the challenge finalized is a weak protection since the system is open to anyone to register as a node (even without depositing any funds.)
• block time is subject to fluctuations. With the default of 43204 blocks, the challenge might expire at 5 days (10 seconds block time), 6.5 days (13 seconds Ethereum target median block time), 7 days (14 seconds), or more with historic block times going up to 20 seconds for shorter periods.

A minority of trusted nodes may use this functionality to boot other trusted node members off the DAO issuing challenges once a day until the DAO member number is low enough to allow them to reach quorum for their own proposals or until the member threshold allows them to add new nodes without having to go through the proposal process at all.

Examples

rocketpool-2.5-Tokenomics-updates/contracts/contract/dao/node/settings/RocketDAONodeTrustedSettingsMembers.sol:L22-L24

setSettingUint('members.challenge.cooldown', 6172);              // How long a member must wait before performing another challenge, approx. 1 day worth of blocks
setSettingUint('members.challenge.window', 43204);               // How long a member has to respond to a challenge. 7 days worth of blocks
setSettingUint('members.challenge.cost', 1 ether);               // How much it costs a non-member to challenge a members node. It's free for current members to challenge other members.

rocketpool-2.5-Tokenomics-updates/contracts/contract/dao/node/RocketDAONodeTrustedActions.sol:L204-L206

// 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.

Recommendation

Implement the challenge-response process before enabling users to challenge other nodes. Implement means to detect misuse of this feature for griefing e.g. when one trusted node member forces another trusted node to defeat challenges over and over again (technical controls, monitoring).

Description

The onlyDAOProtocolProposal modifier guards all state-changing methods in this contract. However, analog to issue 6.5, the access control is disabled until the variable settingsNameSpace.deployed is set. If this contract is not deployed and configured in one transaction, anyone can update the contract while left unprotected on the blockchain.

See issue 6.5 for a similar issue.

Examples

rocketpool-2.5-Tokenomics-updates/contracts/contract/dao/protocol/settings/RocketDAOProtocolSettings.sol:L18-L23

modifier onlyDAOProtocolProposal() {
    // If this contract has been initialised, only allow access from the proposals contract
    if(getBool(keccak256(abi.encodePacked(settingNameSpace, "deployed")))) require(getContractAddress('rocketDAOProtocolProposals') == msg.sender, "Only DAO Protocol Proposals contract can update a setting");
    _;
}

rocketpool-2.5-Tokenomics-updates/contracts/contract/dao/node/settings/RocketDAONodeTrustedSettings.sol:L18-L22

modifier onlyDAONodeTrustedProposal() {
    // If this contract has been initialised, only allow access from the proposals contract
    if(getBool(keccak256(abi.encodePacked(settingNameSpace, "deployed")))) require(getContractAddress('rocketDAONodeTrustedProposals') == msg.sender, "Only DAO Node Trusted Proposals contract can update a setting");
    _;
}

There are at least 9 more occurrences of this pattern.

Recommendation

Restrict access to the methods to a temporary trusted account (e.g. guardian) until the system bootstrapping phase ends by setting deployed to true.

Description

According to the deployment script, the contract is deployed, and settings are configured in multiple transactions. This also means that for a period of time, the contract is left unprotected on the blockchain. Anyone can delete/set any value in the centralized data store. An attacker might monitor the mempool for new deployments of the RocketStorage contract and front-run calls to contract.storage.initialised setting arbitrary values in the system.

Examples

rocketpool-2.5-Tokenomics-updates/contracts/contract/RocketStorage.sol:L24-L31

modifier onlyLatestRocketNetworkContract() {
    // The owner and other contracts are only allowed to set the storage upon deployment to register the initial contracts/settings, afterwards their direct access is disabled
    if (boolStorage[keccak256(abi.encodePacked("contract.storage.initialised"))] == true) {
        // Make sure the access is permitted to only contracts in our Dapp
        require(boolStorage[keccak256(abi.encodePacked("contract.exists", msg.sender))], "Invalid or outdated network contract");
    }
    _;
}

Recommendation

Restrict access to the methods to a temporary trusted account (e.g. guardian) until the system bootstrapping phase ends by setting initialised to true.

Description

A proposal can be voted and passed when it enters the ACTIVE state. Voting starts when the current block.number is greater than the startBlock configured in the proposal (up until the endBlock). The requirement for the startBlock is to be at least greater than block.number when the proposal is submitted.

rocketpool-2.5-Tokenomics-updates/contracts/contract/dao/RocketDAOProposal.sol:L167-L170

require(_startBlock > block.number, "Proposal start block must be in the future");
require(_durationBlocks > 0, "Proposal cannot have a duration of 0 blocks");
require(_expiresBlocks > 0, "Proposal cannot have a execution expiration of 0 blocks");
require(_votesRequired > 0, "Proposal cannot have a 0 votes required to be successful");

The default vote delay configured in the system is 1 block.

rocketpool-2.5-Tokenomics-updates/contracts/contract/dao/node/settings/RocketDAONodeTrustedSettingsProposals.sol:L21-L21

setSettingUint('proposal.vote.delay.blocks', 1);                 // How long before a proposal can be voted on after it is created. Approx. Next Block

A vote is immediately passed when the required quorum is reached which allows it to be executed. This means that a group that is holding enough voting power can propose a change, wait for two blocks (block.number (of time of proposal creation) + configuredDelay (1) + 1 (for ACTIVE state), then vote and execute for the proposal to pass for it to take effect almost immediately after only 2 blocks (<30seconds).

Settings can be changed after 30 seconds which might be unpredictable for other DAO members and not give them enough time to oppose and leave the DAO.

Recommendation

The underlying issue is that users of the system can’t be sure what the behavior of a function call will be, and this is because the behavior can change after two blocks. The only guarantee is that users can be sure the settings don’t change for the next block if no proposal is active.

We recommend giving the user advance notice of changes with a delay. For example, all upgrades should require two steps with a mandatory time window between them. The first step merely broadcasts to users that a particular change is coming, and the second step commits that change after a suitable waiting period.

Description

Nodes/TrustedNodes earn rewards based on the current share of the effective RPL stake provided backing the number of Minipools they run. The reward is paid out regardless of when the effective node stake was provided, as long as it is present just before the call to claim(). This means the reward does not take into account how long the stake was provided. The effective RPL stake is the nodes RPL stake capped at a maximum of halfDepositUserAmount * 150% * nr_of_minipools(node) / RPLPrice. If the node does not run any Minipools, the effective RPL stake is zero.

Since effective stake can be added just before calling the claim() method (effectively trying to get a reward for a period that passed without RPL being staked for the full duration), this might create an unpredictable outcome for other participants, as adding significant stake (requires creating Minipools and staking the max per pool; the stake is locked for at least the duration of a reward period rpl.rewards.claim.period.blocks) shifts the shares users get for the fixed total amount of rewards. This can be unfair if the first users claimed their reward, and then someone is artificially inflating the total amount of shares by adding more stake to get a bigger part of the remaining reward. However, this comes at the cost of the registered node having to create more Minipools to stake more, requiring an initial deposit (16ETH, or 0ETH under certain circumstances for trusted nodes) by the actor attempting to get a larger share of the rewards. The risk of losing funds for this actor, however, is rather low, as they can immediately dissolve() and close() the Minipool to refund their node deposit as NETH right after claiming the reward only losing the gas spent on the various transactions.

This can be extended to a node operator creating a Minipool and staking the maximum amount before calling claim to remove the Minipool right after, freeing up the ETH that was locked in the Minipool until the next reward period starts. The node operator is not providing any service to the network, loses some value in ETH for gas but may compensate that with the RPL staking rewards. If the node amassed a significant amount of RPL stake, they might even try to flash-loan enough ETH to spawn Minipools to inflate their effective stake and earn most of the rewards to return the loan RPL profit.

-- reward period ends -- front-run other claimers to maximize profits
[create x minipools]
[stake to max effective RPL for amount of minipools; locked for 14 days]
[claim rewards for inflated effective RPL stake]
[dissolve(), close() minipools -> refund NETH]
[burn NETH for ETH]
... wait 14 days
[withdraw stake OR start again creating Minipools, claiming rewards while the Minipools are dissolved right after, freeing the ETH]

By staking just before claiming, the node effectively can earn rewards for 2 reward periods by only staking RPL for the duration of one period (claim the previous period, leave it in for 14 days, claim another period, withdraw).

The stake can be withdrawn at the earliest 14 days after staking. However, it can be added back at any time, and the stake addition takes effect immediately. This allows for optimizing the staking reward as follows (assuming we front-run other claimers to maximize profits and perform all transactions in one block):

[stake max effective amount for the number of minipools]
[claim() to claim the previous period even though we did not provide any stake for the duration]
[optionally dissolve Minipools unlocking ETH]
-- stake is locked for at least 14 days --
-- 14 days forward - new reward period started --
[claim() the period]
[withdraw() (leaving min pool stake OR everything if we dissolve all the Minipool)]
[lend RPL to other platforms and earn interest]
-- 14 days forward -new reward period started --
[get RPL back from another platform]
[stake & create minipools to inflate effective stake]
[claim()]
[optionally dissolve Minipools to unlock node ETH]
-- stake is locked for at least 14 days --
-- 14 days forward - new reward period started --
[claim() the period]
[withdraw() (leaving min pool stake OR everything if we dissolve all the Minipools)]
[lend RPL to other platforms and earn interest]
...

Note that withdraw() can be called right at the time the new reward period starts:

rocketpool-2.5-Tokenomics-updates/contracts/contract/node/RocketNodeStaking.sol:L165-L166

require(block.number.sub(getNodeRPLStakedBlock(msg.sender)) >= rocketDAOProtocolSettingsRewards.getRewardsClaimIntervalBlocks(), "The withdrawal cooldown period has not passed");
// Get & check node's current RPL stake

Examples

• A node may choose to register and stake some RPL to collect rewards but never actually provide registered node duties, e.g., operating a Minipool.

• Node shares for a passed reward epoch are unpredictable as nodes may change their stake (adding) after/before users claim their rewards.

• A node can maximize its rewards by adding stake just before claiming it

• A node can stake to claim rewards, wait 14 days, withdraw, lend on a platform and return the stake in time to claim the next period.

Recommendation

Review the incentive model for the RPL rewards. Consider adjusting it so that nodes that provide a service get a better share of the rewards. Consider accruing rewards for the duration the stake was provided instead of taking a snapshot whenever the node calls claim(). Require stake to be locked for > 14 days instead of >=14 days (withdraw()) or have users skip the first reward period after staking.

Description

Oracle nodes update the Minipools’ balance and progress it to the withdrawable state when they observe the minipools stake to become withdrawable. If the observed stakingEndBalance is less than the user deposit for that pool, the node operator is punished for the difference.

rocketpool-2.5-Tokenomics-updates/contracts/contract/minipool/RocketMinipoolStatus.sol:L89-L94

rocketMinipoolManager.setMinipoolWithdrawalBalances(_minipoolAddress, _stakingEndBalance, nodeAmount);
// Apply node penalties by liquidating RPL stake
if (_stakingEndBalance < userDepositBalance) {
    RocketNodeStakingInterface rocketNodeStaking = RocketNodeStakingInterface(getContractAddress("rocketNodeStaking"));
    rocketNodeStaking.slashRPL(minipool.getNodeAddress(), userDepositBalance - _stakingEndBalance);
}

The amount slashed is at max userDepositBalance - stakingEndBalance. The userDepositBalance is at least 16 ETH (minipool.half/.full) and at max 32 ETH (minipool.empty). The maximum amount to be slashed is therefore 32 ETH (endBalance = 0, minipool.empty).

The slashing amount is denoted in ETH. The RPL price (in ETH) is updated regularly by oracle nodes (see related issue https://github.com/ConsenSys/rocketpool-audit-2021-03/issues/32; note that the RPL token is potentially affected by a similar issue as one can stake RPL, wait for the cooldown period & wait for the price to change, and withdraw stake at higher RPL price/ETH). The ETH amount to be slashed is converted to RPL, and the corresponding RPL stake is slashed.

rocketpool-2.5-Tokenomics-updates/contracts/contract/node/RocketNodeStaking.sol:L188-L196

uint256 rplSlashAmount = calcBase.mul(_ethSlashAmount).div(rocketNetworkPrices.getRPLPrice());
// Cap slashed amount to node's RPL stake
uint256 rplStake = getNodeRPLStake(_nodeAddress);
if (rplSlashAmount > rplStake) { rplSlashAmount = rplStake; }
// Transfer slashed amount to auction contract
rocketVault.transferToken("rocketAuctionManager", getContractAddress("rocketTokenRPL"), rplSlashAmount);
// Update RPL stake amounts
decreaseTotalRPLStake(rplSlashAmount);
decreaseNodeRPLStake(_nodeAddress, rplSlashAmount);

If the node does not have a sufficient RPL stake to cover the losses, the slashing amount is capped at whatever amount of RPL the node has left staked.

The minimum amount of RPL a node needs to have staked if it operates minipools is calculated as follows:

rocketpool-2.5-Tokenomics-updates/contracts/contract/node/RocketNodeStaking.sol:L115-L120

    // Calculate minimum RPL stake
    return rocketDAOProtocolSettingsMinipool.getHalfDepositUserAmount()
        .mul(rocketDAOProtocolSettingsNode.getMinimumPerMinipoolStake())
        .mul(rocketMinipoolManager.getNodeMinipoolCount(_nodeAddress))
        .div(rocketNetworkPrices.getRPLPrice());
}

With the current configuration, this would resolve in a minimum stake of 16 ETH * 0.1 (10% collateralization) * 1 (nr_minipools) * RPL_Price for a node operating 1 minipool. This means a node operator basically only needs to have 10% of 16 ETH staked to operate one minipool.

An operator can withdraw their stake at any time, but they have to wait at least 14 days after the last time they staked (cooldown period). They can, at max, withdraw all but the minimum stake required to run the pools (nr_of_minipools * 16 ETH * 10%). This also means that after the cooldown period, they can reduce their stake to 10% of the half deposit amount (16ETH), then perform a voluntary exit on ETH2 so that the minipool becomes withdrawable. If they end up with less than the userDepositBalance in staking rewards, they would only get slashed the 1.6 ETH at max (10% of 16ETH half deposit amount for 1 minipool) even though they incurred a loss that may be up to 32 ETH (empty Minipool empty amount).

Furthermore, if a node operator runs multiple minipools, let’s say 5, then they would have to provide at least 5*16ETH*0.1 = 8ETH as a security guarantee in the form of staked RPL. If the node operator incurs a loss with one of their minipools, their 8 ETH RPL stake will likely be slashed in full. Their other - still operating - minipools are not backed by any RPL anymore, and they effectively cannot be slashed anymore. This means that a malicious node operator can create multiple minipools, stake the minimum amount of RPL, get slashed for one minipool, and still operate the others without having the minimum RPL needed to run the minipools staked (getNodeMinipoolLimit).

The RPL stake is donated to the RocketAuctionManager, where they can attempt to buy back RPL potentially at a discount.

Note: Staking more RPL (e.g., to add another Minipool) resets the cooldown period for the total RPL staked (not only for the newly added)

Recommendation

It is recommended to redesign the withdrawal process to prevent users from withdrawing their stake while slashable actions can still occur. A potential solution may be to add a locking period in the process. A node operator may schedule the withdrawal of funds, and after a certain time has passed, may withdraw them. This prevents the immediate withdrawal of funds that may need to be reduced while slashable events can still occur. E.g.:

• A node operator requests to withdraw all but the minimum required stake to run their pools.
• The funds are scheduled for withdrawal and locked until a period of X days has passed.
• (optional) In this period, a slashable event occurs. The funds for compensation are taken from the user’s stake including the funds scheduled for withdrawal.
• After the time has passed, the node operator may call a function to trigger the withdrawal and get paid out.

Description

RocketTokenRPL allows users to swap their fixed-rate tokens to the inflationary RocketTokenRPL ERC20 token via a swapToken function. The DAO defines the inflation rate of this token and is initially set to be 5% APY. This APY is configured as a daily inflation rate (APD) with the corresponding 1 day in blocks inflation interval in the rocketDAOProtocolSettingsInflation contract. The DAO members control the inflation settings.

Anyone can call inflationMintTokens to inflate the token, which mints tokens to the contracts RocketVault. Tokens are minted for discreet intervals since the last time inflationMintTokens was called (recorded as inflationCalcBlock). The inflation is then calculated for the passed intervals without taking the current not yet completed interval. However, the inflationCalcBlock is set to the current block.number, effectively skipping some “time”/blocks of the APY calculation.

The more often inflationMintTokens is called, the higher the APY likelihood dropping below the configured 5%. In the worst case, one could manipulate the APY down to 2.45% (assuming that the APD for a 5% APY was configured) by calling inflationMintTokens close to the end of every second interval. This would essentially restart the APY interval at block.number, skipping blocks of the current interval that have not been accounted for.

The following diagram illustrates the skipped blocks due to the incorrect recording of inflationCalcBlock as block.number. The example assumes that we are in interval 4 but have not completed it. 3 APD intervals have passed, and this is what the inflation rate is based on. However, the inflationCalcBlock is updated to the current block.number, skipping some time/blocks that are now unaccounted in the APY restarting the 4th interval at block.number.

• Note: updating the inflation rate will directly affect past inflation intervals that have not been minted! this might be undesirable, and it could be considered to force an inflation mint if the APY changes
• Note: if the interval is small enough and there is a history of unaccounted intervals to be minted, and the Ethereum network is congested, gas fees may be high and block limits hit, the calculations in the for loop might be susceptible to DoS the inflation mechanism because of gas constraints.
• Note: The inflation seems only to be triggered regularly on RocketRewardsPool.claim (or at any point by external actors). If the price establishes based on the total supply of tokens, then this may give attackers an opportunity to front-run other users trading large amounts of RPL that may previously have calculated their prices based on the un-inflated supply.
• Note: that the discrete interval-based inflation (e.g., once a day) might create dynamics that put pressure on users to trade their RPL in windows instead of consecutively

Examples

• the inflation intervals passed is the number of completed intervals. The current interval that is started is not included.

rocketpool-2.5-Tokenomics-updates/contracts/contract/token/RocketTokenRPL.sol:L108-L119

function getInlfationIntervalsPassed() override public view returns(uint256) {
    // The block that inflation was last calculated at
    uint256 inflationLastCalculatedBlock = getInflationCalcBlock();
    // Get the daily inflation in blocks
    uint256 inflationInterval = getInflationIntervalBlocks();
    // Calculate now if inflation has begun
    if(inflationLastCalculatedBlock > 0) {
        return (block.number).sub(inflationLastCalculatedBlock).div(inflationInterval);
    }else{
        return 0;
    }
}

• the inflation calculation calculates the to-be-minted tokens for the inflation rate at newTokens = supply * rateAPD^intervals - supply

rocketpool-2.5-Tokenomics-updates/contracts/contract/token/RocketTokenRPL.sol:L126-L148

function inflationCalculate() override public view returns (uint256) {
    // The inflation amount
    uint256 inflationTokenAmount = 0;
    // Optimisation
    uint256 inflationRate = getInflationIntervalRate();
    // Compute the number of inflation intervals elapsed since the last time we minted infation tokens
    uint256 intervalsSinceLastMint = getInlfationIntervalsPassed();
    // Only update  if last interval has passed and inflation rate is > 0
    if(intervalsSinceLastMint > 0 && inflationRate > 0) {
        // Our inflation rate
        uint256 rate = inflationRate;
        // Compute inflation for total inflation intervals elapsed
        for (uint256 i = 1; i < intervalsSinceLastMint; i++) {
            rate = rate.mul(inflationRate).div(10 ** 18);
        }
        // Get the total supply now
        uint256 totalSupplyCurrent = totalSupply();
        // Return inflation amount
        inflationTokenAmount = totalSupplyCurrent.mul(rate).div(10 ** 18).sub(totalSupplyCurrent);
    }
    // Done
    return inflationTokenAmount;
}

Recommendation

Properly track inflationCalcBlock as the end of the previous interval, as this is up to where the inflation was calculated, instead of the block at which the method was invoked.

Ensure APY/APD and interval configuration match up. Ensure the interval is not too small (potential gas DoS blocking inflation mint and RocketRewardsPool.claim).

Description

The system might end up in a stale state with minipools never being setWithdrawable or network and prices being severely outdated because trusted nodes don’t fulfill their duty of providing oracle values. Minipools not being able to advance to the Withdrawable state will severely harm the system as no rewards can be paid out. Outdated balances and prices may affect token economics around the tokens involved (specifically rETH price depends on oracle observations).

There is an incentive to be an oracle node as you get paid to provide oracle node duties when enrolled with the DAO. However, it is not enforced that nodes actually fulfill their duty of calling the respective onlyTrustedNode oracle functions to submit prices/balances/minipool rewards.

Therefore, a smart Rocket Pool trusted node operator might consider patching their client software to not or only sporadically fulfill their duties to save considerable amounts of gas, making more profit than other trusted nodes would.

There is no means to directly incentivize trusted nodes to call certain functions as they get their rewards anyway. The only risk they run is that other trusted nodes might detect their antisocial behavior and attempt to kick them out of the DAO. To detect this, monitoring tools and processes need to be established; it is questionable whether users would participate in high maintenance DAO operators.

Furthermore, trusted nodes might choose to gas optimize their submissions to avoid calling the actual action once quorum was established. They can, for example, attempt to submit prices as early as possible, avoiding that they’re the first to hit the 51% threshold.

Recommendation

Create monitoring tools and processes to detect participants that do not fulfill their trusted DAO duties. Create direct incentives for trusted nodes to provide oracle services by, e.g., recording their participation rate and only payout rewards based on how active they are.

Description

When adding a new contract, it is checked whether the address is already in use. This check is missing when upgrading a named contract to a new implementation, potentially allowing someone to register one address to multiple names creating an inconsistent configuration.

The crux of this is, that, getContractAddress() will now return a contract address that is not registered anymore (while getContractName may throw). getContractAddress can therefore not relied upon when checking ACL.

• add contract name=test, address=0xfefe –>

   sets contract.exists.0xfefe=true
   sets contract.name.0xfefe=test
   sets contract.address.test=0xfefe
   sets contract.abi.test=abi
  

• add another contract name=badcontract, address=0xbadbad –>

  sets contract.exists.0xbadbad=true
  sets contract.name.0xbadbad=badcontract
  sets contract.address.badcontract=0xbadbad
  sets contract.abi.badcontract=abi
  

• update contract name=test, address=0xbadbad reusing badcontradcts address, the address is now bound to 2 names (test, badcontract)

  overwrites contract.exists.0xbadbad=true` (even though its already true)
  updates contract.name.0xbadbad=test (overwrites the reference to badcontract; badcontracts config is now inconsistent)
  updates contract.address.test=0xbadbad (ok, expected)
  updates contract.abi.test=abi (ok, expected)
  removes contract.name.0xfefe (ok)
  removes contract.exists.0xfefe (ok)
  

• update contract name=test, address=0xc0c0

  sets contract.exists.0xc0c0=true
  sets contract.name.0xc0c0=test (ok, expected)
  updates contract.address.test=0xc0c0 (ok, expected)
  updates contract.abi.test=abi (ok, expected)
  removes contract.name.0xbadbad (the contract is still registered as badcontract, but is indirectly removed now)
  removes contract.exists.0xbadbad (the contract is still registered as badcontract, but is indirectly removed now)
  

After this, badcontract is partially cleared, getContractName(0xbadbad) throws while getContractAddress(badcontract) returns 0xbadbad which is already unregistered (contract.exists.0xbadbad=false)

(removed) contract.exists.0xbadbad
(removed) contract.name.0xbadbad=badcontract
sets contract.address.badcontract=0xbadbad
sets contract.abi.badcontract=abi

Examples

• check in `_addContract``

rocketpool-2.5-Tokenomics-updates/contracts/contract/dao/node/RocketDAONodeTrustedUpgrade.sol:L76-L76

require(_contractAddress != address(0x0), "Invalid contract address");

• no checks in upgrade.

rocketpool-2.5-Tokenomics-updates/contracts/contract/dao/node/RocketDAONodeTrustedUpgrade.sol:L53-L59

require(_contractAddress != address(0x0), "Invalid contract address");
require(_contractAddress != oldContractAddress, "The contract address cannot be set to its current address");
// Register new contract
setBool(keccak256(abi.encodePacked("contract.exists", _contractAddress)), true);
setString(keccak256(abi.encodePacked("contract.name", _contractAddress)), _name);
setAddress(keccak256(abi.encodePacked("contract.address", _name)), _contractAddress);
setString(keccak256(abi.encodePacked("contract.abi", _name)), _contractAbi);

Recommendation

Check that the address being upgraded to is not yet registered and properly clean up contract.address.<name|abi>.

Description

ValidateTimezoneLocation and ValidateDAOMemberEmail are only used to validate user input from the command line. Timezone location information and member email addresses are stored in the smart contract’s string storage, e.g., using the setTimezoneLocation function of the RocketNodeManager contract. This function only validates that a minimum length of 4 has been given.

Through direct interaction with the contract, an attacker can submit arbitrary information, which is not validated on the CLI’s side. With additional integrations of the Rocketpool smart contracts, the timezone location field may be used by an attacker to inject malicious code (e.g., for cross-site scripting attacks) or injecting false information (e.g. Balance: 1000 RPL or Status: Trusted), which is directly displayed on a user-facing application.

On the command line, control characters such as newline characters can be injected to alter how text is presented to the user, effectively exploiting user trust in the official application.

Examples

rocketpool-go-2.5-Tokenomics/node/node.go:L134-L153

    wg.Go(func() error {
        var err error
        timezoneLocation, err = GetNodeTimezoneLocation(rp, nodeAddress, opts)
        return err
    })

    // Wait for data
    if err := wg.Wait(); err != nil {
        return NodeDetails{}, err
    }

    // Return
    return NodeDetails{
        Address: nodeAddress,
        Exists: exists,
        WithdrawalAddress: withdrawalAddress,
        TimezoneLocation: timezoneLocation,
    }, nil

}

smartnode-2.5-Tokenomics/rocketpool-cli/odao/members.go:L34-L44

for _, member := range members.Members {
    fmt.Printf("--------------------\n")
    fmt.Printf("\n")
    fmt.Printf("Member ID:            %s\n", member.ID)
    fmt.Printf("Email address:        %s\n", member.Email)
    fmt.Printf("Joined at block:      %d\n", member.JoinedBlock)
    fmt.Printf("Last proposal block:  %d\n", member.LastProposalBlock)
    fmt.Printf("RPL bond amount:      %.6f\n", math.RoundDown(eth.WeiToEth(member.RPLBondAmount), 6))
    fmt.Printf("Unbonded minipools:   %d\n", member.UnbondedValidatorCount)
    fmt.Printf("\n")
}

Recommendation

Validate user input before storing it on the blockchain. Validate and sanitize stored user tainted data before presenting it. Establish a register of data validation rules (e.g., email format, timezone format, etc.). Reject nodes operating with nodes that do not honor data validation rules.

Validate the correct format of variables (e.g., timezone location, email, name, …) on the storage level (if applicable) and the lowest level of the go library to offer developers a strong foundation to build on and mitigate the risk in future integrations. Furthermore, on-chain validation might not be implemented (due to increased gas consumption) should be mentioned in the developer documentation security section as they need to be handled with special caution by consumer applications. Sanitize output before presenting it to avoid control character injections in terminal applications or other presentation technologies (e.g., SQL or HTML).

Review all usage of the fmt lib (especially Sprintf and string handling/concatenating functions). Ensure only sanitized data can reach this sink. Review the logging library and ensure it is hardened against control character injection by encoding non-printables and CR-LF.

Description

Various commands in the Rocketpool CLI make use of the readOutput and printOutput functions. These do not perform sanitization of user-supplied inputs and allow an attacker to supply malicious values which can be used to execute arbitrary commands on the user’s system.

Examples

All commands using the Client.readOutput, Client.printOutput and Client.compose functions are affected.

Furthermore, Client.callAPI is used for API-related calls throughout the Rocketpool service. However, it does not validate that the values passed into it are valid API commands. This can lead to arbitrary command execution, also inside the container using docker exec.

Recommendation

Perform strict validation on all user-supplied parameters. If parameter values need to be inserted into a command template string, the %q format string or other restrictive equivalents should be used.

Description

The ACL for changing settings in the centralized RocketStorage allows any registered contract (listed under contract.exists) to change settings that belong to other parts of the system.

The concern is that if someone finds a way to add their malicious contract to the registered contact list, they will override any setting in the system. The storage is authoritative when checking certain ACLs. Being able to set any value might allow an attacker to gain control of the complete system. Allowing any contract to overwrite other contracts’ settings dramatically increases the attack surface.

Examples

rocketpool-2.5-Tokenomics-updates/contracts/contract/RocketStorage.sol:L24-L32

modifier onlyLatestRocketNetworkContract() {
    // The owner and other contracts are only allowed to set the storage upon deployment to register the initial contracts/settings, afterwards their direct access is disabled
    if (boolStorage[keccak256(abi.encodePacked("contract.storage.initialised"))] == true) {
        // Make sure the access is permitted to only contracts in our Dapp
        require(boolStorage[keccak256(abi.encodePacked("contract.exists", msg.sender))], "Invalid or outdated network contract");
    }
    _;
}

rocketpool-2.5-Tokenomics-updates/contracts/contract/RocketStorage.sol:L78-L85

function setAddress(bytes32 _key, address _value) onlyLatestRocketNetworkContract override external {
    addressStorage[_key] = _value;
}

/// @param _key The key for the record
function setUint(bytes32 _key, uint _value) onlyLatestRocketNetworkContract override external {
    uIntStorage[_key] = _value;
}

Recommendation

Allow contracts to only change settings related to their namespace.

Description

If the DAO falls below the minimum viable membership threshold, voting for proposals still continues as DAO proposals do not require a minimum participation quorum. In the worst case, this would allow the last standing DAO member to create a proposal that would be passable with only one vote even if new members would be immediately ready to join via the recovery mode (which has its own risks) as the minimum votes requirement for proposals is set as >0.

rocketpool-2.5-Tokenomics-updates/contracts/contract/dao/RocketDAOProposal.sol:L170-L170

require(_votesRequired > 0, "Proposal cannot have a 0 votes required to be successful");

rocketpool-2.5-Tokenomics-updates/contracts/contract/dao/node/RocketDAONodeTrustedProposals.sol:L57-L69

function propose(string memory _proposalMessage, bytes memory _payload) override public onlyTrustedNode(msg.sender) onlyLatestContract("rocketDAONodeTrustedProposals", address(this)) returns (uint256) {
    // Load contracts
    RocketDAOProposalInterface daoProposal = RocketDAOProposalInterface(getContractAddress('rocketDAOProposal'));
    RocketDAONodeTrustedInterface daoNodeTrusted = RocketDAONodeTrustedInterface(getContractAddress('rocketDAONodeTrusted'));
    RocketDAONodeTrustedSettingsProposalsInterface rocketDAONodeTrustedSettingsProposals = RocketDAONodeTrustedSettingsProposalsInterface(getContractAddress("rocketDAONodeTrustedSettingsProposals"));
    // Check this user can make a proposal now
    require(daoNodeTrusted.getMemberLastProposalBlock(msg.sender).add(rocketDAONodeTrustedSettingsProposals.getCooldown()) <= block.number, "Member has not waited long enough to make another proposal");
    // Record the last time this user made a proposal
    setUint(keccak256(abi.encodePacked(daoNameSpace, "member.proposal.lastblock", msg.sender)), block.number);
    // Create the proposal
    return daoProposal.add(msg.sender, 'rocketDAONodeTrustedProposals', _proposalMessage, block.number.add(rocketDAONodeTrustedSettingsProposals.getVoteDelayBlocks()), rocketDAONodeTrustedSettingsProposals.getVoteBlocks(), rocketDAONodeTrustedSettingsProposals.getExecuteBlocks(), daoNodeTrusted.getMemberQuorumVotesRequired(), _payload);
}

Sidenote: Since a proposals acceptance quorum is recorded on proposal creation, this may lead to another scenario where proposals acceptance quorum may never be reached if members leave the DAO. This would require a re-submission of the proposal.

Recommendation

Do not accept proposals if the member count falls below the minimum DAO membercount threshold.

Description

upgradeContract defines a hardcoded list of contracts that cannot be upgraded because they manage their own settings (statevars) or they hold value in the system.

• the list is hardcoded and cannot be extended when new contracts are added via addcontract. E.g. what if another contract holding value is added to the system? This would require an upgrade of the upgrade contract to update the whitelist (gas hungry, significant risk of losing access to the upgrade mechanisms if a bug is being introduced).
• a contract named rocketPoolToken is blacklisted from being upgradeable but the system registers no contract called rocketPoolToken. This may be an oversight or artifact of a previous iteration of the code. However, it may allow a malicious group of nodes to add a contract that is not yet in the system which cannot be removed anymore as there is no removeContract functionality and upgradeContract to override the malicious contract will fail due to the blacklist.
• Note that upgrading RocketTokenRPL requires an account balance migration as contracts in the system may hold value in RPL (e.g. a lot in AuctionManager) that may vanish after an upgrade. The contract is not exempt from upgrading. A migration may not be easy to perform as the system cannot be paused to e.g. snapshot balances.

Examples

rocketpool-2.5-Tokenomics-updates/contracts/contract/dao/node/RocketDAONodeTrustedUpgrade.sol:L41-L49

function _upgradeContract(string memory _name, address _contractAddress, string memory _contractAbi) internal {
    // Check contract being upgraded
    bytes32 nameHash = keccak256(abi.encodePacked(_name));
    require(nameHash != keccak256(abi.encodePacked("rocketVault")),        "Cannot upgrade the vault");
    require(nameHash != keccak256(abi.encodePacked("rocketPoolToken")),    "Cannot upgrade token contracts");
    require(nameHash != keccak256(abi.encodePacked("rocketTokenRETH")),     "Cannot upgrade token contracts");
    require(nameHash != keccak256(abi.encodePacked("rocketTokenNETH")), "Cannot upgrade token contracts");
    require(nameHash != keccak256(abi.encodePacked("casperDeposit")),      "Cannot upgrade the casper deposit contract");
    // Get old contract address & check contract exists

Recommendation

• Consider implementing a whitelist of contracts that are allowed to be upgraded instead of a more error-prone blacklist of contracts that cannot be upgraded.
• Provide documentation that outlines what contracts are upgradeable and why.
• Create a process to verify the blacklist before deploying/operating the system.
• Plan for migration paths when upgrading contracts in the system
• Any proposal that reaches the upgrade contract must be scrutinized for potential malicious activity (e.g. as any registered contract can directly modify storage or may contain subtle backdoors. Upgrading without performing a thorough security inspection may easily put the DAO at risk)

Description

If a DAO member behaves badly other DAO members may propose the node be evicted from the DAO. If for some reason, RocketVault does not hold enough RPL to pay back the DAO member bond actionKick will throw. The node is not evicted.

Now this is a somewhat exotic scenario as the vault should always hold the bond for the members in the system. However, if the node was kicked for stealing RPL (e.g. passing an upgrade proposal to perform an attack) it might be impossible to execute the eviction.

Recommendation

Ensure that there is no way a node can influence a succeeded kick proposal to fail. Consider burning the bond (by keeping it) as there is a reason for evicting the node or allow them to redeem it in a separate step.

Description

Changes in the DAO’s trusted node members are reflected in the RocketDAONodeTrusted.getMemberCount() function. When compared with the vote on consensus threshold, a DAO-driven decision is made, e.g., when updating token price feeds and changing Minipool states.

Especially in the early phase of the DAO, the functions below can get stuck as execution is restricted to DAO members who have not voted yet. Consider the following scenario:

• The DAO consists of five members
• Two members vote to make a Minipool withdrawable
• The other three members are inactive, the community votes, and they get kicked from the DAO
• The two remaining members have no way to change the Minipool state now. All method calls to trigger the state update fails because the members have already voted before.

Note: votes of members that are kicked/leave are still count towards the quorum!

Examples

Setting a Minipool into the withdrawable state:

rocketpool-2.5-Tokenomics-updates/contracts/contract/minipool/RocketMinipoolStatus.sol:L62-L65

RocketDAONodeTrustedInterface rocketDAONodeTrusted = RocketDAONodeTrustedInterface(getContractAddress("rocketDAONodeTrusted"));
if (calcBase.mul(submissionCount).div(rocketDAONodeTrusted.getMemberCount()) >= rocketDAOProtocolSettingsNetwork.getNodeConsensusThreshold()) {
    setMinipoolWithdrawable(_minipoolAddress, _stakingStartBalance, _stakingEndBalance);
}

Submitting a block’s network balances:

rocketpool-2.5-Tokenomics-updates/contracts/contract/network/RocketNetworkBalances.sol:L94-L97

RocketDAONodeTrustedInterface rocketDAONodeTrusted = RocketDAONodeTrustedInterface(getContractAddress("rocketDAONodeTrusted"));
if (calcBase.mul(submissionCount).div(rocketDAONodeTrusted.getMemberCount()) >= rocketDAOProtocolSettingsNetwork.getNodeConsensusThreshold()) {
    updateBalances(_block, _totalEth, _stakingEth, _rethSupply);
}

Submitting a block’s RPL price information:

rocketpool-2.5-Tokenomics-updates/contracts/contract/network/RocketNetworkPrices.sol:L69-L72

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

Recommendation

The conditional check and update of price feed information, Minipool state transition, etc., should be externalized into a separate public function. This function is also called internally in the existing code. In case the DAO gets into the scenario above, anyone can call the function to trigger a reevaluation of the condition with updated membership numbers and thus get the process unstuck.

Description

Trusted/oracle nodes submit various ETH2 observations to the RocketPool contracts. When 51% of nodes submitted the same observation, the result is stored in the contract. However, while it is recorded that a node already voted for a specific minipool (being withdrawable & balance) or block (price/balance), a re-submission with different parameters for the same minipool/block is not rejected.

Since the oracle values should be distinct, clear, and there can only be one valid value, it should not be allowed for trusted nodes to change their mind voting for multiple different outcomes within one block or one minipool

Examples

• RocketMinipoolStatus - a trusted node can submit multiple different results for one minipool

Note that setBool(keccak256(abi.encodePacked("minipool.withdrawable.submitted.node", msg.sender, _minipoolAddress)), true); is recorded but never checked. (as for the other two instances)

rocketpool-2.5-Tokenomics-updates/contracts/contract/minipool/RocketMinipoolStatus.sol:L48-L57

// Get submission keys
bytes32 nodeSubmissionKey = keccak256(abi.encodePacked("minipool.withdrawable.submitted.node", msg.sender, _minipoolAddress, _stakingStartBalance, _stakingEndBalance));
bytes32 submissionCountKey = keccak256(abi.encodePacked("minipool.withdrawable.submitted.count", _minipoolAddress, _stakingStartBalance, _stakingEndBalance));
// Check & update node submission status
require(!getBool(nodeSubmissionKey), "Duplicate submission from node");
setBool(nodeSubmissionKey, true);
setBool(keccak256(abi.encodePacked("minipool.withdrawable.submitted.node", msg.sender, _minipoolAddress)), true);
// Increment submission count
uint256 submissionCount = getUint(submissionCountKey).add(1);
setUint(submissionCountKey, submissionCount);

• RocketNetworkBalances - a trusted node can submit multiple different results for the balances at a specific block

rocketpool-2.5-Tokenomics-updates/contracts/contract/network/RocketNetworkBalances.sol:L80-L92

// Get submission keys
bytes32 nodeSubmissionKey = keccak256(abi.encodePacked("network.balances.submitted.node", msg.sender, _block, _totalEth, _stakingEth, _rethSupply));
bytes32 submissionCountKey = keccak256(abi.encodePacked("network.balances.submitted.count", _block, _totalEth, _stakingEth, _rethSupply));
// Check & update node submission status
require(!getBool(nodeSubmissionKey), "Duplicate submission from node");
setBool(nodeSubmissionKey, true);
setBool(keccak256(abi.encodePacked("network.balances.submitted.node", msg.sender, _block)), true);
// Increment submission count
uint256 submissionCount = getUint(submissionCountKey).add(1);
setUint(submissionCountKey, submissionCount);
// Emit balances submitted event
emit BalancesSubmitted(msg.sender, _block, _totalEth, _stakingEth, _rethSupply, block.timestamp);
// Check submission count & update network balances

• RocketNetworkPrices - a trusted node can submit multiple different results for the price at a specific block

rocketpool-2.5-Tokenomics-updates/contracts/contract/network/RocketNetworkPrices.sol:L55-L67

// Get submission keys
bytes32 nodeSubmissionKey = keccak256(abi.encodePacked("network.prices.submitted.node", msg.sender, _block, _rplPrice));
bytes32 submissionCountKey = keccak256(abi.encodePacked("network.prices.submitted.count", _block, _rplPrice));
// Check & update node submission status
require(!getBool(nodeSubmissionKey), "Duplicate submission from node");
setBool(nodeSubmissionKey, true);
setBool(keccak256(abi.encodePacked("network.prices.submitted.node", msg.sender, _block)), true);
// Increment submission count
uint256 submissionCount = getUint(submissionCountKey).add(1);
setUint(submissionCountKey, submissionCount);
// Emit prices submitted event
emit PricesSubmitted(msg.sender, _block, _rplPrice, block.timestamp);
// Check submission count & update network prices

Recommendation

Only allow one vote per minipool/block. Don’t give nodes the possibility to vote multiple times for different outcomes.

Description

The nETH token is paid to node operators when minipool becomes withdrawable. nETH is supposed to be backed by ETH 1:1. However, in most cases, this will not be the case.

The nETH minting and deposition of collateral happens in two different stages of a minipool. nETH is minted in the minipool state transition from Staking to Withdrawable when the trusted/oracle nodes find consensus on the fact that the minipool became withdrawable (submitWinipoolWithdrawable).

rocketpool-2.5-Tokenomics-updates/contracts/contract/minipool/RocketMinipoolStatus.sol:L63-L65

if (calcBase.mul(submissionCount).div(rocketDAONodeTrusted.getMemberCount()) >= rocketDAOProtocolSettingsNetwork.getNodeConsensusThreshold()) {
    setMinipoolWithdrawable(_minipoolAddress, _stakingStartBalance, _stakingEndBalance);
}

When consensus is found on the state of the minipool, nETH tokens are minted to the minipool address according to the withdrawal amount observed by the trusted/oracle nodes. At this stage, ETH backing the newly minted nETH was not yet provided.

rocketpool-2.5-Tokenomics-updates/contracts/contract/minipool/RocketMinipoolStatus.sol:L80-L87

uint256 nodeAmount = getMinipoolNodeRewardAmount(
    minipool.getNodeFee(),
    userDepositBalance,
    minipool.getStakingStartBalance(),
    minipool.getStakingEndBalance()
);
// Mint nETH to minipool contract
if (nodeAmount > 0) { rocketTokenNETH.mint(nodeAmount, _minipoolAddress); }

The nETH token contract now holds more nETH.totalsupply than actual ETH collateral. It is out of sync with the ETH reserve and therefore becomes undercollateralized. This should generally be avoided as the security guarantees that for every nETH someone deposited, ETH does not hold. However, the newly minted nETH is locked to the minipoolAddress, and the minipool has no means of redeeming the nETH for ETH directly (via nETH.burn()).

The transition from Withdrawable to Destroyed the actual collateral for the previously minted nETH (still locked to minipoolAddress) is provided by the Eth2 withdrawal contract. There is no specification for the withdrawal contract as of now. Still, it is assumed that some entity triggers the payout for the Eth2 rewards on the withdrawal contract, which sends the amount of ETH to the configured withdrawal address (the minipoolAddress).

The minipool.receive() function receives the ETH

rocketpool-2.5-Tokenomics-updates/contracts/contract/minipool/RocketMinipool.sol:L109-L112

receive() external payable {
    (bool success, bytes memory data) = getContractAddress("rocketMinipoolDelegate").delegatecall(abi.encodeWithSignature("receiveValidatorBalance()"));
    if (!success) { revert(getRevertMessage(data)); }
}

and forwards it to minipooldelegate.receiveValidatorBalance

rocketpool-2.5-Tokenomics-updates/contracts/contract/minipool/RocketMinipoolDelegate.sol:L227-L231

require(msg.sender == rocketDAOProtocolSettingsNetworkInterface.getSystemWithdrawalContractAddress(), "The minipool's validator balance can only be sent by the eth1 system withdrawal contract");
// Set validator balance withdrawn status
validatorBalanceWithdrawn = true;
// Process validator withdrawal for minipool
rocketNetworkWithdrawal.processWithdrawal{value: msg.value}();

Which calculates the nodeAmount based on the ETH received and submits it as collateral to back the previously minted nodeAmount of nETH.

rocketpool-2.5-Tokenomics-updates/contracts/contract/network/RocketNetworkWithdrawal.sol:L46-L60

uint256 totalShare = rocketMinipoolManager.getMinipoolWithdrawalTotalBalance(msg.sender);
uint256 nodeShare = rocketMinipoolManager.getMinipoolWithdrawalNodeBalance(msg.sender);
uint256 userShare = totalShare.sub(nodeShare);
// Get withdrawal amounts based on shares
uint256 nodeAmount = 0;
uint256 userAmount = 0;
if (totalShare > 0) {
    nodeAmount = msg.value.mul(nodeShare).div(totalShare);
    userAmount = msg.value.mul(userShare).div(totalShare);
}
// Set withdrawal processed status
rocketMinipoolManager.setMinipoolWithdrawalProcessed(msg.sender);
// Transfer node balance to nETH contract
if (nodeAmount > 0) { rocketTokenNETH.depositRewards{value: nodeAmount}(); }
// Transfer user balance to rETH contract or deposit pool

Looking at how the nodeAmount of nETH that was minted was calculated and comparing it to how nodeAmount of ETH is calculated, we can observe the following:

• the nodeAmount of nETH minted is an absolute number of tokens based on the rewards observed by the trusted/oracle nodes. the nodeAmount is stored in the storage and later used to calculate the collateral deposit in a later step.
• the nodeAmount calculated when depositing the collateral is first assumed to be a nodeShare (line 47), while it is actually an absolute number. the nodeShare is then turned into a nodeAmount relative to the ETH supplied to the contract.
• Due to rounding errors, this might not always exactly match the nETH minted (see https://github.com/ConsenSys/rocketpool-audit-2021-03/issues/26).
• The collateral calculation is based on the ETH value provided to the contract. If this value does not exactly match what was reported by the oracle/trusted nodes when minting nETH, less/more collateral will be provided.
  • Note: excess collateral will be locked in the nETH contract as it is unaccounted for in the nETH token contract and therefore cannot be redeemed.
  • Note: providing less collateral will go unnoticed and mess up the 1:1 nETH:ETH peg. In the worst case, there will be less nETH than ETH. Not everybody will be able to redeem their ETH.
• Note: keep in mind that the receive() function might be subject to gas restrictions depending on the implementation of the withdrawal contract (.call() vs. .transfer())

The nETH minted is initially uncollateralized and locked to the minipoolAddress, which cannot directly redeem it for ETH. The next step (next stage) is collateralized with the staking rewards (which, as noted, might not always completely add up to the minted nETH). At the last step in withdraw(), the nETH is transferred to the withdrawalAddress of the minipool.

rocketpool-2.5-Tokenomics-updates/contracts/contract/minipool/RocketMinipoolDelegate.sol:L201-L210

uint256 nethBalance = rocketTokenNETH.balanceOf(address(this));
if (nethBalance > 0) {
    // Get node withdrawal address
    RocketNodeManagerInterface rocketNodeManager = RocketNodeManagerInterface(getContractAddress("rocketNodeManager"));
    address nodeWithdrawalAddress = rocketNodeManager.getNodeWithdrawalAddress(nodeAddress);
    // Transfer
    require(rocketTokenNETH.transfer(nodeWithdrawalAddress, nethBalance), "nETH balance was not successfully transferred to node operator");
    // Emit nETH withdrawn event
    emit NethWithdrawn(nodeWithdrawalAddress, nethBalance, block.timestamp);
}

Since the nETH initially minted can never take part in the nETH token market (as it is locked to the minipool address, which can only transfer it to the withdrawal address in the last step), the question arises why it is actually minted early in the lifecycle of the minipool. At the same time, it could as well be just directly minted to withdrawalAddress when providing the right amount of collateral in the last step of the minipool lifecycle. Furthermore, if nETH is minted at this stage, it should be questioned why nETH is actually needed when you can directly forward the nodeAmount to the withdrawalAddress instead of minting an intermediary token that is pegged 1:1 to ETH.

For reference, depositRewards (providing collateral) and mint are not connected at all, hence the risk of nETH being an undercollateralized token.

rocketpool-2.5-Tokenomics-updates/contracts/contract/token/RocketTokenNETH.sol:L28-L42

function depositRewards() override external payable onlyLatestContract("rocketNetworkWithdrawal", msg.sender) {
    // Emit ether deposited event
    emit EtherDeposited(msg.sender, msg.value, block.timestamp);
}

// Mint nETH
// Only accepts calls from the RocketMinipoolStatus contract
function mint(uint256 _amount, address _to) override external onlyLatestContract("rocketMinipoolStatus", msg.sender) {
    // Check amount
    require(_amount > 0, "Invalid token mint amount");
    // Update balance & supply
    _mint(_to, _amount);
    // Emit tokens minted event
    emit TokensMinted(_to, _amount, block.timestamp);
}

Recommendation

• It looks like nETH might not be needed at all, and it should be discussed if the added complexity of having a potentially out-of-sync nETH token contract is necessary and otherwise remove it from the contract system as the nodeAmount of ETH can directly be paid out to the withdrawalAddress in the receiveValidatorBalance or withdraw transitions.
• If nETH cannot be removed, consider minting nodeAmount of nETH directly to withdrawalAddress on withdraw instead of first minting uncollateralized tokens. This will also reduce the gas footprint of the Minipool.
• Ensure that the initial nodeAmount calculation matches the minted nETH and deposited to the contract as collateral (absolute amount vs. fraction).
• Enforce that nETH requires collateral to be provided when minting tokens.

Description

When destroying the MiniPool, leftover ETH is sent to the RocketVault. Since RocketVault has no means to recover “unaccounted” ETH (not deposited via depositEther), funds forcefully sent to the vault will end up being locked.

Examples

rocketpool-2.5-Tokenomics-updates/contracts/contract/minipool/RocketMinipoolDelegate.sol:L314-L321

// Destroy the minipool
function destroy() private {
    // Destroy minipool
    RocketMinipoolManagerInterface rocketMinipoolManager = RocketMinipoolManagerInterface(getContractAddress("rocketMinipoolManager"));
    rocketMinipoolManager.destroyMinipool();
    // Self destruct & send any remaining ETH to vault
    selfdestruct(payable(getContractAddress("rocketVault")));
}

Recommendation

Implement means to recover and reuse ETH that was forcefully sent to the contract by MiniPool instances.

Description

Like a DAO user’s e-mail address, PII is stored on-chain and can, therefore, be accessed by anyone. This may allow de-pseudonymize users (and correlate Ethereum addresses to user email addresses) and be used for spamming or targeted phishing campaigns putting the DAO users at risk.

Examples

rocketpool-go-2.5-Tokenomics/dao/trustednode/dao.go:L173-L183

// Return
return MemberDetails{
    Address: memberAddress,
    Exists: exists,
    ID: id,
    Email: email,
    JoinedBlock: joinedBlock,
    LastProposalBlock: lastProposalBlock,
    RPLBondAmount: rplBondAmount,
    UnbondedValidatorCount: unbondedValidatorCount,
}, nil

rocketpool-2.5-Tokenomics-updates/contracts/contract/dao/node/RocketDAONodeTrusted.sol:L110-L112

function getMemberEmail(address _nodeAddress) override public view returns (string memory) {
    return getString(keccak256(abi.encodePacked(daoNameSpace, "member.email", _nodeAddress))); 
}

Recommendation

Avoid storing PII on-chain where it is readily available for anyone.

Description

The SSH client factory returns instances that have an insecure HostKeyCallback set. This means that SSH servers’ public key will not be validated and thus initialize a potentially insecure connection. The function should not be used for production code.

Examples

smartnode-2.5-Tokenomics/shared/services/rocketpool/client.go:L87

HostKeyCallback: ssh.InsecureIgnoreHostKey(),

Description

By default, Docker containers run commands as the root user. This means that there is little to no resistance for an attacker who has managed to break into the container and execute commands. This effectively negates file permissions already set into the system, such as storing wallet-related information with 0600 as an attacker will most likely drop into the container as root already.

Examples

Missing USER instructions affect both SmartNode Dockerfiles:

smartnode-2.5-Tokenomics/docker/rocketpool-dockerfile:L25-L36

# Start from ubuntu image
FROM ubuntu:20.10

# Install OS dependencies
RUN apt-get update && apt-get install -y ca-certificates

# Copy binary
COPY --from=builder /go/bin/rocketpool /go/bin/rocketpool

# Container entry point
ENTRYPOINT ["/go/bin/rocketpool"]

smartnode-2.5-Tokenomics/docker/rocketpool-pow-proxy-dockerfile:L24-L35

# Start from ubuntu image
FROM ubuntu:20.10

# Install OS dependencies
RUN apt-get update && apt-get install -y ca-certificates

# Copy binary
COPY --from=builder /go/bin/rocketpool-pow-proxy /go/bin/rocketpool-pow-proxy

# Container entry point
ENTRYPOINT ["/go/bin/rocketpool-pow-proxy"]