search

zeta-chain / node

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

Depositing ERC-20 fails #2435

Closed fadeev closed 1 day ago

fadeev commented 4 months ago

When depositing ERC-20 tokens through the ERC-20 custody contract, ZetaChain incorrectly reads the recipient from the first 20 bytes of the message, instead of the recipient value from the deposit function call.

Using the exact same contract for two examples.

Example 1

https://zetachain-athens-3.blockscout.com/address/0xf0002B6f1ced3472e7F51738Be7160cE4A875520

Gas deposit:

https://sepolia.etherscan.io/tx/0x2285c75db0ca6df12aadc4f72fd70db6d2fd4ef739189357961f985c9866fd3a

✅ https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/in_tx_hash_to_cctx_data/0x2285c75db0ca6df12aadc4f72fd70db6d2fd4ef739189357961f985c9866fd3a

ERC-20 deposit:

https://sepolia.etherscan.io/tx/0x684f9d8eb559bae27f28b5d8ee9759f8ee40e52c15f12fa076fc97ae03699194

🚨 This should have succeeded! The recipient is correctly specified in the ERC-20 custody contract call.

❌ https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/in_tx_hash_to_cctx_data/0x684f9d8eb559bae27f28b5d8ee9759f8ee40e52c15f12fa076fc97ae03699194

Example 2

https://zetachain-athens-3.blockscout.com/address/0xD0502066f4B8F5E8AbDD57de94c0C31289dBCc51

Gas deposit:

https://sepolia.etherscan.io/tx/0x8e965fca562784972b61506463c680b0be1b9eae9789b6059203c485d5c30595

✅ https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/in_tx_hash_to_cctx_data/0x8e965fca562784972b61506463c680b0be1b9eae9789b6059203c485d5c30595

ERC-20 deposit:

https://sepolia.etherscan.io/tx/0x52965bf5a7ad45b3d7e93711d7f5d65924a89a1fec1a59bc94c4fa00a1ff85f9

🚨 This should not have succeeded! I've intentionally put omnichain contract address in the message to show that ZetaChain reads the address from the first 20 bytes of the message even for ERC-20 deposits.

✅ https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/in_tx_hash_to_cctx_data/0x52965bf5a7ad45b3d7e93711d7f5d65924a89a1fec1a59bc94c4fa00a1ff85f9

fadeev commented 4 months ago

Thanks to jhuntbach from Discord for the bug report!

fadeev commented 4 months ago

Related: https://github.com/zeta-chain/node/issues/1906

lumtis commented 4 months ago

to is set to receiver but is overwritten if an address is parsed from the message https://github.com/zeta-chain/node/blob/f45a56b51b05203726979fc9bdb81a2ef4506920/x/crosschain/keeper/evm_deposit.go#L73 I think an issue is this design choice is poorly documented in the protocol:https://github.com/zeta-chain/node/blob/f45a56b51b05203726979fc9bdb81a2ef4506920/pkg/chains/conversion.go#L35 if the data are length is higher than 20, then the message is used to parse the address.

jhuntbach-bc commented 4 months ago

This is also an issue because we might intend to pass data to an omnichain contract in this message, but if the message is >20 bytes then the keeper will try to parse an address from that data, and then call the wrong contract on zetachain.

We should either always parse an address from the first 20 bytes, in which case we can just add it as a prefix to the message which is fine, or we should never parse the address from the message in which case we can just use the recipient address. The ambiguity here is an issue.

I guess if this design is intentional and going to stay, it should be made clear in the docs that callers need to check if their message is > 20 bytes and if so they MUST prepend the recipient to the message.

brewmaster012 commented 4 months ago

this design inconsistency should be fixed in the revamp of contracts @lumtis @skosito in the meantime, we should update the doc to reflect current behavior. cc @fadeev

lumtis commented 1 day ago

Deisgn flaw will be fixed with gateway upgrade