IBC packet non-timeout with invalid RevisionNumber

[lapidix]

Summary of Bug

When sending an IBC transaction, setting the RevisionNumber in the --packet-timeout-height flag to a value higher than the actual chain’s RevisionNumber can result in a transaction that never times out.

Allowing a high RevisionNumber in test code appears be intentional; however, possible to generate transactions without timeout feature, as demonstrated in the following link, appears abnormal and could potentially be a vulnerability: https://www.mintscan.io/milkyway/tx/D2D58B250EA83999D477E9B5E732355D0074FC6FCCE87B88945B7011877CCD93?sector=logs

I’m aware that the packet-timeout-height was removed in IBC v2. However, to my knowledge, most chains are currently running with ibc-go v8.x.x, which is based on IBC v1 and still includes the issue described above.

Additionally, I know that recent versions no longer support relative timeouts using block height at the CLI level, relying instead on timestamp-based timeouts. However, I believe it would be more robust to strengthen the validation logic at the protocol or state machine level as well.

Expected Behavior

Timeout prioritizes height over timestamp as shown in the code below:

func (t Timeout) Elapsed(height clienttypes.Height, timestamp uint64) bool {
	return t.heightElapsed(height) || t.timestampElapsed(timestamp)
}

Additionally, in the Compare() function that validates Height, it unconditionally passes as long as the revision number is greater than the counterparty's revision number.

func (h Height) Compare(other exported.Height) int64 {
	height, ok := other.(Height)
	if !ok {
		panic(fmt.Errorf("cannot compare against invalid height type: %T. expected height type: %T", other, h))
	}
	var a, b big.Int
	if h.RevisionNumber != height.RevisionNumber {
		a.SetUint64(h.RevisionNumber)
		b.SetUint64(height.RevisionNumber)
	} else {
		a.SetUint64(h.RevisionHeight)
		b.SetUint64(height.RevisionHeight)
	}
	return int64(a.Cmp(&b))
}

Through this, packet timeout block heights like 9000-123456 are also passing validation checks, and when the timeout timestamp is 0, it's possible to create transactions that never timeout.

Version

All versions including main

Steps to Reproduce

I have tested this using a simple Docker-based PoC environment below, and this is actually usable on chains currently running ibc-go v8.x.x through the CLI commands below.

• https://github.com/lapidix/ibc-timeout-bug-poc

osmosisd tx ibc-transfer transfer [src-port] [src-channel] [receiver] [amount] [chain-id] --packet-timeout-height 9000-9100 --packet-timeout-timestamp 0 

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged/assigned
• Estimate provided

[linear]

IBCGO-2381 IBC packet non-timeout with invalid RevisionNumber

[lapidix]

Potential Fix Approaches

As far as I understand, since the client's revision number is typically modified within the UpdateClient() function, this issue could be resolved through one of the following approaches:

Approach 1: Pre-timeout Validation in SendPacket()

Add validation checks before timeout creation in /modules/core/04-channel/keeper/packet.go's SendPacket() function:

func (k *Keeper) SendPacket(
	ctx sdk.Context,
	sourcePort string,
	sourceChannel string,
	timeoutHeight clienttypes.Height,
	timeoutTimestamp uint64,
	data []byte,
) (uint64, error) {
	// ...
	
	latestHeight := k.clientKeeper.GetClientLatestHeight(ctx, connectionEnd.ClientId)
	if latestHeight.IsZero() {
		return 0, errorsmod.Wrapf(clienttypes.ErrInvalidHeight, "cannot send packet using client (%s) with zero height", connectionEnd.ClientId)
	}

	latestTimestamp, err := k.clientKeeper.GetClientTimestampAtHeight(ctx, connectionEnd.ClientId, latestHeight)
	if err != nil {
		return 0, err
	}
	
	// [Added] Validate that the timeout height's revision number matches the destination chain's current revision number
	if latestHeight.RevisionNumber != packet.GetTimeoutHeight().(clienttypes.Height).RevisionNumber {
		return 0, errorsmod.Wrapf(clienttypes.ErrInvalidHeight, "invalid packet revision number")
	}
	
	// check if packet is timed out on the receiving chain
	timeout := types.NewTimeout(packet.GetTimeoutHeight().(clienttypes.Height), packet.GetTimeoutTimestamp())
	if timeout.Elapsed(latestHeight, latestTimestamp) {
		return 0, errorsmod.Wrap(timeout.ErrTimeoutElapsed(latestHeight, latestTimestamp), "invalid packet timeout")
	}
	
	// ... 
}

Approach 2: Enhanced Validation in heightElapsed()

Alternatively, we could add validation logic to the heightElapsed() function in modules/core/04-channel/types/timeout.go:

// heightElapsed returns true if the timeout height is non empty
// and either the revision numbers don't match or the timeout height is greater than or equal to the relative height.
func (t Timeout) heightElapsed(height clienttypes.Height) bool {
	return !t.Height.IsZero() && (t.Height.RevisionNumber != height.RevisionNumber || height.GTE(t.Height))
}

Both approaches would prevent the creation of packets with invalid timeout heights like 9000-123456 and ensure proper timeout behavior.