fix: only check conditions for param that is changing when changing provision params (OZ L-05)

Signed-off-by: Tomás Migone <[email protected]>

Author: tmigone

packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol

@@ -700,10 +700,15 @@ interface IHorizonStakingMain {
 
     /**
      * @notice Stages a provision parameter update. Note that the change is not effective until the verifier calls
-     * {acceptProvisionParameters}.
+     * {acceptProvisionParameters}. Calling this function is a no-op if the new parameters are the same as the current
+     * ones.
      * @dev This two step update process prevents the service provider from changing the parameters
      * without the verifier's consent.
      *
+     * Requirements:
+     * - `thawingPeriod` must be less than or equal to `_maxThawingPeriod`. Note that if `_maxThawingPeriod` changes the
+     * function will not revert if called with the same thawing period as the current one.
+     *
      * Emits a {ProvisionParametersStaged} event if at least one of the parameters changed.
      *
      * @param serviceProvider The service provider address

packages/horizon/contracts/staking/HorizonStaking.sol

@@ -219,19 +219,26 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
         uint32 newMaxVerifierCut,
         uint64 newThawingPeriod
     ) external override notPaused onlyAuthorized(serviceProvider, verifier) {
-        require(PPMMath.isValidPPM(newMaxVerifierCut), HorizonStakingInvalidMaxVerifierCut(newMaxVerifierCut));
-        require(
-            newThawingPeriod <= _maxThawingPeriod,
-            HorizonStakingInvalidThawingPeriod(newThawingPeriod, _maxThawingPeriod)
-        );
-
         // Provision must exist
         Provision storage prov = _provisions[serviceProvider][verifier];
         require(prov.createdAt != 0, HorizonStakingInvalidProvision(serviceProvider, verifier));
 
-        if ((prov.maxVerifierCutPending != newMaxVerifierCut) || (prov.thawingPeriodPending != newThawingPeriod)) {
-            prov.maxVerifierCutPending = newMaxVerifierCut;
-            prov.thawingPeriodPending = newThawingPeriod;
+        bool verifierCutChanged = prov.maxVerifierCutPending != newMaxVerifierCut;
+        bool thawingPeriodChanged = prov.thawingPeriodPending != newThawingPeriod;
+
+        if (verifierCutChanged || thawingPeriodChanged) {
+            if (verifierCutChanged) {
+                require(PPMMath.isValidPPM(newMaxVerifierCut), HorizonStakingInvalidMaxVerifierCut(newMaxVerifierCut));
+                prov.maxVerifierCutPending = newMaxVerifierCut;
+            }
+            if (thawingPeriodChanged) {
+                require(
+                    newThawingPeriod <= _maxThawingPeriod,
+                    HorizonStakingInvalidThawingPeriod(newThawingPeriod, _maxThawingPeriod)
+                );
+                prov.thawingPeriodPending = newThawingPeriod;
+            }
+
             prov.lastParametersStagedAt = block.timestamp;
             emit ProvisionParametersStaged(serviceProvider, verifier, newMaxVerifierCut, newThawingPeriod);
         }

packages/horizon/test/staking/provision/parameters.t.sol

@@ -70,6 +70,41 @@ contract HorizonStakingProvisionParametersTest is HorizonStakingTest {
         staking.setProvisionParameters(users.indexer, subgraphDataServiceAddress, maxVerifierCut, thawingPeriod);
     }
 
+    function test_ProvisionParametersSet_MaxMaxThawingPeriodChanged(
+        uint256 amount,
+        uint32 maxVerifierCut,
+        uint64 thawingPeriod
+    ) public useIndexer {
+        vm.assume(amount > 0);
+        vm.assume(amount <= MAX_STAKING_TOKENS);
+        vm.assume(maxVerifierCut <= MAX_PPM);
+        
+        // create provision with initial parameters
+        uint32 initialMaxVerifierCut = 1000;
+        uint64 initialThawingPeriod = 14 days;  // Max thawing period is 28 days
+        _createProvision(users.indexer, subgraphDataServiceAddress, amount, initialMaxVerifierCut, initialThawingPeriod);
+
+        // change the max thawing period allowed so that the initial thawing period is not valid anymore
+        uint64 newMaxThawingPeriod = 7 days;
+        resetPrank(users.governor);
+        _setMaxThawingPeriod(newMaxThawingPeriod);
+
+        // set the verifier cut to a new value - keep the thawing period the same, it should be allowed
+        resetPrank(users.indexer);
+        _setProvisionParameters(users.indexer, subgraphDataServiceAddress, maxVerifierCut, initialThawingPeriod);
+
+        // now try to change the thawing period to a new value that is invalid
+        vm.assume(thawingPeriod > initialThawingPeriod);
+        vm.expectRevert(
+            abi.encodeWithSelector(
+                IHorizonStakingMain.HorizonStakingInvalidThawingPeriod.selector,
+                thawingPeriod,
+                newMaxThawingPeriod
+            )
+        );
+        staking.setProvisionParameters(users.indexer, subgraphDataServiceAddress, maxVerifierCut, thawingPeriod);
+    }
+
     function test_ProvisionParametersAccept(
         uint256 amount,
         uint32 maxVerifierCut,