Gas limited ETH transfers can lead to a denial of service

[hats-bug-reporter[bot]]

Github username: @erictee2802 Twitter username: 0xEricTee Submission hash (on-chain): 0x4b4e64dca99544efc8afa26aff0f7511551ac63aa3fb7f4290af6694280eebaa Severity: low

Description: Description\

ETH transfers are executed by using the either send or transfer() functions which are gas bound and can potentially lead to an accidental denial of service.

The IncentivizedMessageEscrow.sol, IncentivizedMockEscrow.sol, OnRecvIncentivizedMockEscrow.sol & TimeoutExtension.sol contracts needs to execute ETH transfers in several places across its codebase:

./IncentivizedMessageEscrow.sol:218:                payable(incentive.refundGasTo).transfer(gasRefund); 
./IncentivizedMessageEscrow.sol:437:        if(!payable(refundGasTo).send(refund)) {
./IncentivizedMessageEscrow.sol:438:            payable(SEND_LOST_GAS_TO).transfer(refund);  // If we don't send the gas somewhere, the gas is lost forever.
./IncentivizedMessageEscrow.sol:444:            payable(sourceFeeRecipient).transfer(actualFee);  // If this reverts, then the relayer that is executing this tx provided a bad input.
./IncentivizedMessageEscrow.sol:459:            if(!payable(destinationFeeRecipient).send(deliveryFee)) {  // If this returns false, it implies that the transfer failed.
./IncentivizedMessageEscrow.sol:461:                payable(SEND_LOST_GAS_TO).transfer(deliveryFee);  // If we don't send the gas somewhere, the gas is lost forever.
./IncentivizedMessageEscrow.sol:463:            payable(sourceFeeRecipient).transfer(ackFee);  // If this reverts, then the relayer that is executing this tx provided a bad input.
./IncentivizedMessageEscrow.sol:511:        if(!payable(destinationFeeRecipient).send(forDestinationRelayer)) {
./IncentivizedMessageEscrow.sol:512:            payable(SEND_LOST_GAS_TO).transfer(forDestinationRelayer);  // If we don't send the gas somewhere, the gas is lost forever.
./IncentivizedMessageEscrow.sol:520:        payable(sourceFeeRecipient).transfer(forSourceRelayer);  // If this reverts, then the relayer that is executing this tx provided a bad input.
./apps/mock/IncentivizedMockEscrow.sol:29:        payable(owner()).transfer(accumulator - 1);
./TimeoutExtension.sol:93:            payable(SEND_LOST_GAS_TO).transfer(refund);  // If we don't send the gas somewhere, the gas is lost forever.
./TimeoutExtension.sol:91:        if(!payable(refundGasTo).send(refund)) {  // If this returns false, it implies that the transfer failed.
./TimeoutExtension.sol:95:        payable(sourceFeeRecipient).transfer(ackFee + deliveryFee);  // If this reverts, then the relayer that is executing this tx provided a bad input.

In all of these cases, the methods used to send ETH are send() and transfer() functions. As stated in the documentation, this function is limited to 2300 units of gas. If the receiver is a contract then it can only rely on 2300 units of gas to execute its logic. If the call fails due to being out of gas, the transfer() function reverts, causing the whole transaction to be reverted.

Attack Scenario\

This can be quite problematic as smart wallets and account abstraction are gaining traction and adoption. If the transfer triggers some logic in the receiving contract, the call could potentially be aborted due to gas constraints.

If the receiver is a contract, then there is a potential risk of an accidental denial of service that could prevent calling any of the functions that execute a transfer() call.

Attachments NA

1. Proof of Concept (PoC) File

NA

2. Revised Code File (Optional)

   Use the call() function to transfer ETH, since this is not limited in gas by the compiler. The OpenZeppelin library contains a utility function called sendValue() that implements this behavior. Care should be taken to avoid reentrancy issues and nonReentrant modifier should be used.

[reednaa]

We cannot add additional gas call to these transfers since the relayer would have to carry it. It could also subject the relayer to griefing by other relayers or applications.

Provider a better fix otherwise this is a non-issue.

[0xEricTee]

@reednaa First and foremost, this is indeed a valid issue regardless of whether a proper fix is proposed. We have real world example for your reference: https://medium.com/coinmonks/gemstoneido-contract-stuck-with-921-eth-an-analysis-of-why-transfer-does-not-work-on-zksync-era-d5a01807227d

Some similar issues being reported in Code4rena contests: https://github.com/code-423n4/2023-05-particle-findings/issues/22

Additionally, you can consider implements pull mechanism instead of push mechanism which allows user to reclaim their extra ETH sent to the contract instead of letting contract directly sending back the extra ETH back to users.

[reednaa]

Thanks for the further documentation and including a fix.

The reason why a fix is important for us is that it allows us to better understand what the problem is.

We will have to discuss internally. Do note that ZKSyncEra is not part of the scope of the chains this will be deployed to.

[reednaa]

We have decided to classify this issue as won’t fix. Our decision is based on the following arguments:

• We don’t intend to support ZKSync Era with the current implementation.
• We believe it is within common sense that the payoff should be automatic. Since alternative solutions comes with severe tradeoffs, specifically non-automatic refunds for the end user, it should be given that refunds are sent automatically rather pulled.

Based on these arguments, we have determined the issue is won’t fix.