onTokenTransfer should returns a magicValue that is validated after.

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-thegraph/blob/309a188f7215fa42c745b136357702400f91b4ff/contracts/l2/gateway/L2GraphTokenGateway.sol#L244 https://github.com/code-423n4/2022-10-thegraph/blob/309a188f7215fa42c745b136357702400f91b4ff/contracts/gateway/ICallhookReceiver.sol#L18-L22

Vulnerability details

Impact

onTokenTransfer should return a magicValue that is validated after. If a magicValue is not returned or incorrectly returned, it may possible that onTokenTransfer is failed but not reverted or it is a different onTokenTransfer implementation than the one we are looking for.

Proof of Concept

ICallhookReceiver(_to).onTokenTransfer(_from, _amount, callhookData);

There aren't any return values that indicate whether onTokenTransfer is a success or not.

Recommended Mitigation Steps

onTokenTransfer should returns a magicValue that is validated after in finalizeInboundTransfer.

    function onTokenTransfer(
        address _from,
        uint256 _amount,
        bytes calldata _data
    ) external returns(bytes4);

...

    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));
            }
            bytes4 magicValue = ICallhookReceiver(_to).onTokenTransfer(_from, _amount, callhookData);
            require(magicValue == ..., "...");
        }

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

[0xean]

closing as invalid, the docs already clear state what the failure scenario should be

onTokenTransfer(uint256 _from, uint256 _amount, bytes _data): Triggers any additional behavior by the receiver after the tokens have been transferred. The L1 sender address is available in _from, the amount of tokens in _amount, and the additional (usually ABI-encoded) data sent from L1 is available in _data. If the function reverts, the token transfer will revert, so the retryable ticket can be retried until it expires. Therefore it’s recommended that this function only reverts in critical failure scenarios, otherwise the transferred tokens will be lost.