fix(Horizon): add input validation for set provision parameters (#1055)

* fix(Horizon): add input validation for set provision parameters

* fix: subgraph service tests

* fix: use new parameter names

* fix: subgraph-service tests

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

@@ -341,9 +341,8 @@ interface IHorizonStakingMain {
     /**
      * @notice Thrown when attempting to create a provision with an invalid maximum verifier cut.
      * @param maxVerifierCut The maximum verifier cut
-     * @param maxMaxVerifierCut The maximum `maxVerifierCut` allowed
      */
-    error HorizonStakingInvalidMaxVerifierCut(uint32 maxVerifierCut, uint32 maxMaxVerifierCut);
+    error HorizonStakingInvalidMaxVerifierCut(uint32 maxVerifierCut);
 
     /**
      * @notice Thrown when attempting to create a provision with an invalid thawing period.
@@ -528,7 +527,7 @@ interface IHorizonStakingMain {
      * @dev Requirements:
      * - `tokens` cannot be zero.
      * - The `serviceProvider` must have enough idle stake to cover the tokens to provision.
-     * - `maxVerifierCut` must be less than or equal to `MAX_MAX_VERIFIER_CUT`.
+     * - `maxVerifierCut` must be a valid PPM.
      * - `thawingPeriod` must be less than or equal to `_maxThawingPeriod`.
      *
      * Emits a {ProvisionCreated} event.

packages/horizon/contracts/staking/HorizonStaking.sol

@@ -31,10 +31,6 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
     using PPMMath for uint256;
     using LinkedList for LinkedList.List;
 
-    /// @dev Maximum value that can be set as the maxVerifierCut in a provision.
-    /// It is equivalent to 100% in parts-per-million
-    uint32 private constant MAX_MAX_VERIFIER_CUT = uint32(PPMMath.MAX_PPM);
-
     /// @dev Fixed point precision
     uint256 private constant FIXED_POINT_PRECISION = 1e18;
 
@@ -224,6 +220,11 @@ 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 storage prov = _provisions[serviceProvider][verifier];
         require(prov.createdAt != 0, HorizonStakingInvalidProvision(serviceProvider, verifier));
 
@@ -626,10 +627,7 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
         uint64 _thawingPeriod
     ) private {
         require(_tokens > 0, HorizonStakingInvalidZeroTokens());
-        require(
-            _maxVerifierCut <= MAX_MAX_VERIFIER_CUT,
-            HorizonStakingInvalidMaxVerifierCut(_maxVerifierCut, MAX_MAX_VERIFIER_CUT)
-        );
+        require(PPMMath.isValidPPM(_maxVerifierCut), HorizonStakingInvalidMaxVerifierCut(_maxVerifierCut));
         require(
             _thawingPeriod <= _maxThawingPeriod,
             HorizonStakingInvalidThawingPeriod(_thawingPeriod, _maxThawingPeriod)

packages/horizon/test/shared/horizon-staking/HorizonStakingShared.t.sol

@@ -81,7 +81,7 @@ abstract contract HorizonStakingSharedTest is GraphBaseTest {
         // use assume instead of bound to avoid the bounding falling out of scope
         vm.assume(tokens > 0);
         vm.assume(tokens <= MAX_STAKING_TOKENS);
-        vm.assume(maxVerifierCut <= MAX_MAX_VERIFIER_CUT);
+        vm.assume(maxVerifierCut <= MAX_PPM);
         vm.assume(thawingPeriod <= MAX_THAWING_PERIOD);
 
         _createProvision(users.indexer, dataService, tokens, maxVerifierCut, thawingPeriod);

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

@@ -24,7 +24,7 @@ contract HorizonStakingProvisionLockedTest is HorizonStakingTest {
             users.indexer,
             subgraphDataServiceAddress,
             amount,
-            MAX_MAX_VERIFIER_CUT,
+            MAX_PPM,
             MAX_THAWING_PERIOD
         );
 
@@ -52,7 +52,7 @@ contract HorizonStakingProvisionLockedTest is HorizonStakingTest {
             users.indexer,
             subgraphDataServiceAddress,
             amount,
-            MAX_MAX_VERIFIER_CUT,
+            MAX_PPM,
             MAX_THAWING_PERIOD
         );
     }
@@ -75,7 +75,7 @@ contract HorizonStakingProvisionLockedTest is HorizonStakingTest {
             users.indexer,
             subgraphDataServiceAddress,
             amount,
-            MAX_MAX_VERIFIER_CUT,
+            MAX_PPM,
             MAX_THAWING_PERIOD
         );
     }

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

@@ -7,6 +7,17 @@ import { HorizonStakingTest } from "../HorizonStaking.t.sol";
 import { IHorizonStakingMain } from "../../../contracts/interfaces/internal/IHorizonStakingMain.sol";
 
 contract HorizonStakingProvisionParametersTest is HorizonStakingTest {
+
+    /*
+     * MODIFIERS
+     */
+
+    modifier useValidParameters(uint32 maxVerifierCut, uint64 thawingPeriod) {
+        vm.assume(maxVerifierCut <= MAX_PPM);
+        vm.assume(thawingPeriod <= MAX_THAWING_PERIOD);
+        _;
+    }
+
     /*
      * TESTS
      */
@@ -15,14 +26,14 @@ contract HorizonStakingProvisionParametersTest is HorizonStakingTest {
         uint256 amount,
         uint32 maxVerifierCut,
         uint64 thawingPeriod
-    ) public useIndexer useProvision(amount, 0, 0) {
+    ) public useIndexer useProvision(amount, 0, 0) useValidParameters(maxVerifierCut, thawingPeriod) {
         _setProvisionParameters(users.indexer, subgraphDataServiceAddress, maxVerifierCut, thawingPeriod);
     }
 
     function test_ProvisionParametersSet_RevertWhen_ProvisionNotExists(
         uint32 maxVerifierCut,
         uint64 thawingPeriod
-    ) public useIndexer {
+    ) public useIndexer useValidParameters(maxVerifierCut, thawingPeriod) {
         vm.expectRevert(
             abi.encodeWithSignature(
                 "HorizonStakingInvalidProvision(address,address)",
@@ -71,4 +82,37 @@ contract HorizonStakingProvisionParametersTest is HorizonStakingTest {
         _acceptProvisionParameters(users.indexer);
         vm.stopPrank();
     }
+
+    function test_ProvisionParameters_RevertIf_InvalidMaxVerifierCut(
+        uint256 amount,
+        uint32 maxVerifierCut,
+        uint64 thawingPeriod
+    ) public useIndexer useProvision(amount, maxVerifierCut, thawingPeriod) {
+        maxVerifierCut = uint32(bound(maxVerifierCut, MAX_PPM + 1, type(uint32).max));
+        vm.assume(thawingPeriod <= MAX_THAWING_PERIOD);
+        vm.expectRevert(
+            abi.encodeWithSelector(
+                IHorizonStakingMain.HorizonStakingInvalidMaxVerifierCut.selector,
+                maxVerifierCut
+            )
+        );
+        staking.setProvisionParameters(users.indexer, subgraphDataServiceAddress, maxVerifierCut, thawingPeriod);
+    }
+
+    function test_ProvisionParameters_RevertIf_InvalidThawingPeriod(
+        uint256 amount,
+        uint32 maxVerifierCut,
+        uint64 thawingPeriod
+    ) public useIndexer useProvision(amount, maxVerifierCut, thawingPeriod) {
+        vm.assume(maxVerifierCut <= MAX_PPM);
+        thawingPeriod = uint64(bound(thawingPeriod, MAX_THAWING_PERIOD + 1, type(uint64).max));
+        vm.expectRevert(
+            abi.encodeWithSelector(
+                IHorizonStakingMain.HorizonStakingInvalidThawingPeriod.selector,
+                thawingPeriod,
+                MAX_THAWING_PERIOD
+            )
+        );
+        staking.setProvisionParameters(users.indexer, subgraphDataServiceAddress, maxVerifierCut, thawingPeriod);
+    }
 }

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

@@ -12,7 +12,7 @@ contract HorizonStakingProvisionTest is HorizonStakingTest {
 
     function testProvision_Create(uint256 tokens, uint32 maxVerifierCut, uint64 thawingPeriod) public useIndexer {
         tokens = bound(tokens, 1, MAX_STAKING_TOKENS);
-        maxVerifierCut = uint32(bound(maxVerifierCut, 0, MAX_MAX_VERIFIER_CUT));
+        maxVerifierCut = uint32(bound(maxVerifierCut, 0, MAX_PPM));
         thawingPeriod = uint32(bound(thawingPeriod, 0, MAX_THAWING_PERIOD));
 
         _createProvision(users.indexer, subgraphDataServiceAddress, tokens, maxVerifierCut, thawingPeriod);
@@ -28,11 +28,10 @@ contract HorizonStakingProvisionTest is HorizonStakingTest {
         uint256 amount,
         uint32 maxVerifierCut
     ) public useIndexer useStake(amount) {
-        vm.assume(maxVerifierCut > MAX_MAX_VERIFIER_CUT);
+        vm.assume(maxVerifierCut > MAX_PPM);
         bytes memory expectedError = abi.encodeWithSignature(
-            "HorizonStakingInvalidMaxVerifierCut(uint32,uint32)",
-            maxVerifierCut,
-            MAX_MAX_VERIFIER_CUT
+            "HorizonStakingInvalidMaxVerifierCut(uint32)",
+            maxVerifierCut
         );
         vm.expectRevert(expectedError);
         staking.provision(users.indexer, subgraphDataServiceAddress, amount, maxVerifierCut, 0);

packages/horizon/test/staking/serviceProvider/serviceProvider.t.sol

@@ -84,7 +84,7 @@ contract HorizonStakingServiceProviderTest is HorizonStakingTest {
         uint256 amount,
         uint256 delegationAmount,
         uint32 delegationRatio
-    ) public useIndexer useProvision(amount, MAX_MAX_VERIFIER_CUT, MAX_THAWING_PERIOD) useDelegation(delegationAmount) {
+    ) public useIndexer useProvision(amount, MAX_PPM, MAX_THAWING_PERIOD) useDelegation(delegationAmount) {
         uint256 tokensAvailable = staking.getTokensAvailable(users.indexer, subgraphDataServiceAddress, delegationRatio);
 
         uint256 tokensDelegatedMax = amount * (uint256(delegationRatio));
@@ -95,15 +95,15 @@ contract HorizonStakingServiceProviderTest is HorizonStakingTest {
     function testServiceProvider_GetProviderTokensAvailable(
         uint256 amount,
         uint256 delegationAmount
-    ) public useIndexer useProvision(amount, MAX_MAX_VERIFIER_CUT, MAX_THAWING_PERIOD) useDelegation(delegationAmount) {
+    ) public useIndexer useProvision(amount, MAX_PPM, MAX_THAWING_PERIOD) useDelegation(delegationAmount) {
         uint256 providerTokensAvailable = staking.getProviderTokensAvailable(users.indexer, subgraphDataServiceAddress);
         // Should not include delegated tokens
         assertEq(providerTokensAvailable, amount);
     }
 
     function testServiceProvider_HasStake(
         uint256 amount
-    ) public useIndexer useProvision(amount, MAX_MAX_VERIFIER_CUT, MAX_THAWING_PERIOD) {
+    ) public useIndexer useProvision(amount, MAX_PPM, MAX_THAWING_PERIOD) {
         assertTrue(staking.hasStake(users.indexer));
 
         _thaw(users.indexer, subgraphDataServiceAddress, amount);
@@ -116,7 +116,7 @@ contract HorizonStakingServiceProviderTest is HorizonStakingTest {
 
     function testServiceProvider_GetIndexerStakedTokens(
         uint256 amount
-    ) public useIndexer useProvision(amount, MAX_MAX_VERIFIER_CUT, MAX_THAWING_PERIOD) {
+    ) public useIndexer useProvision(amount, MAX_PPM, MAX_THAWING_PERIOD) {
         assertEq(staking.getIndexerStakedTokens(users.indexer), amount);
 
         _thaw(users.indexer, subgraphDataServiceAddress, amount);

packages/horizon/test/staking/slash/slash.t.sol

@@ -52,12 +52,11 @@ contract HorizonStakingSlashTest is HorizonStakingTest {
         uint256 slashTokens,
         uint256 verifierCutAmount,
         uint256 delegationTokens
-    ) public useIndexer useProvision(tokens, MAX_MAX_VERIFIER_CUT, 0) {
+    ) public useIndexer useProvision(tokens, MAX_PPM, 0) {
         vm.assume(slashTokens > tokens);
         delegationTokens = bound(delegationTokens, 1, MAX_STAKING_TOKENS);
-        verifierCutAmount = bound(verifierCutAmount, 0, MAX_MAX_VERIFIER_CUT);
-        uint256 maxVerifierTokens = (tokens * MAX_MAX_VERIFIER_CUT) / MAX_PPM;
-        vm.assume(verifierCutAmount <= maxVerifierTokens);
+        verifierCutAmount = bound(verifierCutAmount, 0, MAX_PPM);
+        vm.assume(verifierCutAmount <= tokens);
 
         resetPrank(users.delegator);
         _delegate(users.indexer, subgraphDataServiceAddress, delegationTokens, 0);
@@ -71,12 +70,10 @@ contract HorizonStakingSlashTest is HorizonStakingTest {
         uint256 slashTokens,
         uint256 verifierCutAmount,
         uint256 delegationTokens
-    ) public useIndexer useProvision(tokens, MAX_MAX_VERIFIER_CUT, 0) useDelegationSlashing() {
+    ) public useIndexer useProvision(tokens, MAX_PPM, 0) useDelegationSlashing() {
         delegationTokens = bound(delegationTokens, 1, MAX_STAKING_TOKENS);
         slashTokens = bound(slashTokens, tokens + 1, tokens + 1 + delegationTokens);
-        verifierCutAmount = bound(verifierCutAmount, 0, MAX_MAX_VERIFIER_CUT);
-        uint256 maxVerifierTokens = (tokens * MAX_MAX_VERIFIER_CUT) / MAX_PPM;
-        vm.assume(verifierCutAmount <= maxVerifierTokens);
+        verifierCutAmount = bound(verifierCutAmount, 0, tokens);
 
         resetPrank(users.delegator);
         _delegate(users.indexer, subgraphDataServiceAddress, delegationTokens, 0);
@@ -89,7 +86,7 @@ contract HorizonStakingSlashTest is HorizonStakingTest {
         uint256 tokens,
         uint256 slashTokens,
         uint256 delegationTokens
-    ) public useIndexer useProvision(tokens, MAX_MAX_VERIFIER_CUT, 0) {
+    ) public useIndexer useProvision(tokens, MAX_PPM, 0) {
         delegationTokens = bound(delegationTokens, 0, MAX_STAKING_TOKENS);
         vm.assume(slashTokens > tokens + delegationTokens);
 

packages/horizon/test/utils/Constants.sol

@@ -2,7 +2,7 @@
 pragma solidity 0.8.27;
 
 abstract contract Constants {
-    uint256 internal constant MAX_PPM = 1000000; // 100% in parts per million
+    uint32 internal constant MAX_PPM = 1000000; // 100% in parts per million
     uint256 internal constant delegationFeeCut = 100000; // 10% in parts per million
     uint256 internal constant MAX_STAKING_TOKENS = 10_000_000_000 ether;
     // GraphEscrow parameters
@@ -12,7 +12,6 @@ abstract contract Constants {
     uint256 internal constant protocolPaymentCut = 10000;
     // Staking constants
     uint256 internal constant MAX_THAW_REQUESTS = 100;
-    uint32 internal constant MAX_MAX_VERIFIER_CUT = 1000000; // 100% in parts per million
     uint64 internal constant MAX_THAWING_PERIOD = 28 days;
     uint32 internal constant THAWING_PERIOD_IN_BLOCKS = 300;
     // Epoch manager

packages/subgraph-service/test/subgraphService/provision/accept.t.sol

@@ -22,7 +22,7 @@ contract SubgraphServiceProvisionAcceptTest is SubgraphServiceTest {
         tokens = bound(tokens, minimumProvisionTokens, MAX_TOKENS);
         vm.assume(newVerifierCut >= fishermanRewardPercentage);
         vm.assume(newVerifierCut <= MAX_PPM);
-        vm.assume(newDisputePeriod >= disputePeriod);
+        newDisputePeriod = uint64(bound(newDisputePeriod, disputePeriod, MAX_WAIT_PERIOD));
 
         // Setup indexer
         _createProvision(users.indexer, tokens, fishermanRewardPercentage, disputePeriod);
@@ -58,7 +58,7 @@ contract SubgraphServiceProvisionAcceptTest is SubgraphServiceTest {
         uint32 newVerifierCut
     ) public useIndexer {
         tokens = bound(tokens, minimumProvisionTokens, MAX_TOKENS);
-        vm.assume(newVerifierCut > MAX_PPM);
+        vm.assume(newVerifierCut < maxSlashingPercentage);
 
         // Setup indexer
         _createProvision(users.indexer, tokens, fishermanRewardPercentage, disputePeriod);

packages/subgraph-service/test/utils/Constants.sol

@@ -6,7 +6,7 @@ abstract contract Constants {
     uint256 internal constant MAX_PPM = 1_000_000;
     uint256 internal constant EPOCH_LENGTH = 1;
     // Dispute Manager
-    uint64 internal constant disputePeriod = 300; // 5 minutes
+    uint64 internal constant disputePeriod = 7 days;
     uint256 internal constant disputeDeposit = 100 ether; // 100 GRT
     uint32 internal constant fishermanRewardPercentage = 500000; // 50%
     uint32 internal constant maxSlashingPercentage = 500000; // 50%