Merge pull request #1123 from graphprotocol/tmigone/horizon-pre-audit-assesment

fix: pre-audit assessment fixes

Author: tmigone

packages/contracts/contracts/rewards/RewardsManager.sol

@@ -3,14 +3,10 @@
 pragma solidity ^0.7.6;
 pragma abicoder v2;
 
-import "@openzeppelin/contracts/math/SafeMath.sol";
-
 import "../upgrades/GraphUpgradeable.sol";
 import "../staking/libs/MathUtils.sol";
 
 import "./RewardsManagerStorage.sol";
-import "./IRewardsManager.sol";
-import { IRewardsIssuer } from "./IRewardsIssuer.sol";
 
 /**
  * @title Rewards Manager Contract

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

@@ -27,6 +27,8 @@ import { ProvisionManager } from "./utilities/ProvisionManager.sol";
  * 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.
+ * @custom:security-contact Please email [email protected] if you find any
+ * bugs. We may have an active bug bounty program.
  */
 abstract contract DataService is GraphDirectory, ProvisionManager, DataServiceV1Storage, IDataService {
     /**
@@ -35,38 +37,29 @@ abstract contract DataService is GraphDirectory, ProvisionManager, DataServiceV1
      */
     constructor(address controller) GraphDirectory(controller) {}
 
-    /**
-     * @notice See {IDataService-getThawingPeriodRange}.
-     */
+    /// @inheritdoc IDataService
     function getThawingPeriodRange() external view returns (uint64, uint64) {
         return _getThawingPeriodRange();
     }
 
-    /**
-     * @notice See {IDataService-getVerifierCutRange}.
-     */
+    /// @inheritdoc IDataService
     function getVerifierCutRange() external view returns (uint32, uint32) {
         return _getVerifierCutRange();
     }
 
-    /**
-     * @notice See {IDataService-getProvisionTokensRange}.
-     */
+    /// @inheritdoc IDataService
     function getProvisionTokensRange() external view returns (uint256, uint256) {
         return _getProvisionTokensRange();
     }
 
-    /**
-     * @notice See {IDataService-getDelegationRatio}.
-     */
+    /// @inheritdoc IDataService
     function getDelegationRatio() external view returns (uint32) {
         return _getDelegationRatio();
     }
 
     /**
      * @notice Initializes the contract and any parent contracts.
      */
-    // solhint-disable-next-line func-name-mixedcase
     function __DataService_init() internal onlyInitializing {
         __ProvisionManager_init_unchained();
         __DataService_init_unchained();
@@ -75,6 +68,5 @@ abstract contract DataService is GraphDirectory, ProvisionManager, DataServiceV1
     /**
      * @notice Initializes the contract.
      */
-    // solhint-disable-next-line func-name-mixedcase
     function __DataService_init_unchained() internal onlyInitializing {}
 }

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

@@ -1,6 +1,12 @@
 // SPDX-License-Identifier: GPL-3.0-or-later
 pragma solidity 0.8.27;
 
+/**
+ * @title DataServiceStorage
+ * @dev This contract holds the storage variables for the DataService contract.
+ * @custom:security-contact Please email [email protected] if you find any
+ * bugs. We may have an active bug bounty program.
+ */
 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

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

@@ -16,14 +16,14 @@ import { DataServiceFeesV1Storage } from "./DataServiceFeesStorage.sol";
  * 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.
+ * @custom:security-contact Please email [email protected] if you find any
+ * bugs. We may have an active bug bounty program.
  */
 abstract contract DataServiceFees is DataService, DataServiceFeesV1Storage, IDataServiceFees {
     using ProvisionTracker for mapping(address => uint256);
     using LinkedList for LinkedList.List;
 
-    /**
-     * @notice See {IDataServiceFees-releaseStake}
-     */
+    /// @inheritdoc IDataServiceFees
     function releaseStake(uint256 numClaimsToRelease) external virtual override {
         _releaseStake(msg.sender, numClaimsToRelease);
     }
@@ -42,7 +42,7 @@ abstract contract DataServiceFees is DataService, DataServiceFeesV1Storage, IDat
      */
     function _lockStake(address _serviceProvider, uint256 _tokens, uint256 _unlockTimestamp) internal {
         require(_tokens != 0, DataServiceFeesZeroTokens());
-        feesProvisionTracker.lock(_graphStaking(), _serviceProvider, _tokens, delegationRatio);
+        feesProvisionTracker.lock(_graphStaking(), _serviceProvider, _tokens, _delegationRatio);
 
         LinkedList.List storage claimsList = claimsLists[_serviceProvider];
 
@@ -61,7 +61,11 @@ abstract contract DataServiceFees is DataService, DataServiceFeesV1Storage, IDat
     }
 
     /**
-     * @notice See {IDataServiceFees-releaseStake}
+     * @notice Releases expired stake claims for a service provider.
+     * @dev This function can be overriden and/or disabled.
+     * @dev Emits a {StakeClaimsReleased} event, and a {StakeClaimReleased} event for each claim released.
+     * @param _serviceProvider The address of the service provider
+     * @param _numClaimsToRelease Amount of stake claims to process. If 0, all stake claims are processed.
      */
     function _releaseStake(address _serviceProvider, uint256 _numClaimsToRelease) internal {
         LinkedList.List storage claimsList = claimsLists[_serviceProvider];
@@ -116,6 +120,7 @@ abstract contract DataServiceFees is DataService, DataServiceFeesV1Storage, IDat
     /**
      * @notice Gets the details of a stake claim
      * @param _claimId The ID of the stake claim
+     * @return The stake claim details
      */
     function _getStakeClaim(bytes32 _claimId) private view returns (StakeClaim memory) {
         StakeClaim memory claim = claims[_claimId];
@@ -127,6 +132,7 @@ abstract contract DataServiceFees is DataService, DataServiceFeesV1Storage, IDat
      * @notice Gets the next stake claim in the linked list
      * @dev This function is used as a callback in the stake claims linked list traversal.
      * @param _claimId The ID of the stake claim
+     * @return The next stake claim ID
      */
     function _getNextStakeClaim(bytes32 _claimId) private view returns (bytes32) {
         return claims[_claimId].nextClaim;
@@ -136,6 +142,7 @@ abstract contract DataServiceFees is DataService, DataServiceFeesV1Storage, IDat
      * @notice Builds a stake claim ID
      * @param _serviceProvider The address of the service provider
      * @param _nonce A nonce of the stake claim
+     * @return The stake claim ID
      */
     function _buildStakeClaimId(address _serviceProvider, uint256 _nonce) private view returns (bytes32) {
         return keccak256(abi.encodePacked(address(this), _serviceProvider, _nonce));

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

@@ -7,6 +7,8 @@ import { LinkedList } from "../../libraries/LinkedList.sol";
 
 /**
  * @title Storage layout for the {DataServiceFees} extension contract.
+ * @custom:security-contact Please email [email protected] if you find any
+ * bugs. We may have an active bug bounty program.
  */
 abstract contract DataServiceFeesV1Storage {
     mapping(address serviceProvider => uint256 tokens) public feesProvisionTracker;

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

@@ -16,6 +16,8 @@ import { DataService } from "../DataService.sol";
  * 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.
+ * @custom:security-contact Please email [email protected] if you find any
+ * bugs. We may have an active bug bounty program.
  */
 abstract contract DataServicePausable is Pausable, DataService, IDataServicePausable {
     /// @notice List of pause guardians and their allowed status
@@ -29,26 +31,19 @@ abstract contract DataServicePausable is Pausable, DataService, IDataServicePaus
         _;
     }
 
-    /**
-     * @notice See {IDataServicePausable-pause}
-     */
+    /// @inheritdoc IDataServicePausable
     function pause() external override onlyPauseGuardian whenNotPaused {
         _pause();
     }
 
-    /**
-     * @notice See {IDataServicePausable-pause}
-     */
+    /// @inheritdoc IDataServicePausable
     function unpause() external override onlyPauseGuardian whenPaused {
         _unpause();
     }
 
     /**
      * @notice Sets a pause guardian.
      * @dev Internal function to be used by the derived contract to set pause guardians.
-     *
-     * Emits a {PauseGuardianSet} event.
-     *
      * @param _pauseGuardian The address of the pause guardian
      * @param _allowed The allowed status of the pause guardian
      */

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

@@ -12,6 +12,8 @@ import { DataService } from "../DataService.sol";
  * @dev Upgradeable version of the {DataServicePausable} contract.
  * @dev This contract inherits from {DataService} which needs to be initialized, please see
  * {DataService} for detailed instructions.
+ * @custom:security-contact Please email [email protected] if you find any
+ * bugs. We may have an active bug bounty program.
  */
 abstract contract DataServicePausableUpgradeable is PausableUpgradeable, DataService, IDataServicePausable {
     /// @notice List of pause guardians and their allowed status
@@ -28,24 +30,19 @@ abstract contract DataServicePausableUpgradeable is PausableUpgradeable, DataSer
         _;
     }
 
-    /**
-     * @notice See {IDataServicePausable-pause}
-     */
+    /// @inheritdoc IDataServicePausable
     function pause() external override onlyPauseGuardian whenNotPaused {
         _pause();
     }
 
-    /**
-     * @notice See {IDataServicePausable-pause}
-     */
+    /// @inheritdoc IDataServicePausable
     function unpause() external override onlyPauseGuardian whenPaused {
         _unpause();
     }
 
     /**
      * @notice Initializes the contract and parent contracts
      */
-    // solhint-disable-next-line func-name-mixedcase
     function __DataServicePausable_init() internal onlyInitializing {
         __Pausable_init_unchained();
         __DataServicePausable_init_unchained();
@@ -54,7 +51,6 @@ abstract contract DataServicePausableUpgradeable is PausableUpgradeable, DataSer
     /**
      * @notice Initializes the contract
      */
-    // solhint-disable-next-line func-name-mixedcase
     function __DataServicePausable_init_unchained() internal onlyInitializing {}
 
     /**

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

@@ -19,6 +19,8 @@ import { SafeERC20 } from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.s
  * 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.
+ * @custom:security-contact Please email [email protected] if you find any
+ * bugs. We may have an active bug bounty program.
  */
 abstract contract DataServiceRescuable is DataService, IDataServiceRescuable {
     /// @notice List of rescuers and their allowed status
@@ -36,16 +38,12 @@ abstract contract DataServiceRescuable is DataService, IDataServiceRescuable {
         _;
     }
 
-    /**
-     * @notice See {IDataServiceRescuable-rescueGRT}
-     */
+    /// @inheritdoc IDataServiceRescuable
     function rescueGRT(address to, uint256 tokens) external virtual onlyRescuer {
         _rescueTokens(to, address(_graphToken()), tokens);
     }
 
-    /**
-     * @notice See {IDataServiceRescuable-rescueETH}
-     */
+    /// @inheritdoc IDataServiceRescuable
     function rescueETH(address payable to, uint256 tokens) external virtual onlyRescuer {
         _rescueTokens(to, Denominations.NATIVE_TOKEN, tokens);
     }

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

@@ -12,6 +12,8 @@ import { IGraphPayments } from "../../interfaces/IGraphPayments.sol";
  * the implementation code and the documentation.
  * @dev This interface is expected to be inherited and extended by a data service interface. It can be
  * used to interact with it however it's advised to use the more specific parent interface.
+ * @custom:security-contact Please email [email protected] if you find any
+ * bugs. We may have an active bug bounty program.
  */
 interface IDataService {
     /**
@@ -110,7 +112,6 @@ interface IDataService {
      * @notice Collects payment earnt by the service provider.
      * @dev The implementation of this function is expected to interact with {GraphPayments}
      * to collect payment from the service payer, which is done via {IGraphPayments-collect}.
-     * @param serviceProvider The address of the service provider.
      *
      * Emits a {ServicePaymentCollected} event.
      *

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

@@ -18,22 +18,24 @@ import { IDataService } from "./IDataService.sol";
  * @dev Note that this implementation uses the entire provisioned stake as collateral for the payment.
  * It can be used to provide economic security for the payments collected as long as the provisioned
  * stake is not being used for other purposes.
+ * @custom:security-contact Please email [email protected] if you find any
+ * bugs. We may have an active bug bounty program.
  */
 interface IDataServiceFees is IDataService {
     /**
      * @notice A stake claim, representing provisioned stake that gets locked
      * to be released to a service provider.
      * @dev StakeClaims are stored in linked lists by service provider, ordered by
      * creation timestamp.
+     * @param tokens The amount of tokens to be locked in the claim
+     * @param createdAt The timestamp when the claim was created
+     * @param releasableAt The timestamp when the tokens can be released
+     * @param nextClaim The next claim in the linked list
      */
     struct StakeClaim {
-        // The amount of tokens to be locked in the claim
         uint256 tokens;
-        // Timestamp when the claim was created
         uint256 createdAt;
-        // Timestamp when the claim will expire and tokens can be released
         uint256 releasableAt;
-        // Next claim in the linked list
         bytes32 nextClaim;
     }
 
@@ -75,6 +77,7 @@ interface IDataServiceFees is IDataService {
 
     /**
      * @notice Thrown when attempting to get a stake claim that does not exist.
+     * @param claimId The id of the stake claim
      */
     error DataServiceFeesClaimNotFound(bytes32 claimId);
 

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

@@ -8,6 +8,8 @@ import { IDataService } from "./IDataService.sol";
  * @notice Extension for the {IDataService} contract, adds pausing functionality
  * to the data service. Pausing is controlled by privileged accounts called
  * pause guardians.
+ * @custom:security-contact Please email [email protected] if you find any
+ * bugs. We may have an active bug bounty program.
  */
 interface IDataServicePausable is IDataService {
     /**

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

@@ -7,6 +7,8 @@ import { IDataService } from "./IDataService.sol";
  * @title Interface for the {IDataServicePausable} contract.
  * @notice Extension for the {IDataService} contract, adds the ability to rescue
  * any ERC20 token or ETH from the contract, controlled by a rescuer privileged role.
+ * @custom:security-contact Please email [email protected] if you find any
+ * bugs. We may have an active bug bounty program.
  */
 interface IDataServiceRescuable is IDataService {
     /**
@@ -20,6 +22,8 @@ interface IDataServiceRescuable is IDataService {
 
     /**
      * @notice Emitted when a rescuer is set.
+     * @param account The address of the rescuer
+     * @param allowed Whether the rescuer is allowed to rescue tokens
      */
     event RescuerSet(address indexed account, bool allowed);
 
@@ -30,6 +34,7 @@ interface IDataServiceRescuable is IDataService {
 
     /**
      * @notice Thrown when the caller is not a rescuer.
+     * @param account The address of the account that attempted the rescue
      */
     error DataServiceRescuableNotRescuer(address account);
 

packages/horizon/contracts/data-service/libraries/ProvisionTracker.sol

@@ -10,10 +10,14 @@ import { IHorizonStaking } from "../../interfaces/IHorizonStaking.sol";
  * their services.
  * The library provides two primitives, lock and release to signal token usage and free up tokens respectively. It
  * does not make any assumptions about the conditions under which tokens are locked or released.
+ * @custom:security-contact Please email [email protected] if you find any
+ * bugs. We may have an active bug bounty program.
  */
 library ProvisionTracker {
     /**
      * @notice Thrown when trying to lock more tokens than available
+     * @param tokensAvailable The amount of tokens available
+     * @param tokensRequired The amount of tokens required
      */
     error ProvisionTrackerInsufficientTokens(uint256 tokensAvailable, uint256 tokensRequired);
 
@@ -62,6 +66,7 @@ library ProvisionTracker {
      * @param graphStaking The HorizonStaking contract
      * @param serviceProvider The service provider address
      * @param delegationRatio A delegation ratio to limit the amount of delegation that's usable
+     * @return true if the service provider has enough tokens available to lock, false otherwise
      */
     function check(
         mapping(address => uint256) storage self,