Lack of safety check on ```_to``` address in ```onApprove``` function (L1CrossDomainMessenger)

[mehdi-defiesta]

Describe the bug Eventhough the data seems to be appropriately unpacked in unpackOnApproveData function, there is a lack of safety check on _to address in onApprove function (L1CrossDomainMessenger). Users could send native token to the wrong wallet if the data encoded is not appropriately setup

function onApprove(
        address _owner,
        address,
        uint256 _amount,
        bytes calldata _data
    )
        external
        override
        returns (bool)
    {
        require(msg.sender == address(nativeTokenAddress()), "only accept native token approve callback");

        //@audit-issue lack of safety check on _to address. users could send native token to the wrong wallet if the data encoded is not appropriately setup
        (address from, address to, uint256 amount, uint32 minGasLimit, bytes calldata message) =
            unpackOnApproveData(_data);
        require(_owner == from && _amount == amount && amount > 0, "invalid onApprove data");
        _sendNativeTokenMessage(from, to, amount, minGasLimit, message);
        return true;
    }

impact user seeing their funds lost if the data is not encoded appropriately.

Severity: Low Severity is low because the user encoding the wrong _to address would only blame himself.

Recommendation adding a parameter _to and a requirement to check if the data is correct :

function onApprove(
        address _owner,
        address _to,
        uint256 _amount,
        bytes calldata _data
    )
        external
        override
        returns (bool)
    {
        require(msg.sender == address(nativeTokenAddress()), "only accept native token approve callback");
        (address from, address to, uint256 amount, uint32 minGasLimit, bytes calldata message) =
            unpackOnApproveData(_data);
        require(_owner == from && _amount == amount && amount > 0, "invalid onApprove data");
     @> require(_to == to, "wrong _to address");
        _sendNativeTokenMessage(from, to, amount, minGasLimit, message);
        return true;
    }

Demo

[nguyenzung]

Thank you @mehdi-defiesta

I am thinking about simplifying the data structure of bytes calldata _data by eliminating redundant info about from, to, amount

What do you think if we update like:

    function onApprove(
            address _owner,
            address _to,
            uint256 _amount,
            bytes calldata _data
    )
        external
        override
        returns (bool)
    {
            require(msg.sender == address(nativeTokenAddress()), "only accept native token approve callback");
            (uint32 minGasLimit, bytes calldata message) = unpackOnApproveData(_data);
            _sendNativeTokenMessage(_owner, _to, _amount, minGasLimit, message);
            return true;
    }

[mehdi-defiesta]

this one is safer and works as well :)

[nguyenzung]

Please check: https://github.com/tokamak-network/tokamak-thanos/tree/OR-1807-fix-issues-from-the-internal-audit-s-feedback

[theo-learner]

@mehdi-defiesta Could you check https://github.com/tokamak-network/tokamak-thanos/commit/8b4e841ce091b6bde8630e5a8c052189b2d0579a?

[mehdi-defiesta]

Sorry for the delay. The modification seems to mitigate the risk. Thabk you @boohyung