Open code423n4 opened 1 year ago
Seems like a very unlikely mistake to make, to add router to the whitelist. The project mentioned only very few contracts like Rewarder would get in.
I think this comes down to QA, there could be any number of potential addresses that would allow someone to do something that is unexpected, which is why the whitelist exists.
So this issue seems to boil down to if address X is accidentally added to the whitelist, address X can use the hook.... well yes, that is true.
Agree this is more QA than Med, but good find nonetheless
Lines of code
https://github.com/code-423n4/2022-10-thegraph/blob/7ea88cc41f17f2d49961aafec7ebe72daeaad3f9/contracts/gateway/L1GraphTokenGateway.sol#L212-L214 https://github.com/code-423n4/2022-10-thegraph/blob/7ea88cc41f17f2d49961aafec7ebe72daeaad3f9/contracts/gateway/L1GraphTokenGateway.sol#L152-L157
Vulnerability details
Impact
If
l1Router
is added to thecallhookWhitelist
, anybody who is using the router can use the call hook. There is no safe guard against addingl1Router
to thecallhookWhitelist
Proof of Concept
In the
L1GraphTokenGateway::outboundTransfer
function, it checks whether themsg.sender
is in the whitelist. If themsg.sender
is whitelisted, it allows to use the callhook on the L2 side. When someone is using the router to send graph token, the transaction will always have the router asmsg.sender
. Therefore, if the router is added as acallhookWhitelist
, it allows anybody who uses the router to use the callhook, which is not the intended usage based on the specification.In the
L1GraphTokenGateway::addToCallhookWhitelist
(line 152), it does not check the_newWhitelisted
against thel1Router
, therefore thel1Router
.Tools Used
Manual review
Recommended Mitigation Steps
Add the check
require(_newWhitelisted != l1Router);
. To be really sure, whenl1Router
is updated, check whetherl1Router
is in the whitelist.