RocketTokenNETH - Pot. discrepancy between minted tokens and deposited collateral
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.\\nThe `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).\\n```\\nif (calcBase.mul(submissionCount).div(rocketDAONodeTrusted.getMemberCount()) >= rocketDAOProtocolSettingsNetwork.getNodeConsensusThreshold()) {\\n    setMinipoolWithdrawable(\\_minipoolAddress, \\_stakingStartBalance, \\_stakingEndBalance);\\n}\\n```\\n\\nWhen 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.\\n```\\nuint256 nodeAmount = getMinipoolNodeRewardAmount(\\n    minipool.getNodeFee(),\\n    userDepositBalance,\\n    minipool.getStakingStartBalance(),\\n    minipool.getStakingEndBalance()\\n);\\n// Mint nETH to minipool contract\\nif (nodeAmount > 0) { rocketTokenNETH.mint(nodeAmount, \\_minipoolAddress); }\\n```\\n\\nThe `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()).\\nThe 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).\\nThe `minipool.receive()` function receives the `ETH`\\n```\\nreceive() external payable {\\n    (bool success, bytes memory data) = getContractAddress("rocketMinipoolDelegate").delegatecall(abi.encodeWithSignature("receiveValidatorBalance()"));\\n    if (!success) { revert(getRevertMessage(data)); }\\n}\\n```\\n\\nand forwards it to `minipooldelegate.receiveValidatorBalance`\\n```\\nrequire(msg.sender == rocketDAOProtocolSettingsNetworkInterface.getSystemWithdrawalContractAddress(), "The minipool's validator balance can only be sent by the eth1 system withdrawal contract");\\n// Set validator balance withdrawn status\\nvalidatorBalanceWithdrawn = true;\\n// Process validator withdrawal for minipool\\nrocketNetworkWithdrawal.processWithdrawal{value: msg.value}();\\n```\\n\\nWhich calculates the `nodeAmount` based on the `ETH` received and submits it as collateral to back the previously minted `nodeAmount` of `nETH`.\\n```\\nuint256 totalShare = rocketMinipoolManager.getMinipoolWithdrawalTotalBalance(msg.sender);\\nuint256 nodeShare = rocketMinipoolManager.getMinipoolWithdrawalNodeBalance(msg.sender);\\nuint256 userShare = totalShare.sub(nodeShare);\\n// Get withdrawal amounts based on shares\\nuint256 nodeAmount = 0;\\nuint256 userAmount = 0;\\nif (totalShare > 0) {\\n    nodeAmount = msg.value.mul(nodeShare).div(totalShare);\\n    userAmount = msg.value.mul(userShare).div(totalShare);\\n}\\n// Set withdrawal processed status\\nrocketMinipoolManager.setMinipoolWithdrawalProcessed(msg.sender);\\n// Transfer node balance to nETH contract\\nif (nodeAmount > 0) { rocketTokenNETH.depositRewards{value: nodeAmount}(); }\\n// Transfer user balance to rETH contract or deposit pool\\n```\\n\\nLooking 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:\\nthe `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.\\nthe `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.\\nDue to rounding errors, this might not always exactly match the `nETH` minted (see https://github.com/ConsenSys/rocketpool-audit-2021-03/issues/26).\\nThe 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.\\nNote: excess collateral will be locked in the `nETH` contract as it is unaccounted for in the `nETH` token contract and therefore cannot be redeemed.\\nNote: 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`.\\nNote: keep in mind that the `receive()` function might be subject to gas restrictions depending on the implementation of the withdrawal contract (.call() vs. .transfer())\\nThe `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.\\n```\\nuint256 nethBalance = rocketTokenNETH.balanceOf(address(this));\\nif (nethBalance > 0) {\\n    // Get node withdrawal address\\n    RocketNodeManagerInterface rocketNodeManager = RocketNodeManagerInterface(getContractAddress("rocketNodeManager"));\\n    address nodeWithdrawalAddress = rocketNodeManager.getNodeWithdrawalAddress(nodeAddress);\\n    // Transfer\\n    require(rocketTokenNETH.transfer(nodeWithdrawalAddress, nethBalance), "nETH balance was not successfully transferred to node operator");\\n    // Emit nETH withdrawn event\\n    emit NethWithdrawn(nodeWithdrawalAddress, nethBalance, block.timestamp);\\n}\\n```\\n\\nSince 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`.\\nFor reference, `depositRewards` (providing collateral) and `mint` are not connected at all, hence the risk of `nETH` being an undercollateralized token.\\n```\\nfunction depositRewards() override external payable onlyLatestContract("rocketNetworkWithdrawal", msg.sender) {\\n    // Emit ether deposited event\\n    emit EtherDeposited(msg.sender, msg.value, block.timestamp);\\n}\\n\\n// Mint nETH\\n// Only accepts calls from the RocketMinipoolStatus contract\\nfunction mint(uint256 \\_amount, address \\_to) override external onlyLatestContract("rocketMinipoolStatus", msg.sender) {\\n    // Check amount\\n    require(\\_amount > 0, "Invalid token mint amount");\\n    // Update balance & supply\\n    \\_mint(\\_to, \\_amount);\\n    // Emit tokens minted event\\n    emit TokensMinted(\\_to, \\_amount, block.timestamp);\\n}\\n```\\n
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.\\nIf `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.\\nEnsure that the initial `nodeAmount` calculation matches the minted `nETH` and deposited to the contract as collateral (absolute amount vs. fraction).\\nEnforce that `nETH` requires collateral to be provided when minting tokens.
```\\nif (calcBase.mul(submissionCount).div(rocketDAONodeTrusted.getMemberCount()) >= rocketDAOProtocolSettingsNetwork.getNodeConsensusThreshold()) {\\n    setMinipoolWithdrawable(\\_minipoolAddress, \\_stakingStartBalance, \\_stakingEndBalance);\\n}\\n```\\n