Refunding rollups on failed ics20 transfers does not check if sequencer is source or sink of an asset #1514
Comments
SuperFluffy
added
bug
github-merge-queue bot
pushed a commit
that referenced
this issue
Sep 19, 2024
## Summary Fixes incorrect addresses in ics20 refunds, and fixes/adds source vs sink zone detection of received/refunded asets. ## Background This PR started with the goal of using `Ics20WithdrawalFromRollup.rollup_return_address` when emitting ics20 deposit events instead of the rollup's bridge address (which for the rollup was quite meaningless). However, because the original logic was overly convoluted, this patch evolved into a refactor which subsequently revealed 2 more bugs. ## Changes - Refactor ics20 logic: split up `execute_ics20_transfer` into two functions `receive_tokens` and `refund_tokens` - Fix deposit events being emitted with the packet sender/bridge address as the receiving address on the rollup: instead use `Ics20WithdrawalFromRollup.rollup_return_address` which is the address originally supplied and understood by the rollup. - Fix bridge lock events of source tokens being emitted with an extra (port, channel) pair added (this resulting token is likely completely meaningless and not understood by the rollup): instead strip the leading (port, channel) pair to get the original token on sequencer. - Fix refunds to a rollup of failed ics20 transfers not checking if sequencer is their source or sink zone: instead perform the check and decrease the ibc escrow account is necessary. ## Testing Renamed and expanded tests to also check for sink vs source zone assets being received, the correct asset being emitted in bridge lock events, and the correct rollup return address being emitted in refunds. Specifically these tests were added or renamed: - `receive_source_zone_asset_on_sequencer_account` - `receive_sink_zone_asset_on_sequencer_account` - `receive_source_zone_asset_on_bridge_account_and_emit_to_rollup` - `receive_sink_zone_asset_on_bridge_account_and_emit_to_rollup` - `transfer_to_bridge_is_rejected_due_to_invalid_memo` - `transfer_to_bridge_account_is_rejected_due_to_not_permitted_token` - `refund_sequencer_account_with_source_zone_asset` - `refund_sequencer_account_with_sink_zone_asset` - `refund_rollup_with_sink_zone_asset` - `refund_rollup_with_source_zone_asset` - `refund_rollup_with_source_zone_asset_compat_prefix` ## Related Issues Closes #1439 Closes #1496 Closes #1514 --------- Co-authored-by: noot <36753753+noot@users.noreply.github.com>
github-merge-queue bot
pushed a commit
that referenced
this issue
Sep 19, 2024
## Summary Fixes incorrect addresses in ics20 refunds, and fixes/adds source vs sink zone detection of received/refunded asets. ## Background This PR started with the goal of using `Ics20WithdrawalFromRollup.rollup_return_address` when emitting ics20 deposit events instead of the rollup's bridge address (which for the rollup was quite meaningless). However, because the original logic was overly convoluted, this patch evolved into a refactor which subsequently revealed 2 more bugs. ## Changes - Refactor ics20 logic: split up `execute_ics20_transfer` into two functions `receive_tokens` and `refund_tokens` - Fix deposit events being emitted with the packet sender/bridge address as the receiving address on the rollup: instead use `Ics20WithdrawalFromRollup.rollup_return_address` which is the address originally supplied and understood by the rollup. - Fix bridge lock events of source tokens being emitted with an extra (port, channel) pair added (this resulting token is likely completely meaningless and not understood by the rollup): instead strip the leading (port, channel) pair to get the original token on sequencer. - Fix refunds to a rollup of failed ics20 transfers not checking if sequencer is their source or sink zone: instead perform the check and decrease the ibc escrow account is necessary. ## Testing Renamed and expanded tests to also check for sink vs source zone assets being received, the correct asset being emitted in bridge lock events, and the correct rollup return address being emitted in refunds. Specifically these tests were added or renamed: - `receive_source_zone_asset_on_sequencer_account` - `receive_sink_zone_asset_on_sequencer_account` - `receive_source_zone_asset_on_bridge_account_and_emit_to_rollup` - `receive_sink_zone_asset_on_bridge_account_and_emit_to_rollup` - `transfer_to_bridge_is_rejected_due_to_invalid_memo` - `transfer_to_bridge_account_is_rejected_due_to_not_permitted_token` - `refund_sequencer_account_with_source_zone_asset` - `refund_sequencer_account_with_sink_zone_asset` - `refund_rollup_with_sink_zone_asset` - `refund_rollup_with_source_zone_asset` - `refund_rollup_with_source_zone_asset_compat_prefix` ## Related Issues Closes #1439 Closes #1496 Closes #1514 --------- Co-authored-by: noot <36753753+noot@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
When refunding failed ics20 transfers, sequencer short circuits when it can determine that a refund is due to a failed ics20 transfer originating from a rollup:
astria/crates/astria-sequencer/src/ibc/ics20_transfer.rs
Lines 424 to 432 in 1be156e
In
execute_withdrawal_refund_to_rollupit subsequently does not check if sequencer is a source or sink for the refunded asset. This means that the asset will never be taken from the ics20 escrow channel.This issue was found in a review of the refactor PR #1495.
┆Issue Number: ENG-816
The text was updated successfully, but these errors were encountered: