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

tmigone · web-flow · commit 8e1144cbc77e · 2024-12-17T16:50:17.000-03:00
diff --git a/packages/horizon/contracts/data-service/DataService.sol b/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 {
     /**
diff --git a/packages/horizon/contracts/data-service/DataServiceStorage.sol b/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;
 }
diff --git a/packages/horizon/contracts/data-service/extensions/DataServiceFees.sol b/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;
     }
 
     /**
diff --git a/packages/horizon/contracts/data-service/extensions/DataServiceFeesStorage.sol b/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;
 }
diff --git a/packages/horizon/contracts/data-service/extensions/DataServicePausable.sol b/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);
     }
diff --git a/packages/horizon/contracts/data-service/extensions/DataServicePausableUpgradeable.sol b/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);
     }
diff --git a/packages/horizon/contracts/data-service/extensions/DataServiceRescuable.sol b/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.
      */
diff --git a/packages/horizon/contracts/data-service/interfaces/IDataServicePausable.sol b/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`
diff --git a/packages/horizon/contracts/data-service/utilities/ProvisionManager.sol b/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"
+        );
     }
 
     /**
diff --git a/packages/horizon/contracts/data-service/utilities/ProvisionManagerStorage.sol b/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;
 }
diff --git a/packages/horizon/test/data-service/DataService.t.sol b/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());
diff --git a/packages/horizon/test/data-service/extensions/DataServicePausable.t.sol b/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();

Original file line number	Diff line number	Diff line change
`@@ -3,5 +3,6 @@ pragma solidity 0.8.27;`
`3`	`3`
`4`	`4`	`abstract contract DataServiceV1Storage {`
`5`	`5`	`/// @dev Gap to allow adding variables in future upgrades`
	`6`	`+ /// Note that this contract is not upgradeable but might be inherited by an upgradeable contract`
`6`	`7`	`uint256[50] private __gap;`
`7`	`8`	`}`
Original file line number	Diff line number	Diff line change
`@@ -18,5 +18,6 @@ abstract contract DataServiceFeesV1Storage {`
`18`	`18`	`mapping(address serviceProvider => LinkedList.List list) public claimsLists;`
`19`	`19`
`20`	`20`	`/// @dev Gap to allow adding variables in future upgrades`
	`21`	`+ /// Note that this contract is not upgradeable but might be inherited by an upgradeable contract`
`21`	`22`	`uint256[50] private __gap;`
`22`	`23`	`}`
Original file line number	Diff line number	Diff line change
`@@ -28,5 +28,6 @@ abstract contract ProvisionManagerV1Storage {`
`28`	`28`	`uint32 public delegationRatio;`
`29`	`29`
`30`	`30`	`/// @dev Gap to allow adding variables in future upgrades`
	`31`	`+ /// Note that this contract is not upgradeable but might be inherited by an upgradeable contract`
`31`	`32`	`uint256[50] private __gap;`
`32`	`33`	`}`