hats-finance / Tapioca-0xe0b920d38a0900af3bab7ff0ca0af554129f54ad

4 stars 4 forks source link

LZ channel can be blocked due to wrong gas amount being sent #28

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

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

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

Description: Description\ quote function form 0App should be implemented and should be called when estimating gas to be sent with the transaction

Attack Scenario\ In the LayerZero docs it clearly states that

To estimate how much gas a message will cost to be sent and received, you will need to implement a quote function to return an estimate from the Endpoint contract to use as a recommended msg.value.

Currently _claimTwpTapRewardsReceiver is passing this via the call data which is decoded in _claimTwpTapRewardsReceiver before being passed in Line #204 as the msg.value.

  TapTokenSender(rewardToken_).sendPacket{
                value: claimTwTapRewardsMsg_.sendParam[sendParamIndex].fee.nativeFee
            }(claimTwTapRewardsMsg_.sendParam[sendParamIndex], bytes(""));

This could lead to a blocked channel as LayerZero works with blocking channels and a failed message would need to be retried in order to unblock the channel. Attachments

  1. Proof of Concept (PoC) File Below you can see the effected could and see that the fee/gas is currently being taken from the parameters being passed to sendPacket

            TapTokenSender(rewardToken_).sendPacket{
                value: claimTwTapRewardsMsg_.sendParam[sendParamIndex].fee.nativeFee
            }(claimTwTapRewardsMsg_.sendParam[sendParamIndex], bytes(""));
  2. Revised Code File (Optional) Implement a quote() function as per the Layerzero docs. Use the return value from this function to pass that as the nativeFee for _claimTwpTapRewardsReceiver .

function quote(
    uint32 _dstEid, // Destination chain's endpoint ID.
    string memory _message, // The message to send.
    bytes calldata _options, // Message execution options
    bool _payInLzToken // boolean for which token to return fee in
) public view returns (uint256 nativeFee, uint256 lzTokenFee) {
    bytes memory _payload = abi.encode(_message);
    MessagingFee memory fee = _quote(_dstEid, _payload, _options, _payInLzToken);
    return (fee.nativeFee, fee.lzTokenFee);
}
leeftk commented 6 months ago

Forgot to add a link to the docs describing this but in case it helps here it is.

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

0xRektora commented 5 months ago

This was true on LZV1, but LZV2 is asynchronous and non blockable. Attacker will just lose gas trying to spam the network.

leeftk commented 5 months ago

Hmmm I can see how that would effect the impact, so I suppose by that criteria this isn't necessarily a High or an issue at all for that matter. However in my opinion it still makes sense to take another look at adding this functionality. Seems beneficial to users to use the quote() to improve UX and avoid users themselves, having to potentially retry the execution of their message. In the LZ docs it states that

https://docs.layerzero.network/v2/developers/evm/troubleshooting/debugging-messages

"Failure: If the execution fails, the contract reverses the clearing of the payload (re-inserts the payload) and emits an event (LzReceiveAlert) to signal the failure.

Out of Gas: The message fails because the transaction that contains the message doesn't provide enough gas for execution."

If message execution on the destination chain fails due to the reason above, then the execution can be retried without the need for the message itself to be resent from the origin chain.

This can be solved by users or OApp owners by either retrying the message in-browser on LayerZero scan or retrying the call on the LZ endpoint to lzReceive with the proper amount of gas to execute on the destination chain.

I understand this not being "valid" because of the impact, that part makes sense to me. But this does seems like it would greatly improve the user experience and after my research I can't see any downsides to adding this function and verifying that the gas sent stays within a certain deviation of what is returned by the quote() function.