`L1GraphTokenGateway:onlyL2Counterpart` does not check whether l2Counterpart is initialized

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-thegraph/blob/7ea88cc41f17f2d49961aafec7ebe72daeaad3f9/contracts/gateway/L1GraphTokenGateway.sol#L81-L82

Vulnerability details

Impact

If the l2Counterpart is not yet set, L1GraphTokenGateway:finalizeInboundTrasfer can be called by a system message

Proof of Concept

The L1GraphTokenGateway:onlyL2Counterpart does not check whether l2Counterpart or the result from l2ToL1Sender call is the zero address. As the result, it will not revert if both of them are zero, but it should, as the call is not from the L2GraphTokenGateway.

// L1GraphTokenGateway

 73     modifier onlyL2Counterpart() {
 74         require(inbox != address(0), "INBOX_NOT_SET");
 75
 76         // a message coming from the counterpart gateway was executed by the bridge
 77         IBridge bridge = IInbox(inbox).bridge();
 78         require(msg.sender == address(bridge), "NOT_FROM_BRIDGE");
 79
 80         // and the outbox reports that the L2 address of the sender is the counterpart gateway
 81         address l2ToL1Sender = IOutbox(bridge.activeOutbox()).l2ToL1Sender();
 82         require(l2ToL1Sender == l2Counterpart, "ONLY_COUNTERPART_GATEWAY");
 83         _;
 84     }

In the IOutbox.sol from Arbitrum, when the l2ToL1Sender returns the zero address, that means this is a system message.

Below comments are from the current Outbox implementation

// IOutbox.sol

    /// @notice When l2ToL1Sender returns a nonzero address, the message was originated by an L2 account
    ///         When the return value is zero, that means this is a system message
    /// @dev the l2ToL1Sender behaves as the tx.origin, the msg.sender should be validated to protect against reentrancies
    function l2ToL1Sender() external view returns (address);

Tools Used

Manual review

Recommended Mitigation Steps

Add the check require(l2Counterpart != address(0) to the onlyL2Counterpart to ensure that l2Counterpart is set.

[trust1995]

Interesting idea. Indeed this is a check that is best added. Some thoughts:

1. We can probably assume l2counterpart has been set when the gateway is functional for users.
2. The report did not develop the lack of check -> system message idea to any impact / exploit.

[0xean]

Will leave this open for sponsor review, but it does seem pretty well understood by the sponsor this is required

    /**
     * @dev Initialize this contract.
     * The contract will be paused.
     * Note some parameters have to be set separately as they are generally
     * not expected to be available at initialization time:
     * - inbox  and l1Router using setArbitrumAddresses
     * - l2GRT using setL2TokenAddress
     * - l2Counterpart using setL2CounterpartAddress
     * - escrow using setEscrowAddress
     * - whitelisted callhook callers using addToCallhookWhitelist
     * - pauseGuardian using setPauseGuardian
     * @param _controller Address of the Controller that manages this contract
     */

And i do agree with @trust1995 that the warden didn't identify the impact of this very clearly. Probably best downgraded to a QA issue.

[pcarranzav]

We would indeed only unpause the gateway only after l2Counterpart has been set. I hadn't seen the fact that l2ToL1Sender could be zero for a system message though (not sure what that even means in an L2 to L1 message context), but it sounds like it's bad enough that Medium is reasonable severity imo (we've already seen bridges being hacked because of misconfigurations...). Though I agree the report doesn't elaborate on this attack vector / potential exploit paths.

Will work on a fix.

[pcarranzav]

Fix PRd in https://github.com/graphprotocol/contracts/pull/740

[trust1995]

As the C4 docs state: 2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

The report does not discuss any potential impact of accepting Arbitrum system messages during initialization period of GraphTokenGateway. Without a stated impact it is not possible to argue for M severity. We don't know what is possible or not possible with system messages (are they relayable, what constraints do they have, etc.). Without a flow where harm is done it is handwavy to say this can cause impact.

Additionally, the contract is not functional until l2counterpart is set, so accepting system messages is only in the earliest init period.

[0xean]

going to downgrade to QA, definitely on the fence here and I think with a better impact, M would be warranted.

[0xean]

dupe of #198, wardens QA report.