Merge pull request #1076 from graphprotocol/tmigone/trust-fixes-data-…

…service

packages/horizon/contracts/data-service/DataService.sol

@@ -25,6 +25,8 @@ import { ProvisionManager } from "./utilities/ProvisionManager.sol";
  * - If the data service implementation is NOT upgradeable, it must initialize the contract by calling
  * {__DataService_init} or {__DataService_init_unchained} in the constructor. Note that the `initializer`
  * will be required in the constructor.
+ * - Note that in both cases if using {__DataService_init_unchained} variant the corresponding parent
+ * initializers must be called in the implementation.
  */
 abstract contract DataService is GraphDirectory, ProvisionManager, DataServiceV1Storage, IDataService {
     /**

packages/horizon/contracts/data-service/DataServiceStorage.sol

@@ -3,5 +3,6 @@ pragma solidity 0.8.27;
 
 abstract contract DataServiceV1Storage {
     /// @dev Gap to allow adding variables in future upgrades
+    /// Note that this contract is not upgradeable but might be inherited by an upgradeable contract
     uint256[50] private __gap;
 }

packages/horizon/contracts/data-service/extensions/DataServiceFees.sol

@@ -14,6 +14,8 @@ import { DataServiceFeesV1Storage } from "./DataServiceFeesStorage.sol";
  * @dev Implementation of the {IDataServiceFees} interface.
  * @notice Extension for the {IDataService} contract to handle payment collateralization
  * using a Horizon provision. See {IDataServiceFees} for more details.
+ * @dev This contract inherits from {DataService} which needs to be initialized, please see
+ * {DataService} for detailed instructions.
  */
 abstract contract DataServiceFees is DataService, DataServiceFeesV1Storage, IDataServiceFees {
     using ProvisionTracker for mapping(address => uint256);
@@ -127,9 +129,7 @@ abstract contract DataServiceFees is DataService, DataServiceFeesV1Storage, IDat
      * @param _claimId The ID of the stake claim
      */
     function _getNextStakeClaim(bytes32 _claimId) private view returns (bytes32) {
-        StakeClaim memory claim = claims[_claimId];
-        require(claim.createdAt != 0, DataServiceFeesClaimNotFound(_claimId));
-        return claim.nextClaim;
+        return claims[_claimId].nextClaim;
     }
 
     /**

packages/horizon/contracts/data-service/extensions/DataServiceFeesStorage.sol

@@ -18,5 +18,6 @@ abstract contract DataServiceFeesV1Storage {
     mapping(address serviceProvider => LinkedList.List list) public claimsLists;
 
     /// @dev Gap to allow adding variables in future upgrades
+    /// Note that this contract is not upgradeable but might be inherited by an upgradeable contract
     uint256[50] private __gap;
 }

packages/horizon/contracts/data-service/extensions/DataServicePausable.sol

@@ -14,6 +14,8 @@ import { DataService } from "../DataService.sol";
  * pause guardians.
  * @dev Note that this extension does not provide an external function to set pause
  * guardians. This should be implemented in the derived contract.
+ * @dev This contract inherits from {DataService} which needs to be initialized, please see
+ * {DataService} for detailed instructions.
  */
 abstract contract DataServicePausable is Pausable, DataService, IDataServicePausable {
     /// @notice List of pause guardians and their allowed status
@@ -51,6 +53,10 @@ abstract contract DataServicePausable is Pausable, DataService, IDataServicePaus
      * @param _allowed The allowed status of the pause guardian
      */
     function _setPauseGuardian(address _pauseGuardian, bool _allowed) internal {
+        require(
+            pauseGuardians[_pauseGuardian] == !_allowed,
+            DataServicePausablePauseGuardianNoChange(_pauseGuardian, _allowed)
+        );
         pauseGuardians[_pauseGuardian] = _allowed;
         emit PauseGuardianSet(_pauseGuardian, _allowed);
     }

packages/horizon/contracts/data-service/extensions/DataServicePausableUpgradeable.sol

@@ -10,11 +10,16 @@ import { DataService } from "../DataService.sol";
  * @title DataServicePausableUpgradeable contract
  * @dev Implementation of the {IDataServicePausable} interface.
  * @dev Upgradeable version of the {DataServicePausable} contract.
+ * @dev This contract inherits from {DataService} which needs to be initialized, please see
+ * {DataService} for detailed instructions.
  */
 abstract contract DataServicePausableUpgradeable is PausableUpgradeable, DataService, IDataServicePausable {
     /// @notice List of pause guardians and their allowed status
     mapping(address pauseGuardian => bool allowed) public pauseGuardians;
 
+    /// @dev Gap to allow adding variables in future upgrades
+    uint256[50] private __gap;
+
     /**
      * @notice Checks if the caller is a pause guardian.
      */
@@ -61,7 +66,11 @@ abstract contract DataServicePausableUpgradeable is PausableUpgradeable, DataSer
      * @param _pauseGuardian The address of the pause guardian
      * @param _allowed The allowed status of the pause guardian
      */
-    function _setPauseGuardian(address _pauseGuardian, bool _allowed) internal whenNotPaused {
+    function _setPauseGuardian(address _pauseGuardian, bool _allowed) internal {
+        require(
+            pauseGuardians[_pauseGuardian] == !_allowed,
+            DataServicePausablePauseGuardianNoChange(_pauseGuardian, _allowed)
+        );
         pauseGuardians[_pauseGuardian] = _allowed;
         emit PauseGuardianSet(_pauseGuardian, _allowed);
     }

packages/horizon/contracts/data-service/extensions/DataServiceRescuable.sol

@@ -17,11 +17,17 @@ import { SafeERC20 } from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.s
  * that calls this contract's _rescueTokens.
  * @dev Note that this extension does not provide an external function to set
  * rescuers. This should be implemented in the derived contract.
+ * @dev This contract inherits from {DataService} which needs to be initialized, please see
+ * {DataService} for detailed instructions.
  */
 abstract contract DataServiceRescuable is DataService, IDataServiceRescuable {
     /// @notice List of rescuers and their allowed status
     mapping(address rescuer => bool allowed) public rescuers;
 
+    /// @dev Gap to allow adding variables in future upgrades
+    /// Note that this contract is not upgradeable but might be inherited by an upgradeable contract
+    uint256[50] private __gap;
+
     /**
      * @notice Checks if the caller is a rescuer.
      */

packages/horizon/contracts/data-service/interfaces/IDataServicePausable.sol

@@ -23,6 +23,13 @@ interface IDataServicePausable is IDataService {
      */
     error DataServicePausableNotPauseGuardian(address account);
 
+    /**
+     * @notice Emitted when a pause guardian is set to the same allowed status
+     * @param account The address of the pause guardian
+     * @param allowed The allowed status of the pause guardian
+     */
+    error DataServicePausablePauseGuardianNoChange(address account, bool allowed);
+
     /**
      * @notice Pauses the data service.
      * @dev Note that only functions using the modifiers `whenNotPaused`

packages/horizon/contracts/data-service/utilities/ProvisionManager.sol

@@ -202,10 +202,16 @@ abstract contract ProvisionManager is Initializable, GraphDirectory, ProvisionMa
 
     /**
      * @notice Checks if the provision tokens of a service provider are within the valid range.
+     * Note that thawing tokens are not considered in this check.
      * @param _provision The provision to check.
      */
     function _checkProvisionTokens(IHorizonStaking.Provision memory _provision) internal view virtual {
-        _checkValueInRange(_provision.tokens, minimumProvisionTokens, maximumProvisionTokens, "tokens");
+        _checkValueInRange(
+            _provision.tokens - _provision.tokensThawing,
+            minimumProvisionTokens,
+            maximumProvisionTokens,
+            "tokens"
+        );
     }
 
     /**

packages/horizon/contracts/data-service/utilities/ProvisionManagerStorage.sol

@@ -28,5 +28,6 @@ abstract contract ProvisionManagerV1Storage {
     uint32 public delegationRatio;
 
     /// @dev Gap to allow adding variables in future upgrades
+    /// Note that this contract is not upgradeable but might be inherited by an upgradeable contract
     uint256[50] private __gap;
 }

packages/horizon/test/data-service/DataService.t.sol

@@ -65,6 +65,25 @@ contract DataServiceTest is HorizonStakingSharedTest {
         assertEq(max, dataServiceOverride.PROVISION_TOKENS_MAX());
     }
 
+    function test_ProvisionTokens_WhenCheckingAValidProvision_WithThawing(uint256 tokens, uint256 tokensThaw) external useIndexer {
+        dataService.setProvisionTokensRange(dataService.PROVISION_TOKENS_MIN(), dataService.PROVISION_TOKENS_MAX());
+        tokens = bound(tokens, dataService.PROVISION_TOKENS_MIN(), dataService.PROVISION_TOKENS_MAX());
+        tokensThaw = bound(tokensThaw, tokens - dataService.PROVISION_TOKENS_MIN() + 1, tokens);
+
+        _createProvision(users.indexer, address(dataService), tokens, 0, 0);
+        staking.thaw(users.indexer, address(dataService), tokensThaw);
+        vm.expectRevert(
+            abi.encodeWithSelector(
+                ProvisionManager.ProvisionManagerInvalidValue.selector,
+                "tokens",
+                tokens - tokensThaw,
+                dataService.PROVISION_TOKENS_MIN(),
+                dataService.PROVISION_TOKENS_MAX()
+            )
+        );
+        dataService.checkProvisionTokens(users.indexer);
+    }
+
     function test_ProvisionTokens_WhenCheckingAValidProvision(uint256 tokens) external useIndexer {
         dataService.setProvisionTokensRange(dataService.PROVISION_TOKENS_MIN(), dataService.PROVISION_TOKENS_MAX());
         tokens = bound(tokens, dataService.PROVISION_TOKENS_MIN(), dataService.PROVISION_TOKENS_MAX());

packages/horizon/test/data-service/extensions/DataServicePausable.t.sol

@@ -70,6 +70,23 @@ contract DataServicePausableTest is HorizonStakingSharedTest {
         _assert_setPauseGuardian(address(this), false);
     }
 
+    function test_SetPauseGuardian_RevertWhen_AlreadyPauseGuardian() external {
+        _assert_setPauseGuardian(address(this), true);
+        vm.expectRevert(
+            abi.encodeWithSignature("DataServicePausablePauseGuardianNoChange(address,bool)", address(this), true)
+        );
+        dataService.setPauseGuardian(address(this), true);
+    }
+
+    function test_SetPauseGuardian_RevertWhen_AlreadyNotPauseGuardian() external {
+        _assert_setPauseGuardian(address(this), true);
+        _assert_setPauseGuardian(address(this), false);
+        vm.expectRevert(
+            abi.encodeWithSignature("DataServicePausablePauseGuardianNoChange(address,bool)", address(this), false)
+        );
+        dataService.setPauseGuardian(address(this), false);
+    }
+
     function test_PausedProtectedFn_RevertWhen_TheProtocolIsPaused() external {
         _assert_setPauseGuardian(address(this), true);
         _assert_pause();