tokamak-network / tokamak-bridge

An interface designed to bridge assets between Titan and Ethereum chains.
https://app.bridge.tokamak.network
6 stars 0 forks source link

[Cross Trade - Contract Bugs]: Missing transfer fee #190

Closed blackcow1987 closed 4 weeks ago

blackcow1987 commented 1 month ago

What happened?

When claiming a token in the _request() method, the transfer fee is missing. For tokens with a transfer fee, claimCT/cancelCT always fails because amount of the token is different due to the fee.

Relevant log output

No response

suahnkim commented 1 month ago

this issue is specific for any L2 ERC20 tokens that do not have a mapping to an L1 ERC20 token; this is because any standardBridge from Optimism do not support bridging of L1 tokens that have

I don't think we should support these tokens for the request() as it can complicates the overall logic. If we really want to add support from them, it makes sense to add special interfaces for those cases.

To prevent usage of these types of tokens, I think we have two options:

  1. add logic: check the balance before transfer with balance after transfer (rebase logic tokens will still have issues)
  2. Or add guide/comment that request() do not support tokens with transfer on fee or hooks or rebase logic

2nd option seems best for now.

blackcow1987 commented 1 month ago

@suahnkim Since the provideCT method escrows the target token, tokens with a transfer fee can affect other people's tokens that are escrowed. It seems more appropriate to add code to the L2 contract that can simulate the balance before/after transfer, and use the method to verify it off-chain and then alert the user.

zzooppii commented 1 month ago

@blackcow1987 @suahnkim Since we had no intention of supporting tokens with such special functions, I think it would be right to go in the direction 2 suggested by Suah.

suahnkim commented 1 month ago

@blackcow1987 If it is ok, we won't support this feature until we finish other urgent tasks. Until then, we will make a comment directly on the code that we won't support these types of tokens in the code.

blackcow1987 commented 1 month ago

@suahnkim Yes, it's a low priority task !

suahnkim commented 1 month ago

@blackcow1987 thank you for understanding.

@zzooppii please add relevant comment to the code and share it here. And then, we can close this issue

zzooppii commented 4 weeks ago

@blackcow1987 https://github.com/tokamak-network/crossTrade/commit/2cd40b4bb8e29a1e3d191237d3df91064427d185

In conclusion, We decided to add a comment, so I added a comment.

Thank you.