From bd0c99d369540fc29f08c207248e679b1f6332ce Mon Sep 17 00:00:00 2001 From: Ariel Barmat Date: Mon, 19 Jul 2021 00:34:09 -0300 Subject: [PATCH 1/6] gns: use convenience token utils for transferring tokens --- contracts/discovery/GNS.sol | 38 ++++++++++++------------------ contracts/discovery/GNSStorage.sol | 1 - 2 files changed, 15 insertions(+), 24 deletions(-) diff --git a/contracts/discovery/GNS.sol b/contracts/discovery/GNS.sol index 4cbdc768c..8bf60ae8e 100644 --- a/contracts/discovery/GNS.sol +++ b/contracts/discovery/GNS.sol @@ -7,6 +7,7 @@ import "@openzeppelin/contracts/math/SafeMath.sol"; import "../bancor/BancorFormula.sol"; import "../upgrades/GraphUpgradeable.sol"; +import "../utils/TokenUtils.sol"; import "./IGNS.sol"; import "./GNSStorage.sol"; @@ -408,10 +409,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 +459,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 +477,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 +518,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 +561,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 +579,8 @@ contract GNS is GNSV1Storage, GraphUpgradeable, IGNS { uint256 _tokensIn ) public - override view + override returns ( uint256, uint256, @@ -615,7 +607,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 +625,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 +653,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 +675,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 +687,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 0ad8514b2..12e79abdb 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 { From 026c864ea577fdae1816d068623ee5d92c83c5dc Mon Sep 17 00:00:00 2001 From: Ariel Barmat Date: Mon, 19 Jul 2021 01:17:01 -0300 Subject: [PATCH 2/6] gns: reduce bytecode by using an inline function for access --- contracts/discovery/GNS.sol | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/contracts/discovery/GNS.sol b/contracts/discovery/GNS.sol index 8bf60ae8e..3d3164edc 100644 --- a/contracts/discovery/GNS.sol +++ b/contracts/discovery/GNS.sol @@ -22,6 +22,8 @@ import "./GNSStorage.sol"; contract GNS is GNSV1Storage, GraphUpgradeable, IGNS { using SafeMath for uint256; + // -- Constants -- + uint256 private constant MAX_UINT256 = 2**256 - 1; // 100% in parts per million @@ -137,16 +139,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 { 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. */ From d2a9f725ac2ee7afd4eb658bb0291ba8291de393 Mon Sep 17 00:00:00 2001 From: Ariel Barmat Date: Mon, 19 Jul 2021 02:17:13 -0300 Subject: [PATCH 3/6] gns: add multicall for batch calls --- contracts/base/IMultiCall.sol | 17 +++++++++++++++++ contracts/base/MultiCall.sol | 34 ++++++++++++++++++++++++++++++++++ contracts/discovery/GNS.sol | 7 +++++-- test/gns.test.ts | 27 +++++++++++++++++++++++++-- 4 files changed, 81 insertions(+), 4 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..26e69dc3a --- /dev/null +++ b/contracts/base/IMultiCall.sol @@ -0,0 +1,17 @@ +// SPDX-License-Identifier: MIT + +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..12561fd81 --- /dev/null +++ b/contracts/base/MultiCall.sol @@ -0,0 +1,34 @@ +// SPDX-License-Identifier: MIT + +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 3d3164edc..0fdbd8121 100644 --- a/contracts/discovery/GNS.sol +++ b/contracts/discovery/GNS.sol @@ -5,6 +5,7 @@ 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"; @@ -18,8 +19,10 @@ 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 -- @@ -145,7 +148,7 @@ contract GNS is GNSV1Storage, GraphUpgradeable, IGNS { * @dev Check if the owner is the graph account * @param _graphAccount Address of the graph account */ - function _isGraphAccountOwner(address _graphAccount) private { + function _isGraphAccountOwner(address _graphAccount) private view { address graphAccountOwner = erc1056Registry.identityOwner(_graphAccount); require(graphAccountOwner == msg.sender, "GNS: Only graph account owner can call"); } diff --git a/test/gns.test.ts b/test/gns.test.ts index 12986b21d..43c7a1546 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,27 @@ 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]) + }) + }) }) From 94b846437fcf5257b5b42b7beeac912ea29a342b Mon Sep 17 00:00:00 2001 From: Ariel Barmat Date: Thu, 22 Jul 2021 20:29:08 -0300 Subject: [PATCH 4/6] gns: add security tests to multicall --- test/gns.test.ts | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/test/gns.test.ts b/test/gns.test.ts index 43c7a1546..6ae539206 100644 --- a/test/gns.test.ts +++ b/test/gns.test.ts @@ -1005,5 +1005,46 @@ describe('GNS', () => { // 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", + ) + }) }) }) From 8373b573ab8550e7520b79393f24e4c9836c1af6 Mon Sep 17 00:00:00 2001 From: Ariel Barmat Date: Tue, 7 Sep 2021 14:55:55 -0300 Subject: [PATCH 5/6] gns: make the multicall contract name to match the file --- contracts/base/{IMultiCall.sol => IMulticall.sol} | 0 contracts/base/{MultiCall.sol => Multicall.sol} | 2 +- contracts/discovery/GNS.sol | 2 +- 3 files changed, 2 insertions(+), 2 deletions(-) rename contracts/base/{IMultiCall.sol => IMulticall.sol} (100%) rename contracts/base/{MultiCall.sol => Multicall.sol} (97%) diff --git a/contracts/base/IMultiCall.sol b/contracts/base/IMulticall.sol similarity index 100% rename from contracts/base/IMultiCall.sol rename to contracts/base/IMulticall.sol diff --git a/contracts/base/MultiCall.sol b/contracts/base/Multicall.sol similarity index 97% rename from contracts/base/MultiCall.sol rename to contracts/base/Multicall.sol index 12561fd81..535c88dd5 100644 --- a/contracts/base/MultiCall.sol +++ b/contracts/base/Multicall.sol @@ -3,7 +3,7 @@ pragma solidity ^0.7.3; pragma experimental ABIEncoderV2; -import "./IMultiCall.sol"; +import "./IMulticall.sol"; // Inspired by https://github.com/Uniswap/uniswap-v3-periphery/blob/main/contracts/base/Multicall.sol // Note: Removed payable from the multicall diff --git a/contracts/discovery/GNS.sol b/contracts/discovery/GNS.sol index 0fdbd8121..52764db8b 100644 --- a/contracts/discovery/GNS.sol +++ b/contracts/discovery/GNS.sol @@ -5,7 +5,7 @@ pragma experimental ABIEncoderV2; import "@openzeppelin/contracts/math/SafeMath.sol"; -import "../base/MultiCall.sol"; +import "../base/Multicall.sol"; import "../bancor/BancorFormula.sol"; import "../upgrades/GraphUpgradeable.sol"; import "../utils/TokenUtils.sol"; From 2f18eb08880346c2c02d401ccc57afe7f0326f41 Mon Sep 17 00:00:00 2001 From: Ariel Barmat Date: Tue, 7 Sep 2021 15:19:09 -0300 Subject: [PATCH 6/6] gns: update license pragma --- contracts/base/IMulticall.sol | 2 +- contracts/base/Multicall.sol | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/base/IMulticall.sol b/contracts/base/IMulticall.sol index 26e69dc3a..87daa0453 100644 --- a/contracts/base/IMulticall.sol +++ b/contracts/base/IMulticall.sol @@ -1,4 +1,4 @@ -// SPDX-License-Identifier: MIT +// SPDX-License-Identifier: GPL-2.0-or-later pragma solidity ^0.7.3; pragma experimental ABIEncoderV2; diff --git a/contracts/base/Multicall.sol b/contracts/base/Multicall.sol index 535c88dd5..d3ff3055b 100644 --- a/contracts/base/Multicall.sol +++ b/contracts/base/Multicall.sol @@ -1,4 +1,4 @@ -// SPDX-License-Identifier: MIT +// SPDX-License-Identifier: GPL-2.0-or-later pragma solidity ^0.7.3; pragma experimental ABIEncoderV2;