From 329e92fb3c897f68bddbd93abf12278167116266 Mon Sep 17 00:00:00 2001 From: Ariel Barmat Date: Tue, 7 Sep 2021 15:32:00 -0300 Subject: [PATCH] Allow batch transactions using multicall in the GNS (#485) Add multicall for batched calls to the GNS By using a multicall a user can batch multiple operations into a single transaction. An example of this is if the owner publishing a subgraph decides to mint some signal on that subgraph. * Use convenience token utils for transferring tokens * Reduce bytecode by using an inline function for access * Introduce a multicall abstract contract for batch calls * Add security tests to multicall --- contracts/base/IMulticall.sol | 17 ++++++++ contracts/base/Multicall.sol | 34 +++++++++++++++ contracts/discovery/GNS.sol | 65 ++++++++++++++++------------ contracts/discovery/GNSStorage.sol | 1 - test/gns.test.ts | 68 +++++++++++++++++++++++++++++- 5 files changed, 154 insertions(+), 31 deletions(-) create mode 100644 contracts/base/IMulticall.sol create mode 100644 contracts/base/Multicall.sol diff --git a/contracts/base/IMulticall.sol b/contracts/base/IMulticall.sol new file mode 100644 index 000000000..87daa0453 --- /dev/null +++ b/contracts/base/IMulticall.sol @@ -0,0 +1,17 @@ +// SPDX-License-Identifier: GPL-2.0-or-later + +pragma solidity ^0.7.3; +pragma experimental ABIEncoderV2; + +/** + * @title Multicall interface + * @notice Enables calling multiple methods in a single call to the contract + */ +interface IMulticall { + /** + * @notice Call multiple functions in the current contract and return the data from all of them if they all succeed + * @param data The encoded function data for each of the calls to make to this contract + * @return results The results from each of the calls passed in via data + */ + function multicall(bytes[] calldata data) external returns (bytes[] memory results); +} diff --git a/contracts/base/Multicall.sol b/contracts/base/Multicall.sol new file mode 100644 index 000000000..d3ff3055b --- /dev/null +++ b/contracts/base/Multicall.sol @@ -0,0 +1,34 @@ +// SPDX-License-Identifier: GPL-2.0-or-later + +pragma solidity ^0.7.3; +pragma experimental ABIEncoderV2; + +import "./IMulticall.sol"; + +// Inspired by https://github.com/Uniswap/uniswap-v3-periphery/blob/main/contracts/base/Multicall.sol +// Note: Removed payable from the multicall + +/** + * @title Multicall + * @notice Enables calling multiple methods in a single call to the contract + */ +abstract contract Multicall is IMulticall { + /// @inheritdoc IMulticall + function multicall(bytes[] calldata data) external override returns (bytes[] memory results) { + results = new bytes[](data.length); + for (uint256 i = 0; i < data.length; i++) { + (bool success, bytes memory result) = address(this).delegatecall(data[i]); + + if (!success) { + // Next 5 lines from https://ethereum.stackexchange.com/a/83577 + if (result.length < 68) revert(); + assembly { + result := add(result, 0x04) + } + revert(abi.decode(result, (string))); + } + + results[i] = result; + } + } +} diff --git a/contracts/discovery/GNS.sol b/contracts/discovery/GNS.sol index 90b91f9ef..ea50ea4bc 100644 --- a/contracts/discovery/GNS.sol +++ b/contracts/discovery/GNS.sol @@ -5,8 +5,10 @@ pragma experimental ABIEncoderV2; import "@openzeppelin/contracts/math/SafeMath.sol"; +import "../base/Multicall.sol"; import "../bancor/BancorFormula.sol"; import "../upgrades/GraphUpgradeable.sol"; +import "../utils/TokenUtils.sol"; import "./IGNS.sol"; import "./GNSStorage.sol"; @@ -17,10 +19,14 @@ import "./GNSStorage.sol"; * used in the scope of the Graph Network. It translates subgraph names into subgraph versions. * Each version is associated with a Subgraph Deployment. The contract has no knowledge of * human-readable names. All human readable names emitted in events. + * The contract implements a multicall behaviour to support batching multiple calls in a single + * transaction. */ -contract GNS is GNSV1Storage, GraphUpgradeable, IGNS { +contract GNS is GNSV1Storage, GraphUpgradeable, IGNS, Multicall { using SafeMath for uint256; + // -- Constants -- + uint256 private constant MAX_UINT256 = 2**256 - 1; // 100% in parts per million @@ -136,16 +142,28 @@ contract GNS is GNSV1Storage, GraphUpgradeable, IGNS { uint256 withdrawnGRT ); + // -- Modifiers -- + /** - @dev Modifier that allows a function to be called by owner of a graph account - @param _graphAccount Address of the graph account - */ - modifier onlyGraphAccountOwner(address _graphAccount) { + * @dev Check if the owner is the graph account + * @param _graphAccount Address of the graph account + */ + function _isGraphAccountOwner(address _graphAccount) private view { address graphAccountOwner = erc1056Registry.identityOwner(_graphAccount); require(graphAccountOwner == msg.sender, "GNS: Only graph account owner can call"); + } + + /** + * @dev Modifier that allows a function to be called by owner of a graph account + * @param _graphAccount Address of the graph account + */ + modifier onlyGraphAccountOwner(address _graphAccount) { + _isGraphAccountOwner(_graphAccount); _; } + // -- Functions -- + /** * @dev Initialize this contract. */ @@ -408,10 +426,7 @@ contract GNS is GNSV1Storage, GraphUpgradeable, IGNS { ); // Pull tokens from sender - require( - graphToken().transferFrom(msg.sender, address(this), _tokensIn), - "GNS: Cannot transfer tokens to mint n signal" - ); + TokenUtils.pullTokens(graphToken(), msg.sender, _tokensIn); // Get name signal to mint for tokens deposited (uint256 vSignal, ) = curation().mint(namePool.subgraphDeploymentID, _tokensIn, 0); @@ -461,11 +476,8 @@ contract GNS is GNSV1Storage, GraphUpgradeable, IGNS { namePool.nSignal = namePool.nSignal.sub(_nSignal); namePool.curatorNSignal[msg.sender] = namePool.curatorNSignal[msg.sender].sub(_nSignal); - // Return the tokens to the nameCurator - require( - graphToken().transfer(msg.sender, tokens), - "GNS: Error sending nameCurators tokens" - ); + // Return the tokens to the curator + TokenUtils.pushTokens(graphToken(), msg.sender, tokens); emit NSignalBurned(_graphAccount, _subgraphNumber, msg.sender, _nSignal, vSignal, tokens); } @@ -482,6 +494,7 @@ contract GNS is GNSV1Storage, GraphUpgradeable, IGNS { // If no nSignal, then no need to burn vSignal if (namePool.nSignal != 0) { + // Note: No slippage, burn at any cost namePool.withdrawableGRT = curation().burn( namePool.subgraphDeploymentID, namePool.vSignal, @@ -522,10 +535,8 @@ contract GNS is GNSV1Storage, GraphUpgradeable, IGNS { namePool.nSignal = namePool.nSignal.sub(curatorNSignal); namePool.withdrawableGRT = namePool.withdrawableGRT.sub(tokensOut); - require( - graphToken().transfer(msg.sender, tokensOut), - "GNS: Error withdrawing tokens for nameCurator" - ); + // Return tokens to the curator + TokenUtils.pushTokens(graphToken(), msg.sender, tokensOut); emit GRTWithdrawn(_graphAccount, _subgraphNumber, msg.sender, curatorNSignal, tokensOut); } @@ -567,10 +578,8 @@ contract GNS is GNSV1Storage, GraphUpgradeable, IGNS { uint256 ownerTaxAdjustedUp = totalAdjustedUp.sub(_tokens); // Get the owner of the subgraph to reimburse the curation tax - require( - graphToken().transferFrom(_owner, address(this), ownerTaxAdjustedUp), - "GNS: Error reimbursing curation tax" - ); + TokenUtils.pullTokens(graphToken(), _owner, ownerTaxAdjustedUp); + return totalAdjustedUp; } @@ -587,8 +596,8 @@ contract GNS is GNSV1Storage, GraphUpgradeable, IGNS { uint256 _tokensIn ) public - override view + override returns ( uint256, uint256, @@ -615,7 +624,7 @@ contract GNS is GNSV1Storage, GraphUpgradeable, IGNS { address _graphAccount, uint256 _subgraphNumber, uint256 _nSignalIn - ) public override view returns (uint256, uint256) { + ) public view override returns (uint256, uint256) { NameCurationPool storage namePool = nameSignals[_graphAccount][_subgraphNumber]; uint256 vSignal = nSignalToVSignal(_graphAccount, _subgraphNumber, _nSignalIn); uint256 tokensOut = curation().signalToTokens(namePool.subgraphDeploymentID, vSignal); @@ -633,7 +642,7 @@ contract GNS is GNSV1Storage, GraphUpgradeable, IGNS { address _graphAccount, uint256 _subgraphNumber, uint256 _vSignalIn - ) public override view returns (uint256) { + ) public view override returns (uint256) { NameCurationPool storage namePool = nameSignals[_graphAccount][_subgraphNumber]; // Handle initialization by using 1:1 version to name signal @@ -661,7 +670,7 @@ contract GNS is GNSV1Storage, GraphUpgradeable, IGNS { address _graphAccount, uint256 _subgraphNumber, uint256 _nSignalIn - ) public override view returns (uint256) { + ) public view override returns (uint256) { NameCurationPool storage namePool = nameSignals[_graphAccount][_subgraphNumber]; return BancorFormula(bondingCurve).calculateSaleReturn( @@ -683,7 +692,7 @@ contract GNS is GNSV1Storage, GraphUpgradeable, IGNS { address _graphAccount, uint256 _subgraphNumber, address _curator - ) public override view returns (uint256) { + ) public view override returns (uint256) { return nameSignals[_graphAccount][_subgraphNumber].curatorNSignal[_curator]; } @@ -695,8 +704,8 @@ contract GNS is GNSV1Storage, GraphUpgradeable, IGNS { */ function isPublished(address _graphAccount, uint256 _subgraphNumber) public - override view + override returns (bool) { return subgraphs[_graphAccount][_subgraphNumber] != 0; diff --git a/contracts/discovery/GNSStorage.sol b/contracts/discovery/GNSStorage.sol index c9caf51af..a451fba13 100644 --- a/contracts/discovery/GNSStorage.sol +++ b/contracts/discovery/GNSStorage.sol @@ -6,7 +6,6 @@ pragma experimental ABIEncoderV2; import "../governance/Managed.sol"; import "./erc1056/IEthereumDIDRegistry.sol"; - import "./IGNS.sol"; contract GNSV1Storage is Managed { diff --git a/test/gns.test.ts b/test/gns.test.ts index 12986b21d..6ae539206 100644 --- a/test/gns.test.ts +++ b/test/gns.test.ts @@ -2,11 +2,11 @@ import { expect } from 'chai' import { ethers, ContractTransaction, BigNumber, Event } from 'ethers' import { GNS } from '../build/types/GNS' -import { getAccounts, randomHexBytes, Account, toGRT } from './lib/testHelpers' -import { NetworkFixture } from './lib/fixtures' import { GraphToken } from '../build/types/GraphToken' import { Curation } from '../build/types/Curation' +import { getAccounts, randomHexBytes, Account, toGRT } from './lib/testHelpers' +import { NetworkFixture } from './lib/fixtures' import { toBN, formatGRT } from './lib/testHelpers' interface Subgraph { @@ -983,4 +983,68 @@ describe('GNS', () => { await gns.connect(me.signer).mintNSignal(me.address, 1, toGRT('10'), 0) }) }) + + describe('batch calls', function () { + it('should publish new subgraph and mint signal in single transaction', async function () { + // Create a subgraph + const tx1 = await gns.populateTransaction.publishNewSubgraph( + me.address, + subgraph0.subgraphDeploymentID, + subgraph0.versionMetadata, + subgraph0.subgraphMetadata, + ) + // Curate on the subgraph + const subgraphNumber = await gns.graphAccountSubgraphNumbers(me.address) + const tx2 = await gns.populateTransaction.mintNSignal( + me.address, + subgraphNumber, + toGRT('90000'), + 0, + ) + + // Batch send transaction + await gns.connect(me.signer).multicall([tx1.data, tx2.data]) + }) + + it('should revert if batching a call to non-authorized function', async function () { + // Call a forbidden function + const tx1 = await gns.populateTransaction.setOwnerTaxPercentage(100) + + // Create a subgraph + const tx2 = await gns.populateTransaction.publishNewSubgraph( + me.address, + subgraph0.subgraphDeploymentID, + subgraph0.versionMetadata, + subgraph0.subgraphMetadata, + ) + + // Batch send transaction + const tx = gns.connect(me.signer).multicall([tx1.data, tx2.data]) + await expect(tx).revertedWith('Caller must be Controller governor') + }) + + it('should revert if trying to call a private function', async function () { + // Craft call a private function + const hash = ethers.utils.id('_setOwnerTaxPercentage(uint32)') + const functionHash = hash.slice(0, 10) + const calldata = ethers.utils.arrayify( + ethers.utils.defaultAbiCoder.encode(['uint32'], ['100']), + ) + const bogusPayload = ethers.utils.concat([functionHash, calldata]) + + // Create a subgraph + const tx2 = await gns.populateTransaction.publishNewSubgraph( + me.address, + subgraph0.subgraphDeploymentID, + subgraph0.versionMetadata, + subgraph0.subgraphMetadata, + ) + + // Batch send transaction + const tx = gns.connect(me.signer).multicall([bogusPayload, tx2.data]) + await expect(tx).revertedWith( + "function selector was not recognized and there's no fallback function", + ) + }) + }) })