IRIS Network security record

Files: - `nft-transfer/types/trace.go` - `nft-transfer/keeper/relay.go` Commits: - [`cfc058309adcf73478d3828a524ecf8e546420ed`](https://github.com/bianjieai/nft-transfer/tree/cfc058309adcf73478d3828a524ecf8e546420ed) - [`87cdf3b7dd969237c9ffc8e056bfc24cab5e2bba`](https://github.com/bianjieai/nft-transfer/tree/87cdf3b7dd969237c9ffc8e056bfc24cab5e2bba) - [`fa7bd3f3cb1ebe24f781953a781add96509b4c2a`](https://github.com/bianjieai/nft-transfer/tree/fa7bd3f3cb1ebe24f781953a781add96509b4c2a) The voucher class ID is determined by the trace path and the base class id, which could allow malicious users to issue the voucher class ID and mint the voucher NFT beforehand to steal the NFT on source chain via the interchain NFT transfer. The `fullClassPath` passed in the `packetData` is the base class ID if the NFT is transferred out from the source (native) chain. Once the NFT is transferred out from source chain, the NFT will be sent to the escrow account and the packet will be relayed. **nft-transfer/keeper/relay.go** ```go=227 packetData := types.NewNonFungibleTokenPacketData( fullClassPath, class.GetURI(), class.GetData(), tokenIDs, tokenURIs, sender.String(), receiver, tokenData, memo, ) ``` - Suppose the base class ID is known by malicious users from the log or the metrics on source chain. If an NFT of the same class ID has been transferred to target chain before and the full class path has been stored, the malicious user could use the `ibc/hash(full class path)` and token ID to mint the voucher NFT because the full class path will be fetched in the following code: ```go=227 if strings.HasPrefix(classID, "ibc/") { fullClassPath, err = k.ClassPathFromHash(ctx, classID) if err != nil { return types.NonFungibleTokenPacketData{}, err } } ``` - On the other hand, the malicious users on the target chain could also issue the class ID prefixed with the target chain's port ID and channel ID and mint the voucher NFT with the same token ID. Immediately, the malicious users send back the newly minted voucher NFT to the source chain. During the handling of the packet data with the function `processReceivedPacket()`, it enters the following branch: ```go=337 unprefixedClassID, err := types.RemoveClassPrefix(packet.GetSourcePort(), packet.GetSourceChannel(), data.ClassId) if err != nil { return err } voucherClassID := types.ParseClassTrace(unprefixedClassID).IBCClassID() escrowAddress := types.GetEscrowAddress(packet.GetDestPort(), packet.GetDestChannel()) for i, tokenID := range data.TokenIds { //NOTE: It must be verified here whether the nft is escrowed by the <destPort, destChannel> account //FIX https://github.com/game-of-nfts/gon-evidence/issues/346 owner := k.nftKeeper.GetOwner(ctx, voucherClassID, tokenID) if !escrowAddress.Equals(owner) { return errorsmod.Wrap(sdkerrors.ErrUnauthorized, "not token owner") } if err := k.nftKeeper.Transfer(ctx, voucherClassID, tokenID, types.GetIfExist(i, data.TokenData), receiver); err != nil { return err } } ``` After removal of the port ID and channel ID, the full class trace of the "fake" NFT is empty and it creates the original base class ID (i.e., the `voucherClassID` in line 342. As a result, the malicious users could transfer out the native NFT from the escrow account to any receiver.

Files: - `nft-transfer/types/trace.go` - `nft-transfer/keeper/relay.go` Commits: - [`cfc058309adcf73478d3828a524ecf8e546420ed`](https://github.com/bianjieai/nft-transfer/tree/cfc058309adcf73478d3828a524ecf8e546420ed) - [`87cdf3b7dd969237c9ffc8e056bfc24cab5e2bba`](https://github.com/bianjieai/nft-transfer/tree/87cdf3b7dd969237c9ffc8e056bfc24cab5e2bba) - [`fa7bd3f3cb1ebe24f781953a781add96509b4c2a`](https://github.com/bianjieai/nft-transfer/tree/fa7bd3f3cb1ebe24f781953a781add96509b4c2a) The voucher class ID is determined by the class trace and the base class id, which could lead to the class ID collision. Moreover, malicious users is able to issue the voucher class ID and mint the voucher NFT beforehand to prevent the interchain NFT transfer. The voucher class ID is generated with function `IBCClassID()`: **nft-transfer/types/trace.go** ```go=108 func (ct ClassTrace) IBCClassID() string { if ct.Path != "" { return fmt.Sprintf("%s/%s", ClassPrefix, ct.Hash()) } return ct.BaseClassId } ``` which is the concatenation of the “ibc" + "/" + hash of the full class path if the class path is nonempty; Otherwise, it is the base class ID. This means that the IBC class ID can be determined if the full class path is known. In this case, if malicious users create this known voucher class ID and mint the voucher NFT with same token ID before the interchain NFT transfer to the chain. Then this transfer will fail because the mint of the voucher NFT will fail as the same class ID and token ID exists on the target chain. ```go func (k Keeper) Mint(ctx sdk.Context, token nft.NFT, receiver sdk.AccAddress) error { if !k.HasClass(ctx, token.ClassId) { return sdkerrors.Wrap(nft.ErrClassNotExists, token.ClassId) } if k.HasNFT(ctx, token.ClassId, token.Id) { return sdkerrors.Wrap(nft.ErrNFTExists, token.Id) } k.mintWithNoCheck(ctx, token, receiver) return nil } ``` As a result, if some malicious users create the voucherClassID and mint the NFT beforehand, then the receiver will not be able to receive the voucher NFT and the interchain NFT transfer fails.

Commits: - [`cfc058309adcf73478d3828a524ecf8e546420ed`](https://github.com/bianjieai/nft-transfer/tree/cfc058309adcf73478d3828a524ecf8e546420ed) - [`87cdf3b7dd969237c9ffc8e056bfc24cab5e2bba`](https://github.com/bianjieai/nft-transfer/tree/87cdf3b7dd969237c9ffc8e056bfc24cab5e2bba) - [`fa7bd3f3cb1ebe24f781953a781add96509b4c2a`](https://github.com/bianjieai/nft-transfer/tree/fa7bd3f3cb1ebe24f781953a781add96509b4c2a) In the codebase, the deprecated package `sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"` is used in many places. The functionality of the package `sdkerrors` has been moved to its own module: `cosmossdk.io/errors`.

Files: - `nft-transfer/keeper/keeper.go` - `nft-transfer/keeper/genesis.go` Commits: - [`cfc058309adcf73478d3828a524ecf8e546420ed`](https://github.com/bianjieai/nft-transfer/tree/cfc058309adcf73478d3828a524ecf8e546420ed) - [`87cdf3b7dd969237c9ffc8e056bfc24cab5e2bba`](https://github.com/bianjieai/nft-transfer/tree/87cdf3b7dd969237c9ffc8e056bfc24cab5e2bba) - [`fa7bd3f3cb1ebe24f781953a781add96509b4c2a`](https://github.com/bianjieai/nft-transfer/tree/fa7bd3f3cb1ebe24f781953a781add96509b4c2a) The escrow account is used to lock and release the NFT during the interchain NFT transfer. The current implementation does not store any information in the NFT transfer module and init/export the corresponding information related to the escrow account.

Files: - `nft-transfer/types/packet.go` - `nft-transfer/client/cli/tx.go` Commits: - [`cfc058309adcf73478d3828a524ecf8e546420ed`](https://github.com/bianjieai/nft-transfer/tree/cfc058309adcf73478d3828a524ecf8e546420ed) - [`87cdf3b7dd969237c9ffc8e056bfc24cab5e2bba`](https://github.com/bianjieai/nft-transfer/tree/87cdf3b7dd969237c9ffc8e056bfc24cab5e2bba) - [`fa7bd3f3cb1ebe24f781953a781add96509b4c2a`](https://github.com/bianjieai/nft-transfer/tree/fa7bd3f3cb1ebe24f781953a781add96509b4c2a) The default `DefaultRelativePacketTimeoutHeight = "0-0"` is specified via the NFT transfer CLI. According to the IBC documentation, the `timeoutHeight` is used as a timeout mechanism for a packet that has not been processed on the destination chain. > Since IBC works over a distributed network and relies on potentially faulty relayers to relay messages between ledgers, IBC must handle the case where a packet does not get sent to its destination in a timely manner or at all. Packets must specify a non-zero value for timeout height (TimeoutHeight) or timeout timestamp (TimeoutTimestamp ) after which a packet can no longer be successfully received on the destination chain. > The timeoutHeight indicates a consensus height on the destination chain after which the packet is no longer be processed, and instead counts as having timed-out. > The timeoutTimestamp indicates a timestamp on the destination chain after which the packet is no longer be processed, and instead counts as having timed-out. The `timeoutHeight` needs to be nonzero, so a nonzero default timeout height seems to be more reasonable. **Reference**: - https://github.com/cosmos/ibc-go/blob/main/docs/docs/01-ibc/01-overview.md

Files: - `types/packet.go` Commits: - [`cfc058309adcf73478d3828a524ecf8e546420ed`](https://github.com/bianjieai/nft-transfer/tree/cfc058309adcf73478d3828a524ecf8e546420ed) - [`87cdf3b7dd969237c9ffc8e056bfc24cab5e2bba`](https://github.com/bianjieai/nft-transfer/tree/87cdf3b7dd969237c9ffc8e056bfc24cab5e2bba) - [`fa7bd3f3cb1ebe24f781953a781add96509b4c2a`](https://github.com/bianjieai/nft-transfer/tree/fa7bd3f3cb1ebe24f781953a781add96509b4c2a) The `TokenData` was appended with the data from the upper level NFT modules in the `NonFungibleTokenPacketData` in function `createOutgoingPacket`. ```go=248 tokenData = append(tokenData, nft.GetData()) ``` After that, a validation is performed to check these data including the tokenData in method `ValidateBasic` of the `NonFungibleTokenPacketData`. ```go=48 func (nftpd NonFungibleTokenPacketData) ValidateBasic() error { ... if (len(nftpd.TokenData) != 0) && (len(nftpd.TokenIds) != len(nftpd.TokenData)) { return sdkerrors.Wrap(ErrInvalidPacket, "the length of tokenData must be 0 or the same as the length of TokenIds") } ... ``` This implementation follows the specification of [`ics-721-nft-transfer`](https://github.com/cosmos/ibc/blob/main/spec/app/ics-721-nft-transfer/README.md#data-structures) on data structure of `tokenData`. However, there is some mismatch on the application level design of [`ADR-043`](https://github.com/cosmos/cosmos-sdk/blob/main/docs/architecture/adr-043-nft-module.md#nft). The data could be optional which means that it's not required by the upper level NFT modules. Then there could be mismatch with the size of token IDs and the token data, leading to failure when validation on the `NonFungibleTokenPacketData`. The packets could not be routed to external chains as designed, resulting in nft transfer failure for all the tokens under the same class id. **Reference**: - https://github.com/cosmos/cosmos-sdk/blob/main/docs/architecture/adr-043-nft-module.md#nft

Files: - `nft-transfer/types/trace.go` Commits: - [`cfc058309adcf73478d3828a524ecf8e546420ed`](https://github.com/bianjieai/nft-transfer/tree/cfc058309adcf73478d3828a524ecf8e546420ed) - [`87cdf3b7dd969237c9ffc8e056bfc24cab5e2bba`](https://github.com/bianjieai/nft-transfer/tree/87cdf3b7dd969237c9ffc8e056bfc24cab5e2bba) - [`fa7bd3f3cb1ebe24f781953a781add96509b4c2a`](https://github.com/bianjieai/nft-transfer/tree/fa7bd3f3cb1ebe24f781953a781add96509b4c2a) The function `validateTraceIdentifiers()` is intended to validate the identifiers (i.e., port ids and channel ids). ```go=141 if err := host.ChannelIdentifierValidator(identifiers[i+1]); err != nil { return errorsmod.Wrapf(err, "invalid channel ID at position %d", i) } ``` When an error is returned from `host.ChannelIdentifierValidator()`, the error message does not specify the correct index in the error message, `return sdkerrors.Wrapf(err, "invalid channel ID at position %d", i)`, which is supposed to be `i+1`.

Files: - `keeper/relay.go` Commits: - [`cfc058309adcf73478d3828a524ecf8e546420ed`](https://github.com/bianjieai/nft-transfer/tree/cfc058309adcf73478d3828a524ecf8e546420ed) - [`fa7bd3f3cb1ebe24f781953a781add96509b4c2a`](https://github.com/bianjieai/nft-transfer/tree/fa7bd3f3cb1ebe24f781953a781add96509b4c2a) Function `createOutgoingPacket()` is implemented to relay packet data as designed in specification: [`ics721-nft-transfer`](https://github.com/cosmos/ibc/blob/main/spec/app/ics-721-nft-transfer/README.md#packet-relay). However, the implementation only creates `NonFungibleTokenPacketData` without sending in the code snippet, which is inconsistent with the design of the specification. As a result, the below inputs: - destinationPort - destinationChannel - timeoutHeight - timeoutTimestamp are not used at all. ```go=199 func (k Keeper) createOutgoingPacket(ctx sdk.Context, sourcePort, sourceChannel, destinationPort, destinationChannel, classID string, tokenIDs []string, sender sdk.AccAddress, receiver string, timeoutHeight clienttypes.Height, timeoutTimestamp uint64, memo string, ) (types.NonFungibleTokenPacketData, error) { ``` Furthermore, the input `sender` defined in the spec is in format of string. However, in the implementation, the format of input `sender` is type of `AccAddress`. **Reference**: - https://github.com/cosmos/ibc/blob/main/spec/app/ics-721-nft-transfer/README.md#packet-relay

Files: - `types/msgs.go` - `types/packet.go` Commits: - [`cfc058309adcf73478d3828a524ecf8e546420ed`](https://github.com/bianjieai/nft-transfer/tree/cfc058309adcf73478d3828a524ecf8e546420ed) - [`87cdf3b7dd969237c9ffc8e056bfc24cab5e2bba`](https://github.com/bianjieai/nft-transfer/tree/87cdf3b7dd969237c9ffc8e056bfc24cab5e2bba) - [`fa7bd3f3cb1ebe24f781953a781add96509b4c2a`](https://github.com/bianjieai/nft-transfer/tree/fa7bd3f3cb1ebe24f781953a781add96509b4c2a) The classID in the message `MsgTransfer` and packet data `NonFungibleTokenPacketData` has specific format that is supposed to be validated instead of only checking it's nonempty. **types/msgs.go** ```go func (msg MsgTransfer) ValidateBasic() error { if err := host.PortIdentifierValidator(msg.SourcePort); err != nil { return sdkerrors.Wrap(err, "invalid source port ID") } if err := host.ChannelIdentifierValidator(msg.SourceChannel); err != nil { return sdkerrors.Wrap(err, "invalid source channel ID") } if strings.TrimSpace(msg.ClassId) == "" { return sdkerrors.Wrap(ErrInvalidClassID, "classId cannot be blank") } ... ``` **types/packet.go** ```go func (nftpd NonFungibleTokenPacketData) ValidateBasic() error { if strings.TrimSpace(nftpd.ClassId) == "" { return sdkerrors.Wrap(ErrInvalidClassID, "classId cannot be blank") } ... ```

Files: - `nft-transfer/keeper/keeper.go` Commits: - [`cfc058309adcf73478d3828a524ecf8e546420ed`](https://github.com/bianjieai/nft-transfer/tree/cfc058309adcf73478d3828a524ecf8e546420ed) - [`87cdf3b7dd969237c9ffc8e056bfc24cab5e2bba`](https://github.com/bianjieai/nft-transfer/tree/87cdf3b7dd969237c9ffc8e056bfc24cab5e2bba) - [`fa7bd3f3cb1ebe24f781953a781add96509b4c2a`](https://github.com/bianjieai/nft-transfer/tree/fa7bd3f3cb1ebe24f781953a781add96509b4c2a) The function `NewKeeper()` is used to create the keeper of the NFT transfer module: ```go=36 func NewKeeper( cdc codec.Codec, key storetypes.StoreKey, authority string, ics4Wrapper porttypes.ICS4Wrapper, channelKeeper types.ChannelKeeper, portKeeper types.PortKeeper, authKeeper types.AccountKeeper, nftKeeper types.NFTKeeper, scopedKeeper capabilitykeeper.ScopedKeeper, ) Keeper { return Keeper{ storeKey: key, cdc: cdc, authority: authority, ics4Wrapper: ics4Wrapper, channelKeeper: channelKeeper, portKeeper: portKeeper, nftKeeper: nftKeeper, authKeeper: authKeeper, scopedKeeper: scopedKeeper, } } ``` However, it misses the validation of the NFT transfer module account and the authority account.