If L1GraphTokenGateway's outboundTransfer is called by a contract, the entire msg.value is blackholed, whether the ticket got redeemed or not.

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-thegraph/blob/309a188f7215fa42c745b136357702400f91b4ff/contracts/gateway/L1GraphTokenGateway.sol#L236

Vulnerability details

The outboundTransfer function in L1GraphTokenGateway is used to transfer user's Graph tokens to L2. To do that it eventually calls the standard Arbitrum Inbox's createRetryableTicket. The issue is that it passes caller's address in the submissionRefundAddress and valueRefundAddress. This behaves fine if caller is an EOA, but if it's called by a contract it will lead to loss of the submissionRefund (ETH passed to outboundTransfer() minus the total submission fee), or in the event of failed L2 ticket creation, the whole submission fee. The reason it's fine for EOA is because of the fact that ETH and Arbitrum addresses are congruent. However, the calling contract probably does not exist on L2 and even in the rare case it does, it might not have a function to move out the refund.

The docs don't suggest contracts should not use the TokenGateway, and it is fair to assume it will be used in this way. Multisigs are inherently contracts, which is one of the valid use cases. Since likelihood is high and impact is medium (loss of submission fee), I believe it to be a HIGH severity find.

Impact

If L1GraphTokenGateway's outboundTransfer is called by a contract, the entire msg.value is blackholed, whether the ticket got redeemed or not.

Proof of Concept

Alice has a multisig wallet. She sends 100 Graph tokens to L1GraphTokenGateway, and passes X ETH for submission. She receives an L1 ticket, but since the max gas was too low, the creation failed on L2 and the funds got sent to the multisig address at L2. Therefore, Alice loses X ETH.

Tools Used

https://github.com/OffchainLabs/arbitrum/blob/master/docs/L1_L2_Messages.md Manual audit

Recommended Mitigation Steps

A possible fix is to add an isContract flag. If sender is a contract, require the flag to be true.

Another option is to add a refundAddr address parameter to the API.

[0xean]

I think adding the refundAddr to the API is a good suggestion. I don't see how this qualifies as H however and think M severity would be more appropriate given we are talking about gas refunds.

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.

This seems like a leak of value situation rather than Assets being directly lost or stolen.

[pcarranzav]

The POC described by the warden is no longer valid with Arbitrum Nitro. Retryable ticket submission fee is now checked in L1, so it's impossible that the ticket creation fails in L2 (Arbitrum docs might be outdated on this).

It is true, however, that any refunded value would be lost if the sender is a contract, though they could be reused in future retryable tickets by the same sender (since, if I recall correctly, the refund address will be used to pay for gas in L2). This is standard behavior for retryable tickets though, and users sending funds through the bridge should be aware of this when bridging tokens: in the worst case, the sender has specified a max submission fee, gas price and max gas, and the resulting behavior would be equivalent to exactly those amounts being used.

As for the suggested mitigation, it would require adding a new/overloaded interface, since the existing API is designed to be compatible with Arbitrum's Gateway Router and is standard for Arbitrum token bridges.

For these reasons, I think this finding might be more of a QA severity?

[0xean]

While there may not be a reasonable fix for this I still think it is a M severity issue that integrators / callers from multi-sigs should be aware of.

[pcarranzav]

Sounds fair 👍 I'd appreciate it if the final report noted that the ticket creation failure on L2 is not an issue though

[0xean]

Our comments should appear on the report, so it will be noted.