Executing unnecessary transaction when msg.value is zero in _initiateBridgeETH

[Zena-park]

_initiateBridgeETH function should not execute transaction when msg.value is 0.

https://github.com/tokamak-network/tokamak-thanos/blob/5ad9baac98217a0c1533969b00076d9a4443edba/packages/tokamak/contracts-bedrock/src/L1/L1StandardBridge.sol#L241-L251

function _initiateBridgeETH(
        address _from,
        address _to,
        uint256 _amount,
        uint32 _minGasLimit,
        bytes memory _extraData
    )
        internal
        override
    {
        require(msg.value != 0, "StandardBridge: msg.value is zero amount");

        require(msg.value == _amount, "StandardBridge: bridging ETH must include sufficient ETH value");

[nguyenzung]

Let me update it!

Thank you @Zena-park

[NegruGeorge]

isn't it worth adding the check in the L2StandardBridge's _initiateBridgeNativeToken as well ? @nguyenzung @Zena-park

before:

  function _initiateBridgeNativeToken(
        address _from,
        address _to,
        uint256 _amount,
        uint32 _minGasLimit,
        bytes memory _extraData
    )
        internal
        override
    {
        require(msg.value == _amount, "StandardBridge: Incorrect Native token value");

        // Emit the correct events. By default this will be _amount, but child
        // contracts may override this function in order to emit legacy events as well.
        _emitNativeTokenBridgeInitiated(_from, _to, _amount, _extraData);

        messenger.sendMessage{ value: _amount }({
            _target: address(otherBridge),
            _message: abi.encodeWithSelector(this.finalizeBridgeNativeToken.selector, _from, _to, _amount, _extraData),
            _minGasLimit: _minGasLimit
        });
    }

after:

  function _initiateBridgeNativeToken(
        address _from,
        address _to,
        uint256 _amount,
        uint32 _minGasLimit,
        bytes memory _extraData
    )
        internal
        override
    {
        require(msg.value != 0, "StandardBridge: msg.value is zero amount");
        require(msg.value == _amount, "StandardBridge: Incorrect Native token value");

        // Emit the correct events. By default this will be _amount, but child
        // contracts may override this function in order to emit legacy events as well.
        _emitNativeTokenBridgeInitiated(_from, _to, _amount, _extraData);

        messenger.sendMessage{ value: _amount }({
            _target: address(otherBridge),
            _message: abi.encodeWithSelector(this.finalizeBridgeNativeToken.selector, _from, _to, _amount, _extraData),
            _minGasLimit: _minGasLimit
        });
    }

[NegruGeorge]

maybe in this one as well: finalizeBridgeNativeToken. before:

 function finalizeBridgeNativeToken(
        address _from,
        address _to,
        uint256 _amount,
        bytes calldata _extraData
    )
        public
        payable
        override
        onlyOtherBridge
    {
        require(paused() == false, "StandardBridge: paused");
        require(msg.value == _amount, "StandardBridge: amount sent does not match amount required");
        require(_to != address(this), "StandardBridge: cannot send to self");
        require(_to != address(messenger), "StandardBridge: cannot send to messenger");

        bool success = SafeCall.call(_to, gasleft(), _amount, hex"");
        require(success, "StandardBridge: Native token transfer failed");

        // Emit the correct events. By default this will be _amount, but child
        // contracts may override this function in order to emit legacy events as well.
        _emitNativeTokenBridgeFinalized(_from, _to, _amount, _extraData);
    }

after:

 function finalizeBridgeNativeToken(
        address _from,
        address _to,
        uint256 _amount,
        bytes calldata _extraData
    )
        public
        payable
        override
        onlyOtherBridge
    {
        require(paused() == false, "StandardBridge: paused");
        require(msg.value != 0, "StandardBridge: msg.value is zero amount");
        require(msg.value == _amount, "StandardBridge: amount sent does not match amount required");
        require(_to != address(this), "StandardBridge: cannot send to self");
        require(_to != address(messenger), "StandardBridge: cannot send to messenger");

        bool success = SafeCall.call(_to, gasleft(), _amount, hex"");
        require(success, "StandardBridge: Native token transfer failed");

        // Emit the correct events. By default this will be _amount, but child
        // contracts may override this function in order to emit legacy events as well.
        _emitNativeTokenBridgeFinalized(_from, _to, _amount, _extraData);
    }

[nguyenzung]

Thank you @NegruGeorge

Let me add more!

[NegruGeorge]

Do you think we should have separate issues for every function or is fine to just have everything here ?

[nguyenzung]

I think it is enough for one issue. :joy:

[nguyenzung]

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

@NegruGeorge I tried to update the code as you suggested. However, some unittest cases failed. So I keep the current L2StandardBridge :cry:

[suahnkim]

Question: with regarding _initiateBridgeETH, isn't there a case where, even if the value is 0, we still want to send _extraData (just to emit the event as a data on L2?)

[Zena-park]

_initiateBridgeETH function is a function of L1StandardBridge. If the L1 bridge does not allow transactions that deposit 0 ether amount, it makes sense to revert the transaction when the amount is 0.

[Zena-park]

This proposal has been accepted. https://github.com/tokamak-network/tokamak-thanos/blob/1f974febe8f2992691a349048572a57f4aa4ae57/packages/tokamak/contracts-bedrock/src/L1/L1StandardBridge.sol#L245

I think it would be better to create a separate issue for suggestions on other topics. I will this issue.