Address audit

audits/internal4/README.md

@@ -24,23 +24,27 @@ unstake:
 Doesn't match the pattern Checks, Effects, and Interactions (CEI):
 _withdraw(sInfo.multisig, sInfo.reward); in middle
 ```
+[x] fixed
 
 Reentrancy critical issue: <br>
 ```solidity
 ServiceStaking.unstake -> _withdraw -> to.call{value: amount}("") -> ServiceStaking.unstake (sInfo.reward > 0 ??) -> _withdraw -> to.call{value: amount}("")
 ```
+[x] fixed, protected by the unstake() function as the service data will be deleted
 
 Reentrancy medium issue: <br>
 ```solidity
 ServiceStakingToken.unstake -> _withdraw -> SafeTransferLib.safeTransfer(stakingToken, to, amount); -> ServiceStaking.unstake
 via custom stakingToken
 ```
+[x] fixed, protected by the unstake() function as the service data will be deleted
 
 Reentrancy low issue: <br>
 ```solidity
 ServiceStakingToken.deposit -> SafeTransferLib.safeTransfer(stakingToken, to, amount); -> ServiceStaking.deposit
 via custom stakingToken
 ```
+[x] fixed
 
 Pointer bug (thanks Aleksandr Kuperman) <br>
 ```
@@ -60,7 +64,7 @@ uint256 curServiceId = serviceIds[i];
 Details: https://github.com/crytic/slither/wiki/Detector-Documentation#variable-names-too-similar
 Reusing a same variable name in different scopes.
 ```
-
+[x] fixed
 
 ```solidity
     if (state != 4) {
@@ -69,6 +73,7 @@ Reusing a same variable name in different scopes.
 It's better to use the original type enum.
 Details: https://github.com/pessimistic-io/slitherin/blob/master/docs/magic_number.md
 ```
+[x] fixed
 
 # Low optimization
 ```
@@ -84,11 +89,14 @@ if (size > 0) {
         }
 if size == 0 then for(i = 0; i < 0; i++) -> no loop
 ```
+[x] fixed
 ```
         // Transfer the service for staking
         IService(serviceRegistry).safeTransferFrom(msg.sender, address(this), serviceId);
         Last operation?
 ```
+[x] verified, this reentrancy cannot take place as the ERC721 implementation calls ERC721TokenReceiver(to), where to is
+a staking contract
 
 ### General considerations
 Measuring the number of live transactions through a smart contract has fundamental limitations: <br>
@@ -115,6 +123,7 @@ hash(multisig.code) vs well-know hash
 ```
 Is it possible to replace the multisig with some kind of fake one without losing the opportunity to receive rewards? <br>
 Notes: re-checking logic in contracts\multisigs\GnosisSafeSameAddressMultisig.sol (!) and update logic if necessary.
+[x] fixed, but the GnosisSafeSameAddressMultisig contract needs to be addressed as well at some point
 
 2. "Normal" multisig. Open question
 ```

contracts/staking/ServiceStakingBase.sol

@@ -18,6 +18,15 @@ interface IService {
         Agent
     }
 
+    enum ServiceState {
+        NonExistent,
+        PreRegistration,
+        ActiveRegistration,
+        FinishedRegistration,
+        Deployed,
+        TerminatedBonded
+    }
+
     /// @dev Transfers the service that was previously approved to this contract address.
     /// @param from Account address to transfer from.
     /// @param to Account address to transfer to.
@@ -152,11 +161,14 @@ abstract contract ServiceStakingBase is ERC721TokenReceiver, IErrorsRegistries {
     mapping (uint256 => ServiceInfo) public mapServiceInfo;
     // Set of currently staking serviceIds
     uint256[] public setServiceIds;
+    // Map of approved multisig proxy hashes
+    mapping(bytes32 => bool) mapMultisigHashes;
 
     /// @dev ServiceStakingBase constructor.
     /// @param _stakingParams Service staking parameters.
     /// @param _serviceRegistry ServiceRegistry contract address.
-    constructor(StakingParams memory _stakingParams, address _serviceRegistry) {
+    /// @param _multisigProxyAddresses Multisig proxy addresses.
+    constructor(StakingParams memory _stakingParams, address _serviceRegistry, address[] memory _multisigProxyAddresses) {
         // Initial checks
         if (_stakingParams.maxNumServices == 0 || _stakingParams.rewardsPerSecond == 0 ||
             _stakingParams.minStakingDeposit == 0 || _stakingParams.livenessPeriod == 0 ||
@@ -181,17 +193,32 @@ abstract contract ServiceStakingBase is ERC721TokenReceiver, IErrorsRegistries {
         configHash = _stakingParams.configHash;
 
         // Assign agent Ids, if applicable
-        uint256 size = _stakingParams.agentIds.length;
         uint256 agentId;
-        if (size > 0) {
-            for (uint256 i = 0; i < size; ++i) {
-                // Agent Ids must be unique and in ascending order
-                if (_stakingParams.agentIds[i] <= agentId) {
-                    revert WrongAgentId(_stakingParams.agentIds[i]);
-                }
-                agentId = _stakingParams.agentIds[i];
-                agentIds.push(agentId);
+        for (uint256 i = 0; i < _stakingParams.agentIds.length; ++i) {
+            // Agent Ids must be unique and in ascending order
+            if (_stakingParams.agentIds[i] <= agentId) {
+                revert WrongAgentId(_stakingParams.agentIds[i]);
+            }
+            agentId = _stakingParams.agentIds[i];
+            agentIds.push(agentId);
+        }
+
+        // There must be at least one multisig proxy address
+        uint256 size = _multisigProxyAddresses.length;
+        if (size == 0) {
+            revert ZeroValue();
+        }
+        // Hash the bytecode of provided multisig proxies
+        for (uint256 i = 0; i < size; ++i) {
+            address proxy = _multisigProxyAddresses[i];
+            // Check for the zero address
+            if (proxy == address(0)) {
+                revert ZeroAddress();
             }
+
+            // Hash the proxy bytecode
+            bytes32 proxyHash = keccak256(proxy.code);
+            mapMultisigHashes[proxyHash] = true;
         }
 
         // Set the checkpoint timestamp to be the deployment one
@@ -244,9 +271,16 @@ abstract contract ServiceStakingBase is ERC721TokenReceiver, IErrorsRegistries {
             revert WrongServiceConfiguration(serviceId);
         }
         // The service must be deployed
-        if (state != 4) {
+        if (IService.ServiceState(state) != IService.ServiceState.Deployed) {
             revert WrongServiceState(state, serviceId);
         }
+
+        // Check that the multisig address corresponds to the authorized multisig proxy
+        bytes32 proxyHash = keccak256(multisig.code);
+        if (!mapMultisigHashes[proxyHash]) {
+            revert UnauthorizedMultisig(multisig);
+        }
+
         // Check the agent Ids requirement, if applicable
         uint256 size = agentIds.length;
         if (size > 0) {
@@ -387,12 +421,12 @@ abstract contract ServiceStakingBase is ERC721TokenReceiver, IErrorsRegistries {
 
         // If there are eligible services, proceed with staking calculation and update rewards
         if (numServices > 0) {
+            uint256 curServiceId;
             // If total allocated rewards are not enough, adjust the reward value
             if (totalRewards > lastAvailableRewards) {
                 // Traverse all the eligible services and adjust their rewards proportional to leftovers
                 uint256 updatedReward;
                 uint256 updatedTotalRewards;
-                uint256 curServiceId;
                 for (uint256 i = 1; i < numServices; ++i) {
                     // Calculate the updated reward
                     updatedReward = (eligibleServiceRewards[i] * lastAvailableRewards) / totalRewards;
@@ -419,7 +453,7 @@ abstract contract ServiceStakingBase is ERC721TokenReceiver, IErrorsRegistries {
                 // Traverse all the eligible services and add to their rewards
                 for (uint256 i = 0; i < numServices; ++i) {
                     // Add reward to the service overall reward
-                    uint256 curServiceId = eligibleServiceIds[i];
+                    curServiceId = eligibleServiceIds[i];
                     mapServiceInfo[curServiceId].reward += eligibleServiceRewards[i];
                 }
 
@@ -455,7 +489,7 @@ abstract contract ServiceStakingBase is ERC721TokenReceiver, IErrorsRegistries {
     /// @dev Unstakes the service.
     /// @param serviceId Service Id.
     function unstake(uint256 serviceId) external {
-        ServiceInfo memory sInfo = mapServiceInfo[serviceId];
+        ServiceInfo storage sInfo = mapServiceInfo[serviceId];
         // Check for the service ownership
         if (msg.sender != sInfo.owner) {
             revert OwnerOnly(msg.sender, sInfo.owner);
@@ -473,13 +507,11 @@ abstract contract ServiceStakingBase is ERC721TokenReceiver, IErrorsRegistries {
             }
         }
 
-        // Transfer the service back to the owner
-        IService(serviceRegistry).safeTransferFrom(address(this), msg.sender, serviceId);
-
-        // Transfer accumulated rewards to the service multisig
-        if (sInfo.reward > 0) {
-            _withdraw(sInfo.multisig, sInfo.reward);
-        }
+        // Get the unstaked service data
+        uint256 reward = sInfo.reward;
+        uint256 nonce = sInfo.nonce;
+        uint256 tsStart = sInfo.tsStart;
+        address multisig = sInfo.multisig;
 
         // Clear all the data about the unstaked service
         // Delete the service info struct
@@ -489,7 +521,15 @@ abstract contract ServiceStakingBase is ERC721TokenReceiver, IErrorsRegistries {
         setServiceIds[idx] = setServiceIds[setServiceIds.length - 1];
         setServiceIds.pop();
 
-        emit ServiceUnstaked(serviceId, msg.sender, sInfo.multisig, sInfo.nonce, sInfo.reward, sInfo.tsStart);
+        // Transfer the service back to the owner
+        IService(serviceRegistry).safeTransferFrom(address(this), msg.sender, serviceId);
+
+        // Transfer accumulated rewards to the service multisig
+        if (reward > 0) {
+            _withdraw(multisig, reward);
+        }
+
+        emit ServiceUnstaked(serviceId, msg.sender, multisig, nonce, reward, tsStart);
     }
 
     /// @dev Calculates service staking reward at current timestamp.

contracts/staking/ServiceStakingNativeToken.sol

@@ -11,8 +11,9 @@ contract ServiceStakingNativeToken is ServiceStakingBase {
     /// @dev ServiceStakingNativeToken constructor.
     /// @param _stakingParams Service staking parameters.
     /// @param _serviceRegistry ServiceRegistry contract address.
-    constructor(StakingParams memory _stakingParams, address _serviceRegistry)
-        ServiceStakingBase(_stakingParams, _serviceRegistry)
+    /// @param _multisigProxyAddresses Multisig proxy addresses.
+    constructor(StakingParams memory _stakingParams, address _serviceRegistry, address[] memory _multisigProxyAddresses)
+        ServiceStakingBase(_stakingParams, _serviceRegistry, _multisigProxyAddresses)
     {}
 
     /// @dev Withdraws the reward amount to a service owner.

contracts/staking/ServiceStakingToken.sol

@@ -44,13 +44,15 @@ contract ServiceStakingToken is ServiceStakingBase {
     /// @param _serviceRegistry ServiceRegistry contract address.
     /// @param _serviceRegistryTokenUtility ServiceRegistryTokenUtility contract address.
     /// @param _stakingToken Address of a service staking token.
+    /// @param _multisigProxyAddresses Multisig proxy addresses.
     constructor(
         StakingParams memory _stakingParams,
         address _serviceRegistry,
         address _serviceRegistryTokenUtility,
-        address _stakingToken
+        address _stakingToken,
+        address[] memory _multisigProxyAddresses
     )
-        ServiceStakingBase(_stakingParams, _serviceRegistry)
+        ServiceStakingBase(_stakingParams, _serviceRegistry, _multisigProxyAddresses)
     {
         // Initial checks
         if (_stakingToken == address(0) || _serviceRegistryTokenUtility == address(0)) {
@@ -94,9 +96,6 @@ contract ServiceStakingToken is ServiceStakingBase {
     /// @dev Deposits funds for staking.
     /// @param amount Token amount to deposit.
     function deposit(uint256 amount) external {
-        // Add to the overall balance
-        SafeTransferLib.safeTransferFrom(stakingToken, msg.sender, address(this), amount);
-
         // Add to the contract and available rewards balances
         uint256 newBalance = balance + amount;
         uint256 newAvailableRewards = availableRewards + amount;
@@ -105,6 +104,9 @@ contract ServiceStakingToken is ServiceStakingBase {
         balance = newBalance;
         availableRewards = newAvailableRewards;
 
+        // Add to the overall balance
+        SafeTransferLib.safeTransferFrom(stakingToken, msg.sender, address(this), amount);
+
         emit Deposit(msg.sender, amount, newBalance, newAvailableRewards);
     }
 }

contracts/test/ReentrancyStakingAttacker.sol

@@ -44,6 +44,10 @@ contract ReentrancyStakingAttacker is ERC721TokenReceiver {
     address public immutable serviceRegistry;
     // Attack argument
     bool public attack = true;
+    // Nonce
+    uint256 internal _nonce;
+    // Owners
+    address[] public owners;
 
     constructor(address _serviceStaking, address _serviceRegistry) {
         serviceStaking = _serviceStaking;
@@ -59,6 +63,14 @@ contract ReentrancyStakingAttacker is ERC721TokenReceiver {
         attack = status;
     }
 
+    function setOwner(address owner) external {
+        owners.push(owner);
+    }
+
+    function inceraseNonce() external {
+        _nonce += 1000;
+    }
+
     /// @dev Malicious contract function call during the service token return.
     function onERC721Received(address, address, uint256 serviceId, bytes memory) public virtual override returns (bytes4) {
         if (attack) {
@@ -78,4 +90,22 @@ contract ReentrancyStakingAttacker is ERC721TokenReceiver {
         IServiceStaking(serviceStaking).stake(serviceId);
         IServiceStaking(serviceStaking).checkpoint();
     }
+
+    /// @dev Gets set of owners.
+    /// @return owners Set of Safe owners.
+    function getOwners() external view returns (address[] memory) {
+        return owners;
+    }
+
+    /// @dev Gets threshold.
+    /// @return Threshold
+    function getThreshold() external view returns (uint256) {
+        return owners.length;
+    }
+
+    /// @dev Gets the multisig nonce.
+    /// @return Multisig nonce.
+    function nonce() external view returns (uint256) {
+        return _nonce;
+    }
 }