[Feature/Hotfix] Prevent Sig Reusability and call distributeBalance p…

…atch (#159)

* complete tasks

* update

* typo

* update

* rm beginUserDistribute

* admin only

* add docs

contracts/Operator/NodeAccount.sol

@@ -118,6 +118,7 @@ contract NodeAccount is UpgradeableBase, Errors {
     }
 
     function _createMinipool(ValidatorConfig calldata _config, bytes memory _sig) internal {
+        vaf.validateSigUsed(_sig);
         if (vaf.preSignedExitMessageCheck()) {
             console.log('_createMinipool: message hash');
             console.logBytes32(
@@ -177,7 +178,7 @@ contract NodeAccount is UpgradeableBase, Errors {
         }
 
         IMinipool minipool = IMinipool(_minipool);
-        OperatorDistributor(_directory.getOperatorDistributorAddress()).onNodeOperatorDissolved(
+        OperatorDistributor(_directory.getOperatorDistributorAddress()).onNodeMinipoolDestroy(
             nodeOperator,
             configs[_minipool].bondAmount
         );
@@ -217,12 +218,21 @@ contract NodeAccount is UpgradeableBase, Errors {
         require(_implementationAddress == vaf.implementationAddress(), 'only upgradable to vaf.implementationAddress');
     }
 
-    function distributeBalance(
-        bool _rewardsOnly,
-        address _minipool
-    ) external onlyNodeOperatorOrProtocol hasConfig(_minipool) {
+    /**
+     * @dev Restricting this function to admin is the only way we can technologically enforce
+     * a node operator to not slash the capital they get from constellation depositors. If we
+     * open this function to node operators, they could inappropriately finalize and fully
+     * withdraw a minipool, creating slashings on depositor capital.
+     */
+    function distributeBalance(bool _rewardsOnly, address _minipool) external onlyAdmin hasConfig(_minipool) {
         IMinipool minipool = IMinipool(_minipool);
         minipool.distributeBalance(_rewardsOnly);
+        if (minipool.getFinalised()) {
+            OperatorDistributor(_directory.getOperatorDistributorAddress()).onNodeMinipoolDestroy(
+                nodeOperator,
+                configs[_minipool].bondAmount
+            );
+        }
     }
 
     function setDelegate(address _newDelegate) external onlyNodeOperatorOrProtocol {

contracts/Operator/NodeAccountFactory.sol

@@ -24,6 +24,7 @@ contract NodeAccountFactory is UpgradeableBase, Errors {
 
     bool public preSignedExitMessageCheck;
 
+    mapping(bytes => bool) public sigsUsed;
     mapping(address => address) public minipoolNodeAccountMap;
 
     /**
@@ -130,6 +131,11 @@ contract NodeAccountFactory is UpgradeableBase, Errors {
         lockUpTime = _newLockUpTime;
     }
 
+    function validateSigUsed(bytes memory _sig) external onlyProtocol {
+        require(!sigsUsed[_sig], "sig already used");
+        sigsUsed[_sig] = true;
+    }
+
     function setImplementation(address _implementationAddress) external {
         if (!_directory.hasRole(Constants.ADMIN_ROLE, msg.sender)) {
             revert BadRole(Constants.ADMIN_ROLE, msg.sender);

contracts/Operator/OperatorDistributor.sol

@@ -24,7 +24,11 @@ contract OperatorDistributor is UpgradeableBase, Errors {
     event MinipoolCreated(address indexed _minipoolAddress, address indexed _nodeAddress);
     event MinipoolDestroyed(address indexed _minipoolAddress, address indexed _nodeAddress);
     event WarningNoMiniPoolsToHarvest();
-    event WarningMinipoolNotStaking(address indexed _minipoolAddress, MinipoolStatus indexed _status);
+    event WarningMinipoolNotStaking(
+        address indexed _minipoolAddress,
+        MinipoolStatus indexed _status,
+        bool indexed _isFinalized
+    );
 
     using Math for uint256;
 
@@ -243,20 +247,14 @@ contract OperatorDistributor is UpgradeableBase, Errors {
             }
             // stakeRPLOnBehalfOf
             // transfer RPL to deposit pool
-            IERC20(_directory.getRPLAddress()).transfer(
-                _directory.getDepositPoolAddress(),
-                _requiredStake
-            );
+            IERC20(_directory.getRPLAddress()).transfer(_directory.getDepositPoolAddress(), _requiredStake);
             FundRouter(_directory.getDepositPoolAddress()).stakeRPLFor(_NodeAccount, _requiredStake);
         } else {
             if (currentRplBalance == 0) {
                 return;
             }
             // stake what we have
-            IERC20(_directory.getRPLAddress()).transfer(
-                _directory.getDepositPoolAddress(),
-                currentRplBalance
-            );
+            IERC20(_directory.getRPLAddress()).transfer(_directory.getDepositPoolAddress(), currentRplBalance);
             FundRouter(_directory.getDepositPoolAddress()).stakeRPLFor(_NodeAccount, currentRplBalance);
         }
     }
@@ -359,17 +357,19 @@ contract OperatorDistributor is UpgradeableBase, Errors {
      * @dev Processes a single minipool by performing RPL top-up and distributing balance if certain conditions are met.
      */
     function _processNextMinipool() internal {
-        if(nextMinipoolHavestIndex > minipoolAddresses.length - 1) {
+        if (nextMinipoolHavestIndex > minipoolAddresses.length - 1) {
             nextMinipoolHavestIndex = 0;
         }
 
         uint256 index = nextMinipoolHavestIndex;
         IMinipool minipool = IMinipool(minipoolAddresses[index]);
 
         MinipoolStatus minipoolStatus = minipool.getStatus();
+        bool isFinalized = minipool.getFinalised();
         console.log('_processNextMinipool.status=', uint256(minipoolStatus));
-        if (minipoolStatus != MinipoolStatus.Staking) {
-            emit WarningMinipoolNotStaking(address(minipool), minipoolStatus);
+        console.log('_processNextMinipool.isFinalized=', isFinalized);
+        if (minipoolStatus != MinipoolStatus.Staking || isFinalized) {
+            emit WarningMinipoolNotStaking(address(minipool), minipoolStatus, isFinalized);
             return;
         }
 
@@ -383,18 +383,17 @@ contract OperatorDistributor is UpgradeableBase, Errors {
         console.log('_processNextMinipool.ethStaked', ethStaked);
 
         rebalanceRplStake(nodeAddress, ethStaked);
-        //performTopDown(nodeAddress, ethStaked);
-
-
-        uint256 balance = minipool.getNodeDepositBalance();
-        // TODO: Talk to Mike: used to pass  balance >= 8 ether but whent this is true, it will always revert
-        if (balance >= 8 ether) {
-            //minipool.distributeBalance(false);
-            NodeAccount(
-                NodeAccountFactory(_directory.getNodeAccountFactoryAddress()).minipoolNodeAccountMap(
-                    address(minipool)
-                )
-            ).distributeBalance(false, address(minipool));
+
+        uint256 totalBalance = address(minipool).balance - minipool.getNodeRefundBalance();
+
+        /**
+         * @dev We are only calling distributeBalance with a true flag to prevent griefing vectors. This ensures we
+         * are only collecting skimmed rewards and not doing anything related to full withdrawals or finalizations.
+         * One example is that if we pass false to distributeBalance, it opens a scenario where operators are forced to
+         * finalize due to having more than 8 ETH. This could result in slashings from griefing vectors.
+         */
+        if (totalBalance < 8 ether) {
+            minipool.distributeBalance(true);
         }
 
         nextMinipoolHavestIndex++;
@@ -419,12 +418,11 @@ contract OperatorDistributor is UpgradeableBase, Errors {
         targetStakeRatio = _targetStakeRatio;
     }
 
-    function onNodeOperatorDissolved(address _nodeOperator, uint256 _bond) external onlyProtocol {
+    function onNodeMinipoolDestroy(address _nodeOperator, uint256 _bond) external onlyProtocol {
         fundedEth -= _bond;
         nodeOperatorEthStaked[_nodeOperator] -= _bond;
     }
 
-
     /**
      * @notice Retrieves the list of minipool addresses managed by the contract.
      * @dev This function provides a way to fetch all the current minipool addresses in memory.

contracts/Whitelist/Whitelist.sol

@@ -33,6 +33,7 @@ contract Whitelist is UpgradeableBase {
     mapping(address => Operator) public nodeMap;
     mapping(uint256 => address) public nodeIndexMap;
     mapping(address => uint256) public reverseNodeIndexMap;
+    mapping(bytes => bool) public sigsUsed;
 
     uint256 public numOperators;
 
@@ -111,6 +112,8 @@ contract Whitelist is UpgradeableBase {
     /// @param _operator The address of the operator to be added.
     /// @return An Operator struct containing details about the newly added operator.
     function _addOperator(address _operator, bytes memory _sig) internal returns (Operator memory) {
+        require(!sigsUsed[_sig], 'sig already used');
+        sigsUsed[_sig] = true;
         bytes32 messageHash = keccak256(abi.encodePacked(_operator, address(this)));
         console.log('_addOperator: message hash');
         console.logBytes32(messageHash);