hats-finance / Tapioca-0xe0b920d38a0900af3bab7ff0ca0af554129f54ad

4 stars 4 forks source link

Attacker can DOS LZ channel due to lack of minimum gas check #38

Open hats-bug-reporter[bot] opened 4 months ago

hats-bug-reporter[bot] commented 4 months ago

Github username: @leeftk Twitter username: SolidityAuditor Submission hash (on-chain): 0x092f5b4c34586e7f03b2ab8077a62564b45d3e31f652dccfa4e1ce03869a29ac Severity: high

Description: Description\ Malicious users can specify low gas fees to block a channel. This issue is similar to one that was found in a previous Tapioca contest which can be seen here. The idea is that the sendPacket function calls the TapiocaOmniChainSender. Inside this contract, we can see the following sendPacket function.

    function sendPacket(LZSendParam calldata _lzSendParam, bytes calldata _composeMsg)
        external
        payable
        returns (MessagingReceipt memory msgReceipt, OFTReceipt memory oftReceipt)
    {
        // @dev Applies the token transfers regarding this send() operation.
        // - amountDebitedLD is the amount in local decimals that was ACTUALLY debited from the sender.
        // - amountToCreditLD is the amount in local decimals that will be credited to the recipient on the remote OFT instance.
        (uint256 amountDebitedLD, uint256 amountToCreditLD) =
            _debit(_lzSendParam.sendParam.amountLD, _lzSendParam.sendParam.minAmountLD, _lzSendParam.sendParam.dstEid);

        // @dev Builds the options and OFT message to quote in the endpoint.
        (bytes memory message, bytes memory options) =
            _buildOFTMsgAndOptions(_lzSendParam.sendParam, _lzSendParam.extraOptions, _composeMsg, amountToCreditLD);

        // @dev Sends the message to the LayerZero endpoint and returns the LayerZero msg receipt.
        msgReceipt =
            _lzSend(_lzSendParam.sendParam.dstEid, message, options, _lzSendParam.fee, _lzSendParam.refundAddress);
        // @dev Formulate the OFT receipt.
        oftReceipt = OFTReceipt(amountDebitedLD, amountToCreditLD);

        emit OFTSent(msgReceipt.guid, _lzSendParam.sendParam.dstEid, msg.sender, amountDebitedLD);
    }

In the function above we can still see that the fee is being passed along as an argument meaning that no validation is being done to see if it is above a certain threshold. This allows a malicious attacker to block this channel by submitting transactions below the minimum required fee.

Attack Scenario\ The attacker called sendPacket with a very low msg.value or gas amount being sent with a message via _lzSendParam. This then gets sent to the Relayer which delivers it to the destination chain and passes this amount to be executed. The line where this happens is shown below. ILayerZeroReceiver(_receiver).lzReceive{ value: msg.value }(_origin, _guid, _message, msg.sender, _extraData);

  1. Proof of Concept (PoC) File

The issue here is that due to the small value being passed with the message the message will actually revert in the lzReceive function causing the channel to be blocked and the payload to them to be cleared by a delegate.

    function clear(address _oapp, Origin calldata _origin, bytes32 _guid, bytes calldata _message) external {
        _assertAuthorized(_oapp);

        bytes memory payload = abi.encodePacked(_guid, _message);
        _clearPayload(_oapp, _origin.srcEid, _origin.sender, _origin.nonce, payload);
        emit PacketDelivered(_origin, _oapp);
    }

Link here.

  1. Remediation Steps

Ideally, calling quote() on the LayerZero EndpointV2 should get you close to estimating enough gas, as stated in their comments.

Since the quotes are given on chain there is a
    /// race condition in which the prices could change between the time the user gets their quote and the time they
    /// submit their message.

So ideally, you do enough testing on LZ testnet to find a good threshold for min gas. You could also call quote() and add a minimum amount on top of that and require that any transaction sent has a gaslimit above this number. Any excess gas would be refunded, ensuring that if the gas changes between the time quote() is called and the function is executed, it will at least be enough for the channel not to get blocked.

0xRektora commented 3 months ago

This is based off an outdated version of the protocol that used LZv1. Current code uses LZv2, which gets rid of this attack vector.

leeftk commented 3 months ago

You can see from the LZ docs here that while a transaction may not be blocked it can still be stalled due to OUT_OF_GAS issues on the destination chain.

https://docs.layerzero.network/v2/developers/evm/gas-settings/tx-pricing

To mitigate the risk of transactions stalling due to OUT-OF-GAS issues on the destination, it is advisable to test gas costs for your _lzReceive or lzCompose contract logic, and incorporate a gas buffer by allocating additional gas upfront depending on the chain.

Also by hats finance rules for Medium severity issues here.

Vulnerabilities that impact the user experience but do not compromise the overall security of the protocol.

I agree this isn't a high as the impact is just on the UX and not user funds themselves. However, I still believe this is a valid issue and should be remediated as not sending enough gas with the transaction will cause it to be stalled and the fix given is still valid.