`L1GraphTokenGateway` should not allow `l1Router` as `callhookWhitelist`

[code423n4]

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 the callhookWhitelist, anybody who is using the router can use the call hook.
There is no safe guard against adding l1Router to the callhookWhitelist

Proof of Concept

In the L1GraphTokenGateway::outboundTransfer function, it checks whether the msg.sender is in the whitelist. If the msg.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 as msg.sender. Therefore, if the router is added as a callhookWhitelist, 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 the l1Router, therefore the l1Router.

// L1GraphTokenGateway
// outboundTransfer

212                 (from, maxSubmissionCost, extraData) = parseOutboundData(_data);
213                 require(
214                     extraData.length == 0 || callhookWhitelist[msg.sender] == true,

// L1GraphTokenGateway

152     function addToCallhookWhitelist(address _newWhitelisted) external onlyGovernor {
153         require(_newWhitelisted != address(0), "INVALID_ADDRESS");
154         require(!callhookWhitelist[_newWhitelisted], "ALREADY_WHITELISTED");
155         callhookWhitelist[_newWhitelisted] = true;
156         emit AddedToCallhookWhitelist(_newWhitelisted);
157     }

Tools Used

Manual review

Recommended Mitigation Steps

Add the check require(_newWhitelisted != l1Router);. To be really sure, when l1Router is updated, check whether l1Router is in the whitelist.

[trust1995]

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.

[0xean]

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.

[pcarranzav]

Agree this is more QA than Med, but good find nonetheless