search

tokamak-network / tokamak-thanos

MIT License
7 stars 3 forks source link

Event value when withdrawing from L2 #240

Closed zzooppii closed 2 weeks ago

zzooppii commented 2 weeks ago

Describe the bug When withdrawing from L2, NativeTON is fine, but when it is ETH or ERC20, the SentMessageExtension1 value is wrong.

Configuration

Impact Incorrect amount information

Recommendation Just as there is sendNativeTokenMessage for nativeTON in L1, it is also called separately in L2.

Exploit Scenario NativeTON https://github.com/tokamak-network/tokamak-thanos/blob/5ad9baac98217a0c1533969b00076d9a4443edba/packages/tokamak/contracts-bedrock/src/L2/L2StandardBridge.sol#L306-L327

ERC20 https://github.com/tokamak-network/tokamak-thanos/blob/5ad9baac98217a0c1533969b00076d9a4443edba/packages/tokamak/contracts-bedrock/src/L2/L2StandardBridge.sol#L371-L392

ETH https://github.com/tokamak-network/tokamak-thanos/blob/5ad9baac98217a0c1533969b00076d9a4443edba/packages/tokamak/contracts-bedrock/src/universal/StandardBridge.sol#L417-L461

SendMessage https://github.com/tokamak-network/tokamak-thanos/blob/5ad9baac98217a0c1533969b00076d9a4443edba/packages/tokamak/contracts-bedrock/src/universal/CrossDomainMessenger.sol#L176-L196

sendNativeTokenMessage https://github.com/tokamak-network/tokamak-thanos/blob/5ad9baac98217a0c1533969b00076d9a4443edba/packages/tokamak/contracts-bedrock/src/L1/L1CrossDomainMessenger.sol#L180-L215

Just as you need to sendMessage separately when depositing NativeTON in L1, if you do not sendMessage separately when withdrawing ERC20 or ETH in L2, the value of SentMessageExtension1 will always be 0 for ERC20 or ETH.

Demo

0x6e616d commented 2 weeks ago

I think it is correct behavior, Optimism team added this event to support the SDK to query the data(couldn't modify the old event SentMessage). PR: https://github.com/ethereum-optimism/optimism/pull/3066/files

And on the SDK, they extract the value param https://github.com/tokamak-network/tokamak-thanos/blob/main/packages/tokamak/sdk/src/cross-chain-messenger.ts#L345

nguyenzung commented 2 weeks ago

Thank you @zzooppii

It is fine here. The amount in the event is only used for native token.

zzooppii commented 2 weeks ago

okay Thank you