code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

It appears that user funds can be lost in case of onboarding is made for a recipient account that doesn't exist or has an unsupported public key type in the Canto network, which is why I submit this as a High.

So in case a user provide valid recipient in the onboarding (so the format is totally valid), there is a possibility that the account doesn't exist, in which case the expected behavior should be to short-circuit the onboarding (confirmed with t_k___ in discord DM), but instead the onboarding works as if the account would exist, which means afterward the coins transfered will not be accessible by anyone and be lost. As I understand it compare to ETH network, it would be like if you transfer the coin to valid address but that you don't control.

Additionally, since the recovery middleware do some check about if an account have a public key supported by Canto, it seems that this condition would also be required here to cover the similar case, where the account exist, but with a public key that is not supported by Canto, which would cause the same problematic.

Proof of Concept

Keep the code as it is in the repo. 1) Comment this line : account := k.accountKeeper.GetAccount(ctx, recipient) 2) Add this line instead: var account authtypes.AccountI 3) Run the unit test swap / convert remaining ibc token - swap and erc20 conversion are successful, it will pass, which confirm the bug, as it should fails as the expected result should be that onboarding is disabled in such case.

Tools Used

Go v1.20 and Goland IDE

Recommended Mitigation Steps

I made the following modification that you can apply to address the current submission. Additionally, added small improvements the code which I would still recommend (Low) 1) I added a break statement when looking for the whitelist, as once one is found, there is no reason to continue looping. 2) Moved standardDenom down to be along where it is used (cleaner code), and also using params instead of calling k.GetParams(ctx) which is just a waste.

Finally, I had another concern for a specific case where sender == recipient, but could not confirm if this is a real problem, but I would recommend the team to think about that scenario.

diff --git a/Canto/x/onboarding/keeper/ibc_callbacks.go b/Canto/x/onboarding/keeper/ibc_callbacks.go
index a3421bb..a62ec17 100644
--- a/Canto/x/onboarding/keeper/ibc_callbacks.go
+++ b/Canto/x/onboarding/keeper/ibc_callbacks.go
@@ -15,6 +15,7 @@ import (
        "github.com/ethereum/go-ethereum/common"

        "github.com/Canto-Network/Canto/v6/ibc"
+       canto "github.com/Canto-Network/Canto/v6/types"
        coinswaptypes "github.com/Canto-Network/Canto/v6/x/coinswap/types"
        erc20types "github.com/Canto-Network/Canto/v6/x/erc20/types"
        "github.com/Canto-Network/Canto/v6/x/onboarding/types"
@@ -46,6 +47,7 @@ func (k Keeper) OnRecvPacket(
        for _, s := range params.WhitelistedChannels {
                if s == packet.DestinationChannel {
                        found = true
+                       break
                }
        }

@@ -60,6 +62,7 @@ func (k Keeper) OnRecvPacket(
        }

        //get the recipient account
+       //var account authtypes.AccountI // TODO: uncomment this line and comment the next line to test the case of account's not found (but valid recipient)
        account := k.accountKeeper.GetAccount(ctx, recipient)

        // onboarding is not supported for module accounts
@@ -67,7 +70,15 @@ func (k Keeper) OnRecvPacket(
                return ack
        }

-       standardDenom := k.coinswapKeeper.GetStandardDenom(ctx)
+       // onboarding is aborted if account doesn't exist
+       if _, isAccount := account.(authtypes.AccountI); !isAccount {
+               return ack
+       }
+
+       /* TODO: integration pass with the following code, but not the unit test as they don't mock properly the account as they don't inject pub key. Please fix unit test first.
+       // onboarding is aborted if account pub key is not supported by Canto
+       if !canto.IsSupportedKey(account.GetPubKey()) {
+               return ack
+       }
+       */

        var data transfertypes.FungibleTokenPacketData
        if err = transfertypes.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil {
@@ -84,7 +95,8 @@ func (k Keeper) OnRecvPacket(
                data.Denom, data.Amount,
        )

-       autoSwapThreshold := k.GetParams(ctx).AutoSwapThreshold
+       standardDenom := k.coinswapKeeper.GetStandardDenom(ctx)
+       autoSwapThreshold := params.AutoSwapThreshold
        swapCoins := sdk.NewCoin(standardDenom, autoSwapThreshold)
        standardCoinBalance := k.bankKeeper.SpendableCoins(ctx, recipient).AmountOf(standardDenom)
        swappedAmount := sdk.ZeroInt()

Assessed type

Other

c4-pre-sort commented 1 year ago

JeffCX marked the issue as primary issue

tkkwon1998 commented 1 year ago

I definitely agree doing a check in OnRecvPacket to make sure a sender exists in the accounts module is good practice.

However, assets that are transferred via IBC are bounced back to the original chain if the user does not exist on canto. Therefore, the onboarding middleware layer would not get triggered because the IBC transfer wouldn't be successful.

In testing, the warden changes the recipient inside the onboarding middleware. Since the recipient was initially valid, the call has gone through all previous checks successfully and the IBC transfer is successful. Hence the onboarding middleware still executes, even though the recipient is technically invalid now.

c4-sponsor commented 1 year ago

tkkwon1998 marked the issue as disagree with severity

0xean commented 1 year ago

This sounds like input sanitization at its root to me, typically these types of issues are QA in C4 contests.

c4-judge commented 1 year ago

0xean changed the severity to QA (Quality Assurance)