RocketNodeDistributorDelegate - Reentrancy in distribute() allows node owner to drain distributor funds
The `distribute()` function distributes the contract's balance between the node operator and the user. The node operator is returned their initial collateral, including a fee. The rest is returned to the RETH token contract as user collateral.\\nAfter determining the node owner's share, the contract transfers `ETH` to the node withdrawal address, which can be the configured withdrawal address or the node address. Both addresses may potentially be a malicious contract that recursively calls back into the `distribute()` function to retrieve the node share multiple times until all funds are drained from the contract. The `distribute()` function is not protected against reentrancy:\\n```\\n/// @notice Distributes the balance of this contract to its owners\\nfunction distribute() override external {\\n    // Calculate node share\\n    uint256 nodeShare = getNodeShare();\\n    // Transfer node share\\n    address withdrawalAddress = rocketStorage.getNodeWithdrawalAddress(nodeAddress);\\n    (bool success,) = withdrawalAddress.call{value : nodeShare}("");\\n    require(success);\\n    // Transfer user share\\n    uint256 userShare = address(this).balance;\\n    address rocketTokenRETH = rocketStorage.getAddress(rocketTokenRETHKey);\\n    payable(rocketTokenRETH).transfer(userShare);\\n    // Emit event\\n    emit FeesDistributed(nodeAddress, userShare, nodeShare, block.timestamp);\\n}\\n```\\n\\nWe also noticed that any address could set a withdrawal address as there is no check for the caller to be a registered node. In fact, the caller can be the withdrawal address or node operator.\\n```\\n// Set a node's withdrawal address\\nfunction setWithdrawalAddress(address \\_nodeAddress, address \\_newWithdrawalAddress, bool \\_confirm) external override {\\n    // Check new withdrawal address\\n    require(\\_newWithdrawalAddress != address(0x0), "Invalid withdrawal address");\\n    // Confirm the transaction is from the node's current withdrawal address\\n    address withdrawalAddress = getNodeWithdrawalAddress(\\_nodeAddress);\\n    require(withdrawalAddress == msg.sender, "Only a tx from a node's withdrawal address can update it");\\n    // Update immediately if confirmed\\n    if (\\_confirm) {\\n        updateWithdrawalAddress(\\_nodeAddress, \\_newWithdrawalAddress);\\n    }\\n    // Set pending withdrawal address if not confirmed\\n    else {\\n        pendingWithdrawalAddresses[\\_nodeAddress] = \\_newWithdrawalAddress;\\n    }\\n}\\n```\\n
Resolution\\nFixed in https://github.com/rocket-pool/rocketpool/tree/77d7cca65b7c0557cfda078a4fc45f9ac0cc6cc6 by implementing a custom reentrancy guard via a new state variable `lock` that is appended to the end of the storage layout. The reentrancy guard is functionally equivalent to the OpenZeppelin implementation. The method was not refactored to give user funds priority over the node share. Additionally, the client provided the following statement:\\nWe acknowledge this as a critical issue and have solved with a reentrancy guard.\\nWe followed OpenZeppelin's design for a reentrancy guard. We were unable to use it directly as it is hardcoded to use storage slot 0 and because we already have deployment of this delegate in the wild already using storage slot 0 for another purpose, we had to append it to the end of the existing storage layout.\\nAdd a reentrancy guard to functions that interact with untrusted contracts. Adhere to the checks-effects pattern and send user funds to the ‘trusted' RETH contract first. Only then send funds to the node's withdrawal address.
```\\n/// @notice Distributes the balance of this contract to its owners\\nfunction distribute() override external {\\n    // Calculate node share\\n    uint256 nodeShare = getNodeShare();\\n    // Transfer node share\\n    address withdrawalAddress = rocketStorage.getNodeWithdrawalAddress(nodeAddress);\\n    (bool success,) = withdrawalAddress.call{value : nodeShare}("");\\n    require(success);\\n    // Transfer user share\\n    uint256 userShare = address(this).balance;\\n    address rocketTokenRETH = rocketStorage.getAddress(rocketTokenRETHKey);\\n    payable(rocketTokenRETH).transfer(userShare);\\n    // Emit event\\n    emit FeesDistributed(nodeAddress, userShare, nodeShare, block.timestamp);\\n}\\n```\\n