Add a check for GetStandardDenom's return value

[code423n4]

Lines of code

https://github.com/code-423n4/2023-06-canto/blob/main/Canto/x/onboarding/keeper/ibc_callbacks.go#L70 https://github.com/code-423n4/2023-06-canto/blob/main/Canto/x/onboarding/keeper/ibc_callbacks.go#L88

Vulnerability details

Impact

OnRecvPacket calls k.coinswapKeeper.GetStandardDenom to get the standard denom of the coinswap module. There should be a check for the return value. Or the user could swap the transferred token for the wrong standard denom.

Proof of Concept

OnRecvPacket calls k.coinswapKeeper.GetStandardDenom to get the standard denom. And it is used for the swap. https://github.com/code-423n4/2023-06-canto/blob/main/Canto/x/onboarding/keeper/ibc_callbacks.go#L70 https://github.com/code-423n4/2023-06-canto/blob/main/Canto/x/onboarding/keeper/ibc_callbacks.go#L88

func (k Keeper) OnRecvPacket(
    ctx sdk.Context,
    packet channeltypes.Packet,
    ack exported.Acknowledgement,
) exported.Acknowledgement {
    …

    standardDenom := k.coinswapKeeper.GetStandardDenom(ctx)

    …
    autoSwapThreshold := k.GetParams(ctx).AutoSwapThreshold
    swapCoins := sdk.NewCoin(standardDenom, autoSwapThreshold)
    standardCoinBalance := k.bankKeeper.SpendableCoins(ctx, recipient).AmountOf(standardDenom)

}

However, the standard denom is not a fixed value, it could be set by SetStandardDenom. https://github.com/code-423n4/2023-06-canto/blob/main/Canto/x/coinswap/keeper/keeper.go#L333

func (k Keeper) SetStandardDenom(ctx sdk.Context, denom string) {
    store := ctx.KVStore(k.storeKey)
    denomWrap := gogotypes.StringValue{Value: denom}
    bz := k.cdc.MustMarshal(&denomWrap)
    store.Set(types.KeyStandardDenom, bz)
}

If the standard can be changed. There should be a reasonable check in OnRecvPacket.

Tools Used

Manual Review

Recommended Mitigation Steps

Add a check for the standard denom in OnRecvPacket.

Assessed type

Invalid Validation

[c4-pre-sort]

JeffCX marked the issue as primary issue

[tkkwon1998]

SetStandardDenom is a function that can only be called inside the client. There is no external interface for this to be called by users. To use this function maliciously, canto would have to go through a chain upgrade via governance.

Given this, there is no possibility of setStandardDenom being called after it is initially set.

[c4-sponsor]

tkkwon1998 marked the issue as sponsor disputed

[c4-judge]

0xean marked the issue as unsatisfactory: Invalid