crypto-org-chain / cronos

Cronos is the first Ethereum-compatible blockchain network built on Cosmos SDK technology. Cronos aims to massively scale the DeFi, GameFi, and overall Web3 user community by providing builders with the ability to instantly port apps and crypto assets from other chains while benefiting from low transaction fees, high throughput, and fast finality.
Other
290 stars 235 forks source link

Problem: Not being able to clean "bad" transaction included in batch (gravity bridge) #574

Open thomas-nguy opened 2 years ago

thomas-nguy commented 2 years ago

Some transaction can cause the contract to revert. If someone setup a high fee, then there is a risk of DOS attack.

yihuang commented 2 years ago

but it still need to pay the fee, at least the gas used part?

thomas-nguy commented 2 years ago

not sure if it is better to "cancel" the transaction (and they will get refund) or just delete the transaction and send the fund to community pool.

The tricky part is when they are inserted into a batch. We need to delete the batch and free all the transactions but we need to be 100% that the batch "cannot" be relayed otherwise there is a risk of double spend

yihuang commented 2 years ago

Do you mean some send_to_ethereum request will revert at ethereum side, and it's treated as success at cronos side, thus the double spend risk?

thomas-nguy commented 2 years ago

Once a batch is created, then they are "valid" to be relayed if they gather enough signature.

If we cancel the batch and free the transactions, then those transaction can be picked again to form a new batch.

If the transaction is included in two batches (invalid one and new one) and someone send the invalid batch and the new one, then the transaction will be executed twice on Ethereum side, so is the double spend.

thomas-nguy commented 2 years ago

One solution could be to invalidate /cancel "all" the transaction in the batch. But that may affect innocent users

yihuang commented 2 years ago

The issue is there's no flow in gravity-bridge to handle the failure case on the ethereum side, but it could fail in reality, right?

thomas-nguy commented 2 years ago

yes that is correct.

Also the fact that all txs are batched create more complex situation to handle individual failure

I think ideally, we could catch the "revert" in the smart contract and relay this information in the event.

Upon batch execution, if there are some reverted cases on "IERC20.SafeTransfer", the chain can perform an action such as refund the user.

But this would quite a change, and we need to pass the txIds to the smart contract that may introduce extra logic check.

yihuang commented 2 years ago

I guess on a "normal" erc20 contract, it shouldn't fail, but if the token contract has some custom logic attached to the fund transfer process, then anything can happen.

thomas-nguy commented 2 years ago

USDC and USDT have special logic such as "Blacklist" functionality

https://etherscan.io/address/0xdac17f958d2ee523a2206206994597c13d831ec7#code

while some addresses (such as zero address) are predictable, the list can be updated dynamically so it is hard in our side to deal with every cases

thomas-nguy commented 2 years ago

One more controversial solution is just to ignore the revert in the smart contract.

The user sending the "bad transaction" will lose money but at least the other transactions will to go through.

This can be done with minimum change.

calvinaco commented 2 years ago

The user sending the "bad transaction" will lose money

Lose money means the bridged funds are lost too?

thomas-nguy commented 2 years ago

Lose money means the bridged funds are lost too?

Lose money means that tokens will be burn on cronos but the receiver will not receive its token.

Technically the token will still be existing but lock on the gravity contract

calvinaco commented 2 years ago

Another bold idea:

Can we record the reverted Tx when we ignore the error thrown during batch execution? Then we can allow the sender to "claim" the bridged token by signing a newly-introduced transaction on Ethereum Gravity contract and send the token back to Cronos without deducing the ERC20 token, it will be using the same ERC20 -> CRC20 route in Gravity (with destination always set to the sender, hence achieving a revert).

The good thing is the batch continues to execute, so no DoS. Second the sender pays the fee for the claim Tx as well.

One uncertain here is when we ignore the error thrown, does it already mean that particular Tx execution is totally reverted? Or there's a possibility the transfer is at a limbo state?

Another issue is it is creating a new bridge entry point, which requires careful review of the code.

thomas-nguy commented 2 years ago

We can add an event in case the transaction is reverted that can be relayed back.

For since the contract has an admin, the claim would be done through manual operation but yeah, this could be automatised in the future

thomas-nguy commented 2 years ago

I have proposed a POC here to solve this issue relying only on the smart contract

https://github.com/crypto-org-chain/gravity-bridge/pull/67