code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

The callhook for whitelisted contracts adds an additional layer of complexity that can have multiple points of failure. If the execution of L2GraphTokenGateway.finalizeInboundTransfer fails indefinitely, there is currently no way to recover the Graph-tokens locked in the bridge.

Scenarios where such indefinite reverts might occur are a misconfiguration or faulty programming of the ICallhookReceiver as well as a whitelisted contract being compromised and an attacker passing an invalid address or invalid data to grief (though a hacker would be more likely to just steal the tokens by bridging them back if he had that much control)

Proof of Concept

The issue is already noted in the comments of L1GraphTokenGateway.outboundTransfer ("L2 transaction can revert if the callhook reverts, potentially locking the tokens on the bridge if the callhook never succeeds"), but no mitigation method is provided. Just to re-elaborate, the problem lies within the fact that minting of L2-tokens and the call to the ICallhookReceiver both happen in L2GraphTokenGateway.finalizeInboundTransfer:

...
    L2GraphToken(calculateL2TokenAddress(l1GRT)).bridgeMint(_to, _amount);

    if (_data.length > 0) {
        bytes memory callhookData;
        {
            bytes memory gatewayData;
            (gatewayData, callhookData) = abi.decode(_data, (bytes, bytes));
        }
        ICallhookReceiver(_to).onTokenTransfer(_from, _amount, callhookData);
    }
...

If the execution of this function fails, no tokens are minted on L2 and consequently nothing can be bridged back to unlock the tokens on L1.

Tools Used

Manual review

Recommended Mitigation Steps

In L2GraphTokenGateway.finalizeInboundTransfer, catch reverts of ICallhookReceiver.onTokenTransfer and perform an outbound transfer back to L1, something like the following (does not catch all cases, e.g. if _to is an EOA; this just serves to illustrate the approach):

try ICallhookReceiver(_to).onTokenTransfer(_from, _amount, callhookData) {
    // emit event or just do nothing, as there are no return values to process
} catch {
    // perform an outbound transfer back to L1 with sender==_to and receiver==_from and same amount.
    // can't just call outboundTransfer(...) due to reentrancy protection. Could move the logic to an internal
    // _outboundTransfer(...) function for that matter to avoid code duplication. 
}

The drawback to this approach would be a possibly lengthy delay of 7 days before the tokens are back on L1 and the tx can be resubmitted (unless there are enough spare tokens). Also the ability to immediately retry on L2 is lost in case that the cause was just a temporary failure due to gas or a quickly fixable problem.

trust1995 commented 1 year ago

This is not providing any new information for the project.

0xean commented 1 year ago

Closing as invalid, project is already aware.