code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

The finalizeInboundTransfer() function in both L1GraphTokenGateway.sol and L2GraphTokenGateway.sol have notPaused modifier, which may results in token bridge transaction is stuck and never go through when one of the token gateway is paused.

Proof of Concept

Let's take a situation of L1 token gateway is paused while L2 gateway is not paused.

Then a user initiated a token bridging with L2 token gateway to transfer token from L2 to L1. The outboundTransfer() in L2 finish successfully.

But the calling finalizeInboundTransfer() on L1 token gateway fails due to the notPaused modifier.

It will result in the token is stuck in the bridge. It will cause the user think his token is lost in the bridge as when he initiates the transaction he does not receive any warning or error messages but the token disappear.

Tools Used

Recommended Mitigation Steps

Remove the notPaused modifier from the signature for the finalizeInboundTransfer() function in both L1GraphTokenGateway.sol and L2GraphTokenGateway.sol.

trust1995 commented 1 year ago

It seems to be important for the project to be able to pause these functions in case a big issue is raised. The team has 7 days to fix any issues that occur and unpause the bridge for completion.

0xean commented 1 year ago

Also presumably having the bridges in a mismatched state would be highly unlikely to persist for any significant amount of time. Will leave open for sponsor comment.

pcarranzav commented 1 year ago

Yes, the pause functionality is desired to allow the team to react if there's suspicion of a bridge hack or bug, so we do need to pause on the finalize part as well.

It's also worth noting that after the bridge is unpaused, the user can retry the transaction and get their tokens (though in the L1-to-L2 case it would require keeping the retryable ticket alive if the pause condition persists for too long).