code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

L2GraphTokenGateway.finalizeInboundTransfer() is "payable for ITokenGateway compatibility, but msg.value must be zero.", and has a check to ensure msg.value == 0.

But L1GraphTokenGateway.finalizeInboundTransfer() does not have this check, meaning any msg.value passed into it is locked forever.

Impact

Medium

Proof Of Concept

Let us take the example of a L2 to L1 withdrawal:

After calling L2GraphTokenGateway.outboundTransfer() and waiting for the dispute period, Alice calls Outbox.executeTransaction()to execute L1GraphTokenGateway.finalizeInboundTransfer().
She passes a non-zero _amount to executeTransaction(), which will in turn mean msg.value in L1GraphTokenGateway.finalizeInboundTransfer() will also be non-zero.
Because there is no withdrawal function in L1GraphTokenGateway, that msg.value is lost.

Tools Used

Manual Analysis

Mitigation

Add a check in L1GraphTokenGateway.finalizeInboundTransfer() to ensure msg.value is zero.

270:         IGraphToken token = graphToken();
271:         require(_l1Token == address(token), "TOKEN_NOT_GRT");
+            require(msg.value == 0, "INVALID_NONZERO_VALUE");

trust1995 commented 1 year ago

L1's finalizeInboundTransfer can only be called from L2's outboundTransfer, which has the msg.value == 0 check.

0xean commented 1 year ago

closing as invalid, @trust1995 is correct here.

joestakey commented 1 year ago

@0xean msg.value on the L2 call is independent from the one on L1 though:

Looking at how a L2-to L1 withdrawal is specified in GIP-0031:

On the L2 side, the L2 Gateway burns GRT from the sender’s account, and initiates sending a message to the L1 Gateway using Arbitrum’s ArbSys interface.
On the L1 side, when the dispute period is complete, a user initiates a transaction through the Arbitrum Outbox, which calls the L1 Gateway.
The L1 Gateway releases the tokens from escrow and sends them to the destination address.

So the withdrawal takes two user calls: -one to L2GraphTokenGateway.finalizeInboundTransfer() on Arbitrum -then a call to Outbox.executeTransaction() on L1.

The msg.value in the L1 call to L1 Gateway is determined in Outbox.executeTransaction() by the parameter _amount. So the msg.value in L1 is independent from the msg.value in L2, and if a user passes a non-zero _amount in Outbox.executeTransaction(), that amount would be lost as L1GraphTokenGateway.finalizeInboundTransfer does not do any msg.value validation?

pcarranzav commented 1 year ago

@joestakey they are not independent though - someone calling Outbox.executeTransaction() would need to provide a proof that the corresponding amount was bridged on L2, see https://github.com/OffchainLabs/nitro/blob/master/contracts/src/bridge/Outbox.sol#L123-L144