penumbra-zone / penumbra

Penumbra is a fully private proof-of-stake network and decentralized exchange for the Cosmos ecosystem.
https://penumbra.zone
Apache License 2.0
364 stars 289 forks source link

ibc: packet timeouts elapse, but withdrawn funds aren't refunded #4634

Closed conorsch closed 1 month ago

conorsch commented 1 month ago

Describe the bug We believe that the Penumbra IBC implementation is not honoring packet timeouts on chain A messages.

To Reproduce

There are two ways to reproduce this behavior. The most comprehensive is to run ICT on the Penumbra integration branches and view the test failure: TK

A lighter approach is to test against the Testnet 77 (which has a Hermes relayer running on it):

  1. Craft a withdrawal with an intentionally low timeout-timestamp: pcli tx withdraw --channel 8 --timeout-timestamp 5 100gn --to osmo1dlp27fw9ecdk6rwlxq4pqtjycheundlt09h7u7
  2. Observe that balance is decremented by the withdrawn value, i.e. 100gn in the command above.
  3. Wait a few minutes, check balance again, confirm that the withdrawn value remains decremented, when it should be refunded.

Expected behavior I expect that if I packet timeout is triggered on an IBC withdrawal out of Penumbra, then the withdrawn funds are refunded. That's not what we're seeing.

Additional context This problem was reported by @jtieri of SL, in the course of writing Penumbra integration for interchaintest.

avahowell commented 1 month ago

Note that the test here uses a timeout timestamp that is already expired for any chain; in this case we should just reject the tx to prevent users from sending a withdrawal that can never succeed. #4636 does this

avahowell commented 1 month ago

My current suspicion is:

if the above is true then the fix is to merge https://github.com/penumbra-zone/penumbra/pull/4636 to prevent (1) from ever happening, and to work with SL to make sure that ICT quantizes their timestamps

avahowell commented 1 month ago

We found a separate issue that would prevent timeouts from being validated, see #4638