Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Users can directly transfer hashed IBC denoms using MsgSendPacket #7848

Open
4 tasks
srdtrk opened this issue Jan 15, 2025 · 2 comments
Open
4 tasks

Users can directly transfer hashed IBC denoms using MsgSendPacket #7848

srdtrk opened this issue Jan 15, 2025 · 2 comments
Labels
20-transfer feat: ibc-eureka type: bug Something isn't working as expected

Comments

@srdtrk
Copy link
Member

srdtrk commented Jan 15, 2025

Summary of Bug

When users are allowed to construct their own ICS20 packets through MsgSendPacket, they are allowed to send (hashed) IBC denoms directly without unwrapping the trace path that would have happened if they were using the standard MsgTransfer route.

This allows users to construct two representations for the same escrowed token using the same channel. This might have broader impacts then I'm anticipating at the moment.

Expected Behaviour

Hashed IBC denoms should never be sent directly in an ICS20 packet. Because of the bug, the resolved denom in the final chain looks like:

transfer/channel-1/ibc/A3DC0033289F515D80F1E63EC511759A4CD416F6E4BFDFB60D9460D54D04CF88

instead, it should always look like:

transfer/channel-1/transfer/channel-0/basedenom

Version

feat/ibc-eureka

Steps to Reproduce

I've ran into this issue while writing end to end tests for cosmos/solidity-ibc-eureka#59.

		denomOnSimdA := transfertypes.NewDenom(s.contractAddresses.Erc20, transfertypes.NewHop(transfertypes.PortID, ibctesting.FirstChannelID))
		timeout := uint64(time.Now().Add(30 * time.Minute).Unix())

		transferPayload := ics20lib.ICS20LibFungibleTokenPacketData{
			Denom:    denomOnSimdA.IBCDenom(), // BUG: Allowing user to choose this is a bug
			Amount:   transferAmount,
			Sender:   simdAUser.FormattedAddress(),
			Receiver: simdBUser.FormattedAddress(),
			Memo:     "",
		}
		transferBz, err := ics20lib.EncodeFungibleTokenPacketData(transferPayload)
		s.Require().NoError(err)

		payload := channeltypesv2.Payload{
			SourcePort:      transfertypes.PortID,
			DestinationPort: transfertypes.PortID,
			Version:         transfertypes.V1,
			Encoding:        transfertypes.EncodingABI,
			Value:           transferBz,
		}

		resp, err := s.BroadcastMessages(ctx, simdA, simdAUser, 2_000_000, &channeltypesv2.MsgSendPacket{
			SourceChannel:    ibctesting.SecondChannelID,
			TimeoutTimestamp: timeout,
			Payloads: []channeltypesv2.Payload{
				payload,
			},
			Signer: simdAUser.FormattedAddress(),
		})
		s.Require().NoError(err)
		s.Require().NotEmpty(resp.TxHash)

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
  • Estimate provided
@srdtrk srdtrk added 20-transfer feat: ibc-eureka type: bug Something isn't working as expected labels Jan 15, 2025
@srdtrk srdtrk changed the title Users can send hashed IBC denoms using MsgSendPacket Users can directly transfer hashed IBC denoms using MsgSendPacket Jan 15, 2025
@NisTun
Copy link
Contributor

NisTun commented Jan 17, 2025

I see we have func (k Keeper) tokenFromCoin here, so the denom should be not in type .../ibc/hash... right?

@AdityaSripal
Copy link
Member

Hmm, can we just disable this by checking if the token is an IBC denom first before allowing it? Or we can convert it back to the full denom. Nice catch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
20-transfer feat: ibc-eureka type: bug Something isn't working as expected
Projects
Status: Backlog
Development

No branches or pull requests

3 participants