diff --git a/audits/Trust/2023-02-operator-decentralization-pr749.pdf b/audits/Trust/2023-02-operator-decentralization-pr749.pdf new file mode 100644 index 000000000..ebfe2ba98 Binary files /dev/null and b/audits/Trust/2023-02-operator-decentralization-pr749.pdf differ diff --git a/config/graph.arbitrum-goerli.yml b/config/graph.arbitrum-goerli.yml index 9fa2ab0be..ec7167713 100644 --- a/config/graph.arbitrum-goerli.yml +++ b/config/graph.arbitrum-goerli.yml @@ -123,9 +123,6 @@ contracts: - fn: "setSlasher" slasher: "${{DisputeManager.address}}" allowed: true - - fn: "setAssetHolder" - assetHolder: "${{AllocationExchange.address}}" - allowed: true - fn: "syncAllContracts" RewardsManager: proxy: true diff --git a/config/graph.arbitrum-localhost.yml b/config/graph.arbitrum-localhost.yml index 6a8216fcc..7bee87748 100644 --- a/config/graph.arbitrum-localhost.yml +++ b/config/graph.arbitrum-localhost.yml @@ -123,9 +123,6 @@ contracts: - fn: "setSlasher" slasher: "${{DisputeManager.address}}" allowed: true - - fn: "setAssetHolder" - assetHolder: "${{AllocationExchange.address}}" - allowed: true - fn: "syncAllContracts" RewardsManager: proxy: true diff --git a/config/graph.arbitrum-one.yml b/config/graph.arbitrum-one.yml index 0525a6d2e..f9dae1862 100644 --- a/config/graph.arbitrum-one.yml +++ b/config/graph.arbitrum-one.yml @@ -123,9 +123,6 @@ contracts: - fn: "setSlasher" slasher: "${{DisputeManager.address}}" allowed: true - - fn: "setAssetHolder" - assetHolder: "${{AllocationExchange.address}}" - allowed: true - fn: "syncAllContracts" RewardsManager: proxy: true diff --git a/config/graph.goerli.yml b/config/graph.goerli.yml index f76c7e215..d09610334 100644 --- a/config/graph.goerli.yml +++ b/config/graph.goerli.yml @@ -126,9 +126,6 @@ contracts: - fn: "setSlasher" slasher: "${{DisputeManager.address}}" allowed: true - - fn: "setAssetHolder" - assetHolder: "${{AllocationExchange.address}}" - allowed: true - fn: "syncAllContracts" RewardsManager: proxy: true diff --git a/config/graph.localhost.yml b/config/graph.localhost.yml index 883feb5a1..578809842 100644 --- a/config/graph.localhost.yml +++ b/config/graph.localhost.yml @@ -131,9 +131,6 @@ contracts: - fn: "setSlasher" slasher: "${{DisputeManager.address}}" allowed: true - - fn: "setAssetHolder" - assetHolder: "${{AllocationExchange.address}}" - allowed: true - fn: "syncAllContracts" RewardsManager: proxy: true diff --git a/config/graph.mainnet.yml b/config/graph.mainnet.yml index fd1dd62d1..ff2c9124f 100644 --- a/config/graph.mainnet.yml +++ b/config/graph.mainnet.yml @@ -126,9 +126,6 @@ contracts: - fn: "setSlasher" slasher: "${{DisputeManager.address}}" allowed: true - - fn: "setAssetHolder" - assetHolder: "${{AllocationExchange.address}}" - allowed: true - fn: "syncAllContracts" RewardsManager: proxy: true diff --git a/contracts/staking/IStakingBase.sol b/contracts/staking/IStakingBase.sol index acc8ac678..086e29a7d 100644 --- a/contracts/staking/IStakingBase.sol +++ b/contracts/staking/IStakingBase.sol @@ -93,12 +93,6 @@ interface IStakingBase is IStakingData { uint32 cooldownBlocks ); - /** - * @dev Emitted when `caller` set `assetHolder` address as `allowed` to send funds - * to staking contract. - */ - event AssetHolderUpdate(address indexed caller, address indexed assetHolder, bool allowed); - /** * @dev Emitted when `indexer` set `operator` access. */ @@ -219,14 +213,6 @@ interface IStakingBase is IStakingData { uint32 _lambdaDenominator ) external; - /** - * @notice Set an address as allowed asset holder. - * @dev This function can only be called by the governor. - * @param _assetHolder Address of allowed source for state channel funds - * @param _allowed True if asset holder is allowed - */ - function setAssetHolder(address _assetHolder, bool _allowed) external; - /** * @notice Authorize or unauthorize an address to be an operator for the caller. * @param _operator Address to authorize or unauthorize diff --git a/contracts/staking/IStakingExtension.sol b/contracts/staking/IStakingExtension.sol index 815577e2f..3499aa135 100644 --- a/contracts/staking/IStakingExtension.sol +++ b/contracts/staking/IStakingExtension.sol @@ -237,15 +237,6 @@ interface IStakingExtension is IStakingData { */ function rewardsDestination(address _indexer) external view returns (address); - /** - * @notice Getter for assetHolders[_maybeAssetHolder]: - * returns true if the address is an asset holder, i.e. an entity that can collect - * query fees into the Staking contract. - * @param _maybeAssetHolder The address that may or may not be an asset holder - * @return True if the address is an asset holder - */ - function assetHolders(address _maybeAssetHolder) external view returns (bool); - /** * @notice Getter for subgraphAllocations[_subgraphDeploymentId]: * returns the amount of tokens allocated to a subgraph deployment. diff --git a/contracts/staking/Staking.sol b/contracts/staking/Staking.sol index 589141210..07b98a301 100644 --- a/contracts/staking/Staking.sol +++ b/contracts/staking/Staking.sol @@ -220,17 +220,6 @@ abstract contract Staking is StakingV4Storage, GraphUpgradeable, IStakingBase, M ); } - /** - * @notice Set an address as allowed asset holder. - * @param _assetHolder Address of allowed source for state channel funds - * @param _allowed True if asset holder is allowed - */ - function setAssetHolder(address _assetHolder, bool _allowed) external override onlyGovernor { - require(_assetHolder != address(0), "!assetHolder"); - __assetHolders[_assetHolder] = _allowed; - emit AssetHolderUpdate(msg.sender, _assetHolder, _allowed); - } - /** * @notice Authorize or unauthorize an address to be an operator for the caller. * @param _operator Address to authorize or unauthorize @@ -354,7 +343,6 @@ abstract contract Staking is StakingV4Storage, GraphUpgradeable, IStakingBase, M /** * @dev Collect and rebate query fees from state channels to the indexer - * Funds received are only accepted from a valid sender. * To avoid reverting on the withdrawal from channel flow this function will accept calls with zero tokens. * We use an exponential rebate formula to calculate the amount of tokens to rebate to the indexer. * This implementation allows collecting multiple times on the same allocation, keeping track of the @@ -366,13 +354,17 @@ abstract contract Staking is StakingV4Storage, GraphUpgradeable, IStakingBase, M // Allocation identifier validation require(_allocationID != address(0), "!alloc"); - // The contract caller must be an authorized asset holder - require(__assetHolders[msg.sender] == true, "!assetHolder"); - // Allocation must exist AllocationState allocState = _getAllocationState(_allocationID); require(allocState != AllocationState.Null, "!collect"); + // If the query fees are zero, we don't want to revert + // but we also don't need to do anything, so just return + if (_tokens == 0) { + return; + } + + // Get allocation Allocation storage alloc = __allocations[_allocationID]; bytes32 subgraphDeploymentID = alloc.subgraphDeploymentID; @@ -382,9 +374,8 @@ abstract contract Staking is StakingV4Storage, GraphUpgradeable, IStakingBase, M uint256 queryRebates = 0; // Tokens to distribute to indexer uint256 delegationRewards = 0; // Tokens to distribute to delegators - // Process query fees only if non-zero amount - if (queryFees > 0) { - // -- Pull tokens from the authorized sender -- + { + // -- Pull tokens from the sender -- IGraphToken graphToken = graphToken(); TokenUtils.pullTokens(graphToken, msg.sender, queryFees); @@ -789,7 +780,7 @@ abstract contract Staking is StakingV4Storage, GraphUpgradeable, IStakingBase, M // Creates an allocation // Allocation identifiers are not reused - // The assetHolder address can send collected funds to the allocation + // Anyone can send collected funds to the allocation using collect() Allocation memory alloc = Allocation( _indexer, _subgraphDeploymentID, diff --git a/contracts/staking/StakingExtension.sol b/contracts/staking/StakingExtension.sol index 0667b89fb..3080bfadb 100644 --- a/contracts/staking/StakingExtension.sol +++ b/contracts/staking/StakingExtension.sol @@ -301,17 +301,6 @@ contract StakingExtension is StakingV4Storage, GraphUpgradeable, IStakingExtensi return __rewardsDestination[_indexer]; } - /** - * @notice Getter for assetHolders[_maybeAssetHolder]: - * returns true if the address is an asset holder, i.e. an entity that can collect - * query fees into the Staking contract. - * @param _maybeAssetHolder The address that may or may not be an asset holder - * @return True if the address is an asset holder - */ - function assetHolders(address _maybeAssetHolder) external view override returns (bool) { - return __assetHolders[_maybeAssetHolder]; - } - /** * @notice Getter for operatorAuth[_indexer][_maybeOperator]: * returns true if the operator is authorized to operate on behalf of the indexer. diff --git a/contracts/staking/StakingStorage.sol b/contracts/staking/StakingStorage.sol index 88c149e22..340709f2b 100644 --- a/contracts/staking/StakingStorage.sol +++ b/contracts/staking/StakingStorage.sol @@ -89,8 +89,8 @@ contract StakingV1Storage is Managed { // -- Asset Holders -- - /// @dev Allowed AssetHolders that can collect query fees: assetHolder => is allowed - mapping(address => bool) internal __assetHolders; + /// @dev DEPRECATED: Allowed AssetHolders: assetHolder => is allowed + mapping(address => bool) private __DEPRECATED_assetHolders; // solhint-disable-line var-name-mixedcase } /** diff --git a/e2e/deployment/config/staking.test.ts b/e2e/deployment/config/staking.test.ts index e162f0676..3759c2567 100644 --- a/e2e/deployment/config/staking.test.ts +++ b/e2e/deployment/config/staking.test.ts @@ -26,11 +26,6 @@ describe('Staking configuration', () => { expect(isSlasher).eq(true) }) - it('should allow AllocationExchange to collect query fees', async function () { - const allowed = await Staking.assetHolders(AllocationExchange.address) - expect(allowed).eq(true) - }) - it('minimumIndexerStake should match "minimumIndexerStake" in the config file', async function () { const value = await Staking.minimumIndexerStake() const expected = getItemValue(graphConfig, `contracts/${contractName}/init/minimumIndexerStake`) diff --git a/test/disputes/poi.test.ts b/test/disputes/poi.test.ts index e0129411a..fdb6bb954 100644 --- a/test/disputes/poi.test.ts +++ b/test/disputes/poi.test.ts @@ -105,9 +105,6 @@ describe('DisputeManager:POI', async () => { // Give some funds to the fisherman await grt.connect(governor.signer).mint(fisherman.address, fishermanTokens) await grt.connect(fisherman.signer).approve(disputeManager.address, fishermanTokens) - - // Allow the asset holder - await staking.connect(governor.signer).setAssetHolder(assetHolder.address, true) }) beforeEach(async function () { diff --git a/test/disputes/query.test.ts b/test/disputes/query.test.ts index b4ef9bcdf..10edc39f2 100644 --- a/test/disputes/query.test.ts +++ b/test/disputes/query.test.ts @@ -153,9 +153,6 @@ describe('DisputeManager:Query', async () => { await grt.connect(dst.signer).approve(disputeManager.address, fishermanTokens) } - // Allow the asset holder - await staking.connect(governor.signer).setAssetHolder(assetHolder.address, true) - // Create an attestation const attestation = await buildAttestation(receipt, indexer1ChannelKey.privKey) diff --git a/test/payments/allocationExchange.test.ts b/test/payments/allocationExchange.test.ts index fba672e03..941d4d9f9 100644 --- a/test/payments/allocationExchange.test.ts +++ b/test/payments/allocationExchange.test.ts @@ -77,7 +77,6 @@ describe('AllocationExchange', () => { await grt.connect(governor.signer).mint(allocationExchange.address, exchangeTokens) // Ensure the exchange is correctly setup - await staking.connect(governor.signer).setAssetHolder(allocationExchange.address, true) await allocationExchange.connect(governor.signer).setAuthority(authority.address, true) await allocationExchange.approveAll() }) diff --git a/test/rewards/rewards.test.ts b/test/rewards/rewards.test.ts index 7fa60d5f6..d3fcc4bc7 100644 --- a/test/rewards/rewards.test.ts +++ b/test/rewards/rewards.test.ts @@ -1013,7 +1013,6 @@ describe('Rewards', () => { // allow the asset holder const tokensToCollect = toGRT('10000') - await staking.connect(governor.signer).setAssetHolder(assetHolder.address, true) // signal in two subgraphs in the same block const subgraphs = [subgraphDeploymentID1, subgraphDeploymentID2] diff --git a/test/staking/allocation.test.ts b/test/staking/allocation.test.ts index fa7b71765..10458a2b8 100644 --- a/test/staking/allocation.test.ts +++ b/test/staking/allocation.test.ts @@ -117,9 +117,13 @@ describe('Staking:Allocation', () => { // This function tests collect with state updates const shouldCollect = async ( tokensToCollect: BigNumber, - _allocationID?: string, + options: { + allocationID?: string + expectEvent?: boolean + } = {}, ): Promise<{ queryRebates: BigNumber; queryFeesBurnt: BigNumber }> => { - const alloID = _allocationID ?? allocationID + const expectEvent = options.expectEvent ?? true + const alloID = options.allocationID ?? allocationID const alloStateBefore = await staking.getAllocationState(alloID) // Should have a particular state before collecting expect(alloStateBefore).to.be.oneOf([AllocationState.Active, AllocationState.Closed]) @@ -174,21 +178,26 @@ describe('Staking:Allocation', () => { // Collect tokens from allocation const tx = staking.connect(assetHolder.signer).collect(tokensToCollect, alloID) - await expect(tx) - .emit(staking, 'RebateCollected') - .withArgs( - assetHolder.address, - indexer.address, - subgraphDeploymentID, - alloID, - await epochManager.currentEpoch(), - tokensToCollect, - protocolFees, - curationFees, - queryFees, - queryRebates, - delegationRewards, - ) + if (expectEvent) { + await expect(tx) + .emit(staking, 'RebateCollected') + .withArgs( + assetHolder.address, + indexer.address, + subgraphDeploymentID, + alloID, + await epochManager.currentEpoch(), + tokensToCollect, + protocolFees, + curationFees, + queryFees, + queryRebates, + delegationRewards, + ) + } else { + await expect(tx).to.not.be.reverted + await expect(tx).to.not.emit(staking, 'RebateCollected') + } // After state const afterTokenSupply = await grt.totalSupply() @@ -255,8 +264,9 @@ describe('Staking:Allocation', () => { ) // Collect `tokensToCollect` with a single voucher - const rebatedAmountFull = (await shouldCollect(totalTokensToCollect, anotherAllocationID)) - .queryRebates + const rebatedAmountFull = ( + await shouldCollect(totalTokensToCollect, { allocationID: anotherAllocationID }) + ).queryRebates // Check rebated amounts match, allow a small error margin of 5 wei // Due to rounding it's not possible to guarantee an exact match in case of multiple collections @@ -322,9 +332,6 @@ describe('Staking:Allocation', () => { // Give some funds to the delegator and approve staking contract to use funds on delegator behalf await grt.connect(governor.signer).mint(delegator.address, tokensToDelegate) await grt.connect(delegator.signer).approve(staking.address, tokensToDelegate) - - // Allow the asset holder - await staking.connect(governor.signer).setAssetHolder(assetHolder.address, true) }) beforeEach(async function () { @@ -593,7 +600,7 @@ describe('Staking:Allocation', () => { }) it('should collect zero tokens', async function () { - await shouldCollect(toGRT('0')) + await shouldCollect(toGRT('0'), { expectEvent: false }) }) it('should allow multiple collections on the same allocation', async function () { @@ -641,7 +648,7 @@ describe('Staking:Allocation', () => { ) // Collect from closed allocation, should get no rebates - const rebates = await shouldCollect(tokensToCollect, anotherAllocationID) + const rebates = await shouldCollect(tokensToCollect, { allocationID: anotherAllocationID }) expect(rebates.queryRebates).eq(BigNumber.from(0)) expect(rebates.queryFeesBurnt).eq(tokensToCollect) }) @@ -665,7 +672,9 @@ describe('Staking:Allocation', () => { // First collection // Indexer gets 100% of the query fees due to α = 0 - const firstRebates = await shouldCollect(firstTokensToCollect, anotherAllocationID) + const firstRebates = await shouldCollect(firstTokensToCollect, { + allocationID: anotherAllocationID, + }) expect(firstRebates.queryRebates).eq(firstTokensToCollect) expect(firstRebates.queryFeesBurnt).eq(BigNumber.from(0)) @@ -675,14 +684,18 @@ describe('Staking:Allocation', () => { // Second collection // Indexer gets 0% of the query fees // Parameters changed so now they are over-rebated and should get "negative rebates", instead they get 0 - const secondRebates = await shouldCollect(secondTokensToCollect, anotherAllocationID) + const secondRebates = await shouldCollect(secondTokensToCollect, { + allocationID: anotherAllocationID, + }) expect(secondRebates.queryRebates).eq(BigNumber.from(0)) expect(secondRebates.queryFeesBurnt).eq(secondTokensToCollect) // Third collection // Previous collection plus this new one tip the balance and indexer is no longer over-rebated // They get rebates and burn again - const thirdRebates = await shouldCollect(thirdTokensToCollect, anotherAllocationID) + const thirdRebates = await shouldCollect(thirdTokensToCollect, { + allocationID: anotherAllocationID, + }) expect(thirdRebates.queryRebates).gt(BigNumber.from(0)) expect(thirdRebates.queryFeesBurnt).gt(BigNumber.from(0)) }) @@ -706,7 +719,9 @@ describe('Staking:Allocation', () => { // First collection // Indexer gets rebates and burn - const firstRebates = await shouldCollect(firstTokensToCollect, anotherAllocationID) + const firstRebates = await shouldCollect(firstTokensToCollect, { + allocationID: anotherAllocationID, + }) expect(firstRebates.queryRebates).gt(BigNumber.from(0)) expect(firstRebates.queryFeesBurnt).gt(BigNumber.from(0)) @@ -716,18 +731,26 @@ describe('Staking:Allocation', () => { // Second collection // Indexer gets 100% of the query fees // Parameters changed so now they are under-rebated and should get more than the available amount but we cap it - const secondRebates = await shouldCollect(secondTokensToCollect, anotherAllocationID) + const secondRebates = await shouldCollect(secondTokensToCollect, { + allocationID: anotherAllocationID, + }) expect(secondRebates.queryRebates).eq(secondTokensToCollect) expect(secondRebates.queryFeesBurnt).eq(BigNumber.from(0)) // Third collection // Previous collection plus this new one tip the balance and indexer is no longer under-rebated // They get rebates and burn again - const thirdRebates = await shouldCollect(thirdTokensToCollect, anotherAllocationID) + const thirdRebates = await shouldCollect(thirdTokensToCollect, { + allocationID: anotherAllocationID, + }) expect(thirdRebates.queryRebates).gt(BigNumber.from(0)) expect(thirdRebates.queryFeesBurnt).gt(BigNumber.from(0)) }) + it('should collect zero tokens', async function () { + await shouldCollect(toGRT('0'), { expectEvent: false }) + }) + it('should get stuck under-rebated if alpha is changed to zero', async function () { // Set up a new allocation with `tokensToAllocate` staked await staking.connect(indexer.signer).stake(tokensToStake) @@ -742,7 +765,9 @@ describe('Staking:Allocation', () => { // First collection // Indexer gets rebates and burn - const firstRebates = await shouldCollect(tokensToCollect, anotherAllocationID) + const firstRebates = await shouldCollect(tokensToCollect, { + allocationID: anotherAllocationID, + }) expect(firstRebates.queryRebates).gt(BigNumber.from(0)) expect(firstRebates.queryFeesBurnt).gt(BigNumber.from(0)) @@ -754,7 +779,9 @@ describe('Staking:Allocation', () => { // Parameters changed so now they are under-rebated and should get more than the available amount but we cap it // Distributed amount will never catch up due to the initial collection which was less than 100% for (const _i of [...Array(10).keys()]) { - const succesiveRebates = await shouldCollect(tokensToCollect, anotherAllocationID) + const succesiveRebates = await shouldCollect(tokensToCollect, { + allocationID: anotherAllocationID, + }) expect(succesiveRebates.queryRebates).eq(tokensToCollect) expect(succesiveRebates.queryFeesBurnt).eq(BigNumber.from(0)) } diff --git a/test/staking/configuration.test.ts b/test/staking/configuration.test.ts index 4e4a2ed7a..53a0b312c 100644 --- a/test/staking/configuration.test.ts +++ b/test/staking/configuration.test.ts @@ -88,35 +88,6 @@ describe('Staking:Config', () => { }) }) - describe('setAssetHolder', function () { - it('should set `assetHolder`', async function () { - expect(await staking.assetHolders(me.address)).eq(false) - - const tx1 = staking.connect(governor.signer).setAssetHolder(me.address, true) - await expect(tx1) - .emit(staking, 'AssetHolderUpdate') - .withArgs(governor.address, me.address, true) - expect(await staking.assetHolders(me.address)).eq(true) - - const tx2 = staking.connect(governor.signer).setAssetHolder(me.address, false) - await expect(tx2) - .emit(staking, 'AssetHolderUpdate') - .withArgs(governor.address, me.address, false) - await staking.connect(governor.signer).setAssetHolder(me.address, false) - expect(await staking.assetHolders(me.address)).eq(false) - }) - - it('reject set `assetHolder` if not allowed', async function () { - const tx = staking.connect(other.signer).setAssetHolder(me.address, true) - await expect(tx).revertedWith('Only Controller governor') - }) - - it('reject set `assetHolder` to address zero', async function () { - const tx = staking.connect(governor.signer).setAssetHolder(AddressZero, true) - await expect(tx).revertedWith('!assetHolder') - }) - }) - describe('curationPercentage', function () { it('should set `curationPercentage`', async function () { const newValue = toBN('5') diff --git a/test/staking/delegation.test.ts b/test/staking/delegation.test.ts index 35a65f997..3ca326bef 100644 --- a/test/staking/delegation.test.ts +++ b/test/staking/delegation.test.ts @@ -194,9 +194,6 @@ describe('Staking::Delegation', () => { await grt.connect(governor.signer).mint(wallet.address, toGRT('1000000')) await grt.connect(wallet.signer).approve(staking.address, toGRT('1000000')) } - - // Allow the asset holder - await staking.connect(governor.signer).setAssetHolder(assetHolder.address, true) }) beforeEach(async function () {