feat: allow callhooks from whitelisted addresses in the Arbitrum GRT bridge

pcarranzav · pcarranzav · commit 62e927018b87 · 2022-05-31T10:59:53.000-03:00
diff --git a/contracts/gateway/L1GraphTokenGateway.sol b/contracts/gateway/L1GraphTokenGateway.sol
@@ -30,6 +30,8 @@ contract L1GraphTokenGateway is GraphTokenGateway, L1ArbitrumMessenger {
     address public l2Counterpart;
     // Address of the BridgeEscrow contract that holds the GRT in the bridge
     address public escrow;
+    // Address of the L1 Reservoir that is the only sender allowed to send extra data
+    mapping(address => bool) public callhookWhitelist;
 
     // Emitted when an outbound transfer is initiated, i.e. tokens are deposited from L1 to L2
     event DepositInitiated(
@@ -57,6 +59,10 @@ contract L1GraphTokenGateway is GraphTokenGateway, L1ArbitrumMessenger {
     event L2CounterpartAddressSet(address _l2Counterpart);
     // Emitted when the escrow address has been updated
     event EscrowAddressSet(address _escrow);
+    // Emitted when an address is added to the callhook whitelist
+    event AddedToCallhookWhitelist(address newWhitelisted);
+    // Emitted when an address is removed from the callhook whitelist
+    event RemovedFromCallhookWhitelist(address notWhitelisted);
 
     /**
      * @dev Allows a function to be called only by the gateway's L2 counterpart.
@@ -122,6 +128,26 @@ contract L1GraphTokenGateway is GraphTokenGateway, L1ArbitrumMessenger {
         emit EscrowAddressSet(_escrow);
     }
 
+    /**
+     * @dev Adds an address to the callhook whitelist.
+     * This address will be allowed to include callhooks when transferring tokens.
+     * @param newWhitelisted Address to add to the whitelist
+     */
+    function addToCallhookWhitelist(address newWhitelisted) external onlyGovernor {
+        callhookWhitelist[newWhitelisted] = true;
+        emit AddedToCallhookWhitelist(newWhitelisted);
+    }
+
+    /**
+     * @dev Removes an address from the callhook whitelist.
+     * This address will no longer be allowed to include callhooks when transferring tokens.
+     * @param notWhitelisted Address to remove from the whitelist
+     */
+    function removeFromCallhookWhitelist(address notWhitelisted) external onlyGovernor {
+        callhookWhitelist[notWhitelisted] = false;
+        emit RemovedFromCallhookWhitelist(notWhitelisted);
+    }
+
     /**
      * @notice Creates and sends a retryable ticket to transfer GRT to L2 using the Arbitrum Inbox.
      * The tokens are escrowed by the gateway until they are withdrawn back to L1.
@@ -156,7 +182,10 @@ contract L1GraphTokenGateway is GraphTokenGateway, L1ArbitrumMessenger {
             {
                 bytes memory extraData;
                 (from, maxSubmissionCost, extraData) = parseOutboundData(_data);
-                require(extraData.length == 0, "CALL_HOOK_DATA_NOT_ALLOWED");
+                require(
+                    extraData.length == 0 || callhookWhitelist[msg.sender] == true,
+                    "CALL_HOOK_DATA_NOT_ALLOWED"
+                );
                 require(maxSubmissionCost > 0, "NO_SUBMISSION_COST");
 
                 {
diff --git a/contracts/l2/gateway/L2GraphTokenGateway.sol b/contracts/l2/gateway/L2GraphTokenGateway.sol
@@ -30,7 +30,8 @@ contract L2GraphTokenGateway is GraphTokenGateway, L2ArbitrumMessenger {
     address public l1Counterpart;
     // Address of the Arbitrum Gateway Router on L2
     address public l2Router;
-
+    // Addresses in L1 that are whitelisted to have callhooks on transfers
+    mapping(address => bool) public callhookWhitelist;
     // Calldata included in an outbound transfer, stored as a structure for convenience and stack depth
     struct OutboundCalldata {
         address from;
@@ -61,6 +62,12 @@ contract L2GraphTokenGateway is GraphTokenGateway, L2ArbitrumMessenger {
     event L1TokenAddressSet(address _l1GRT);
     // Emitted when the address of the counterpart gateway on L1 has been updated
     event L1CounterpartAddressSet(address _l1Counterpart);
+    // Emitted when an address is added to the callhook whitelist
+    event AddedToCallhookWhitelist(address newWhitelisted);
+    // Emitted when an address is removed from the callhook whitelist
+    event RemovedFromCallhookWhitelist(address notWhitelisted);
+    // Emitted when a callhook call failed
+    event CallhookFailed(address destination);
 
     /**
      * @dev Checks that the sender is the L2 alias of the counterpart
@@ -108,6 +115,26 @@ contract L2GraphTokenGateway is GraphTokenGateway, L2ArbitrumMessenger {
         emit L1CounterpartAddressSet(_l1Counterpart);
     }
 
+    /**
+     * @dev Adds an L1 address to the callhook whitelist.
+     * This address will be allowed to include callhooks when transferring tokens.
+     * @param newWhitelisted Address to add to the whitelist
+     */
+    function addToCallhookWhitelist(address newWhitelisted) external onlyGovernor {
+        callhookWhitelist[newWhitelisted] = true;
+        emit AddedToCallhookWhitelist(newWhitelisted);
+    }
+
+    /**
+     * @dev Removes an L1 address from the callhook whitelist.
+     * This address will no longer be allowed to include callhooks when transferring tokens.
+     * @param notWhitelisted Address to remove from the whitelist
+     */
+    function removeFromCallhookWhitelist(address notWhitelisted) external onlyGovernor {
+        callhookWhitelist[notWhitelisted] = false;
+        emit RemovedFromCallhookWhitelist(notWhitelisted);
+    }
+
     /**
      * @notice Burns L2 tokens and initiates a transfer to L1.
      * The tokens will be available on L1 only after the wait period (7 days) is over,
@@ -196,17 +223,35 @@ contract L2GraphTokenGateway is GraphTokenGateway, L2ArbitrumMessenger {
      * @param _from Address of the sender on L1
      * @param _to Recipient address on L2
      * @param _amount Amount of tokens transferred
+     * @param _data Extra callhook data, only used when the sender is whitelisted
      */
     function finalizeInboundTransfer(
         address _l1Token,
         address _from,
         address _to,
         uint256 _amount,
-        bytes calldata // _data unused in L2
+        bytes calldata _data
     ) external payable override notPaused onlyL1Counterpart {
         require(_l1Token == l1GRT, "TOKEN_NOT_GRT");
         require(msg.value == 0, "INVALID_NONZERO_VALUE");
 
+        if (_data.length > 0 && callhookWhitelist[_from] == true) {
+            bytes memory callhookData;
+            {
+                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");
+            }
+        }
+
         L2GraphToken(calculateL2TokenAddress(l1GRT)).bridgeMint(_to, _amount);
 
         emit DepositFinalized(_l1Token, _from, _to, _amount);
diff --git a/test/gateway/l1GraphTokenGateway.test.ts b/test/gateway/l1GraphTokenGateway.test.ts
@@ -186,6 +186,48 @@ describe('L1GraphTokenGateway', () => {
         expect(await l1GraphTokenGateway.escrow()).eq(bridgeEscrow.address)
       })
     })
+    describe('addToCallhookWhitelist', function () {
+      it('is not callable by addreses that are not the governor', async function () {
+        const tx = l1GraphTokenGateway
+          .connect(tokenSender.signer)
+          .addToCallhookWhitelist(tokenSender.address)
+        await expect(tx).revertedWith('Caller must be Controller governor')
+        expect(await l1GraphTokenGateway.callhookWhitelist(tokenSender.address)).eq(false)
+      })
+      it('adds an address to the callhook whitelist', async function () {
+        const tx = l1GraphTokenGateway
+          .connect(governor.signer)
+          .addToCallhookWhitelist(tokenSender.address)
+        await expect(tx)
+          .emit(l1GraphTokenGateway, 'AddedToCallhookWhitelist')
+          .withArgs(tokenSender.address)
+        expect(await l1GraphTokenGateway.callhookWhitelist(tokenSender.address)).eq(true)
+      })
+    })
+    describe('removeFromCallhookWhitelist', function () {
+      it('is not callable by addreses that are not the governor', async function () {
+        await l1GraphTokenGateway
+          .connect(governor.signer)
+          .addToCallhookWhitelist(tokenSender.address)
+        const tx = l1GraphTokenGateway
+          .connect(tokenSender.signer)
+          .removeFromCallhookWhitelist(tokenSender.address)
+        await expect(tx).revertedWith('Caller must be Controller governor')
+        expect(await l1GraphTokenGateway.callhookWhitelist(tokenSender.address)).eq(true)
+      })
+      it('removes an address from the callhook whitelist', async function () {
+        await l1GraphTokenGateway
+          .connect(governor.signer)
+          .addToCallhookWhitelist(tokenSender.address)
+        const tx = l1GraphTokenGateway
+          .connect(governor.signer)
+          .removeFromCallhookWhitelist(tokenSender.address)
+        await expect(tx)
+          .emit(l1GraphTokenGateway, 'RemovedFromCallhookWhitelist')
+          .withArgs(tokenSender.address)
+        expect(await l1GraphTokenGateway.callhookWhitelist(tokenSender.address)).eq(false)
+      })
+    })
     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)
@@ -230,7 +272,7 @@ describe('L1GraphTokenGateway', () => {
   })
 
   context('> after configuring and unpausing', function () {
-    const createMsgData = function () {
+    const createMsgData = function (callHookData: string) {
       const selector = l1GraphTokenGateway.interface.getSighash('finalizeInboundTransfer')
       const params = utils.defaultAbiCoder.encode(
         ['address', 'address', 'address', 'uint256', 'bytes'],
@@ -239,7 +281,7 @@ describe('L1GraphTokenGateway', () => {
           tokenSender.address,
           l2Receiver.address,
           toGRT('10'),
-          utils.defaultAbiCoder.encode(['bytes', 'bytes'], [emptyCallHookData, emptyCallHookData]),
+          utils.defaultAbiCoder.encode(['bytes', 'bytes'], [emptyCallHookData, callHookData]),
         ],
       )
       const outboundData = utils.hexlify(utils.concat([selector, params]))
@@ -283,7 +325,11 @@ describe('L1GraphTokenGateway', () => {
       )
       return expectedInboxAccsEntry
     }
-    const testValidOutboundTransfer = async function (signer: Signer, data: string) {
+    const testValidOutboundTransfer = async function (
+      signer: Signer,
+      data: string,
+      callHookData: string,
+    ) {
       const tx = l1GraphTokenGateway
         .connect(signer)
         .outboundTransfer(grt.address, l2Receiver.address, toGRT('10'), maxGas, gasPriceBid, data, {
@@ -295,7 +341,7 @@ describe('L1GraphTokenGateway', () => {
         .emit(l1GraphTokenGateway, 'DepositInitiated')
         .withArgs(grt.address, tokenSender.address, l2Receiver.address, expectedSeqNum, toGRT('10'))
 
-      const msgData = createMsgData()
+      const msgData = createMsgData(callHookData)
       const msgDataHash = utils.keccak256(msgData)
       const expectedInboxAccsEntry = createInboxAccsEntry(msgDataHash)
 
@@ -365,15 +411,15 @@ describe('L1GraphTokenGateway', () => {
       })
       it('puts tokens in escrow and creates a retryable ticket', async function () {
         await grt.connect(tokenSender.signer).approve(l1GraphTokenGateway.address, toGRT('10'))
-        await testValidOutboundTransfer(tokenSender.signer, defaultData)
+        await testValidOutboundTransfer(tokenSender.signer, defaultData, emptyCallHookData)
       })
       it('decodes the sender address from messages sent by the router', async function () {
         await grt.connect(tokenSender.signer).approve(l1GraphTokenGateway.address, toGRT('10'))
         const routerEncodedData = utils.defaultAbiCoder.encode(
           ['address', 'bytes'],
           [tokenSender.address, defaultData],
         )
-        await testValidOutboundTransfer(mockRouter.signer, routerEncodedData)
+        await testValidOutboundTransfer(mockRouter.signer, routerEncodedData, emptyCallHookData)
       })
       it('reverts when called with the wrong value', async function () {
         await grt.connect(tokenSender.signer).approve(l1GraphTokenGateway.address, toGRT('10'))
@@ -392,7 +438,7 @@ describe('L1GraphTokenGateway', () => {
           )
         await expect(tx).revertedWith('WRONG_ETH_VALUE')
       })
-      it('reverts when called with nonempty calldata', async function () {
+      it('reverts when called with nonempty calldata, if the sender is not whitelisted', async function () {
         await grt.connect(tokenSender.signer).approve(l1GraphTokenGateway.address, toGRT('10'))
         const tx = l1GraphTokenGateway
           .connect(tokenSender.signer)
@@ -409,6 +455,17 @@ describe('L1GraphTokenGateway', () => {
           )
         await expect(tx).revertedWith('CALL_HOOK_DATA_NOT_ALLOWED')
       })
+      it('allows sending nonempty calldata, if the sender is whitelisted', async function () {
+        await l1GraphTokenGateway
+          .connect(governor.signer)
+          .addToCallhookWhitelist(tokenSender.address)
+        await grt.connect(tokenSender.signer).approve(l1GraphTokenGateway.address, toGRT('10'))
+        await testValidOutboundTransfer(
+          tokenSender.signer,
+          defaultDataWithNotEmptyCallHookData,
+          notEmptyCallHookData,
+        )
+      })
       it('reverts when the sender does not have enough GRT', async function () {
         await grt.connect(tokenSender.signer).approve(l1GraphTokenGateway.address, toGRT('1001'))
         const tx = l1GraphTokenGateway
@@ -503,7 +560,7 @@ describe('L1GraphTokenGateway', () => {
       })
       it('reverts if the gateway is revoked from escrow', async function () {
         await grt.connect(tokenSender.signer).approve(l1GraphTokenGateway.address, toGRT('10'))
-        await testValidOutboundTransfer(tokenSender.signer, defaultData)
+        await testValidOutboundTransfer(tokenSender.signer, defaultData, emptyCallHookData)
         // At this point, the gateway holds 10 GRT in escrow
         // But we revoke the gateway's permission to move the funds:
         await bridgeEscrow.connect(governor.signer).revokeAll(l1GraphTokenGateway.address)
@@ -538,7 +595,7 @@ describe('L1GraphTokenGateway', () => {
       })
       it('sends tokens out of escrow', async function () {
         await grt.connect(tokenSender.signer).approve(l1GraphTokenGateway.address, toGRT('10'))
-        await testValidOutboundTransfer(tokenSender.signer, defaultData)
+        await testValidOutboundTransfer(tokenSender.signer, defaultData, emptyCallHookData)
         // At this point, the gateway holds 10 GRT in escrow
         const encodedCalldata = l1GraphTokenGateway.interface.encodeFunctionData(
           'finalizeInboundTransfer',
diff --git a/test/l2/l2GraphTokenGateway.test.ts b/test/l2/l2GraphTokenGateway.test.ts
