code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Description / Proof of Concept

As stated in its comment header, L1GraphTokenGateway.outboundTransfer() can potentially lock tokens on the bridge if its callhook never succeeds. This issue is considered in terms of the callhook contract call's code reverting in the comment, but consider that the call could also be rendered impossible to complete by its gas parameters.

Consider a situation where gas prices spike well beyond _gasPriceBid on Arbitrum. This would create a temporary block to the resolution of the callhook. Then consider a situation where the gas prices of some of the EVM codes are also changed during this spike period. This could cause the value of _maxGas to become too low. Once this change has occurred, the gas price no longer matters: too many units of gas are now required.

Impact

In the situation described, the tokens would be locked on the bridge until the AVM code gas prices were lowered, which might never happen.

Tools Used

Manual Inspection

Recommended Mitigation Steps

Enforce a buffer to the values of both _gasPriceBid and _maxGas within L1GraphTokenGateway.outboundTransfer().

0xean commented 1 year ago

Downgrading to M for the moment and will wait for sponsor review. M is due to the external factor of gas prices persisting beyond expectations for the duration of the retry window and being low enough when the transaction was created.

pcarranzav commented 1 year ago

This is not really how the retryable ticket works - if the gas parameters supplied from L1 are insufficient, anyone can retry the ticket in L2, in which case the maxGas and gasPriceBid parameters from L1 are irrelevant, the L2 user will specify new values in the L2 transaction.

If the user is not willing to pay the higher prices, and does not want to pay to keep the ticket alive, then yes, the tokens will be lost, but this is standard behavior with retryable tickets and there's not much we can do about it.

In the case of protocol messages containing important callhooks that affect more than a single user, there would be off-chain monitoring to ensure tickets are not lost in flight.