Consider adding refundAddress to prevent fund losing if the callhook never succeeds.

[code423n4]

Lines of code

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

Vulnerability details

Impact

If the callhook never succeeds, the L2 transaction will always revert, locking the tokens on the bridge forever. This can be prevented by adding a refund mechanism and a switch to trigger an emergency refund for a particular contract.

Proof of Concept

    /**
     * @notice Receives token amount from L1 and mints the equivalent tokens to the receiving address
     * @dev Only accepts transactions from the L1 GRT Gateway.
     * The function is payable for ITokenGateway compatibility, but msg.value must be zero.
     * Note that whitelisted senders (some protocol contracts) can include additional calldata
     * for a callhook to be executed on the L2 side when the tokens are received. In this case, the L2 transaction
     * can revert if the callhook reverts, potentially locking the tokens on the bridge if the callhook
     * never succeeds. This requires extra care when adding contracts to the whitelist, but is necessary to ensure that
     * the tickets can be retried in the case of a temporary failure, and to ensure the atomicity of callhooks
     * with token transfers.
     * @param _l1Token L1 Address of GRT
     * @param _from Address of the sender on L1
     * @param _to Recipient address on L2
     * @param _amount Amount of tokens transferred
     * @param _data Extra callhook data, only used when the sender is whitelisted
     */
    function finalizeInboundTransfer(
        address _l1Token,
        address _from,
        address _to,
        uint256 _amount,
        bytes calldata _data
    ) external payable override notPaused onlyL1Counterpart nonReentrant {
        require(_l1Token == l1GRT, "TOKEN_NOT_GRT");
        require(msg.value == 0, "INVALID_NONZERO_VALUE");

        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);
        }

        emit DepositFinalized(_l1Token, _from, _to, _amount);
    }

As your comment said "Note that whitelisted senders (some protocol contracts) can include additional calldata for a callhook to be executed on the L2 side when the tokens are received. In this case, the L2 transaction can revert if the callhook reverts, potentially locking the tokens on the bridge if the callhook never succeeds."

There isn't any mechanism to prevent this from happening now.

Recommended Mitigation Steps

Adding a refund mechanism and a switch to trigger an emergency refund for a particular contract.

    mapping(address => bool) public emergencyCallhookRefund;

    event SetEmergencyCallhookRefund(address indexed to, bool refund);
    function setEmergencyCallhookRefund(address to, bool refund) external onlyGovernor {
        emergencyCallhookRefund[to] = refund;
        emit SetEmergencyCallhookRefund(to, refund);
    }

    /**
     * @notice Receives token amount from L1 and mints the equivalent tokens to the receiving address
     * @dev Only accepts transactions from the L1 GRT Gateway.
     * The function is payable for ITokenGateway compatibility, but msg.value must be zero.
     * Note that whitelisted senders (some protocol contracts) can include additional calldata
     * for a callhook to be executed on the L2 side when the tokens are received. In this case, the L2 transaction
     * can revert if the callhook reverts, potentially locking the tokens on the bridge if the callhook
     * never succeeds. This requires extra care when adding contracts to the whitelist, but is necessary to ensure that
     * the tickets can be retried in the case of a temporary failure, and to ensure the atomicity of callhooks
     * with token transfers.
     * @param _l1Token L1 Address of GRT
     * @param _from Address of the sender on L1
     * @param _to Recipient address on L2
     * @param _amount Amount of tokens transferred
     * @param _data Extra callhook data, only used when the sender is whitelisted
     */
    function finalizeInboundTransfer(
        address _l1Token,
        address _from,
        address _to,
        uint256 _amount,
        bytes calldata _data
    ) external payable override notPaused onlyL1Counterpart nonReentrant {
        require(_l1Token == l1GRT, "TOKEN_NOT_GRT");
        require(msg.value == 0, "INVALID_NONZERO_VALUE");

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

        if (_data.length > 0) {
            bytes memory callhookData;
            {
                bytes memory gatewayData;
                (gatewayData, callhookData) = abi.decode(_data, (bytes, bytes));
            }
            try ICallhookReceiver(_to).onTokenTransfer(_from, _amount, callhookData) returns (bytes4 magicValue) {
                require(magicValue == ..., "...");
            } catch {
                if (emergencyCallhookRefund[_to]) {
                    ... Transfer GRT to the refund address ...
                } else {
                    ... Forward the revert message ...
                }
            }

        }

        emit DepositFinalized(_l1Token, _from, _to, _amount);
    }

[0xean]

leaving open for sponsor review

[csanuragjain]

I guess, This is expected and user should be extra careful while using hooks as also mentioned in the comments "Note that whitelisted senders (some protocol contracts) can include additional calldata

• for a callhook to be executed on the L2 side when the tokens are received. In this case, the L2 transaction
• can revert if the callhook reverts, potentially locking the tokens on the bridge if the callhook
• never succeeds. This requires extra care when adding contracts to the whitelist, but is necessary to ensure that
• the tickets can be retried in the case of a temporary failure, and to ensure the atomicity of callhooks
• with token transfers."

[pcarranzav]

As @csanuragjain noted, this is expected behavior. We want the transfer to revert if the callhook fails, because this allows the ticket to be retried, assuming the failure is temporary. The proposed solution would send the tokens to the refund address, so the tokens wouldn't be lost, but this could potentially break something else in the protocol, as the message from L1 is never received.

Any callhooks that we whitelisted will be checked to ensure they don't have any paths that revert permanently except for critical errors that would indicate something else is going very wrong. Off-chain monitoring can be set up so that if anything really unexpected happens, we should be able to keep tickets alive while we fix/upgrade the destination contract to stop reverting.

[Chomtana]

With emergencyCallhookRefund switch (default to false), the transfer will revert by default and allow the user to retry with retryable ticket as intended. It helps in an extreme case where you can't fix that contract. Without emergencyCallhookRefund switch, the fund may be lost forever. But if you set emergencyCallhookRefund to true, you may bridge the fund back to the source chain and retry again on the newly deployed contract. However, it is up to you to implement it or not.

[0xean]

downgrading to QA

[0xean]

dupe of #168 - wardens QA report