Skip to content

Commit 2e0038b

Browse files
committed
fix: validate Arbitrum addresses and callhook senders are contracts (OZ L-01 from #700)
1 parent 1d5dde2 commit 2e0038b

File tree

2 files changed

+44
-12
lines changed

2 files changed

+44
-12
lines changed

contracts/gateway/L1GraphTokenGateway.sol

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,8 @@ contract L1GraphTokenGateway is GraphTokenGateway, L1ArbitrumMessenger {
109109
function setArbitrumAddresses(address _inbox, address _l1Router) external onlyGovernor {
110110
require(_inbox != address(0), "INVALID_INBOX");
111111
require(_l1Router != address(0), "INVALID_L1_ROUTER");
112+
require(Address.isContract(_inbox), "INBOX_MUST_BE_CONTRACT");
113+
require(Address.isContract(_l1Router), "ROUTER_MUST_BE_CONTRACT");
112114
inbox = _inbox;
113115
l1Router = _l1Router;
114116
emit ArbitrumAddressesSet(_inbox, _l1Router);
@@ -151,6 +153,7 @@ contract L1GraphTokenGateway is GraphTokenGateway, L1ArbitrumMessenger {
151153
*/
152154
function addToCallhookWhitelist(address _newWhitelisted) external onlyGovernor {
153155
require(_newWhitelisted != address(0), "INVALID_ADDRESS");
156+
require(Address.isContract(_newWhitelisted), "MUST_BE_CONTRACT");
154157
require(!callhookWhitelist[_newWhitelisted], "ALREADY_WHITELISTED");
155158
callhookWhitelist[_newWhitelisted] = true;
156159
emit AddedToCallhookWhitelist(_newWhitelisted);

test/gateway/l1GraphTokenGateway.test.ts

Lines changed: 41 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import {
1616
toGRT,
1717
Account,
1818
applyL1ToL2Alias,
19+
provider,
1920
} from '../lib/testHelpers'
2021
import { BridgeEscrow } from '../../build/types/BridgeEscrow'
2122

@@ -61,6 +62,8 @@ describe('L1GraphTokenGateway', () => {
6162
;[governor, tokenSender, l2Receiver, mockRouter, mockL2GRT, mockL2Gateway, pauseGuardian] =
6263
await getAccounts()
6364

65+
// Dummy code on the mock router so that it appears as a contract
66+
await provider().send('hardhat_setCode', [mockRouter.address, '0x1234'])
6467
fixture = new NetworkFixture()
6568
fixtureContracts = await fixture.load(governor.signer)
6669
;({ grt, l1GraphTokenGateway, bridgeEscrow } = fixtureContracts)
@@ -128,6 +131,16 @@ describe('L1GraphTokenGateway', () => {
128131
.setArbitrumAddresses(inboxMock.address, mockRouter.address)
129132
await expect(tx).revertedWith('Caller must be Controller governor')
130133
})
134+
it('rejects setting an EOA as router or inbox', async function () {
135+
let tx = l1GraphTokenGateway
136+
.connect(governor.signer)
137+
.setArbitrumAddresses(tokenSender.address, mockRouter.address)
138+
await expect(tx).revertedWith('INBOX_MUST_BE_CONTRACT')
139+
tx = l1GraphTokenGateway
140+
.connect(governor.signer)
141+
.setArbitrumAddresses(inboxMock.address, tokenSender.address)
142+
await expect(tx).revertedWith('ROUTER_MUST_BE_CONTRACT')
143+
})
131144
it('sets inbox and router address', async function () {
132145
const tx = l1GraphTokenGateway
133146
.connect(governor.signer)
@@ -192,42 +205,56 @@ describe('L1GraphTokenGateway', () => {
192205
it('is not callable by addreses that are not the governor', async function () {
193206
const tx = l1GraphTokenGateway
194207
.connect(tokenSender.signer)
195-
.addToCallhookWhitelist(tokenSender.address)
208+
.addToCallhookWhitelist(fixtureContracts.rewardsManager.address)
196209
await expect(tx).revertedWith('Caller must be Controller governor')
197-
expect(await l1GraphTokenGateway.callhookWhitelist(tokenSender.address)).eq(false)
210+
expect(
211+
await l1GraphTokenGateway.callhookWhitelist(fixtureContracts.rewardsManager.address),
212+
).eq(false)
198213
})
199-
it('adds an address to the callhook whitelist', async function () {
214+
it('rejects adding an EOA to the callhook whitelist', async function () {
200215
const tx = l1GraphTokenGateway
201216
.connect(governor.signer)
202217
.addToCallhookWhitelist(tokenSender.address)
218+
await expect(tx).revertedWith('MUST_BE_CONTRACT')
219+
})
220+
it('adds an address to the callhook whitelist', async function () {
221+
const tx = l1GraphTokenGateway
222+
.connect(governor.signer)
223+
.addToCallhookWhitelist(fixtureContracts.rewardsManager.address)
203224
await expect(tx)
204225
.emit(l1GraphTokenGateway, 'AddedToCallhookWhitelist')
205-
.withArgs(tokenSender.address)
206-
expect(await l1GraphTokenGateway.callhookWhitelist(tokenSender.address)).eq(true)
226+
.withArgs(fixtureContracts.rewardsManager.address)
227+
expect(
228+
await l1GraphTokenGateway.callhookWhitelist(fixtureContracts.rewardsManager.address),
229+
).eq(true)
207230
})
208231
})
209232
describe('removeFromCallhookWhitelist', function () {
210233
it('is not callable by addreses that are not the governor', async function () {
211234
await l1GraphTokenGateway
212235
.connect(governor.signer)
213-
.addToCallhookWhitelist(tokenSender.address)
236+
.addToCallhookWhitelist(fixtureContracts.rewardsManager.address)
214237
const tx = l1GraphTokenGateway
215238
.connect(tokenSender.signer)
216-
.removeFromCallhookWhitelist(tokenSender.address)
239+
.removeFromCallhookWhitelist(fixtureContracts.rewardsManager.address)
217240
await expect(tx).revertedWith('Caller must be Controller governor')
218-
expect(await l1GraphTokenGateway.callhookWhitelist(tokenSender.address)).eq(true)
241+
expect(
242+
await l1GraphTokenGateway.callhookWhitelist(fixtureContracts.rewardsManager.address),
243+
).eq(true)
219244
})
220245
it('removes an address from the callhook whitelist', async function () {
221246
await l1GraphTokenGateway
222247
.connect(governor.signer)
223-
.addToCallhookWhitelist(tokenSender.address)
248+
.addToCallhookWhitelist(fixtureContracts.rewardsManager.address)
224249
const tx = l1GraphTokenGateway
225250
.connect(governor.signer)
226-
.removeFromCallhookWhitelist(tokenSender.address)
251+
.removeFromCallhookWhitelist(fixtureContracts.rewardsManager.address)
227252
await expect(tx)
228253
.emit(l1GraphTokenGateway, 'RemovedFromCallhookWhitelist')
229-
.withArgs(tokenSender.address)
230-
expect(await l1GraphTokenGateway.callhookWhitelist(tokenSender.address)).eq(false)
254+
.withArgs(fixtureContracts.rewardsManager.address)
255+
expect(
256+
await l1GraphTokenGateway.callhookWhitelist(fixtureContracts.rewardsManager.address),
257+
).eq(false)
231258
})
232259
})
233260
describe('Pausable behavior', () => {
@@ -479,6 +506,8 @@ describe('L1GraphTokenGateway', () => {
479506
await expect(tx).revertedWith('CALL_HOOK_DATA_NOT_ALLOWED')
480507
})
481508
it('allows sending nonempty calldata, if the sender is whitelisted', async function () {
509+
// Make the sender a contract so that it can be allowed to send callhooks
510+
await provider().send('hardhat_setCode', [tokenSender.address, '0x1234'])
482511
await l1GraphTokenGateway
483512
.connect(governor.signer)
484513
.addToCallhookWhitelist(tokenSender.address)

0 commit comments

Comments
 (0)