diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c4ed99cf7..55b984f12 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -24,7 +24,7 @@ jobs: - name: Run tests run: yarn test:coverage - name: Upload coverage report - uses: codecov/codecov-action@v2 + uses: codecov/codecov-action@v3 with: token: ${{ secrets.CODECOV_TOKEN }} files: ./coverage.json diff --git a/contracts/gateway/ICallhookReceiver.sol b/contracts/gateway/ICallhookReceiver.sol new file mode 100644 index 000000000..fb7492bb7 --- /dev/null +++ b/contracts/gateway/ICallhookReceiver.sol @@ -0,0 +1,23 @@ +// SPDX-License-Identifier: GPL-2.0-or-later + +/** + * @title Interface for contracts that can receive callhooks through the Arbitrum GRT bridge + * @dev Any contract that can receive a callhook on L2, sent through the bridge from L1, must + * be whitelisted by the governor, but also implement this interface that contains + * the function that will actually be called by the L2GraphTokenGateway. + */ +pragma solidity ^0.7.6; + +interface ICallhookReceiver { + /** + * @dev Receive tokens with a callhook from the bridge + * @param _from Token sender in L1 + * @param _amount Amount of tokens that were transferred + * @param _data ABI-encoded callhook data + */ + function onTokenTransfer( + address _from, + uint256 _amount, + bytes calldata _data + ) external; +} diff --git a/contracts/l2/gateway/L2GraphTokenGateway.sol b/contracts/l2/gateway/L2GraphTokenGateway.sol index 593e0e228..8757a06ca 100644 --- a/contracts/l2/gateway/L2GraphTokenGateway.sol +++ b/contracts/l2/gateway/L2GraphTokenGateway.sol @@ -3,11 +3,13 @@ pragma solidity ^0.7.6; pragma abicoder v2; +import "@openzeppelin/contracts-upgradeable/utils/ReentrancyGuardUpgradeable.sol"; import "@openzeppelin/contracts/math/SafeMath.sol"; import "../../arbitrum/L2ArbitrumMessenger.sol"; import "../../arbitrum/AddressAliasHelper.sol"; import "../../gateway/GraphTokenGateway.sol"; +import "../../gateway/ICallhookReceiver.sol"; import "../token/L2GraphToken.sol"; /** @@ -18,7 +20,7 @@ import "../token/L2GraphToken.sol"; * (See: https://github.com/OffchainLabs/arbitrum/tree/master/packages/arb-bridge-peripherals/contracts/tokenbridge * and https://github.com/livepeer/arbitrum-lpt-bridge) */ -contract L2GraphTokenGateway is GraphTokenGateway, L2ArbitrumMessenger { +contract L2GraphTokenGateway is GraphTokenGateway, L2ArbitrumMessenger, ReentrancyGuardUpgradeable { using SafeMath for uint256; // Address of the Graph Token contract on L1 @@ -85,6 +87,7 @@ contract L2GraphTokenGateway is GraphTokenGateway, L2ArbitrumMessenger { function initialize(address _controller) external onlyImpl { Managed._initialize(_controller); _paused = true; + __ReentrancyGuard_init(); } /** @@ -138,7 +141,7 @@ contract L2GraphTokenGateway is GraphTokenGateway, L2ArbitrumMessenger { uint256, // unused on L2 uint256, // unused on L2 bytes calldata _data - ) public payable override notPaused returns (bytes memory) { + ) public payable override notPaused nonReentrant returns (bytes memory) { require(_l1Token == l1GRT, "TOKEN_NOT_GRT"); require(_amount > 0, "INVALID_ZERO_AMOUNT"); require(msg.value == 0, "INVALID_NONZERO_VALUE"); @@ -226,7 +229,7 @@ contract L2GraphTokenGateway is GraphTokenGateway, L2ArbitrumMessenger { address _to, uint256 _amount, bytes calldata _data - ) external payable override notPaused onlyL1Counterpart { + ) external payable override notPaused onlyL1Counterpart nonReentrant { require(_l1Token == l1GRT, "TOKEN_NOT_GRT"); require(msg.value == 0, "INVALID_NONZERO_VALUE"); @@ -238,15 +241,7 @@ contract L2GraphTokenGateway is GraphTokenGateway, L2ArbitrumMessenger { bytes memory gatewayData; (gatewayData, callhookData) = abi.decode(_data, (bytes, bytes)); } - bool success; - // solhint-disable-next-line avoid-low-level-calls - (success, ) = _to.call(callhookData); - // Callhooks shouldn't revert, but if they do: - // we revert, so that the retryable ticket can be re-attempted - // later. - if (!success) { - revert("CALLHOOK_FAILED"); - } + ICallhookReceiver(_to).onTokenTransfer(_from, _amount, callhookData); } emit DepositFinalized(_l1Token, _from, _to, _amount); diff --git a/contracts/tests/CallhookReceiverMock.sol b/contracts/tests/CallhookReceiverMock.sol new file mode 100644 index 000000000..1e71cf86f --- /dev/null +++ b/contracts/tests/CallhookReceiverMock.sol @@ -0,0 +1,32 @@ +// SPDX-License-Identifier: GPL-2.0-or-later + +pragma solidity ^0.7.6; + +import "../gateway/ICallhookReceiver.sol"; + +/** + * @title GovernedMock contract + */ +contract CallhookReceiverMock is ICallhookReceiver { + event TransferReceived(address from, uint256 amount, uint256 foo, uint256 bar); + + /** + * @dev Receive tokens with a callhook from the bridge + * Expects two uint256 values encoded in _data. + * Reverts if the first of these values is zero. + * @param _from Token sender in L1 + * @param _amount Amount of tokens that were transferred + * @param _data ABI-encoded callhook data + */ + function onTokenTransfer( + address _from, + uint256 _amount, + bytes calldata _data + ) external override { + uint256 foo; + uint256 bar; + (foo, bar) = abi.decode(_data, (uint256, uint256)); + require(foo != 0, "FOO_IS_ZERO"); + emit TransferReceived(_from, _amount, foo, bar); + } +} diff --git a/test/l2/l2GraphTokenGateway.test.ts b/test/l2/l2GraphTokenGateway.test.ts index f4d57dbab..f283aabe1 100644 --- a/test/l2/l2GraphTokenGateway.test.ts +++ b/test/l2/l2GraphTokenGateway.test.ts @@ -3,21 +3,18 @@ import { constants, ContractTransaction, Signer, utils } from 'ethers' import { L2GraphToken } from '../../build/types/L2GraphToken' import { L2GraphTokenGateway } from '../../build/types/L2GraphTokenGateway' +import { CallhookReceiverMock } from '../../build/types/CallhookReceiverMock' import { L2FixtureContracts, NetworkFixture } from '../lib/fixtures' import { FakeContract, smock } from '@defi-wonderland/smock' -import path from 'path' -import { Artifacts } from 'hardhat/internal/artifacts' -const ARTIFACTS_PATH = path.resolve('build/contracts') -const artifacts = new Artifacts(ARTIFACTS_PATH) -const rewardsManagerMockAbi = artifacts.readArtifactSync('RewardsManagerMock').abi - use(smock.matchers) import { getAccounts, toGRT, Account, toBN, getL2SignerFromL1 } from '../lib/testHelpers' import { Interface } from 'ethers/lib/utils' +import { deployContract } from '../lib/deployment' +import { RewardsManager } from '../../build/types/RewardsManager' const { AddressZero } = constants @@ -37,11 +34,15 @@ describe('L2GraphTokenGateway', () => { let fixtureContracts: L2FixtureContracts let grt: L2GraphToken let l2GraphTokenGateway: L2GraphTokenGateway + let callhookReceiverMock: CallhookReceiverMock + let rewardsManager: RewardsManager const senderTokens = toGRT('1000') const defaultData = '0x' - const mockIface = new Interface(rewardsManagerMockAbi) - const notEmptyCallHookData = mockIface.encodeFunctionData('pow', [toBN(1), toBN(2), toBN(3)]) + const notEmptyCallHookData = utils.defaultAbiCoder.encode( + ['uint256', 'uint256'], + [toBN('1337'), toBN('42')], + ) const defaultDataWithNotEmptyCallHookData = utils.defaultAbiCoder.encode( ['bytes', 'bytes'], ['0x', notEmptyCallHookData], @@ -62,7 +63,12 @@ describe('L2GraphTokenGateway', () => { fixture = new NetworkFixture() fixtureContracts = await fixture.loadL2(governor.signer) - ;({ grt, l2GraphTokenGateway } = fixtureContracts) + ;({ grt, l2GraphTokenGateway, rewardsManager } = fixtureContracts) + + callhookReceiverMock = (await deployContract( + 'CallhookReceiverMock', + governor.signer, + )) as unknown as CallhookReceiverMock // Give some funds to the token sender await grt.connect(governor.signer).mint(tokenSender.address, senderTokens) @@ -319,7 +325,9 @@ describe('L2GraphTokenGateway', () => { describe('finalizeInboundTransfer', function () { const testValidFinalizeTransfer = async function ( data: string, + to?: string, ): Promise { + to = to ?? l2Receiver.address const mockL1GatewayL2Alias = await getL2SignerFromL1(mockL1Gateway.address) await me.signer.sendTransaction({ to: await mockL1GatewayL2Alias.getAddress(), @@ -327,24 +335,18 @@ describe('L2GraphTokenGateway', () => { }) const tx = l2GraphTokenGateway .connect(mockL1GatewayL2Alias) - .finalizeInboundTransfer( - mockL1GRT.address, - tokenSender.address, - l2Receiver.address, - toGRT('10'), - data, - ) + .finalizeInboundTransfer(mockL1GRT.address, tokenSender.address, to, toGRT('10'), data) await expect(tx) .emit(l2GraphTokenGateway, 'DepositFinalized') - .withArgs(mockL1GRT.address, tokenSender.address, l2Receiver.address, toGRT('10')) + .withArgs(mockL1GRT.address, tokenSender.address, to, toGRT('10')) - await expect(tx).emit(grt, 'BridgeMinted').withArgs(l2Receiver.address, toGRT('10')) + await expect(tx).emit(grt, 'BridgeMinted').withArgs(to, toGRT('10')) // Unchanged const senderBalance = await grt.balanceOf(tokenSender.address) await expect(senderBalance).eq(toGRT('1000')) // 10 newly minted GRT - const receiverBalance = await grt.balanceOf(l2Receiver.address) + const receiverBalance = await grt.balanceOf(to) await expect(receiverBalance).eq(toGRT('10')) return tx } @@ -375,34 +377,60 @@ describe('L2GraphTokenGateway', () => { it('mints and sends tokens when called by the aliased gateway', async function () { await testValidFinalizeTransfer(defaultData) }) - it('calls a callhook if the sender is whitelisted', async function () { - const rewardsManagerMock = await smock.fake('RewardsManagerMock', { - address: l2Receiver.address, - }) - rewardsManagerMock.pow.returns(1) - await testValidFinalizeTransfer(defaultDataWithNotEmptyCallHookData) - expect(rewardsManagerMock.pow).to.have.been.calledWith(toBN(1), toBN(2), toBN(3)) + it('calls a callhook if the transfer includes calldata', async function () { + const tx = await testValidFinalizeTransfer( + defaultDataWithNotEmptyCallHookData, + callhookReceiverMock.address, + ) + // Emitted by the callhook: + await expect(tx) + .emit(callhookReceiverMock, 'TransferReceived') + .withArgs(tokenSender.address, toGRT('10'), toBN('1337'), toBN('42')) }) it('reverts if a callhook reverts', async function () { - const rewardsManagerMock = await smock.fake('RewardsManagerMock', { - address: l2Receiver.address, + // The 0 will make the callhook revert (see CallhookReceiverMock.sol) + const callHookData = utils.defaultAbiCoder.encode( + ['uint256', 'uint256'], + [toBN('0'), toBN('42')], + ) + const data = utils.defaultAbiCoder.encode(['bytes', 'bytes'], ['0x', callHookData]) + const mockL1GatewayL2Alias = await getL2SignerFromL1(mockL1Gateway.address) + await me.signer.sendTransaction({ + to: await mockL1GatewayL2Alias.getAddress(), + value: utils.parseUnits('1', 'ether'), }) - rewardsManagerMock.pow.reverts() + const tx = l2GraphTokenGateway + .connect(mockL1GatewayL2Alias) + .finalizeInboundTransfer( + mockL1GRT.address, + tokenSender.address, + callhookReceiverMock.address, + toGRT('10'), + data, + ) + await expect(tx).revertedWith('FOO_IS_ZERO') + }) + it('reverts if trying to call a callhook in a contract that does not implement onTokenTransfer', async function () { + const callHookData = utils.defaultAbiCoder.encode(['uint256'], [toBN('0')]) + const data = utils.defaultAbiCoder.encode(['bytes', 'bytes'], ['0x', callHookData]) const mockL1GatewayL2Alias = await getL2SignerFromL1(mockL1Gateway.address) await me.signer.sendTransaction({ to: await mockL1GatewayL2Alias.getAddress(), value: utils.parseUnits('1', 'ether'), }) + // RewardsManager does not implement onTokenTransfer, so this will fail const tx = l2GraphTokenGateway .connect(mockL1GatewayL2Alias) .finalizeInboundTransfer( mockL1GRT.address, tokenSender.address, - l2Receiver.address, + rewardsManager.address, toGRT('10'), - defaultDataWithNotEmptyCallHookData, + data, ) - await expect(tx).revertedWith('CALLHOOK_FAILED') + await expect(tx).revertedWith( + "function selector was not recognized and there's no fallback function", + ) }) }) })