Do not emit events upon ack status failure

[seanchen1991]

Bug Summary

Amulet alerted us to a security report regarding a vulnerability that is similar in nature to the ibc-go Huckleberry vulnerability. We should address the cause(s) of the vulnerability in ibc-rs and communicate with Amulet once the issue has been resolved satisfactorily.

To be clear, ibc-rs's application implementations, ics20 and ics721, are safe from this vulnerability, since those implementations always return ModuleExtras::Empty() upon failure in the packet_recv flow. The main issue to be addressed here is more to do with limiting the impact on downstream implementors.

Details

From the security report we received, upon a failed acknowledgment receipt, ibc-rs will still emit events unconditionally, potentially for transfers / deposits that failed where no events should be emitted at all.

@rnbguy's analysis points to this line as being the possible main culprit:

    Err((extras, error)) => (extras, AcknowledgmentStatus::error(error.into())),

where upon an error being encountered in the logic for handling the execution of transfers, this function could still potentially emit events in the form of extras when it shouldn't.

Per Rano's suggestion, this line should be changed to

    Err((extras, error)) => (ModuleExtras::Empty(), AcknowledgmentStatus::error(error.into())),

to ensure that no extraneous events are emitted when an AcknowledgmentStatus::error is encountered.

Once this issue has been addressed in the ibc-rs codebase, the projects that have thus far integrated ibc-rs should be contacted about the possible impact towards them.

Version

ibc-rs v0.54.0 and older

[rnbguy]

It appears that this issue is not resolved yet in ibc-go. They had to revert the patch from the jump blog via cosmos/ibc-go#5541 because the ICA application module requires the error events.

As an immediate fix, ibc-go emits the module error events with attribute keys prefixed with ibccallbackerror-.

Maybe we can follow a similar pattern and add a separate event here,

https://github.com/cosmos/ibc-rs/blob/60ff9bd16d43d3475ba443069868959f0fdc5bd6/ibc-core/ics25-handler/types/src/events.rs#L91

ModuleError(ModuleEvent)

to handle/emit successful and failed module events differently.

We also confirmed our ics20 and ics721 implementations return empty module events at recv_packet error - so they are completely safe from this bug.

Since the issue is not fully addressed at IBC protocol level and needs further exploration, we will close this for now and open in the future if needed.