Skip to content

Commit

Permalink
feat: allow callhooks from whitelisted addresses in the Arbitrum GRT …
Browse files Browse the repository at this point in the history 
…bridge
  • Loading branch information
@pcarranzav
pcarranzav committed May 31, 2022
1 parent ade21a2 commit f80cad3
Show file tree
Hide file tree
Showing 4 changed files with 256 additions and 29 deletions.
31 changes: 30 additions & 1 deletion contracts/gateway/L1GraphTokenGateway.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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");

{
Expand Down
49 changes: 47 additions & 2 deletions contracts/l2/gateway/L2GraphTokenGateway.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand Down
75 changes: 66 additions & 9 deletions test/gateway/l1GraphTokenGateway.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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'],
Expand All @@ -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]))
Expand Down Expand Up @@ -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, {
Expand All @@ -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)

Expand Down Expand Up @@ -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'))
Expand All @@ -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)
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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',
Expand Down
Loading

0 comments on commit f80cad3

Please sign in to comment.