feat: Prevent panic from nil pointer dereference in swap IBC handler #308
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This PR addresses a runtime panic (
invalid memory address or nil pointer dereference) that occurs during the handling of IBC packet acknowledgements and timeouts in thex/swapmodule.Problem
The panic was triggered by unsafe type assertions and method calls on potentially
nilfields within the IBC handler logic. The stack trace pointed toShouldDeleteCompletedWaitingPacket, but further investigation revealed similar potential issues inOnAcknowledgementOutgoingInFlightPacketandOnTimeoutOutgoingInFlightPacket.The key issues were:
packet.Change.(*types.IncomingInFlightPacket_AckChange)could panic if theoneoffield was not of the expected type, even if it wasn'tnil.t.OutgoingIndexChange.Equal(...)would panic ift.OutgoingIndexChangewasnil.switchstatements did not explicitly handlenilcases foroneoffields, which could lead to unexpected behavior if the field was not set.Solution
This PR introduces several changes to make the IBC handling logic more robust and prevent panics:
ShouldDeleteCompletedWaitingPacketwith theif ack, ok := ...pattern to safely check the type and value ofpacket.Changeandpacket.Forwardbefore accessing their fields.nilchecks inOnAcknowledgementOutgoingInFlightPacketandOnTimeoutOutgoingInFlightPacketbefore calling the.Equal()method onOutgoingIndexChangeandOutgoingIndexForwardfields.switchstatements inShouldDeleteCompletedWaitingPacketto explicitly handle thenilcase forpacket.Changeandpacket.Forward, treating it as a completed state similar toAckChange/AckForward.These changes ensure that the packet handling logic is resilient to unexpected states and prevents the node from crashing, improving the overall stability of the swap module.