From bcb1705e9e5c5a392d7bc770f88259ca11927df6 Mon Sep 17 00:00:00 2001 From: Pablo Carranza Velez Date: Wed, 15 May 2024 12:50:14 -0300 Subject: [PATCH] chore: slippage protection for delegation --- packages/horizon/contracts/GraphDirectory.sol | 7 +++ packages/horizon/contracts/HorizonStaking.sol | 59 ++++++++++++------- .../contracts/HorizonStakingStorage.sol | 14 ++--- .../horizon/contracts/IHorizonStakingBase.sol | 9 ++- .../contracts/mocks/ControllerMock.sol | 11 +++- .../contracts/utils/ExponentialRebates.sol | 4 +- .../horizon/contracts/utils/TokenUtils.sol | 6 ++ 7 files changed, 77 insertions(+), 33 deletions(-) diff --git a/packages/horizon/contracts/GraphDirectory.sol b/packages/horizon/contracts/GraphDirectory.sol index 9e9ac1375..dfd9973ea 100644 --- a/packages/horizon/contracts/GraphDirectory.sol +++ b/packages/horizon/contracts/GraphDirectory.sol @@ -4,6 +4,13 @@ pragma solidity 0.8.24; import { IController } from "@graphprotocol/contracts/contracts/governance/IController.sol"; +/** + * @title GraphDirectory contract + * @notice This contract is meant to be inherited by other contracts that + * need to keep track of the addresses of the core Graph Horizon contracts. + * It fetches the addresses from the Controller supplied during construction, + * and uses immutable variables to minimize gas costs. + */ contract GraphDirectory { address public immutable CONTROLLER; address public immutable STAKING; diff --git a/packages/horizon/contracts/HorizonStaking.sol b/packages/horizon/contracts/HorizonStaking.sol index 560d509bf..87da15d72 100644 --- a/packages/horizon/contracts/HorizonStaking.sol +++ b/packages/horizon/contracts/HorizonStaking.sol @@ -13,30 +13,33 @@ import { HorizonStakingV1Storage } from "./HorizonStakingStorage.sol"; /** * @title HorizonStaking contract - * @dev This contract is the main Staking contract in The Graph protocol after Horizon. + * @dev This contract is the main Staking contract in The Graph protocol after the Horizon upgrade. * It is designed to be deployed as an upgrade to the L2Staking contract from the legacy contracts * package. - * It uses an HorizonStakingExtension contract to implement the full IHorizonStaking interface through delegatecalls. + * It uses a HorizonStakingExtension contract to implement the full IHorizonStaking interface through delegatecalls. * This is due to the contract size limit on Arbitrum (24kB like mainnet). */ contract HorizonStaking is HorizonStakingV1Storage, IHorizonStakingBase, GraphUpgradeable { /// @dev 100% in parts per million uint32 internal constant MAX_PPM = 1000000; - /// Maximum value that can be set as the maxVerifierCut in a provision. + /// @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 = 1000000; // 100% - /// Minimum size of a provision + /// @dev Minimum size of a provision uint256 private constant MIN_PROVISION_SIZE = 1e18; - /// Maximum number of simultaneous stake thaw requests or undelegations + /// @dev Maximum number of simultaneous stake thaw requests (per provision) or undelegations (per delegation) uint256 private constant MAX_THAW_REQUESTS = 100; + /// @dev Fixed point precision uint256 private constant FIXED_POINT_PRECISION = 1e18; - /// Minimum delegation size - uint256 private constant MINIMUM_DELEGATION = 1e18; + /// @dev Minimum amount of delegation to prevent rounding attacks. + /// TODO: remove this after L2 transfer tool for delegation is removed + /// (delegation on L2 has its own slippage protection) + uint256 private constant MIN_DELEGATION = 1e18; address private immutable STAKING_EXTENSION_ADDRESS; address private immutable SUBGRAPH_DATA_SERVICE_ADDRESS; @@ -52,6 +55,7 @@ contract HorizonStaking is HorizonStakingV1Storage, IHorizonStakingBase, GraphUp error HorizonStakingInsufficientCapacityForLegacyAllocations(); error HorizonStakingTooManyThawRequests(); error HorizonStakingInsufficientTokens(uint256 expected, uint256 available); + error HorizonStakingSlippageProtection(uint256 minExpectedAmount, uint256 actualAmount); modifier onlyAuthorized(address _serviceProvider, address _verifier) { if (!isAuthorized(msg.sender, _serviceProvider, _verifier)) { @@ -477,15 +481,21 @@ contract HorizonStaking is HorizonStakingV1Storage, IHorizonStakingBase, GraphUp _withdraw(msg.sender); } - function delegate(address _serviceProvider, address _verifier, uint256 _tokens) public override notPartialPaused { + function delegate( + address _serviceProvider, + address _verifier, + uint256 _tokens, + uint256 _minSharesOut + ) public override notPartialPaused { // Transfer tokens to stake from caller to this contract TokenUtils.pullTokens(_graphToken(), msg.sender, _tokens); - _delegate(_serviceProvider, _verifier, _tokens); + _delegate(_serviceProvider, _verifier, _tokens, _minSharesOut); } // For backwards compatibility, delegates to the subgraph data service + // (Note this one doesn't have splippage/rounding protection!) function delegate(address _serviceProvider, uint256 _tokens) external { - delegate(_serviceProvider, SUBGRAPH_DATA_SERVICE_ADDRESS, _tokens); + delegate(_serviceProvider, SUBGRAPH_DATA_SERVICE_ADDRESS, _tokens, 0); } // For backwards compatibility, undelegates from the subgraph data service @@ -495,15 +505,21 @@ contract HorizonStaking is HorizonStakingV1Storage, IHorizonStakingBase, GraphUp // For backwards compatibility, withdraws delegated tokens from the subgraph data service function withdrawDelegated(address _serviceProvider, address _newServiceProvider) external { - withdrawDelegated(_serviceProvider, SUBGRAPH_DATA_SERVICE_ADDRESS, _newServiceProvider); + withdrawDelegated(_serviceProvider, SUBGRAPH_DATA_SERVICE_ADDRESS, _newServiceProvider, 0); } - function _delegate(address _serviceProvider, address _verifier, uint256 _tokens) internal { - require(_tokens > 0, "!tokens"); - require(provisions[_serviceProvider][_verifier].tokens >= 0, "!provision"); + function _delegate(address _serviceProvider, address _verifier, uint256 _tokens, uint256 _minSharesOut) internal { + if (_tokens == 0) { + revert HorizonStakingInvalidZeroTokens(); + } + // TODO: remove this after L2 transfer tool for delegation is removed + if (_tokens < MIN_DELEGATION) { + revert HorizonStakingInsufficientTokens(MIN_DELEGATION, _tokens); + } + if (provisions[_serviceProvider][_verifier].tokens == 0) { + revert HorizonStakingInvalidProvision(_serviceProvider, _verifier); + } - // Only allow delegations over a minimum, to prevent rounding attacks - require(_tokens >= MINIMUM_DELEGATION, "!minimum-delegation"); DelegationPoolInternal storage pool; if (_verifier == SUBGRAPH_DATA_SERVICE_ADDRESS) { pool = legacyDelegationPools[_serviceProvider]; @@ -514,7 +530,9 @@ contract HorizonStaking is HorizonStakingV1Storage, IHorizonStakingBase, GraphUp // Calculate shares to issue uint256 shares = (pool.tokens == 0) ? _tokens : ((_tokens * pool.shares) / (pool.tokens - pool.tokensThawing)); - require(shares > 0, "!shares"); + if (shares == 0 || shares < _minSharesOut) { + revert HorizonStakingSlippageProtection(_minSharesOut, shares); + } pool.tokens = pool.tokens + _tokens; pool.shares = pool.shares + shares; @@ -546,7 +564,7 @@ contract HorizonStaking is HorizonStakingV1Storage, IHorizonStakingBase, GraphUp delegation.shares = delegation.shares - _shares; if (delegation.shares != 0) { uint256 remainingTokens = (delegation.shares * (pool.tokens - pool.tokensThawing)) / pool.shares; - require(remainingTokens >= MINIMUM_DELEGATION, "!minimum-delegation"); + require(remainingTokens >= MIN_DELEGATION, "!minimum-delegation"); } bytes32 thawRequestId = keccak256( abi.encodePacked(_serviceProvider, _verifier, msg.sender, delegation.nextThawRequestNonce) @@ -573,7 +591,8 @@ contract HorizonStaking is HorizonStakingV1Storage, IHorizonStakingBase, GraphUp function withdrawDelegated( address _serviceProvider, address _verifier, - address _newServiceProvider + address _newServiceProvider, + uint256 _minSharesForNewProvider ) public override notPartialPaused { DelegationPoolInternal storage pool; if (_verifier == SUBGRAPH_DATA_SERVICE_ADDRESS) { @@ -612,7 +631,7 @@ contract HorizonStaking is HorizonStakingV1Storage, IHorizonStakingBase, GraphUp pool.tokensThawing = tokensThawing; if (_newServiceProvider != address(0)) { - _delegate(_newServiceProvider, _verifier, thawedTokens); + _delegate(_newServiceProvider, _verifier, thawedTokens, _minSharesForNewProvider); } else { TokenUtils.pushTokens(_graphToken(), msg.sender, thawedTokens); } diff --git a/packages/horizon/contracts/HorizonStakingStorage.sol b/packages/horizon/contracts/HorizonStakingStorage.sol index a0c13995d..58c563113 100644 --- a/packages/horizon/contracts/HorizonStakingStorage.sol +++ b/packages/horizon/contracts/HorizonStakingStorage.sol @@ -35,7 +35,7 @@ abstract contract HorizonStakingV1Storage is Managed, IHorizonStakingTypes { /// @dev Period for allocation to be finalized uint32 private __DEPRECATED_channelDisputeEpochs; // solhint-disable-line var-name-mixedcase - /// @dev Maximum allocation time + /// @dev Maximum allocation time. Deprecated, allocations now live on the subgraph service contract. uint32 internal __DEPRECATED_maxAllocationEpochs; /// @dev Rebate alpha numerator @@ -59,21 +59,21 @@ abstract contract HorizonStakingV1Storage is Managed, IHorizonStakingTypes { /// Deprecated, now applied on the SubgraphService mapping(bytes32 => uint256) internal __DEPRECATED_subgraphAllocations; - // Rebate pools : epoch => Pool + /// @dev Rebate pools : epoch => Pool + /// Deprecated. mapping(uint256 => uint256) private __DEPRECATED_rebates; // solhint-disable-line var-name-mixedcase // -- Slashing -- /// @dev List of addresses allowed to slash stakes - /// Deprecated, now allowlisted by each service provider by setting a verifier + /// Deprecated, now each verifier can slash the corresponding provision. mapping(address => bool) internal __DEPRECATED_slashers; // -- Delegation -- - /// @dev Set the delegation capacity multiplier defined by the delegation ratio - /// If delegation ratio is 100, and an Indexer has staked 5 GRT, - /// then they can use up to 500 GRT from the delegated stake - uint32 internal delegationRatio; + /// @dev Delegation capacity multiplier defined by the delegation ratio + /// Deprecated, now applied by each data service as needed. + uint32 internal __DEPRECATED_delegationRatio; /// @dev Time in blocks an indexer needs to wait to change delegation parameters (deprecated) uint32 internal __DEPRECATED_delegationParametersCooldown; // solhint-disable-line var-name-mixedcase diff --git a/packages/horizon/contracts/IHorizonStakingBase.sol b/packages/horizon/contracts/IHorizonStakingBase.sol index 52317b581..7e0137917 100644 --- a/packages/horizon/contracts/IHorizonStakingBase.sol +++ b/packages/horizon/contracts/IHorizonStakingBase.sol @@ -132,13 +132,18 @@ interface IHorizonStakingBase is IHorizonStakingTypes { function unstake(uint256 _tokens) external; // delegate tokens to a provider on a data service - function delegate(address _serviceProvider, address _verifier, uint256 _tokens) external; + function delegate(address _serviceProvider, address _verifier, uint256 _tokens, uint256 _minSharesOut) external; // undelegate (thaw) delegated tokens from a provision function undelegate(address _serviceProvider, address _verifier, uint256 _shares) external; // withdraw delegated tokens after thawing - function withdrawDelegated(address _serviceProvider, address _verifier, address _newServiceProvider) external; + function withdrawDelegated( + address _serviceProvider, + address _verifier, + address _newServiceProvider, + uint256 _minSharesForNewProvider + ) external; function slash( address _serviceProvider, diff --git a/packages/horizon/contracts/mocks/ControllerMock.sol b/packages/horizon/contracts/mocks/ControllerMock.sol index c6832e595..f8c976cf9 100644 --- a/packages/horizon/contracts/mocks/ControllerMock.sol +++ b/packages/horizon/contracts/mocks/ControllerMock.sol @@ -21,12 +21,17 @@ contract ControllerMock is IController { /// Emitted when the proxy address for a protocol contract has been set event SetContractProxy(bytes32 indexed id, address contractAddress); + /** + * Constructor for the Controller mock + * @param _governor Address of the governor + */ constructor(address _governor) { governor = _governor; } /** * @notice Getter to access governor + * @return Address of the governor */ function getGovernor() external view override returns (address) { return governor; @@ -56,7 +61,7 @@ contract ControllerMock is IController { /** * @notice Get contract proxy address by its id - * @param _id Contract id + * @param _id Contract id (keccak256 hash of contract name) * @return Address of the proxy contract for the provided id */ function getContractProxy(bytes32 _id) external view override returns (address) { @@ -64,9 +69,9 @@ contract ControllerMock is IController { } /** - * @notice Update contract's controller + * @notice Update a contract's controller * @param _id Contract id (keccak256 hash of contract name) - * @param _controller Controller address + * @param _controller New Controller address */ function updateController(bytes32 _id, address _controller) external override { require(_controller != address(0), "Controller must be set"); diff --git a/packages/horizon/contracts/utils/ExponentialRebates.sol b/packages/horizon/contracts/utils/ExponentialRebates.sol index 12cf317a3..d53efdca3 100644 --- a/packages/horizon/contracts/utils/ExponentialRebates.sol +++ b/packages/horizon/contracts/utils/ExponentialRebates.sol @@ -5,8 +5,10 @@ pragma solidity 0.8.24; import { LibFixedMath } from "./LibFixedMath.sol"; /** - * @title Exponential + * @title ExponentialRebates contract * @notice A library to compute query fee rebates using an exponential formula + * @dev This is only used for backwards compatibility in HorizonStaking, and should + * be removed after the transition period. */ contract ExponentialRebates { /// @dev Maximum value of the exponent for which to compute the exponential before clamping to zero. diff --git a/packages/horizon/contracts/utils/TokenUtils.sol b/packages/horizon/contracts/utils/TokenUtils.sol index 73e838737..7bfc7203c 100644 --- a/packages/horizon/contracts/utils/TokenUtils.sol +++ b/packages/horizon/contracts/utils/TokenUtils.sol @@ -4,6 +4,12 @@ pragma solidity 0.8.24; import { IGraphToken } from "../IGraphToken.sol"; +/** + * @title TokenUtils library + * @notice This library contains utility functions for handling tokens (transfers and burns). + * It is specifically adapted for the GraphToken, so does not need to handle edge cases + * for other tokens. + */ library TokenUtils { /** * @dev Pull tokens from an address to this contract.