code-423n4 / 2023-06-canto-findings

1 stars 0 forks source link

GetStandardDenom at CreatePool might panic on unchecked nil #103

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-06-canto/blob/main/Canto/x/coinswap/keeper/pool.go#L20 https://github.com/cosmos/cosmos-sdk/blob/main/x/authz/keeper/keeper.go#L67

Vulnerability details

Impact

A panic might occur when calling CreatePool and stop the app

Proof of Concept

https://github.com/code-423n4/2023-06-canto/blob/main/Canto/x/coinswap/keeper/pool.go#L20 here we can see CreatePool is creating new struct pool which call k,GetStandardDenom as value for StandardDenom key. now lets check GetStandardDenom body:

func (k Keeper) GetStandardDenom(ctx sdk.Context) string {
    store := ctx.KVStore(k.storeKey)
    bz := store.Get(types.KeyStandardDenom)

    var denomWrap = gogotypes.StringValue{}
    k.cdc.MustUnmarshal(bz, &denomWrap)
    return denomWrap.Value
}

bz value is not checked as if it equal nil or not before calling MustUnmarshal and that will result panic in the program. few examples from cosmos sdk itself about the correct practice to avoid the panic issue: https://github.com/cosmos/cosmos-sdk/blob/main/x/authz/keeper/keeper.go#L67

bz, err := store.Get(skey)
    if err != nil {
        panic(err)
    }

    if bz == nil {
        return grant, false
    }
    k.cdc.MustUnmarshal(bz, &grant)
    return grant, true

as you see below there a check for bz if equal nil so it doesn't panic on MustUnmarshal. because it is possibe that KVStore would return nil for a storeKey.

Tools Used

Manual Review

Recommended Mitigation Steps

return err if bz == nil and check k.GetStandardDenom(ctx) if it has error before creating pool := &types.Pool struct at https://github.com/code-423n4/2023-06-canto/blob/main/Canto/x/coinswap/keeper/pool.go#L18

    if bz == nil {
        return err //or empty string
    }

Assessed type

Other

c4-pre-sort commented 1 year ago

JeffCX marked the issue as low quality report

JeffCX commented 1 year ago

Insufficient detail and proof

c4-judge commented 1 year ago

0xean marked the issue as unsatisfactory: Insufficient proof