RocketMinipoolDelegateOld - Node operator may reenter finalise() to manipulate accounting
In the old Minipool delegate contract, a node operator may call the `finalise()` function to finalize a Minipool. As part of this process, a call to `_refund()` may be performed if there is a node refund balance to be transferred. This will send an amount of `nodeRefundBalance` in ETH to the `nodeWithdrawalAddress` via a low-level call, handing over control flow to an - in terms of the system - untrusted external account that this node operator controls. The node operator, therefore, is granted to opportunity to call back into `finalise()`, which is not protected against reentrancy and violates the checks-effects-interactions pattern (finalised = true is only set at the very end), to manipulate the following system settings:\\nnode.minipools.finalised.count<NodeAddress>: NodeAddress finalised count increased twice instead\\nminipools.finalised.count: global finalised count increased twice\\n`eth.matched.node.amount<NodeAddress>` - NodeAddress eth matched amount potentially reduced too many times; has an impact on `getNodeETHCollateralisationRatio -> GetNodeShare`, `getNodeETHProvided -> getNodeEffectiveRPLStake` and `getNodeETHProvided->getNodeMaximumRPLStake->withdrawRPL` and is the limiting factor when withdrawing RPL to ensure the pools stay collateralized.\\nNote: `RocketMinipoolDelegateOld` is assumed to be the currently deployed MiniPool implementation. Users may upgrade from this delegate to the new version and can roll back at any time and re-upgrade, even within the same transaction (see issue 5.3 ).\\nThe following is an annotated call stack from a node operator calling `minipool.finalise()` reentering `finalise()` once more on their Minipool:\\n```\\nfinalise() --> \\n  status == MinipoolStatus.Withdrawable  //<-- true\\n  withdrawalBlock > 0  //<-- true\\n  _finalise() -->\\n     !finalised  //<-- true\\n        _refund()\\n            nodeRefundBalance = 0  //<-- reset refund balance\\n              ---> extCall: nodeWithdrawalAddress\\n                     ---> reenter: finalise()\\n                        status == MinipoolStatus.Withdrawable  //<-- true\\n                        withdrawalBlock > 0  //<-- true\\n                        _finalise() -->\\n                             !finalised  //<-- true\\n                             nodeRefundBalance > 0  //<-- false; no refund()\\n                             address(this).balance to RETH\\n                             RocketTokenRETHInterface(rocketTokenRETH).depositExcessCollateral()\\n                             rocketMinipoolManager.incrementNodeFinalisedMinipoolCount(nodeAddress)  //<-- 1st time\\n                             eventually call rocketDAONodeTrusted.decrementMemberUnbondedValidatorCount(nodeAddress); \\n                             finalised = true;\\n                   <--- return from reentrant call\\n        <--- return from _refund()\\n     address(this).balance to RETH  //<-- NOP as balance was sent to RETH already\\n     RocketTokenRETHInterface(rocketTokenRETH).depositExcessCollateral();   //<-- does not revert\\n     rocketMinipoolManager.incrementNodeFinalisedMinipoolCount(nodeAddress);  //<-- no revert, increases\\n     'node.minipools.finalised.count', 'minipools.finalised.count', reduces 'eth.matched.node.amount' one to\\n     many times\\n     eventually call rocketDAONodeTrusted.decrementMemberUnbondedValidatorCount(nodeAddress);  //<-- manipulates\\n     'member.validator.unbonded.count' by +1\\n     finalised = true;  //<-- is already 'true', gracefully continues\\n<--- returns      \\n```\\n\\n```\\n// Called by node operator to finalise the pool and unlock their RPL stake\\nfunction finalise() external override onlyInitialised onlyMinipoolOwnerOrWithdrawalAddress(msg.sender) {\\n    // Can only call if withdrawable and can only be called once\\n    require(status == MinipoolStatus.Withdrawable, "Minipool must be withdrawable");\\n    // Node operator cannot finalise the pool unless distributeBalance has been called\\n    require(withdrawalBlock > 0, "Minipool balance must have been distributed at least once");\\n    // Finalise the pool\\n    \\_finalise();\\n}\\n```\\n\\n`_refund()` handing over control flow to `nodeWithdrawalAddress`\\n```\\n// Perform any slashings, refunds, and unlock NO's stake\\nfunction \\_finalise() private {\\n    // Get contracts\\n    RocketMinipoolManagerInterface rocketMinipoolManager = RocketMinipoolManagerInterface(getContractAddress("rocketMinipoolManager"));\\n    // Can only finalise the pool once\\n    require(!finalised, "Minipool has already been finalised");\\n    // If slash is required then perform it\\n    if (nodeSlashBalance > 0) {\\n        \\_slash();\\n    }\\n    // Refund node operator if required\\n    if (nodeRefundBalance > 0) {\\n        \\_refund();\\n    }\\n    // Send any left over ETH to rETH contract\\n    if (address(this).balance > 0) {\\n        // Send user amount to rETH contract\\n        payable(rocketTokenRETH).transfer(address(this).balance);\\n    }\\n    // Trigger a deposit of excess collateral from rETH contract to deposit pool\\n    RocketTokenRETHInterface(rocketTokenRETH).depositExcessCollateral();\\n    // Unlock node operator's RPL\\n    rocketMinipoolManager.incrementNodeFinalisedMinipoolCount(nodeAddress);\\n    // Update unbonded validator count if minipool is unbonded\\n    if (depositType == MinipoolDeposit.Empty) {\\n        RocketDAONodeTrustedInterface rocketDAONodeTrusted = RocketDAONodeTrustedInterface(getContractAddress("rocketDAONodeTrusted"));\\n        rocketDAONodeTrusted.decrementMemberUnbondedValidatorCount(nodeAddress);\\n    }\\n    // Set finalised flag\\n    finalised = true;\\n}\\n```\\n\\n```\\nfunction \\_refund() private {\\n    // Update refund balance\\n    uint256 refundAmount = nodeRefundBalance;\\n    nodeRefundBalance = 0;\\n    // Get node withdrawal address\\n    address nodeWithdrawalAddress = rocketStorage.getNodeWithdrawalAddress(nodeAddress);\\n    // Transfer refund amount\\n    (bool success,) = nodeWithdrawalAddress.call{value : refundAmount}("");\\n    require(success, "ETH refund amount was not successfully transferred to node operator");\\n    // Emit ether withdrawn event\\n    emit EtherWithdrawn(nodeWithdrawalAddress, refundAmount, block.timestamp);\\n}\\n```\\n\\nMethods adjusting system settings called twice:\\n```\\n// Increments \\_nodeAddress' number of minipools that have been finalised\\nfunction incrementNodeFinalisedMinipoolCount(address \\_nodeAddress) override external onlyLatestContract("rocketMinipoolManager", address(this)) onlyRegisteredMinipool(msg.sender) {\\n    // Update the node specific count\\n    addUint(keccak256(abi.encodePacked("node.minipools.finalised.count", \\_nodeAddress)), 1);\\n    // Update the total count\\n    addUint(keccak256(bytes("minipools.finalised.count")), 1);\\n}\\n```\\n\\n```\\n}\\nfunction decrementMemberUnbondedValidatorCount(address \\_nodeAddress) override external onlyLatestContract("rocketDAONodeTrusted", address(this)) onlyRegisteredMinipool(msg.sender) {\\n    subUint(keccak256(abi.encodePacked(daoNameSpace, "member.validator.unbonded.count", \\_nodeAddress)), 1);\\n}\\n```\\n
We recommend setting the `finalised = true` flag immediately after checking for it. Additionally, the function flow should adhere to the checks-effects-interactions pattern whenever possible. We recommend adding generic reentrancy protection whenever the control flow is handed to an untrusted entity.
```\\nfinalise() --> \\n  status == MinipoolStatus.Withdrawable  //<-- true\\n  withdrawalBlock > 0  //<-- true\\n  _finalise() -->\\n     !finalised  //<-- true\\n        _refund()\\n            nodeRefundBalance = 0  //<-- reset refund balance\\n              ---> extCall: nodeWithdrawalAddress\\n                     ---> reenter: finalise()\\n                        status == MinipoolStatus.Withdrawable  //<-- true\\n                        withdrawalBlock > 0  //<-- true\\n                        _finalise() -->\\n                             !finalised  //<-- true\\n                             nodeRefundBalance > 0  //<-- false; no refund()\\n                             address(this).balance to RETH\\n                             RocketTokenRETHInterface(rocketTokenRETH).depositExcessCollateral()\\n                             rocketMinipoolManager.incrementNodeFinalisedMinipoolCount(nodeAddress)  //<-- 1st time\\n                             eventually call rocketDAONodeTrusted.decrementMemberUnbondedValidatorCount(nodeAddress); \\n                             finalised = true;\\n                   <--- return from reentrant call\\n        <--- return from _refund()\\n     address(this).balance to RETH  //<-- NOP as balance was sent to RETH already\\n     RocketTokenRETHInterface(rocketTokenRETH).depositExcessCollateral();   //<-- does not revert\\n     rocketMinipoolManager.incrementNodeFinalisedMinipoolCount(nodeAddress);  //<-- no revert, increases\\n     'node.minipools.finalised.count', 'minipools.finalised.count', reduces 'eth.matched.node.amount' one to\\n     many times\\n     eventually call rocketDAONodeTrusted.decrementMemberUnbondedValidatorCount(nodeAddress);  //<-- manipulates\\n     'member.validator.unbonded.count' by +1\\n     finalised = true;  //<-- is already 'true', gracefully continues\\n<--- returns      \\n```\\n