Better error handling

[code423n4]

Lines of code

https://github.com/code-423n4/2023-06-canto/blob/a4ff2fd2e67e77e36528fad99f9d88149a5e8532/Canto/x/onboarding/keeper/ibc_callbacks.go#L63 https://github.com/code-423n4/2023-06-canto/blob/a4ff2fd2e67e77e36528fad99f9d88149a5e8532/Canto/x/onboarding/keeper/ibc_callbacks.go#L81

Vulnerability details

Impact

Insufficient explicit error handling during the account retrieval process can lead to uncaught or mismanaged errors.

Proof of Concept

The existing implementation fails to explicitly handle the potential error during account retrieval. The following code section depicts this flaw:

account := k.accountKeeper.GetAccount(ctx, recipient)

    // onboarding is not supported for module accounts
    if _, isModuleAccount := account.(authtypes.ModuleAccountI); isModuleAccount {
        return ack
    }

In the above segment, the GetAccount function call might fail due to various reasons, like issues in context or recipient, but the result is not checked for a possible nil value, which could lead to a panic in the subsequent type assertion step.

Later in the code, another scenario unfolds where error handling is needed. The section unmarshals JSON data from a packet and directly handles the error inline. However, it presumes that an error "shouldn't happen" and does not explicate the error cause or conditions:

    var data transfertypes.FungibleTokenPacketData
    if err = transfertypes.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil {
        // NOTE: shouldn't happen as the packet has already
        // been decoded on ICS20 transfer logic
        err = errorsmod.Wrapf(types.ErrInvalidType, "cannot unmarshal ICS-20 transfer packet data")
        return channeltypes.NewErrorAcknowledgement(err.Error())
    }

Tools Used

Manual Code Review

Recommended Mitigation Steps

Implementing more robust error handling during account retrieval will prevent unexpected behavior from taking place. Additionally, expanding the error handling around packet data unmarshaling will provide more insight into potential issues.

The following demonstrates an example of how error handling might be improved:

account, err := k.accountKeeper.GetAccount(ctx, recipient)
if err != nil {
    // handle the error appropriately
    return nil, err
}

// onboarding is not supported for module accounts
if _, isModuleAccount := account.(authtypes.ModuleAccountI); isModuleAccount {
    return ack
}

// ...

var data transfertypes.FungibleTokenPacketData
err = transfertypes.ModuleCdc.UnmarshalJSON(packet.GetData(), &data)
if err != nil {
    // handle the error appropriately, don't presume it won't happen
    return nil, errorsmod.Wrapf(types.ErrInvalidType, "cannot unmarshal ICS-20 transfer packet data: %v", err)
}

By explicitly handling the potential errors, the codebase becomes more resilient and easier to debug and maintain.

Assessed type

Error

[c4-pre-sort]

JeffCX marked the issue as low quality report

[JeffCX]

Lack of clearly described impacts, and the error handle does happen

if err = transfertypes.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil { xxx }

the err != nil { xxx } is the error handling

[c4-judge]

0xean marked the issue as unsatisfactory: Invalid