[C03] Updating or removing a service type causes critical accounting errors

eth-contracts/contracts/ServiceProviderFactory.sol

@@ -189,9 +189,9 @@ contract ServiceProviderFactory is InitializableV2 {
         );
 
         // Update min and max totals for this service provider
-        (uint typeMin, uint typeMax) = ServiceTypeManager(
+        (, uint typeMin, uint typeMax) = ServiceTypeManager(
             serviceTypeManagerAddress
-        ).getServiceTypeStakeInfo(_serviceType);
+        ).getServiceTypeInfo(_serviceType);
         spDetails[msg.sender].minAccountStake = spDetails[msg.sender].minAccountStake.add(typeMin);
         spDetails[msg.sender].maxAccountStake = spDetails[msg.sender].maxAccountStake.add(typeMax);
 
@@ -281,9 +281,9 @@ contract ServiceProviderFactory is InitializableV2 {
         spDetails[msg.sender].numberOfEndpoints -= 1;
 
         // Update min and max totals for this service provider
-        (uint typeMin, uint typeMax) = ServiceTypeManager(
+        (, uint typeMin, uint typeMax) = ServiceTypeManager(
             serviceTypeManagerAddress
-        ).getServiceTypeStakeInfo(_serviceType);
+        ).getServiceTypeInfo(_serviceType);
         spDetails[msg.sender].minAccountStake = spDetails[msg.sender].minAccountStake.sub(typeMin);
         spDetails[msg.sender].maxAccountStake = spDetails[msg.sender].maxAccountStake.sub(typeMax);
 

eth-contracts/contracts/ServiceTypeManager.sol

@@ -21,14 +21,15 @@ contract ServiceTypeManager is InitializableV2 {
     /// @dev List of valid service types
     bytes32[] private validServiceTypes;
 
-    /// @dev Struct representing service type stake requirements
-    struct ServiceTypeStakeRequirements {
+    /// @dev Struct representing service type info
+    struct ServiceTypeInfo {
+        bool isValid;
         uint minStake;
         uint maxStake;
     }
 
-    /// @dev mapping of service type to registered requirements
-    mapping(bytes32 => ServiceTypeStakeRequirements) serviceTypeStakeRequirements;
+    /// @dev mapping of service type info
+    mapping(bytes32 => ServiceTypeInfo) serviceTypeInfo;
 
     event SetServiceVersion(bytes32 _serviceType, bytes32 _serviceVersion);
     event Test(string msg, bool value);
@@ -61,7 +62,6 @@ contract ServiceTypeManager is InitializableV2 {
 
     // ========================================= Service Type Logic =========================================
 
-    /// @notice Add a new service type
     /**
      * @notice Add a new service type
      * @param _serviceType - type of service to add
@@ -78,9 +78,21 @@ contract ServiceTypeManager is InitializableV2 {
 
         require(msg.sender == governanceAddress, "Only callable by Governance contract");
         require(!this.serviceTypeIsValid(_serviceType), "Already known service type");
+        require(
+            _serviceTypeMax > _serviceTypeMin,
+            "Max stake must be non-zero and greater than min stake"
+        );
+
+        // Ensure serviceType cannot be re-added if it previously existed and was removed
+        // stored maxStake > 0 means it was previously added and removed
+        require(
+            serviceTypeInfo[_serviceType].maxStake == 0,
+            "Cannot re-add serviceType after it was removed."
+        );
 
         validServiceTypes.push(_serviceType);
-        serviceTypeStakeRequirements[_serviceType] = ServiceTypeStakeRequirements({
+        serviceTypeInfo[_serviceType] = ServiceTypeInfo({
+            isValid: true,
             minStake: _serviceTypeMin,
             maxStake: _serviceTypeMax
         });
@@ -109,46 +121,24 @@ contract ServiceTypeManager is InitializableV2 {
         uint lastIndex = validServiceTypes.length - 1;
         validServiceTypes[serviceIndex] = validServiceTypes[lastIndex];
         validServiceTypes.length--;
-        // Overwrite values
-        serviceTypeStakeRequirements[_serviceType].minStake = 0;
-        serviceTypeStakeRequirements[_serviceType].maxStake = 0;
-    }
-
-    /**
-     * @notice Update a service type
-     * @param _serviceType - type of service
-     * @param _serviceTypeMin - minimum stake for service type
-     * @param _serviceTypeMax - maximum stake for service type
-     */
-    function updateServiceType(
-        bytes32 _serviceType,
-        uint _serviceTypeMin,
-        uint _serviceTypeMax
-    ) external
-    {
-        _requireIsInitialized();
-        require(
-            msg.sender == governanceAddress,
-            "Only callable by Governance contract"
-        );
-
-        require(this.serviceTypeIsValid(_serviceType), "Invalid service type");
 
-        serviceTypeStakeRequirements[_serviceType].minStake = _serviceTypeMin;
-        serviceTypeStakeRequirements[_serviceType].maxStake = _serviceTypeMax;
+        // Mark as invalid
+        serviceTypeInfo[_serviceType].isValid = false;
+        // Note - stake bounds are not reset so they can be checked to prevent serviceType from being re-added
     }
 
     /**
-     * @notice Get min and max stake for a given service type
+     * @notice Get isValid, min and max stake for a given service type
      * @param _serviceType - type of service
-     * @return min and max stake for type
+     * @return isValid, min and max stake for type
      */
-    function getServiceTypeStakeInfo(bytes32 _serviceType)
-    external view returns (uint min, uint max)
+    function getServiceTypeInfo(bytes32 _serviceType)
+    external view returns (bool isValid, uint minStake, uint maxStake)
     {
         return (
-            serviceTypeStakeRequirements[_serviceType].minStake,
-            serviceTypeStakeRequirements[_serviceType].maxStake
+            serviceTypeInfo[_serviceType].isValid,
+            serviceTypeInfo[_serviceType].minStake,
+            serviceTypeInfo[_serviceType].maxStake
         );
     }
 
@@ -167,7 +157,7 @@ contract ServiceTypeManager is InitializableV2 {
     function serviceTypeIsValid(bytes32 _serviceType)
     external view returns (bool isValid)
     {
-        return serviceTypeStakeRequirements[_serviceType].maxStake > 0;
+        return serviceTypeInfo[_serviceType].isValid;
     }
 
     // ========================================= Service Version Logic =========================================
@@ -191,7 +181,7 @@ contract ServiceTypeManager is InitializableV2 {
             "Already registered"
         );
 
-         // Update array of known types
+         // Update array of known versions for type
         serviceTypeVersions[_serviceType].push(_serviceVersion);
 
         // Update status for this specific service version

eth-contracts/migrations/7_versioning_service_migration.js

@@ -82,10 +82,9 @@ module.exports = (deployer, network, accounts) => {
       callDataCN,
       { from: guardianAddress }
     )
-    const serviceTypeCNStakeInfo = await serviceTypeManager.getServiceTypeStakeInfo.call(serviceTypeCN)
-    const [cnTypeMinV, cnTypeMaxV] = [serviceTypeCNStakeInfo[0], serviceTypeCNStakeInfo[1]]
-    assert.ok(_lib.toBN(cnTypeMin).eq(cnTypeMinV), 'Expected same minStake')
-    assert.ok(_lib.toBN(cnTypeMax).eq(cnTypeMaxV), 'Expected same max Stake')
+    const serviceTypeCNInfo = await serviceTypeManager.getServiceTypeInfo.call(serviceTypeCN)
+    assert.ok(_lib.toBN(cnTypeMin).eq(serviceTypeCNInfo.minStake), 'Expected same minStake')
+    assert.ok(_lib.toBN(cnTypeMax).eq(serviceTypeCNInfo.maxStake), 'Expected same max Stake')
 
     const callDataDP = _lib.abiEncode(
       ['bytes32', 'uint256', 'uint256'],
@@ -98,10 +97,9 @@ module.exports = (deployer, network, accounts) => {
       callDataDP,
       { from: guardianAddress }
     )
-    const serviceTypeDPStakeInfo = await serviceTypeManager.getServiceTypeStakeInfo.call(serviceTypeDP)
-    const [dpTypeMinV, dpTypeMaxV] = [serviceTypeDPStakeInfo[0], serviceTypeDPStakeInfo[1]]
-    assert.ok(_lib.toBN(dpTypeMin).eq(dpTypeMinV), 'Expected same minStake')
-    assert.ok(_lib.toBN(dpTypeMax).eq(dpTypeMaxV), 'Expected same maxStake')
+    const serviceTypeDPInfo = await serviceTypeManager.getServiceTypeInfo.call(serviceTypeDP)
+    assert.ok(_lib.toBN(dpTypeMin).eq(serviceTypeDPInfo.minStake), 'Expected same minStake')
+    assert.ok(_lib.toBN(dpTypeMax).eq(serviceTypeDPInfo.maxStake), 'Expected same maxStake')
 
     // Deploy ServiceProviderFactory logic and proxy contracts + register proxy
     const serviceProviderFactory0 = await deployer.deploy(ServiceProviderFactory, { from: proxyDeployerAddress })

eth-contracts/test/delegateManager.test.js

@@ -17,6 +17,8 @@ const delegateManagerKey = web3.utils.utf8ToHex('DelegateManager')
 const tokenRegKey = web3.utils.utf8ToHex('Token')
 
 const testDiscProvType = web3.utils.utf8ToHex('discovery-provider')
+const serviceTypeMinStake = _lib.audToWei(5)
+const serviceTypeMaxStake = _lib.audToWei(10000000)
 const testEndpoint = 'https://localhost:5000'
 const testEndpoint1 = 'https://localhost:5001'
 const testEndpoint3 = 'https://localhost:5002'
@@ -32,7 +34,7 @@ const callValue0 = _lib.toBN(0)
 
 contract('DelegateManager', async (accounts) => {
   let staking, stakingAddress, token, registry, governance, claimsManager0, claimsManagerProxy
-  let serviceProviderFactory, claimsManager, delegateManager
+  let serviceProviderFactory, serviceTypeManager, claimsManager, delegateManager
 
   // intentionally not using acct0 to make sure no TX accidentally succeeds without specifying sender
   const [, proxyAdminAddress, proxyDeployerAddress] = accounts
@@ -43,6 +45,10 @@ contract('DelegateManager', async (accounts) => {
   const delegatorAccount1 = accounts[11]
   const slasherAccount = stakerAccount
 
+  /**
+   * Initialize Registry, Governance, Token, Staking, ServiceTypeManager, ServiceProviderFactory, ClaimsManager, DelegateManager
+   * Register discprov serviceType
+   */
   beforeEach(async () => {
     registry = await _lib.deployRegistry(artifacts, proxyAdminAddress, proxyDeployerAddress)
 
@@ -99,10 +105,10 @@ contract('DelegateManager', async (accounts) => {
       { from: proxyDeployerAddress }
     )
     await registry.addContract(serviceTypeManagerProxyKey, serviceTypeManagerProxy.address, { from: proxyDeployerAddress })
-    let serviceTypeManager = await ServiceTypeManager.at(serviceTypeManagerProxy.address)
+    serviceTypeManager = await ServiceTypeManager.at(serviceTypeManagerProxy.address)
 
     // Register discprov serviceType
-    await _lib.addServiceType(testDiscProvType, _lib.audToWei(5), _lib.audToWei(10000000), governance, guardianAddress, serviceTypeManagerProxyKey)
+    await _lib.addServiceType(testDiscProvType, serviceTypeMinStake, serviceTypeMaxStake, governance, guardianAddress, serviceTypeManagerProxyKey)
 
     // Deploy ServiceProviderFactory
     let serviceProviderFactory0 = await ServiceProviderFactory.new({ from: proxyDeployerAddress })
@@ -311,6 +317,11 @@ contract('DelegateManager', async (accounts) => {
   describe('Delegation tests', () => {
     let regTx
 
+    /**
+     * Transfer tokens to stakers & delegator
+     * Register service providers
+     * Update SP deployer cuts
+     */
     beforeEach(async () => {
       // Transfer 1000 tokens to stakers
       await token.transfer(stakerAccount, INITIAL_BAL, { from: proxyDeployerAddress })
@@ -467,7 +478,7 @@ contract('DelegateManager', async (accounts) => {
       await time.advanceBlockTo(undelegateRequestInfo.lockupExpiryBlock)
 
       // Undelegate stake
-      delegateManager.undelegateStake({ from: delegatorAccount1 })
+      await delegateManager.undelegateStake({ from: delegatorAccount1 })
 
       // Confirm all state change operations have occurred
       undelegateRequestInfo = await delegateManager.getPendingUndelegateRequest(delegatorAccount1)
@@ -1537,5 +1548,115 @@ contract('DelegateManager', async (accounts) => {
         )
       })
     })
+
+    it('Delegate ops after serviceType removal', async () => {
+      /**
+       * Confirm initial state of serviceType and serviceProvider
+       * Delegate stake to Staker
+       * Remove serviceType
+       * Confirm new state of serviceType and serviceProvider
+       * Confirm delegation to SP still works
+       * Deregister SP of serviceType
+       * Undelegate stake from Staker
+       * Confirm new delegation state
+       */
+
+      const minStakeBN = _lib.toBN(serviceTypeMinStake)
+      const maxStakeBN = _lib.toBN(serviceTypeMaxStake)
+
+      // Confirm initial serviceType info
+      const stakeInfo0 = await serviceTypeManager.getServiceTypeInfo.call(testDiscProvType)
+      assert.isTrue(stakeInfo0.isValid, 'Expected isValid == true')
+      assert.isTrue(stakeInfo0.minStake.eq(minStakeBN), 'Expected same minStake')
+      assert.isTrue(stakeInfo0.maxStake.eq(maxStakeBN), 'Expected same maxStake')
+
+      // Confirm initial SP details
+      const spDetails0 = await serviceProviderFactory.getServiceProviderDetails.call(stakerAccount)
+      assert.isTrue(spDetails0.deployerStake.eq(DEFAULT_AMOUNT), 'Expected deployerStake == default amount')
+      assert.isTrue(spDetails0.validBounds, 'Expected validBounds == true')
+      assert.isTrue(spDetails0.numberOfEndpoints.eq(_lib.toBN(1)), 'Expected one endpoint')
+      assert.isTrue(spDetails0.minAccountStake.eq(minStakeBN), 'Expected minAccountStake == dpTypeMin')
+      assert.isTrue(spDetails0.maxAccountStake.eq(maxStakeBN), 'Expected maxAccountStake == dpTypeMax')
+
+      // Delegate stake to Staker
+      const delegationAmount = _lib.audToWeiBN(60)
+      await token.approve(
+        stakingAddress,
+        delegationAmount,
+        { from: delegatorAccount1 }
+      )
+      await delegateManager.delegateStake(
+        stakerAccount,
+        delegationAmount,
+        { from: delegatorAccount1 }
+      )
+
+      // Remove serviceType
+      await governance.guardianExecuteTransaction(
+        serviceTypeManagerProxyKey,
+        callValue0,
+        'removeServiceType(bytes32)',
+        _lib.abiEncode(['bytes32'], [testDiscProvType]),
+        { from: guardianAddress }
+      )
+
+      // Confirm serviceType info is changed after serviceType removal
+      const stakeInfo1 = await serviceTypeManager.getServiceTypeInfo.call(testDiscProvType)
+      assert.isFalse(stakeInfo1.isValid, 'Expected isValid == false')
+      assert.isTrue(stakeInfo1.minStake.eq(minStakeBN), 'Expected same minStake')
+      assert.isTrue(stakeInfo1.maxStake.eq(maxStakeBN), 'Expected same maxStake')
+
+      // Confirm SP details are unchanged after serviceType removal
+      const spDetails = await serviceProviderFactory.getServiceProviderDetails.call(stakerAccount)
+      assert.isTrue(spDetails.deployerStake.eq(DEFAULT_AMOUNT), 'Expected deployerStake == default amount')
+      assert.isTrue(spDetails.validBounds, 'Expected validBounds == true')
+      assert.isTrue(spDetails.numberOfEndpoints.eq(_lib.toBN(1)), 'Expected one endpoint')
+      assert.isTrue(spDetails.minAccountStake.eq(minStakeBN), 'Expected minAccountStake == dpTypeMin')
+      assert.isTrue(spDetails.maxAccountStake.eq(maxStakeBN), 'Expected maxAccountStake == dpTypeMax')
+
+      // Confirm delegation to SP still works after serviceType removal
+      await token.approve(
+        stakingAddress,
+        delegationAmount,
+        { from: delegatorAccount1 }
+      )
+      await delegateManager.delegateStake(
+        stakerAccount,
+        delegationAmount,
+        { from: delegatorAccount1 }
+      )
+      const totalDelegationAmount = delegationAmount.add(delegationAmount)
+
+      // Deregister SP + unstake
+      await _lib.deregisterServiceProvider(
+        serviceProviderFactory,
+        testDiscProvType,
+        testEndpoint,
+        stakerAccount
+      )
+      const deregisterRequestInfo = await serviceProviderFactory.getPendingDecreaseStakeRequest.call(stakerAccount)
+      await time.advanceBlockTo(deregisterRequestInfo.lockupExpiryBlock)
+      await serviceProviderFactory.decreaseStake({ from: stakerAccount })
+
+      // Undelegate total amount
+      await delegateManager.requestUndelegateStake(
+        stakerAccount,
+        totalDelegationAmount,
+        { from: delegatorAccount1 }
+      )
+      const undelegateRequestInfo = await delegateManager.getPendingUndelegateRequest(delegatorAccount1)
+      await time.advanceBlockTo(undelegateRequestInfo.lockupExpiryBlock)
+      await delegateManager.undelegateStake({ from: delegatorAccount1 })
+
+      // Confirm delegation state
+      const totalStakedForSP = await staking.totalStakedFor(stakerAccount)
+      assert.isTrue(totalStakedForSP.isZero(), 'Expected totalStaked for SP == 0')
+      const delegators = await delegateManager.getDelegatorsList(stakerAccount)
+      assert.equal(delegators.length, 0, 'Expect delegators list length == 0')
+      const delegatedStake = await delegateManager.getTotalDelegatorStake(delegatorAccount1)
+      assert.isTrue(delegatedStake.isZero(), 'Expected delegatedStake == 0')
+      const totalLockedDelegationForSP = await delegateManager.getTotalLockedDelegationForServiceProvider(stakerAccount)
+      assert.isTrue(totalLockedDelegationForSP.isZero(), 'Expected totalLockedDelegationForSP == 0')
+    })
   })
 })