Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

onRevert will fail because the sender has not implemented onZetaRevert execution, resulting in the user being unable to receive a refund #215

Closed
c4-bot-1 opened this issue Dec 7, 2023 · 10 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-489 insufficient quality report This report is not of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@c4-bot-1
Copy link
Contributor

c4-bot-1 commented Dec 7, 2023

Lines of code

https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/protocol-contracts/contracts/evm/ZetaConnector.eth.sol#L90

Vulnerability details

Impact

When sending cross-chain messages, if the sender does not implement onZetaRevert, the sender will not receive a refund after the cross-chain call fails.

Proof of Concept

ZetaConnector contracts, onReceive and onRevert, if the receiver and the sender, do not implement onZetaMessage/onZetaRevert interface, the calls will all fail, resulting in revert.

In onReceive, the recipient must implement the onReceive interface to make sense, and the caller must ensure that the recipient receives the message.

However, the sender may not know that if the onRevert interface is not implemented, it will not be able to receive a refund:

  1. The sender is an EOA account. The EOA account can send cross-chain calls through ZetaConnector, and the calls can be successful. The ZetaConnector#onRevert function will revert because the onZetaRevert interface is not implemented, and the user will not receive a refund.
  2. The sender is a contract account, but onRevert has not been implemented. The sender just wants to send messages and does not know that the onRevert interface needs to be implemented for refund.

Tools Used

vscode manual

Recommended Mitigation Steps

The onRevert function determines whether the receiver has implemented onZetaRevert,
Or try catch,
Or determine whether the caller implements onZetaRevert when sending cross-chain messages.

Assessed type

Error

@c4-bot-1 c4-bot-1 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Dec 7, 2023
c4-bot-6 added a commit that referenced this issue Dec 7, 2023
@c4-pre-sort
Copy link

DadeKuma marked the issue as duplicate of #489

@c4-pre-sort
Copy link

DadeKuma marked the issue as insufficient quality report

@c4-pre-sort c4-pre-sort added the insufficient quality report This report is not of sufficient quality label Dec 22, 2023
@c4-judge
Copy link

0xean marked the issue as unsatisfactory:
Insufficient quality

@c4-judge c4-judge added the unsatisfactory does not satisfy C4 submission criteria; not eligible for awards label Jan 10, 2024
@zhaojio
Copy link

zhaojio commented Jan 12, 2024

@0xean
The comment in #489 says this is a user error,
Can not confirm whether it is the user's error, it seems that the documentation does not explain that EOA accounts cannot initiate cross-chain calls, If the EOA account is prohibited from making calls, it should be checked in the contract.
The actual EOA account can initiate cross-chain calls and the calls can be successful, but after the cross-chain calls fail, the user cannot receive the refund, It will cause the loss of user funds.

@DadeKuma
Copy link

Reasons for insufficient quality:

Users have the burden to check that their tx will work correctly.

There isn't a UI so any EOA users will be technical users that interact directly with this contract.

These users should test their tx with dust amounts, especially if docs don't provide any info about compatibility with EOA.

This is why I'm considering it as user error/QA.

@zhaojio
Copy link

zhaojio commented Jan 12, 2024

Reasons for insufficient quality:

Users have the burden to check that their tx will work correctly.

There isn't a UI so any EOA users will be technical users that interact directly with this contract.

These users should test their tx with dust amounts, especially if docs don't provide any info about compatibility with EOA.

This is why I'm considering it as user error/QA.

However, another problem is that the contract account will fail to be refunded if onReceive is not implemented, and users may use abstract accounts. @DadeKuma @0xean

@zhaojio
Copy link

zhaojio commented Jan 12, 2024

Reasons for insufficient quality:

Users have the burden to check that their tx will work correctly.

There isn't a UI so any EOA users will be technical users that interact directly with this contract.

These users should test their tx with dust amounts, especially if docs don't provide any info about compatibility with EOA.

This is why I'm considering it as user error/QA.

However, another problem is that the contract account will fail to be refunded if onReceive is not implemented, and users may use abstract accounts. @DadeKuma @0xean

We can't assume that the user must use the ui to operate and the transaction will be successfully executed.

@0xean
Copy link

0xean commented Jan 13, 2024

leaving this as judged.

@c4-judge
Copy link

0xean marked the issue as unsatisfactory:
Overinflated severity

@zhaojio
Copy link

zhaojio commented Jan 13, 2024

@0xean
Considering the problem of abstract account, can it be verified as M severity?
I think there is something wrong with the eoa account, so I think it is H.
In some cases, it will cause the loss of users' funds, and I think M is appropriate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-489 insufficient quality report This report is not of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Development

No branches or pull requests

6 participants