From 2c4ac9d8a879557f08bc448f243998282d9f8215 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Migone?= Date: Mon, 24 Oct 2022 12:22:34 -0300 Subject: [PATCH 01/17] fix(c4-117-g03): state variables should be cached in stack variables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tomás Migone --- contracts/governance/Governed.sol | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/contracts/governance/Governed.sol b/contracts/governance/Governed.sol index 2a87389d9..36d96d13a 100644 --- a/contracts/governance/Governed.sol +++ b/contracts/governance/Governed.sol @@ -51,15 +51,16 @@ contract Governed { * This function must called by the pending governor. */ function acceptOwnership() external { + address _pendingGovernor = pendingGovernor; require( - pendingGovernor != address(0) && msg.sender == pendingGovernor, + _pendingGovernor != address(0) && msg.sender == _pendingGovernor, "Caller must be pending governor" ); address oldGovernor = governor; - address oldPendingGovernor = pendingGovernor; + address oldPendingGovernor = _pendingGovernor; - governor = pendingGovernor; + governor = _pendingGovernor; pendingGovernor = address(0); emit NewOwnership(oldGovernor, governor); From 76865a492dc209bdda31bfed32a71d9b7a445517 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Migone?= Date: Mon, 24 Oct 2022 13:31:12 -0300 Subject: [PATCH 02/17] =?UTF-8?q?fix(c4-117-g05):=20require()/revert()?= =?UTF-8?q?=C2=A0strings=20longer=20than=2032=20bytes=20cost=20extra=20gas?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tomás Migone --- contracts/gateway/GraphTokenGateway.sol | 2 +- contracts/governance/Managed.sol | 2 +- contracts/upgrades/GraphProxy.sol | 6 +++--- contracts/upgrades/GraphUpgradeable.sol | 2 +- .../config/l1/l1GraphTokenGateway.test.ts | 12 ++++++------ .../config/l2/l2GraphTokenGateway.test.ts | 6 +++--- test/curation/configuration.test.ts | 8 ++++---- test/disputes/configuration.test.ts | 8 ++++---- test/gateway/bridgeEscrow.test.ts | 4 ++-- test/gateway/l1GraphTokenGateway.test.ts | 18 +++++++++--------- test/gns.test.ts | 6 +++--- test/l2/l2GraphTokenGateway.test.ts | 12 ++++++------ test/rewards/rewards.test.ts | 4 ++-- test/staking/configuration.test.ts | 18 +++++++++--------- test/staking/delegation.test.ts | 6 +++--- 15 files changed, 57 insertions(+), 57 deletions(-) diff --git a/contracts/gateway/GraphTokenGateway.sol b/contracts/gateway/GraphTokenGateway.sol index b239af00a..ed59b149a 100644 --- a/contracts/gateway/GraphTokenGateway.sol +++ b/contracts/gateway/GraphTokenGateway.sol @@ -21,7 +21,7 @@ abstract contract GraphTokenGateway is GraphUpgradeable, Pausable, Managed, ITok modifier onlyGovernorOrGuardian() { require( msg.sender == controller.getGovernor() || msg.sender == pauseGuardian, - "Only Governor or Guardian can call" + "Only Governor or Guardian" ); _; } diff --git a/contracts/governance/Managed.sol b/contracts/governance/Managed.sol index ce18fb009..fcdf73c36 100644 --- a/contracts/governance/Managed.sol +++ b/contracts/governance/Managed.sol @@ -50,7 +50,7 @@ contract Managed { } function _onlyGovernor() internal view { - require(msg.sender == controller.getGovernor(), "Caller must be Controller governor"); + require(msg.sender == controller.getGovernor(), "Only Controller governor"); } function _onlyController() internal view { diff --git a/contracts/upgrades/GraphProxy.sol b/contracts/upgrades/GraphProxy.sol index 69c88fd28..6e29396db 100644 --- a/contracts/upgrades/GraphProxy.sol +++ b/contracts/upgrades/GraphProxy.sol @@ -102,7 +102,7 @@ contract GraphProxy is GraphProxyStorage { * NOTE: Only the admin can call this function. */ function setAdmin(address _newAdmin) external ifAdmin { - require(_newAdmin != address(0), "Cannot change the admin of a proxy to the zero address"); + require(_newAdmin != address(0), "Admin cant be the zero address"); _setAdmin(_newAdmin); } @@ -138,10 +138,10 @@ contract GraphProxy is GraphProxyStorage { */ function _acceptUpgrade() internal { address _pendingImplementation = _pendingImplementation(); - require(Address.isContract(_pendingImplementation), "Implementation must be a contract"); + require(Address.isContract(_pendingImplementation), "Impl must be a contract"); require( _pendingImplementation != address(0) && msg.sender == _pendingImplementation, - "Caller must be the pending implementation" + "Only pending implementation" ); _setImplementation(_pendingImplementation); diff --git a/contracts/upgrades/GraphUpgradeable.sol b/contracts/upgrades/GraphUpgradeable.sol index 2331cae27..3f3b505ba 100644 --- a/contracts/upgrades/GraphUpgradeable.sol +++ b/contracts/upgrades/GraphUpgradeable.sol @@ -29,7 +29,7 @@ contract GraphUpgradeable { * @dev Check if the caller is the implementation. */ modifier onlyImpl() { - require(msg.sender == _implementation(), "Caller must be the implementation"); + require(msg.sender == _implementation(), "Only implementation"); _; } diff --git a/e2e/deployment/config/l1/l1GraphTokenGateway.test.ts b/e2e/deployment/config/l1/l1GraphTokenGateway.test.ts index 1f96bd946..a2d8edbae 100644 --- a/e2e/deployment/config/l1/l1GraphTokenGateway.test.ts +++ b/e2e/deployment/config/l1/l1GraphTokenGateway.test.ts @@ -34,38 +34,38 @@ describe('[L1] L1GraphTokenGateway configuration', function () { unauthorized.address, unauthorized.address, ) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) it('setL2TokenAddress should revert', async function () { const tx = L1GraphTokenGateway.connect(unauthorized).setL2TokenAddress(unauthorized.address) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) it('setL2CounterpartAddress should revert', async function () { const tx = L1GraphTokenGateway.connect(unauthorized).setL2CounterpartAddress( unauthorized.address, ) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) it('setEscrowAddress should revert', async function () { const tx = L1GraphTokenGateway.connect(unauthorized).setEscrowAddress(unauthorized.address) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) it('addToCallhookWhitelist should revert', async function () { const tx = L1GraphTokenGateway.connect(unauthorized).addToCallhookWhitelist( unauthorized.address, ) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) it('removeFromCallhookWhitelist should revert', async function () { const tx = L1GraphTokenGateway.connect(unauthorized).removeFromCallhookWhitelist( unauthorized.address, ) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) it('finalizeInboundTransfer should revert', async function () { diff --git a/e2e/deployment/config/l2/l2GraphTokenGateway.test.ts b/e2e/deployment/config/l2/l2GraphTokenGateway.test.ts index f572a37be..e4626b59b 100644 --- a/e2e/deployment/config/l2/l2GraphTokenGateway.test.ts +++ b/e2e/deployment/config/l2/l2GraphTokenGateway.test.ts @@ -31,19 +31,19 @@ describe('[L2] L2GraphTokenGateway configuration', function () { it('setL2Router should revert', async function () { const tx = L2GraphTokenGateway.connect(unauthorized).setL2Router(unauthorized.address) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) it('setL1TokenAddress should revert', async function () { const tx = L2GraphTokenGateway.connect(unauthorized).setL1TokenAddress(unauthorized.address) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) it('setL1CounterpartAddress should revert', async function () { const tx = L2GraphTokenGateway.connect(unauthorized).setL1CounterpartAddress( unauthorized.address, ) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) it('finalizeInboundTransfer should revert', async function () { diff --git a/test/curation/configuration.test.ts b/test/curation/configuration.test.ts index b2424c784..f475d8f30 100644 --- a/test/curation/configuration.test.ts +++ b/test/curation/configuration.test.ts @@ -55,7 +55,7 @@ describe('Curation:Config', () => { it('reject set `defaultReserveRatio` if not allowed', async function () { const tx = curation.connect(me.signer).setDefaultReserveRatio(defaults.curation.reserveRatio) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) }) @@ -79,7 +79,7 @@ describe('Curation:Config', () => { const tx = curation .connect(me.signer) .setMinimumCurationDeposit(defaults.curation.minimumCurationDeposit) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) }) @@ -99,7 +99,7 @@ describe('Curation:Config', () => { it('reject set `curationTaxPercentage` if not allowed', async function () { const tx = curation.connect(me.signer).setCurationTaxPercentage(0) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) }) @@ -124,7 +124,7 @@ describe('Curation:Config', () => { it('reject set `curationTokenMaster` if not allowed', async function () { const newCurationTokenMaster = curation.address const tx = curation.connect(me.signer).setCurationTokenMaster(newCurationTokenMaster) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) }) }) diff --git a/test/disputes/configuration.test.ts b/test/disputes/configuration.test.ts index 28bc92ed5..c663c31d7 100644 --- a/test/disputes/configuration.test.ts +++ b/test/disputes/configuration.test.ts @@ -49,7 +49,7 @@ describe('DisputeManager:Config', () => { it('reject set `arbitrator` if not allowed', async function () { const tx = disputeManager.connect(me.signer).setArbitrator(arbitrator.address) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) it('reject set `arbitrator` to address zero', async function () { @@ -74,7 +74,7 @@ describe('DisputeManager:Config', () => { it('reject set `minimumDeposit` if not allowed', async function () { const newValue = toBN('1') const tx = disputeManager.connect(me.signer).setMinimumDeposit(newValue) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) }) @@ -97,7 +97,7 @@ describe('DisputeManager:Config', () => { it('reject set `fishermanRewardPercentage` if not allowed', async function () { const tx = disputeManager.connect(me.signer).setFishermanRewardPercentage(50) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) }) @@ -127,7 +127,7 @@ describe('DisputeManager:Config', () => { it('reject set `slashingPercentage` if not allowed', async function () { const tx = disputeManager.connect(me.signer).setSlashingPercentage(50, 50) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) }) }) diff --git a/test/gateway/bridgeEscrow.test.ts b/test/gateway/bridgeEscrow.test.ts index ec3eedb73..fabe9de27 100644 --- a/test/gateway/bridgeEscrow.test.ts +++ b/test/gateway/bridgeEscrow.test.ts @@ -41,7 +41,7 @@ describe('BridgeEscrow', () => { describe('approveAll', function () { it('cannot be called by someone other than the governor', async function () { const tx = bridgeEscrow.connect(tokenReceiver.signer).approveAll(spender.address) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) it('allows a spender to transfer GRT held by the contract', async function () { expect(await grt.allowance(bridgeEscrow.address, spender.address)).eq(0) @@ -62,7 +62,7 @@ describe('BridgeEscrow', () => { describe('revokeAll', function () { it('cannot be called by someone other than the governor', async function () { const tx = bridgeEscrow.connect(tokenReceiver.signer).revokeAll(spender.address) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) it("revokes a spender's permission to transfer GRT held by the contract", async function () { await bridgeEscrow.connect(governor.signer).approveAll(spender.address) diff --git a/test/gateway/l1GraphTokenGateway.test.ts b/test/gateway/l1GraphTokenGateway.test.ts index 70984717a..7c2d3db5e 100644 --- a/test/gateway/l1GraphTokenGateway.test.ts +++ b/test/gateway/l1GraphTokenGateway.test.ts @@ -133,7 +133,7 @@ describe('L1GraphTokenGateway', () => { const tx = l1GraphTokenGateway .connect(tokenSender.signer) .setArbitrumAddresses(inboxMock.address, mockRouter.address) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) it('rejects setting an EOA as router or inbox', async function () { let tx = l1GraphTokenGateway @@ -162,7 +162,7 @@ describe('L1GraphTokenGateway', () => { const tx = l1GraphTokenGateway .connect(tokenSender.signer) .setL2TokenAddress(mockL2GRT.address) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) it('sets l2GRT', async function () { const tx = l1GraphTokenGateway.connect(governor.signer).setL2TokenAddress(mockL2GRT.address) @@ -176,7 +176,7 @@ describe('L1GraphTokenGateway', () => { const tx = l1GraphTokenGateway .connect(tokenSender.signer) .setL2CounterpartAddress(mockL2Gateway.address) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) it('sets L2Counterpart', async function () { const tx = l1GraphTokenGateway @@ -193,7 +193,7 @@ describe('L1GraphTokenGateway', () => { const tx = l1GraphTokenGateway .connect(tokenSender.signer) .setEscrowAddress(bridgeEscrow.address) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) it('sets escrow', async function () { const tx = l1GraphTokenGateway @@ -210,7 +210,7 @@ describe('L1GraphTokenGateway', () => { const tx = l1GraphTokenGateway .connect(tokenSender.signer) .addToCallhookWhitelist(fixtureContracts.rewardsManager.address) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') expect( await l1GraphTokenGateway.callhookWhitelist(fixtureContracts.rewardsManager.address), ).eq(false) @@ -241,7 +241,7 @@ describe('L1GraphTokenGateway', () => { const tx = l1GraphTokenGateway .connect(tokenSender.signer) .removeFromCallhookWhitelist(fixtureContracts.rewardsManager.address) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') expect( await l1GraphTokenGateway.callhookWhitelist(fixtureContracts.rewardsManager.address), ).eq(true) @@ -264,9 +264,9 @@ describe('L1GraphTokenGateway', () => { describe('Pausable behavior', () => { it('cannot be paused or unpaused by someone other than governor or pauseGuardian', async () => { let tx = l1GraphTokenGateway.connect(tokenSender.signer).setPaused(false) - await expect(tx).revertedWith('Only Governor or Guardian can call') + await expect(tx).revertedWith('Only Governor or Guardian') tx = l1GraphTokenGateway.connect(tokenSender.signer).setPaused(true) - await expect(tx).revertedWith('Only Governor or Guardian can call') + await expect(tx).revertedWith('Only Governor or Guardian') }) it('cannot be unpaused if some state variables are not set', async function () { let tx = l1GraphTokenGateway.connect(governor.signer).setPaused(false) @@ -303,7 +303,7 @@ describe('L1GraphTokenGateway', () => { const tx = l1GraphTokenGateway .connect(tokenSender.signer) .setPauseGuardian(pauseGuardian.address) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) it('sets a new pause guardian', async function () { const tx = l1GraphTokenGateway diff --git a/test/gns.test.ts b/test/gns.test.ts index 298d95ab0..1ee46002a 100644 --- a/test/gns.test.ts +++ b/test/gns.test.ts @@ -546,7 +546,7 @@ describe('GNS', () => { it('reject set `ownerTaxPercentage` if not allowed', async function () { const tx = gns.connect(me.signer).setOwnerTaxPercentage(newValue) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) }) @@ -1034,7 +1034,7 @@ describe('GNS', () => { // Batch send transaction const tx = gns.connect(me.signer).multicall([tx1.data, tx2.data]) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) it('should revert if batching a call to initialize', async function () { @@ -1050,7 +1050,7 @@ describe('GNS', () => { // Batch send transaction const tx = gns.connect(me.signer).multicall([tx1.data, tx2.data]) - await expect(tx).revertedWith('Caller must be the implementation') + await expect(tx).revertedWith('Only implementation') }) it('should revert if trying to call a private function', async function () { diff --git a/test/l2/l2GraphTokenGateway.test.ts b/test/l2/l2GraphTokenGateway.test.ts index c987fc281..7c222b73d 100644 --- a/test/l2/l2GraphTokenGateway.test.ts +++ b/test/l2/l2GraphTokenGateway.test.ts @@ -126,7 +126,7 @@ describe('L2GraphTokenGateway', () => { describe('setL2Router', function () { it('is not callable by addreses that are not the governor', async function () { const tx = l2GraphTokenGateway.connect(tokenSender.signer).setL2Router(mockRouter.address) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) it('sets router address', async function () { const tx = l2GraphTokenGateway.connect(governor.signer).setL2Router(mockRouter.address) @@ -140,7 +140,7 @@ describe('L2GraphTokenGateway', () => { const tx = l2GraphTokenGateway .connect(tokenSender.signer) .setL1TokenAddress(mockL1GRT.address) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) it('sets l2GRT', async function () { const tx = l2GraphTokenGateway.connect(governor.signer).setL1TokenAddress(mockL1GRT.address) @@ -154,7 +154,7 @@ describe('L2GraphTokenGateway', () => { const tx = l2GraphTokenGateway .connect(tokenSender.signer) .setL1CounterpartAddress(mockL1Gateway.address) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) it('sets L1Counterpart', async function () { const tx = l2GraphTokenGateway @@ -169,9 +169,9 @@ describe('L2GraphTokenGateway', () => { describe('Pausable behavior', () => { it('cannot be paused or unpaused by someone other than governor or pauseGuardian', async () => { let tx = l2GraphTokenGateway.connect(tokenSender.signer).setPaused(false) - await expect(tx).revertedWith('Only Governor or Guardian can call') + await expect(tx).revertedWith('Only Governor or Guardian') tx = l2GraphTokenGateway.connect(tokenSender.signer).setPaused(true) - await expect(tx).revertedWith('Only Governor or Guardian can call') + await expect(tx).revertedWith('Only Governor or Guardian') }) it('cannot be paused if some state variables are not set', async function () { let tx = l2GraphTokenGateway.connect(governor.signer).setPaused(false) @@ -205,7 +205,7 @@ describe('L2GraphTokenGateway', () => { const tx = l2GraphTokenGateway .connect(tokenSender.signer) .setPauseGuardian(pauseGuardian.address) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) it('sets a new pause guardian', async function () { const tx = l2GraphTokenGateway diff --git a/test/rewards/rewards.test.ts b/test/rewards/rewards.test.ts index 23aee0dfa..beed73a73 100644 --- a/test/rewards/rewards.test.ts +++ b/test/rewards/rewards.test.ts @@ -171,7 +171,7 @@ describe('Rewards', () => { describe('issuance rate update', function () { it('reject set issuance rate if unauthorized', async function () { const tx = rewardsManager.connect(indexer1.signer).setIssuanceRate(toGRT('1.025')) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) it('reject set issuance rate to less than minimum allowed', async function () { @@ -199,7 +199,7 @@ describe('Rewards', () => { const tx = rewardsManager .connect(indexer1.signer) .setSubgraphAvailabilityOracle(oracle.address) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) it('should set subgraph oracle if governor', async function () { diff --git a/test/staking/configuration.test.ts b/test/staking/configuration.test.ts index 39ed30502..52aeaa843 100644 --- a/test/staking/configuration.test.ts +++ b/test/staking/configuration.test.ts @@ -52,7 +52,7 @@ describe('Staking:Config', () => { it('reject set `minimumIndexerStake` if not allowed', async function () { const newValue = toGRT('100') const tx = staking.connect(me.signer).setMinimumIndexerStake(newValue) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) it('reject set `minimumIndexerStake` to zero', async function () { @@ -74,7 +74,7 @@ describe('Staking:Config', () => { it('reject set `slasher` if not allowed', async function () { const tx = staking.connect(other.signer).setSlasher(me.address, true) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) it('reject set `slasher` for zero', async function () { @@ -103,7 +103,7 @@ describe('Staking:Config', () => { it('reject set `assetHolder` if not allowed', async function () { const tx = staking.connect(other.signer).setAssetHolder(me.address, true) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) it('reject set `assetHolder` to address zero', async function () { @@ -122,7 +122,7 @@ describe('Staking:Config', () => { it('reject set `channelDisputeEpochs` if not allowed', async function () { const newValue = toBN('5') const tx = staking.connect(other.signer).setChannelDisputeEpochs(newValue) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) it('reject set `channelDisputeEpochs` to zero', async function () { @@ -146,7 +146,7 @@ describe('Staking:Config', () => { it('reject set `curationPercentage` if not allowed', async function () { const tx = staking.connect(other.signer).setCurationPercentage(50) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) }) @@ -166,7 +166,7 @@ describe('Staking:Config', () => { it('reject set `protocolPercentage` if not allowed', async function () { const tx = staking.connect(other.signer).setProtocolPercentage(50) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) }) @@ -180,7 +180,7 @@ describe('Staking:Config', () => { it('reject set `maxAllocationEpochs` if not allowed', async function () { const newValue = toBN('5') const tx = staking.connect(other.signer).setMaxAllocationEpochs(newValue) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) }) @@ -194,7 +194,7 @@ describe('Staking:Config', () => { it('reject set `thawingPeriod` if not allowed', async function () { const newValue = toBN('5') const tx = staking.connect(other.signer).setThawingPeriod(newValue) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) it('reject set `thawingPeriod` to zero', async function () { @@ -225,7 +225,7 @@ describe('Staking:Config', () => { it('reject set `rebateRatio` if not allowed', async function () { const tx = staking.connect(other.signer).setRebateRatio(1, 1) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) }) }) diff --git a/test/staking/delegation.test.ts b/test/staking/delegation.test.ts index cad46a063..e097dfd9f 100644 --- a/test/staking/delegation.test.ts +++ b/test/staking/delegation.test.ts @@ -217,7 +217,7 @@ describe('Staking::Delegation', () => { it('reject set `delegationRatio` if not allowed', async function () { const tx = staking.connect(me.signer).setDelegationRatio(delegationRatio) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) }) @@ -231,7 +231,7 @@ describe('Staking::Delegation', () => { it('reject set `delegationParametersCooldown` if not allowed', async function () { const tx = staking.connect(me.signer).setDelegationParametersCooldown(cooldown) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) }) @@ -251,7 +251,7 @@ describe('Staking::Delegation', () => { it('reject set `delegationTaxPercentage` if not allowed', async function () { const tx = staking.connect(me.signer).setDelegationTaxPercentage(50) - await expect(tx).revertedWith('Caller must be Controller governor') + await expect(tx).revertedWith('Only Controller governor') }) }) From 18afb3add07b752adfde9a775115b0438415e4c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Migone?= Date: Mon, 24 Oct 2022 14:08:08 -0300 Subject: [PATCH 03/17] =?UTF-8?q?fix(c4-117-g06):=20keccak256()=C2=A0shoul?= =?UTF-8?q?d=20only=20need=20to=20be=20called=20on=20a=20specific=20string?= =?UTF-8?q?=20literal=20once?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tomás Migone --- contracts/governance/Managed.sol | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/contracts/governance/Managed.sol b/contracts/governance/Managed.sol index fcdf73c36..fcec6e47a 100644 --- a/contracts/governance/Managed.sol +++ b/contracts/governance/Managed.sol @@ -28,6 +28,14 @@ contract Managed { mapping(bytes32 => address) private addressCache; uint256[10] private __gap; + // Immutables + bytes32 immutable CURATION = keccak256("Curation"); + bytes32 immutable EPOCH_MANAGER = keccak256("EpochManager"); + bytes32 immutable REWARDS_MANAGER = keccak256("RewardsManager"); + bytes32 immutable STAKING = keccak256("Staking"); + bytes32 immutable GRAPH_TOKEN = keccak256("GraphToken"); + bytes32 immutable GRAPH_TOKEN_GATEWAY = keccak256("GraphTokenGateway"); + // -- Events -- event ParameterUpdated(string param); @@ -111,7 +119,7 @@ contract Managed { * @return Curation contract registered with Controller */ function curation() internal view returns (ICuration) { - return ICuration(_resolveContract(keccak256("Curation"))); + return ICuration(_resolveContract(CURATION)); } /** @@ -119,7 +127,7 @@ contract Managed { * @return Epoch manager contract registered with Controller */ function epochManager() internal view returns (IEpochManager) { - return IEpochManager(_resolveContract(keccak256("EpochManager"))); + return IEpochManager(_resolveContract(EPOCH_MANAGER)); } /** @@ -127,7 +135,7 @@ contract Managed { * @return Rewards manager contract registered with Controller */ function rewardsManager() internal view returns (IRewardsManager) { - return IRewardsManager(_resolveContract(keccak256("RewardsManager"))); + return IRewardsManager(_resolveContract(REWARDS_MANAGER)); } /** @@ -135,7 +143,7 @@ contract Managed { * @return Staking contract registered with Controller */ function staking() internal view returns (IStaking) { - return IStaking(_resolveContract(keccak256("Staking"))); + return IStaking(_resolveContract(STAKING)); } /** @@ -143,7 +151,7 @@ contract Managed { * @return Graph token contract registered with Controller */ function graphToken() internal view returns (IGraphToken) { - return IGraphToken(_resolveContract(keccak256("GraphToken"))); + return IGraphToken(_resolveContract(GRAPH_TOKEN)); } /** @@ -151,7 +159,7 @@ contract Managed { * @return Graph token gateway contract registered with Controller */ function graphTokenGateway() internal view returns (ITokenGateway) { - return ITokenGateway(_resolveContract(keccak256("GraphTokenGateway"))); + return ITokenGateway(_resolveContract(GRAPH_TOKEN_GATEWAY)); } /** From ee5d3cb049be94297ee88e0ebf792623477044de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Migone?= Date: Mon, 24 Oct 2022 14:13:01 -0300 Subject: [PATCH 04/17] =?UTF-8?q?fix(c4-117-g11):=20using=C2=A0>=200=C2=A0?= =?UTF-8?q?costs=20more=20gas=20than=C2=A0!=3D=200=C2=A0when=20used=20on?= =?UTF-8?q?=20a=C2=A0uint=C2=A0in=20a=C2=A0require()?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tomás Migone --- contracts/gateway/L1GraphTokenGateway.sol | 2 +- contracts/l2/gateway/L2GraphTokenGateway.sol | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/gateway/L1GraphTokenGateway.sol b/contracts/gateway/L1GraphTokenGateway.sol index d1cbd417a..5dda7ca31 100644 --- a/contracts/gateway/L1GraphTokenGateway.sol +++ b/contracts/gateway/L1GraphTokenGateway.sol @@ -203,7 +203,7 @@ contract L1GraphTokenGateway is Initializable, GraphTokenGateway, L1ArbitrumMess ) external payable override notPaused returns (bytes memory) { IGraphToken token = graphToken(); require(_l1Token == address(token), "TOKEN_NOT_GRT"); - require(_amount > 0, "INVALID_ZERO_AMOUNT"); + require(_amount != 0, "INVALID_ZERO_AMOUNT"); require(_to != address(0), "INVALID_DESTINATION"); // nested scopes to avoid stack too deep errors diff --git a/contracts/l2/gateway/L2GraphTokenGateway.sol b/contracts/l2/gateway/L2GraphTokenGateway.sol index 251897f6f..f220d2a2c 100644 --- a/contracts/l2/gateway/L2GraphTokenGateway.sol +++ b/contracts/l2/gateway/L2GraphTokenGateway.sol @@ -143,7 +143,7 @@ contract L2GraphTokenGateway is GraphTokenGateway, L2ArbitrumMessenger, Reentran bytes calldata _data ) public payable override notPaused nonReentrant returns (bytes memory) { require(_l1Token == l1GRT, "TOKEN_NOT_GRT"); - require(_amount > 0, "INVALID_ZERO_AMOUNT"); + require(_amount != 0, "INVALID_ZERO_AMOUNT"); require(msg.value == 0, "INVALID_NONZERO_VALUE"); require(_to != address(0), "INVALID_DESTINATION"); From 71314dd94f8abc192327e001dfd76de8f949c3b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Migone?= Date: Mon, 24 Oct 2022 15:10:24 -0300 Subject: [PATCH 05/17] =?UTF-8?q?fix(c4-117-g12):=20splitting=C2=A0require?= =?UTF-8?q?()=C2=A0statements=20that=20use=C2=A0&&=C2=A0saves=20gas?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tomás Migone --- contracts/gateway/L1GraphTokenGateway.sol | 3 ++- contracts/upgrades/GraphProxy.sol | 6 ++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/contracts/gateway/L1GraphTokenGateway.sol b/contracts/gateway/L1GraphTokenGateway.sol index 5dda7ca31..4856ac452 100644 --- a/contracts/gateway/L1GraphTokenGateway.sol +++ b/contracts/gateway/L1GraphTokenGateway.sol @@ -143,7 +143,8 @@ contract L1GraphTokenGateway is Initializable, GraphTokenGateway, L1ArbitrumMess * @param _escrow Address of the BridgeEscrow */ function setEscrowAddress(address _escrow) external onlyGovernor { - require(_escrow != address(0) && Address.isContract(_escrow), "INVALID_ESCROW"); + require(_escrow != address(0), "INVALID_ESCROW"); + require(Address.isContract(_escrow), "MUST_BE_CONTRACT"); escrow = _escrow; emit EscrowAddressSet(_escrow); } diff --git a/contracts/upgrades/GraphProxy.sol b/contracts/upgrades/GraphProxy.sol index 6e29396db..cf7ad7812 100644 --- a/contracts/upgrades/GraphProxy.sol +++ b/contracts/upgrades/GraphProxy.sol @@ -139,10 +139,8 @@ contract GraphProxy is GraphProxyStorage { function _acceptUpgrade() internal { address _pendingImplementation = _pendingImplementation(); require(Address.isContract(_pendingImplementation), "Impl must be a contract"); - require( - _pendingImplementation != address(0) && msg.sender == _pendingImplementation, - "Only pending implementation" - ); + require(_pendingImplementation != address(0), "Impl cannot be zero address"); + require(msg.sender == _pendingImplementation, "Only pending implementation"); _setImplementation(_pendingImplementation); _setPendingImplementation(address(0)); From d5aece460b3ebd3da366a6d18de9667738ff5208 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Migone?= Date: Mon, 24 Oct 2022 15:33:56 -0300 Subject: [PATCH 06/17] =?UTF-8?q?fix(c4-117-g15):=20require()=C2=A0or?= =?UTF-8?q?=C2=A0revert()=C2=A0statements=20that=20check=20input=20argumen?= =?UTF-8?q?ts=20should=20be=20at=20the=20top=20of=20the=20function?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tomás Migone --- contracts/gateway/L1GraphTokenGateway.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/gateway/L1GraphTokenGateway.sol b/contracts/gateway/L1GraphTokenGateway.sol index 4856ac452..5527856c7 100644 --- a/contracts/gateway/L1GraphTokenGateway.sol +++ b/contracts/gateway/L1GraphTokenGateway.sol @@ -203,8 +203,8 @@ contract L1GraphTokenGateway is Initializable, GraphTokenGateway, L1ArbitrumMess bytes calldata _data ) external payable override notPaused returns (bytes memory) { IGraphToken token = graphToken(); - require(_l1Token == address(token), "TOKEN_NOT_GRT"); require(_amount != 0, "INVALID_ZERO_AMOUNT"); + require(_l1Token == address(token), "TOKEN_NOT_GRT"); require(_to != address(0), "INVALID_DESTINATION"); // nested scopes to avoid stack too deep errors From 2ad9f4e3e4c8ad96a108120adba8a3dd4c97a52d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Migone?= Date: Mon, 24 Oct 2022 15:44:54 -0300 Subject: [PATCH 07/17] =?UTF-8?q?fix(c4-6-g07):=20expressions=20for=20cons?= =?UTF-8?q?tant=20values=20such=20as=20a=20call=20to=C2=A0keccak256(),=20s?= =?UTF-8?q?hould=20use=C2=A0=20immutable=C2=A0rather=20than=C2=A0constant?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tomás Migone --- contracts/l2/token/GraphTokenUpgradeable.sol | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/contracts/l2/token/GraphTokenUpgradeable.sol b/contracts/l2/token/GraphTokenUpgradeable.sol index 4ecfb9ffa..e9438cb3c 100644 --- a/contracts/l2/token/GraphTokenUpgradeable.sol +++ b/contracts/l2/token/GraphTokenUpgradeable.sol @@ -31,15 +31,15 @@ contract GraphTokenUpgradeable is GraphUpgradeable, Governed, ERC20BurnableUpgra // -- EIP712 -- // https://github.com/ethereum/EIPs/blob/master/EIPS/eip-712.md#definition-of-domainseparator - bytes32 private constant DOMAIN_TYPE_HASH = + bytes32 private immutable DOMAIN_TYPE_HASH = keccak256( "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract,bytes32 salt)" ); - bytes32 private constant DOMAIN_NAME_HASH = keccak256("Graph Token"); - bytes32 private constant DOMAIN_VERSION_HASH = keccak256("0"); - bytes32 private constant DOMAIN_SALT = + bytes32 private immutable DOMAIN_NAME_HASH = keccak256("Graph Token"); + bytes32 private immutable DOMAIN_VERSION_HASH = keccak256("0"); + bytes32 private immutable DOMAIN_SALT = 0xe33842a7acd1d5a1d28f25a931703e5605152dc48d64dc4716efdae1f5659591; // Randomly generated salt - bytes32 private constant PERMIT_TYPEHASH = + bytes32 private immutable PERMIT_TYPEHASH = keccak256( "Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)" ); From e0440a85c1e8ae313dd3d230fd075b7fdb8927ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Migone?= Date: Mon, 24 Oct 2022 15:56:23 -0300 Subject: [PATCH 08/17] =?UTF-8?q?fix(c4-6-g08):=20duplicated=C2=A0require(?= =?UTF-8?q?)/revert()=C2=A0checks=20should=20be=20refactored=20to=20a=20mo?= =?UTF-8?q?difier=20or=20function?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tomás Migone --- contracts/l2/token/GraphTokenUpgradeable.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/l2/token/GraphTokenUpgradeable.sol b/contracts/l2/token/GraphTokenUpgradeable.sol index e9438cb3c..ce22ba538 100644 --- a/contracts/l2/token/GraphTokenUpgradeable.sol +++ b/contracts/l2/token/GraphTokenUpgradeable.sol @@ -114,7 +114,7 @@ contract GraphTokenUpgradeable is GraphUpgradeable, Governed, ERC20BurnableUpgra * @param _account Address of the minter */ function removeMinter(address _account) external onlyGovernor { - require(_minters[_account], "NOT_A_MINTER"); + require(isMinter(_account), "NOT_A_MINTER"); _removeMinter(_account); } @@ -122,7 +122,7 @@ contract GraphTokenUpgradeable is GraphUpgradeable, Governed, ERC20BurnableUpgra * @dev Renounce to be a minter. */ function renounceMinter() external { - require(_minters[msg.sender], "NOT_A_MINTER"); + require(isMinter(msg.sender), "NOT_A_MINTER"); _removeMinter(msg.sender); } From 3805ca9a2d5595dc3edb0ae1ed8fe111c4f63769 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Migone?= Date: Mon, 24 Oct 2022 15:59:13 -0300 Subject: [PATCH 09/17] =?UTF-8?q?fix(c4-6-13):=20use=C2=A0calldata=C2=A0in?= =?UTF-8?q?stead=20of=C2=A0memory=C2=A0for=20function=20parameters?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tomás Migone --- contracts/gateway/L1GraphTokenGateway.sol | 2 +- contracts/l2/gateway/L2GraphTokenGateway.sol | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/gateway/L1GraphTokenGateway.sol b/contracts/gateway/L1GraphTokenGateway.sol index 5527856c7..7688a5e29 100644 --- a/contracts/gateway/L1GraphTokenGateway.sol +++ b/contracts/gateway/L1GraphTokenGateway.sol @@ -285,7 +285,7 @@ contract L1GraphTokenGateway is Initializable, GraphTokenGateway, L1ArbitrumMess * @return Base ether value required to keep retryable ticket alive * @return Additional data sent to L2 */ - function parseOutboundData(bytes memory _data) + function parseOutboundData(bytes calldata _data) private view returns ( diff --git a/contracts/l2/gateway/L2GraphTokenGateway.sol b/contracts/l2/gateway/L2GraphTokenGateway.sol index f220d2a2c..edd7ff610 100644 --- a/contracts/l2/gateway/L2GraphTokenGateway.sol +++ b/contracts/l2/gateway/L2GraphTokenGateway.sol @@ -283,7 +283,7 @@ contract L2GraphTokenGateway is GraphTokenGateway, L2ArbitrumMessenger, Reentran * @return Sender of the tx * @return Any other data sent to L1 */ - function parseOutboundData(bytes memory _data) private view returns (address, bytes memory) { + function parseOutboundData(bytes calldata _data) private view returns (address, bytes memory) { address from; bytes memory extraData; if (msg.sender == l2Router) { From 54157a76e46a0704bc63b8cb29ddacd1c8d3e259 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Migone?= Date: Mon, 24 Oct 2022 16:15:57 -0300 Subject: [PATCH 10/17] fix(c4-86-g07): emitting or returning storage values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tomás Migone --- contracts/l2/token/L2GraphToken.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/l2/token/L2GraphToken.sol b/contracts/l2/token/L2GraphToken.sol index 0e1d942c9..7a5abf78c 100644 --- a/contracts/l2/token/L2GraphToken.sol +++ b/contracts/l2/token/L2GraphToken.sol @@ -59,7 +59,7 @@ contract L2GraphToken is GraphTokenUpgradeable, IArbToken { function setGateway(address _gw) external onlyGovernor { require(_gw != address(0), "INVALID_GATEWAY"); gateway = _gw; - emit GatewaySet(gateway); + emit GatewaySet(_gw); } /** From 7d29d6d57892e99ed5702df19b91c7920cc40cc4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Migone?= Date: Mon, 24 Oct 2022 16:25:18 -0300 Subject: [PATCH 11/17] fix(c4-172-g02): set allowance to 0 instead of decreasing allowance by approved amount MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tomás Migone --- contracts/gateway/BridgeEscrow.sol | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/contracts/gateway/BridgeEscrow.sol b/contracts/gateway/BridgeEscrow.sol index 1c1d85f54..4d9c11e0d 100644 --- a/contracts/gateway/BridgeEscrow.sol +++ b/contracts/gateway/BridgeEscrow.sol @@ -36,7 +36,6 @@ contract BridgeEscrow is Initializable, GraphUpgradeable, Managed { * @param _spender Address of the spender that will be revoked */ function revokeAll(address _spender) external onlyGovernor { - IGraphToken grt = graphToken(); - grt.decreaseAllowance(_spender, grt.allowance(address(this), _spender)); + graphToken().approve(_spender, 0); } } From 534d224f75895160f0d989a540164e811e906fd5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Migone?= Date: Mon, 24 Oct 2022 16:28:03 -0300 Subject: [PATCH 12/17] fix(c4-287-g01): remove unused local variable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tomás Migone --- contracts/l2/gateway/L2GraphTokenGateway.sol | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/contracts/l2/gateway/L2GraphTokenGateway.sol b/contracts/l2/gateway/L2GraphTokenGateway.sol index edd7ff610..371db711f 100644 --- a/contracts/l2/gateway/L2GraphTokenGateway.sol +++ b/contracts/l2/gateway/L2GraphTokenGateway.sol @@ -238,8 +238,7 @@ contract L2GraphTokenGateway is GraphTokenGateway, L2ArbitrumMessenger, Reentran if (_data.length > 0) { bytes memory callhookData; { - bytes memory gatewayData; - (gatewayData, callhookData) = abi.decode(_data, (bytes, bytes)); + (, callhookData) = abi.decode(_data, (bytes, bytes)); } ICallhookReceiver(_to).onTokenTransfer(_from, _amount, callhookData); } From 0b7ad55d821bcdffed3ed236668be841f22d14fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Migone?= Date: Mon, 24 Oct 2022 16:30:29 -0300 Subject: [PATCH 13/17] fix(c4-272): remove unnecessary use of safe math MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tomás Migone --- contracts/l2/token/GraphTokenUpgradeable.sol | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/contracts/l2/token/GraphTokenUpgradeable.sol b/contracts/l2/token/GraphTokenUpgradeable.sol index ce22ba538..10a77556a 100644 --- a/contracts/l2/token/GraphTokenUpgradeable.sol +++ b/contracts/l2/token/GraphTokenUpgradeable.sol @@ -4,7 +4,6 @@ pragma solidity ^0.7.6; import "@openzeppelin/contracts-upgradeable/token/ERC20/ERC20BurnableUpgradeable.sol"; import "@openzeppelin/contracts/cryptography/ECDSA.sol"; -import "@openzeppelin/contracts/math/SafeMath.sol"; import "../../upgrades/GraphUpgradeable.sol"; import "../../governance/Governed.sol"; @@ -26,7 +25,6 @@ import "../../governance/Governed.sol"; * the original's constructor + non-upgradeable approach. */ contract GraphTokenUpgradeable is GraphUpgradeable, Governed, ERC20BurnableUpgradeable { - using SafeMath for uint256; // -- EIP712 -- // https://github.com/ethereum/EIPs/blob/master/EIPS/eip-712.md#definition-of-domainseparator @@ -96,7 +94,7 @@ contract GraphTokenUpgradeable is GraphUpgradeable, Governed, ERC20BurnableUpgra require(_owner == recoveredAddress, "GRT: invalid permit"); require(_deadline == 0 || block.timestamp <= _deadline, "GRT: expired permit"); - nonces[_owner] = nonces[_owner].add(1); + nonces[_owner] = nonces[_owner] + 1; _approve(_owner, _spender, _value); } From d091781d6468523c6f1aaca5fd16f137e33f60b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Migone?= Date: Mon, 24 Oct 2022 16:34:11 -0300 Subject: [PATCH 14/17] fix(c4-272): remove unnecessary usage of safemath MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tomás Migone --- contracts/l2/token/GraphTokenUpgradeable.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/contracts/l2/token/GraphTokenUpgradeable.sol b/contracts/l2/token/GraphTokenUpgradeable.sol index 10a77556a..ac6ce63f0 100644 --- a/contracts/l2/token/GraphTokenUpgradeable.sol +++ b/contracts/l2/token/GraphTokenUpgradeable.sol @@ -25,7 +25,6 @@ import "../../governance/Governed.sol"; * the original's constructor + non-upgradeable approach. */ contract GraphTokenUpgradeable is GraphUpgradeable, Governed, ERC20BurnableUpgradeable { - // -- EIP712 -- // https://github.com/ethereum/EIPs/blob/master/EIPS/eip-712.md#definition-of-domainseparator From 1f85e126bb51423d6cfd3a6f12aa8a21199cd7d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Migone?= Date: Mon, 24 Oct 2022 16:38:35 -0300 Subject: [PATCH 15/17] fix: further optimize governed contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tomás Migone --- contracts/governance/Governed.sol | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/governance/Governed.sol b/contracts/governance/Governed.sol index 36d96d13a..697f2fa12 100644 --- a/contracts/governance/Governed.sol +++ b/contracts/governance/Governed.sol @@ -51,16 +51,16 @@ contract Governed { * This function must called by the pending governor. */ function acceptOwnership() external { - address _pendingGovernor = pendingGovernor; + address oldPendingGovernor = pendingGovernor; + require( - _pendingGovernor != address(0) && msg.sender == _pendingGovernor, + oldPendingGovernor != address(0) && msg.sender == oldPendingGovernor, "Caller must be pending governor" ); address oldGovernor = governor; - address oldPendingGovernor = _pendingGovernor; - governor = _pendingGovernor; + governor = oldPendingGovernor; pendingGovernor = address(0); emit NewOwnership(oldGovernor, governor); From 27902ccec83688bc895a83c21aebe5ae511a6086 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Migone?= Date: Mon, 24 Oct 2022 16:39:30 -0300 Subject: [PATCH 16/17] fix: add visibility to managed immutable vars MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tomás Migone --- contracts/governance/Managed.sol | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/contracts/governance/Managed.sol b/contracts/governance/Managed.sol index fcec6e47a..562028c15 100644 --- a/contracts/governance/Managed.sol +++ b/contracts/governance/Managed.sol @@ -29,12 +29,12 @@ contract Managed { uint256[10] private __gap; // Immutables - bytes32 immutable CURATION = keccak256("Curation"); - bytes32 immutable EPOCH_MANAGER = keccak256("EpochManager"); - bytes32 immutable REWARDS_MANAGER = keccak256("RewardsManager"); - bytes32 immutable STAKING = keccak256("Staking"); - bytes32 immutable GRAPH_TOKEN = keccak256("GraphToken"); - bytes32 immutable GRAPH_TOKEN_GATEWAY = keccak256("GraphTokenGateway"); + bytes32 private immutable CURATION = keccak256("Curation"); + bytes32 private immutable EPOCH_MANAGER = keccak256("EpochManager"); + bytes32 private immutable REWARDS_MANAGER = keccak256("RewardsManager"); + bytes32 private immutable STAKING = keccak256("Staking"); + bytes32 private immutable GRAPH_TOKEN = keccak256("GraphToken"); + bytes32 private immutable GRAPH_TOKEN_GATEWAY = keccak256("GraphTokenGateway"); // -- Events -- From e3a668ec5939cfe3a926b871fb5d1337def17934 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Migone?= Date: Mon, 24 Oct 2022 16:54:18 -0300 Subject: [PATCH 17/17] fix: update e2e tests to new error messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tomás Migone --- e2e/deployment/config/l1/l1GraphTokenGateway.test.ts | 2 +- e2e/deployment/config/l2/l2GraphTokenGateway.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/e2e/deployment/config/l1/l1GraphTokenGateway.test.ts b/e2e/deployment/config/l1/l1GraphTokenGateway.test.ts index a2d8edbae..cce20c735 100644 --- a/e2e/deployment/config/l1/l1GraphTokenGateway.test.ts +++ b/e2e/deployment/config/l1/l1GraphTokenGateway.test.ts @@ -26,7 +26,7 @@ describe('[L1] L1GraphTokenGateway configuration', function () { describe('calls with unauthorized user', () => { it('initialize should revert', async function () { const tx = L1GraphTokenGateway.connect(unauthorized).initialize(unauthorized.address) - await expect(tx).revertedWith('Caller must be the implementation') + await expect(tx).revertedWith('Only implementation') }) it('setArbitrumAddresses should revert', async function () { diff --git a/e2e/deployment/config/l2/l2GraphTokenGateway.test.ts b/e2e/deployment/config/l2/l2GraphTokenGateway.test.ts index e4626b59b..c8944927c 100644 --- a/e2e/deployment/config/l2/l2GraphTokenGateway.test.ts +++ b/e2e/deployment/config/l2/l2GraphTokenGateway.test.ts @@ -26,7 +26,7 @@ describe('[L2] L2GraphTokenGateway configuration', function () { describe('calls with unauthorized user', () => { it('initialize should revert', async function () { const tx = L2GraphTokenGateway.connect(unauthorized).initialize(unauthorized.address) - await expect(tx).revertedWith('Caller must be the implementation') + await expect(tx).revertedWith('Only implementation') }) it('setL2Router should revert', async function () {