L2 bridge: safer callhook interface

.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

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;
+}

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);

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);
+    }
+}

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,32 +325,28 @@ describe('L2GraphTokenGateway', () => {
     describe('finalizeInboundTransfer', function () {
       const testValidFinalizeTransfer = async function (
         data: string,
+        to?: string,
       ): Promise<ContractTransaction> {
+        to = to ?? l2Receiver.address
         const mockL1GatewayL2Alias = await getL2SignerFromL1(mockL1Gateway.address)
         await me.signer.sendTransaction({
           to: await mockL1GatewayL2Alias.getAddress(),
           value: utils.parseUnits('1', 'ether'),
         })
         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",
+        )
       })
     })
   })