elhaj - Potential ETH Loss Due to transfer Usage in Requestor Contract on `zkSync`

[sherlock-admin2]

elhaj

medium

Potential ETH Loss Due to transfer Usage in Requestor Contract on zkSync

Summary

• The Requestor contract uses transfer to send ETH which has the risk that it will not work if the gas cost increases/decrease(low Likelihood), but it is highly likely to fail on zkSync due to gas limits. This may make users' ETH irretrievable.

Vulnerability Detail

• Users (or bots) interact with RngWitnet to request a random number and start a draw in the DrawManager contract. To generate a random number, users must provide some ETH that will be sent to WitnetRandomness to generate the random number.

  function startDraw(uint256 rngPaymentAmount, DrawManager _drawManager, address _rewardRecipient) external payable returns (uint24) {
          (uint32 requestId,,) = requestRandomNumber(rngPaymentAmount);
          return _drawManager.startDraw(_rewardRecipient, requestId);
  }

• The ETH sent with the transaction may or may not be used (if there is already a request in the same block, it won't be used). Any remaining or unused ETH will be sent to Requestor, so the user can withdraw it later.

• The issue is that the withdraw function in the Requestor contract uses transfer to send ETH to the receiver. This may lead to users being unable to withdraw their funds .

  function withdraw(address payable _to) external onlyCreator returns (uint256) {
      uint256 balance = address(this).balance;
  >>       _to.transfer(balance);
      return balance;
  }

• The protocol will be deployed on different chains including zkSync, on zkSync the use of transfer can lead to issues, as seen with 921 ETH Stuck in zkSync Era.since it has a fixed amount of gas 23000 which won't be anough in some cases even to send eth to an EOA, It is explicitly mentioned in their docs to not use the transfer method to send ETH here.

notice that in case msg.sender is a contract that have some logic on it's receive or fallback function the ETH is definitely not retrievable. since this contract can only withdraw eth to it's own addres which will always revert.

Impact

• Draw Bots' ETH may be irretrievable or undelivered, especially on zkSync, due to the use of .transfer.

  Code Snippet

• https://github.com/sherlock-audit/2024-05-pooltogether/blob/1aa1b8c028b659585e4c7a6b9b652fb075f86db3/pt-v5-rng-witnet/src/Requestor.sol#L27-L30

  Tool used

  Manual Review , Foundry Testing

  Recommendation

• recommendation to use .call() for ETH transfer.

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

infect3d commented:

invalid__ only affect bots that deliberately chose to set code inside the fallback function

[trmid]

The zkSync doc link provided in the issue does not seem to have information on this behaviour anymore. Is there an updated link on the issue?

[nevillehuang]

request poc

[sherlock-admin4]

PoC requested from @elhajin

Requests remaining: 1

[elhajin]

• here the correct quote from the docs : Use call over .send or .transfer

[InfectedIsm]

Escalate

This submission make the assumption that the caller will set code in their callback to process received ETH. This pose the assumption that those callers will not be aware of the limitation of the transfer/send behavior on these chains. Setting code in the callback isn't required at all for the caller. By simply not setting code in their callback, the issue disappear, thus this is a user error and not a vulnerability of the codebase, but rather a proposition of improvement of the implemented mechanism.

from the doc: https://docs.zksync.io/build/developer-reference/best-practices#use-call-over-send-or-transfer

Avoid using payable(addr).send(x)/payable(addr).transfer(x) because the 2300 gas stipend may not be enough for such calls, especially if it involves state changes that require a large amount of L2 gas for data. Instead, we recommend using call.

[sherlock-admin3]

Escalate

This submission make the assumption that the caller will set code in their callback to process received ETH. This pose the assumption that those callers will not be aware of the limitation of the transfer/send behavior on these chains. Setting code in the callback isn't required at all for the caller. By simply not setting code in their callback, the issue disappear, thus this is a user error and not a vulnerability of the codebase, but rather a proposition of improvement of the implemented mechanism.

from the doc: https://docs.zksync.io/build/developer-reference/best-practices#use-call-over-send-or-transfer

Avoid using payable(addr).send(x)/payable(addr).transfer(x) because the 2300 gas stipend may not be enough for such calls, especially if it involves state changes that require a large amount of L2 gas for data. Instead, we recommend using call.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[nevillehuang]

I wouldn't call this user error because any address can be used to transfer funds to creator. Also it does not just affect caller with no code and there is clear evidence from the sponsor acknowledgement and lack of details in contest that the sponsor wasn't aware of this issue.

[0xAadi]

Disagreeing with the Escalation

By simply not setting code in their callback, the issue disappears.

Removing the callbacks from the caller will not solve this issue.

The core issue is not related to the use of code in the callback; rather, it is a limitation of the ZkSync chain. The core issue arises due to the dynamic gas measurement followed in ZkSync.

ZkSync usually requires more than 2300 gas to process transfer()/send() functions. Therefore, the withdraw() function will always revert due to the lack of enough gas to process transfer() on ZkSync.

ZkSync explicitly mentions this in their security and best practice documentation:

Avoid using payable(addr).send(x)/payable(addr).transfer(x) because the 2300 gas stipend may not be enough for such calls.

The situation worsens if there are any callback functionalities in the caller.

This is a known issue in ZkSync, as evidenced by previous occurrences in other protocols:

• 921 ETH Stuck in zkSync Era
• ZkSync Twitter Post

Note: Please see issues #24 and #139, which explain the issue in detail.

[InfectedIsm]

Fair enough, I wasn't able to find that even without callback execution it could spend more than 2300. Sorry for the misunderstanding, and thanks for the detailed explanation!

[0xAadi]

In addition to my previous comment:

This issue causes a loss of ETH in ZkSync.

According to Sherlock's docs, this issue should be considered as valid HIGH:

IV. How to identify a high issue:

1. Definite loss of funds without (extensive) limitations of external conditions.

Please consider upgrading the severity to HIGH.

Please find my thoughts on the duplicates below:

24 and #139 both identify the same issue in ZkSync.

94 missed the aforementioned issue but did identify another medium-risk attack path.

119 missed both of the aforementioned attack paths but identified a more general issue.

[WangSecurity]

Since the escalator agrees with the other side (correct me if it's wrong) and based on the discussion above, I believe this issue should remain as it is. Planning to reject the escalation.

[0xAadi]

the escalator agrees with the other side (correct me if it's wrong)

true, reference

@WangSecurity, As I mentioned in this duplicate and here, This vulnerability can cause loss of ETH on the zkSync chain.

Therefore, please consider upgrading the severity of this issue from medium to high.

[WangSecurity]

Is there a specific number of ETH that has to be transferred in that case, or it can be as small as 1 wei? And as I understand there's still a possibility the transfer would work correctly?

[InfectedIsm]

Rng draw rewards are very low ( <0.0001 WETH), as it can be seen on the Pool Together dashboard: https://analytics.cabana.fi/optimism The loss that will be experienced by rng drawer isn't large enough to be an argument for a high severity imo

[WangSecurity]

I agree with the comment above, I believe it's a small value and only caused on one chain, hence, I believe medium is more appropriate.

The decision remains the same, planning to reject the escalation and leave the issue as it is.

[WangSecurity]

Result: Medium Has duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• InfectedIsm: rejected

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/GenerationSoftware/pt-v5-rng-witnet/pull/9