Packet validation diverges from spec

[hu55a1n1]

It seems like ibc-go disallows packets with a zero timeout height and zero timeout timestamp in ibc-go -> https://github.com/cosmos/ibc-go/blob/48a6ae512b4ea42c29fdf6c6f5363f50645591a2/modules/core/04-channel/types/packet.go#L106-L108

But the ICS004 spec: sendPacket doesn’t require this ->

// check that the timeout height hasn't already passed in the local client tracking the receiving chain
latestClientHeight = provableStore.get(clientPath(connection.clientIdentifier)).latestClientHeight()
abortTransactionUnless(packet.timeoutHeight === 0 || latestClientHeight < packet.timeoutHeight)

Does the spec need to be updated in this case?

Furthermore, the packet.ValidateBasic() function that has this check is only called in the SendPacket handler and not in the ReceivePacket handler. So IIUC, if a packet with a zero timeout height & timestamp is sent from a chain running a different implementation of IBC, then the packet will pass the commitment verification in ibc-go. Is this the expected behaviour?

[crodriguezvega]

Does the spec need to be updated in this case?

@AdityaSripal do you think a check for the timeout timestamp should be added in the spec? In the implementation we do check that, if the timeout timestamp is not zero, then it should be greater than the timestamp of the consensus state stored at the latest height. The spec does specify this check in the recvPacket function:

abortTransactionUnless(packet.timeoutHeight === 0 || getConsensusHeight() < packet.timeoutHeight)
abortTransactionUnless(packet.timeoutTimestamp === 0 || currentTimestamp() < packet.timeoutTimestamp)

Furthermore, the packet.ValidateBasic() function that has this check is only called in the SendPacket handler and not in the ReceivePacket handler. So IIUC, if a packet with a zero timeout height & timestamp is sent from a chain running a different implementation of IBC, then the packet will pass the commitment verification in ibc-go. Is this the expected behaviour?

hmmm, not sure if this is expected, or a bug both on the spec and the implementation, because according to the checks above in recvPacket the spec would allow for the transaction not be aborted if both packet.timeoutHeight and packet.timeoutTimestamp are 0... @AdityaSripal @colin-axner what do you think?

[AdityaSripal]

Thanks for the catch! @hu55a1n1

Yes I think we should update the spec to add the check on the sending side. Disabling timeouts for a packet is certainly something we could consider, but we probably don't want to just use the default nil values to signal this since it is then possible to accidentally disable timeouts on your packet.

So 0 on both timeout values should abort the transaction on the sending side.

It is a more interesting question of what to do on the receiving side. If we reject a packet that has no timeout, then we effectively make it impossible for the packet to ever be processed since it cannot be received on the destination chain and it can also never be timed out on the source chain. So for example, the funds escrowed by a transfer packet will be stuck forever.

We want to make sure that the packet lifecycle will resolve either by getting received or timing out. So the receiving chain should still accept and process a packet which has no timeout. The sending chain should have a safety check to prevent this.

TL;DR:

• Make the change on the sending side
• Do not make the change on the receiving side.

Would you be interested in making a PR @hu55a1n1 ?

[crodriguezvega]

Thanks for the explanation, @AdityaSripal! Then I believe we can close this issue, since there are no changes needed in ibc-go, right?

And as Aditya suggested, you're welcome to open a PR in the spec repo, @hu55a1n1. Otherwise, please open an issue there and we will take care of it. Thanks!

[AdityaSripal]

I think we can just transfer the issue to the spec repo

[hu55a1n1]

Thank you for the detailed clarification @crodriguezvega and @AdityaSripal! :+1: I like the idea of transferring the issue to the spec repo. I'll open a PR to address this on the spec repo.

[crodriguezvega]

Hi @hu55a1n1. Just wondering if you had any time to work on this change. Feel free to delegate to us if you're too busy with other things!

[hu55a1n1]

Sorry for the delay in getting to this. Just submitted PR #847 to resolve this.