Merge pull request #37 from sanbir/audit-3-fix

Audit 3 fix

.github/actions/spelling/allow.txt

@@ -398,3 +398,4 @@ bfa
 feba
 baf
 dac
+cooldown

contracts/access/Ownable2Step.sol

@@ -10,6 +10,11 @@ import "./Ownable.sol";
 */
 error Ownable2Step__CallerNotNewOwner();
 
+/**
+* @notice new owner address should be different from the current owner
+*/
+error Ownable2Step__NewOwnerShouldNotBeCurrentOwner();
+
 /**
  * @dev Contract module which provides access control mechanism, where
  * there is an account (an owner) that can be granted exclusive access to
@@ -43,8 +48,13 @@ abstract contract Ownable2Step is Ownable {
      * Can only be called by the current owner.
      */
     function transferOwnership(address newOwner) public virtual override onlyOwner {
+        address currentOwner = owner();
+        if (newOwner == currentOwner) {
+            revert Ownable2Step__NewOwnerShouldNotBeCurrentOwner();
+        }
+
         s_pendingOwner = newOwner;
-        emit OwnershipTransferStarted(owner(), newOwner);
+        emit OwnershipTransferStarted(currentOwner, newOwner);
     }
 
     /**

contracts/access/OwnableWithOperator.sol

@@ -60,7 +60,7 @@ abstract contract OwnableWithOperator is Ownable2Step, IOwnableWithOperator {
         address currentOwner = owner();
         address currentOperator = s_operator;
 
-        if (currentOperator != _address && currentOwner != _address) {
+        if (_address == address(0) || (currentOperator != _address && currentOwner != _address)) {
             revert Access__AddressNeitherOperatorNorOwner(_address, currentOperator, currentOwner);
         }
     }

contracts/assetRecovering/OwnableTokenRecoverer.sol

@@ -35,15 +35,13 @@ abstract contract OwnableTokenRecoverer is TokenRecoverer, OwnableBase {
      * @param _token address of the ERC721 token
      * @param _recipient address to transfer the token to
      * @param _tokenId id of the individual token
-     * @param _data data to transfer along
      */
     function transferERC721(
         address _token,
         address _recipient,
-        uint256 _tokenId,
-        bytes calldata _data
+        uint256 _tokenId
     ) external onlyOwner {
-        _transferERC721(_token, _recipient, _tokenId, _data);
+        _transferERC721(_token, _recipient, _tokenId);
     }
 
     /**

contracts/assetRecovering/TokenRecoverer.sol

@@ -22,7 +22,7 @@ abstract contract TokenRecoverer {
     using SafeERC20 for IERC20;
 
     event ERC20Transferred(address indexed _token, address indexed _recipient, uint256 _amount);
-    event ERC721Transferred(address indexed _token, address indexed _recipient, uint256 _tokenId, bytes _data);
+    event ERC721Transferred(address indexed _token, address indexed _recipient, uint256 _tokenId);
     event ERC1155Transferred(address indexed _token, address indexed _recipient, uint256 _tokenId, uint256 _amount, bytes _data);
 
     /**
@@ -61,16 +61,14 @@ abstract contract TokenRecoverer {
      * @param _token address of the ERC721 token
      * @param _recipient address to transfer the token to
      * @param _tokenId id of the individual token
-     * @param _data data to transfer along
      */
     function _transferERC721(
         address _token,
         address _recipient,
-        uint256 _tokenId,
-        bytes calldata _data
+        uint256 _tokenId
     ) internal virtual burnDisallowed(_recipient) {
-        IERC721(_token).safeTransferFrom(address(this), _recipient, _tokenId, _data);
-        emit ERC721Transferred(_token, _recipient, _tokenId, _data);
+        IERC721(_token).transferFrom(address(this), _recipient, _tokenId);
+        emit ERC721Transferred(_token, _recipient, _tokenId);
     }
 
     /**

contracts/constants/P2pConstants.sol

@@ -11,6 +11,12 @@ uint256 constant VALIDATORS_MAX_AMOUNT = 400;
 /// @dev Collateral size of 1 validator
 uint256 constant COLLATERAL = 32 ether;
 
+/// @dev Minimal 1 time deposit
+uint256 constant MIN_DEPOSIT = 1 ether;
+
 /// @dev Lockup time to allow P2P to make ETH2 deposits
 /// @dev If there is leftover ETH after this time, it can be refunded
 uint40 constant TIMEOUT = 1 days;
+
+/// @dev Cooldown period for the client to restore their ETH receiving ability to receive their collaterals
+uint256 constant COOLDOWN = 30 days;

contracts/feeDistributor/BaseFeeDistributor.sol

@@ -75,7 +75,7 @@ abstract contract BaseFeeDistributor is Erc4337Account, OwnableTokenRecoverer, O
     function initialize(
         FeeRecipient calldata _clientConfig,
         FeeRecipient calldata _referrerConfig
-    ) external onlyFactory {
+    ) public virtual onlyFactory {
         if (_clientConfig.recipient == address(0)) {
             revert FeeDistributor__ZeroAddressClient();
         }
@@ -85,7 +85,7 @@ abstract contract BaseFeeDistributor is Erc4337Account, OwnableTokenRecoverer, O
         if (s_clientConfig.recipient != address(0)) {
             revert FeeDistributor__ClientAlreadySet(s_clientConfig.recipient);
         }
-        if (_clientConfig.basisPoints > 10000) {
+        if (_clientConfig.basisPoints >= 10000) {
             revert FeeDistributor__InvalidClientBasisPoints(_clientConfig.basisPoints);
         }
 
@@ -96,6 +96,9 @@ abstract contract BaseFeeDistributor is Erc4337Account, OwnableTokenRecoverer, O
             if (_referrerConfig.recipient == _clientConfig.recipient) {
                 revert FeeDistributor__ReferrerAddressEqualsClient(_referrerConfig.recipient);
             }
+            if (_referrerConfig.basisPoints == 0) {
+                revert FeeDistributor__ZeroReferrerBasisPointsForNonZeroReferrer();
+            }
             if (_clientConfig.basisPoints + _referrerConfig.basisPoints > 10000) {
                 revert FeeDistributor__ClientPlusReferralBasisPointsExceed10000(
                     _clientConfig.basisPoints,

contracts/feeDistributor/ContractWcFeeDistributor.sol

@@ -34,15 +34,24 @@ contract ContractWcFeeDistributor is BaseFeeDistributor {
         uint32 _newExitedCount
     );
 
-    /// @notice Emits when new collaterals (multiples of 32 ETH) have been returned to the client
-    /// @param _added number of newly returned collaterals
-    /// @param _newCollateralReturnedCount total number of all collaterals returned to the client
-    event ContractWcFeeDistributor__CollateralReturnedCountIncreased(
-        uint32 _added,
-        uint32 _newCollateralReturnedCount
+    /// @notice Emits when some portion of ETH has been returned to the client as a collateral return
+    /// @param _added amount of newly returned ETH
+    /// @param _newCollateralReturnedValue total collateral value returned to the client
+    event ContractWcFeeDistributor__CollateralReturnedValueIncreased(
+        uint112 _added,
+        uint112 _newCollateralReturnedValue
+    );
+
+    /// @notice Emits when the client reverted on collateral receive.
+    /// @dev Highly unlikely scenario. Turns on a 30 days cooldown period for the client to turn back on their
+    /// ETH receiving functionality. After this cooldown period expires, if the client still doesn't accept ETH,
+    /// their collaterals will be split as regular rewards.
+    /// @param _cooldownUntil block timestamp until which it's impossible to bypass client's revert on ETH receive
+    event ContractWcFeeDistributor__ClientRevertOnCollateralReceive(
+        uint80 _cooldownUntil
     );
 
-    /// @dev depositedCount, exitedCount, collateralReturnedCount stored in 1 storage slot
+    /// @dev depositedCount, exitedCount, collateralReturnedValue stored in 1 storage slot
     ValidatorData private s_validatorData;
 
     /// @dev Set values that are constant, common for all the clients, known at the initial deploy time.
@@ -111,25 +120,52 @@ contract ContractWcFeeDistributor is BaseFeeDistributor {
             revert FeeDistributor__NothingToWithdraw();
         }
 
-        if (balance >= COLLATERAL && s_validatorData.collateralReturnedCount < s_validatorData.exitedCount) {
-            // if exited and some validators withdrawn
-
-            // integer division
-            uint32 collateralsCountToReturn = uint32(balance / COLLATERAL);
+        if (s_validatorData.collateralReturnedValue / COLLATERAL < s_validatorData.exitedCount) {
+            // If exited some of the validators voluntarily.
+            // In case of slashing, the client can still call the voluntaryExit function to claim
+            // non-splittable balance up to 32 ETH per validator,
+            // thus getting some slashing protection covered from EL rewards.
 
-            s_validatorData.collateralReturnedCount += collateralsCountToReturn;
-
-            emit ContractWcFeeDistributor__CollateralReturnedCountIncreased(
-                collateralsCountToReturn,
-                s_validatorData.collateralReturnedCount
+            uint112 collateralValueToReturn = uint112(
+                s_validatorData.exitedCount * COLLATERAL - s_validatorData.collateralReturnedValue
             );
 
+            if (collateralValueToReturn > balance) {
+                collateralValueToReturn = uint112(balance);
+            }
+
             // Send collaterals to client
-            P2pAddressLib._sendValue(
+            bool success = P2pAddressLib._sendValue(
                 s_clientConfig.recipient,
-                collateralsCountToReturn * COLLATERAL
+                collateralValueToReturn
             );
 
+            if (success) {
+                // It's OK to violate the checks-effects-interactions pattern here thanks to nonReentrant
+                s_validatorData.collateralReturnedValue += collateralValueToReturn;
+
+                emit ContractWcFeeDistributor__CollateralReturnedValueIncreased(
+                    collateralValueToReturn,
+                    s_validatorData.collateralReturnedValue
+                );
+
+                if (s_validatorData.cooldownUntil != 0) {
+                    // reset cooldownUntil if the client received their collaterals
+                    s_validatorData.cooldownUntil = 0;
+                }
+            } else {
+                if (s_validatorData.cooldownUntil == 0) {
+                    // set cooldownUntil if it has not been set before
+                    s_validatorData.cooldownUntil = uint80(block.timestamp + COOLDOWN);
+                }
+
+                emit ContractWcFeeDistributor__ClientRevertOnCollateralReceive(s_validatorData.cooldownUntil);
+
+                if (block.timestamp < s_validatorData.cooldownUntil) {
+                    return; // prevent further ETH sending
+                }
+            }
+
             // Balance remainder to split
             balance = address(this).balance;
         }
@@ -179,6 +215,10 @@ contract ContractWcFeeDistributor is BaseFeeDistributor {
     /// refuse to accept ether.
     /// @param _to receiver address
     function recoverEther(address payable _to) external onlyOwner {
+        if (_to == address(0)) {
+            revert FeeDistributor__ZeroAddressEthReceiver();
+        }
+
         this.withdraw();
 
         // get the contract's balance
@@ -190,7 +230,7 @@ contract ContractWcFeeDistributor is BaseFeeDistributor {
             if (success) {
                 emit FeeDistributor__EtherRecovered(_to, balance);
             } else {
-                emit FeeDistributor__EtherRecoveryFailed(_to, balance);
+                revert FeeDistributor__EtherRecoveryFailed(_to, balance);
             }
         }
     }
@@ -207,10 +247,10 @@ contract ContractWcFeeDistributor is BaseFeeDistributor {
         return s_validatorData.exitedCount;
     }
 
-    /// @notice Returns the number of collaterals (multiples of 32 ETH) returned to the client
-    /// @return uint32 number of collaterals
-    function collateralReturnedCount() external view returns (uint32) {
-        return s_validatorData.collateralReturnedCount;
+    /// @notice Returns the amount of ETH returned to the client to cover the collaterals
+    /// @return uint112 ETH value returned
+    function collateralReturnedValue() external view returns (uint112) {
+        return s_validatorData.collateralReturnedValue;
     }
 
     /// @inheritdoc IFeeDistributor

contracts/feeDistributor/ElOnlyFeeDistributor.sol

@@ -81,6 +81,10 @@ contract ElOnlyFeeDistributor is BaseFeeDistributor {
     /// refuse to accept ether.
     /// @param _to receiver address
     function recoverEther(address payable _to) external onlyOwner {
+        if (_to == address(0)) {
+            revert FeeDistributor__ZeroAddressEthReceiver();
+        }
+
         this.withdraw();
 
         // get the contract's balance
@@ -92,7 +96,7 @@ contract ElOnlyFeeDistributor is BaseFeeDistributor {
             if (success) {
                 emit FeeDistributor__EtherRecovered(_to, balance);
             } else {
-                emit FeeDistributor__EtherRecoveryFailed(_to, balance);
+                revert FeeDistributor__EtherRecoveryFailed(_to, balance);
             }
         }
     }

contracts/feeDistributor/FeeDistributorErrors.sol

@@ -23,6 +23,9 @@ error FeeDistributor__ZeroAddressClient();
 /// @param _clientBasisPoints passed incorrect client basis points
 error FeeDistributor__InvalidClientBasisPoints(uint96 _clientBasisPoints);
 
+/// @notice Referrer basis points should be > 0 if the referrer exists
+error FeeDistributor__ZeroReferrerBasisPointsForNonZeroReferrer();
+
 /// @notice The sum of (Client basis points + Referral basis points) should be >= 0 and <= 10000
 /// @param _clientBasisPoints passed client basis points
 /// @param _referralBasisPoints passed referral basis points
@@ -72,3 +75,14 @@ error FeeDistributor__NothingToWithdraw();
 /// @param _caller address of the caller
 /// @param _client address of the client
 error FeeDistributor__CallerNotClient(address _caller, address _client);
+
+/// @notice Throws in case there was some ether left after `withdraw` and it has failed to recover.
+/// @param _to destination address for ether.
+/// @param _amount how much wei the destination address should have received, but didn't.
+error FeeDistributor__EtherRecoveryFailed(
+    address _to,
+    uint256 _amount
+);
+
+/// @notice ETH receiver should not be a zero address
+error FeeDistributor__ZeroAddressEthReceiver();

contracts/feeDistributor/IFeeDistributor.sol

@@ -45,14 +45,6 @@ interface IFeeDistributor is IERC165 {
         uint256 _amount
     );
 
-    /// @notice Emits if case there was some ether left after `withdraw` and it has failed to recover.
-    /// @param _to destination address for ether.
-    /// @param _amount how much wei the destination address should have received, but didn't.
-    event FeeDistributor__EtherRecoveryFailed(
-        address indexed _to,
-        uint256 _amount
-    );
-
     /// @notice Set client address.
     /// @dev Could not be in the constructor since it is different for different clients.
     /// _referrerConfig can be zero if there is no referrer.