zeta-chain / node

ZetaChain’s blockchain node and an observer validator client
https://zetachain.com
MIT License
169 stars 109 forks source link

CCTX is aborted and not refunded when the recipient of the withdrawal is a contract #1430

Closed fadeev closed 3 months ago

fadeev commented 11 months ago

I'm withdrawing ZRC-20 MATIC to Polygon and specifying a contract as a recipient address:

https://explorer.zetachain.com/cc/tx/0x07b11723443c8e3e9252f31fc6ef76e5712cc2adfba95031080601970ce488da

The contract on Polygon:

https://mumbai.polygonscan.com/address/0xFB30650AdE3C6793436758c1654AD646962a8DA1

(please, ignore one event in this contract, I was just testing it with a single-chain transfer, not cross-chain)

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

contract Contract {
    event TokenReceived(address from, uint256 amount);

    receive() external payable {
        emit TokenReceived(msg.sender, msg.value);
    }
}

Expected one of the two outcomes:

What happened is an CCTX abort without a refund.

fadeev commented 11 months ago

@andresaiello you mentioned that a contract is a valid target for withdrawal, is there anything wrong with the contract above?

andresaiello commented 11 months ago

is a gas issue. call a contract needs more gas than a transfer. we should align with protocol how to send more gas there

fadeev commented 11 months ago

It would be a huge feature if we resolved gas issues when withdrawing to contracts.

fadeev commented 11 months ago

@lumtis @brewmaster012 I see this issue was added to v12. If we resolve the gas issue and make it possible to withdraw to contracts, would it also be possible to add arbitrary data passing? Or would it be out of scope for this issue?

brewmaster012 commented 11 months ago

@lumtis @brewmaster012 I see this issue was added to v12. If we resolve the gas issue and make it possible to withdraw to contracts, would it also be possible to add arbitrary data passing? Or would it be out of scope for this issue?

Out of scope for v12. Also for withdrawing to a contract, we'll need to make it clear that 1) a certain gas limit is assumed, so if the withdraw triggers a function then it must be within that gaslimit amount. This would only be a problem for ether, not erc20. 2) if the withdrawal tx reverts, then there would be no recourse or refund.

lumtis commented 11 months ago

@brewmaster012 @fadeev can we create an issue for the specific feature on arbitrary data passing? Is the idea to allow withdraw to do a custom smart contract call?

  1. if the withdrawal tx reverts, then there would be no recourse or refund.

We need to consider general refund on ZetaChain for aborted cctx, this looks important, reported several times by code4rena participants.

fadeev commented 11 months ago

Created issues for arbitrary data passing from ZetaChain: https://github.com/zeta-chain/node/issues/1507 & https://github.com/zeta-chain/node/issues/1508

fadeev commented 11 months ago

a certain gas limit is assumed, so if the withdraw triggers a function then it must be within that gaslimit amount. This would only be a problem for ether, not erc20.

@brewmaster012 can we allow the user to provide to the withdraw method a custom fee amount they're willing to pay? For example, I'm writing an omnichain contract, and it is designed to withdraw to my contract on Polygon. I know how much gas on average my contract on Polygon requires, so I can pass this value in my withdraw call, to make sure it doesn't fail. It's up to the contract maintainer to keep this value up to date.

IZRC20(targetTokenAddress).withdraw(recipientAddress, outputAmount, data, gas);

Both data and gas are optional.

brewmaster012 commented 11 months ago

a certain gas limit is assumed, so if the withdraw triggers a function then it must be within that gaslimit amount. This would only be a problem for ether, not erc20.

@brewmaster012 can we allow the user to provide to the withdraw method a custom fee amount they're willing to pay? For example, I'm writing an omnichain contract, and it is designed to withdraw to my contract on Polygon. I know how much gas on average my contract on Polygon requires, so I can pass this value in my withdraw call, to make sure it doesn't fail. It's up to the contract maintainer to keep this value up to date.

IZRC20(targetTokenAddress).withdraw(recipientAddress, outputAmount, data, gas);

Both data and gas are optional.

@fadeev @lumtis : I'm currently hesitant to allow calling arbitrary contract on external chains with the ZRC20.withdraw() function. The main reason is if it's allowed then we generally should expect such calls may revert, and then on ZetaChain we need to revert the tx that generated the withdrawal event (could be a complex contract interaction). How do we do that? I don't see a reliable way to do that other than having the dApp contract that generated the withdrawal event provide a onRevert() that customizes revert function. If we don't allow this, then we can pretty much assume all withdrawal will succeed (provided you don't make it trigger arbitrary logic). This simplifies things considerably both in the dApp and protocol and in my opinion the most significant advantage over message passing. Another consideration is safely--allowing arbitrary user specified logic signed by the TSS address will need to carefully safeguarded.

There is a tradeoff there between functionality and complexity/safety, and I think it can't be in scope of v12.

lumtis commented 3 months ago

Closing since the new contract architecture fix the issue