tmbytes.HexBytes is not changed to []bytes cause incorrect interface GetDenomTrace implementation

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-05-canto/blob/d1d51b2293d4689f467b8b1c82bba84f8f7ea008/canto-main/x/onboarding/types/interfaces.go#L70

Vulnerability details

Impact

tmbytes.HexBytes is not changed to []bytes cause incorrect interface implementation

Proof of Concept

According to https://github.com/cosmos/cosmos-sdk/blob/v0.50.6/UPGRADING.md

The usage of github.com/cometbft/cometbft/libs/bytes.HexByte has been replaced by []byte.

but in the current code,

In 2024-05-canto/canto-main/x/onboarding/types/interfaces.go

// TransferKeeper defines the expected IBC transfer keeper.
type TransferKeeper interface {
    GetDenomTrace(ctx sdk.Context, denomTraceHash tmbytes.HexBytes) (transfertypes.DenomTrace, bool)
}

the link above explains how the donomTrace is used extensively in the codebase.

https://github.com/cosmos/ibc-go/blob/main/docs/architecture/adr-001-coin-source-tracing.md#xibc-transfer-changes

// GetDenomTrace retrieves the full identifiers trace and base denomination from the store.
func (k Keeper) GetDenomTrace(ctx Context, denomTraceHash []byte) (DenomTrace, bool) {
  store := ctx.KVStore(k.storeKey)
  bz := store.Get(types.KeyDenomTrace(traceHash))
  if bz == nil {
    return &DenomTrace, false
  }

  var denomTrace DenomTrace
  k.cdc.MustUnmarshalBinaryBare(bz, &denomTrace)
  return denomTrace, true
}

// HasDenomTrace checks if a the key with the given trace hash exists on the store.
func (k Keeper) HasDenomTrace(ctx Context, denomTraceHash []byte)  bool {
  store := ctx.KVStore(k.storeKey)
  return store.Has(types.KeyTrace(denomTraceHash))
}

// SetDenomTrace sets a new {trace hash -> trace} pair to the store.
func (k Keeper) SetDenomTrace(ctx Context, denomTrace DenomTrace) {
  store := ctx.KVStore(k.storeKey)
  bz := k.cdc.MustMarshalBinaryBare(&denomTrace)
  store.Set(types.KeyTrace(denomTrace.Hash()), bz)
}

and then

// SendTransfer
// ...

  fullDenomPath := token.Denom

// deconstruct the token denomination into the denomination trace info
// to determine if the sender is the source chain
if strings.HasPrefix(token.Denom, "ibc/") {
@  fullDenomPath, err = k.DenomPathFromHash(ctx, token.Denom)
  if err != nil {
    return err
  }
}

if types.SenderChainIsSource(sourcePort, sourceChannel, fullDenomPath) {
//...

and

// DenomPathFromHash returns the full denomination path prefix from an ibc denom with a hash
// component.
func (k Keeper) DenomPathFromHash(ctx sdk.Context, denom string) (string, error) {
  hexHash := denom[4:]
  hash, err := ParseHexHash(hexHash)
  if err != nil {
    return "", Wrap(ErrInvalidDenomForTransfer, err.Error())
  }

@  denomTrace, found := k.GetDenomTrace(ctx, hash)
  if !found {
    return "", Wrap(ErrTraceNotFound, hexHash)
  }

  fullDenomPath := denomTrace.GetFullDenomPath()
  return fullDenomPath, nil
}

the GetDenomTrace needs correct interface to not block code execution.

Tools Used

Manual Review

Recommended Mitigation Steps

change to

type TransferKeeper interface {
    GetDenomTrace(ctx sdk.Context, denomTraceHash []byte) (transfertypes.DenomTrace, bool)
}

Assessed type

Other

[poorphd]

• Labels
  • sponsor dispute
• reasoning
  • It is true that bytes.HexByte has been changed to []byte in the upgrade guide of SDK v0.50.x.
  • However, the current version of ibc-go used by Canto is [v8.2.1](https://github.com/cosmos/ibc-go/blob/v8.2.1/modules/apps/transfer/keeper/keeper.go#L143-L152), and the transfer keeper method implemented in this version is still using bytes.HexByte.
  • The changes in the interface raised in the Issue have only appeared in the document so far, and the latest release, ibc-go [v8.3.2](https://github.com/cosmos/ibc-go/blob/v8.3.2/modules/apps/transfer/keeper/keeper.go#L143-L152), is still using bytes.HexByte.
• Severity change required
  • Mid → not valid
  • The raised issue is not related to the current version bump of Cosmos SDK and ibc-go.
• Patch
  • N/A

[3docSec]

The quote from the upgrade guide is about changes in the Cosmos SDK.

The "transfer" module comes instead from the IBC, and while it's likely it will change to use []bytes, it didn't yet, so Canto has no other choice than supporting the signature with HexBytes.

[c4-judge]

3docSec marked the issue as unsatisfactory: Invalid