Missing store revert in case of erc20 conversion error can lead to loss of funds

[code423n4]

Lines of code

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

Vulnerability details

Impact

The module is expected to have no changes in case a erc20 conversion failed. It was implemented by swallowing the error with a log and continuing with the flow (finishing the IBC transfer). This is the relevant code section:

if _, err = k.erc20Keeper.ConvertCoin(sdk.WrapSDKContext(ctx), convertMsg); err != nil {
    logger.Error("failed to convert coins", "error", err)
    return ack
}

In reality, the ConvertCoin function might change the store state and not revert it in case of an error. For example, the internal convertCoinNativeCoin function will call SendCoinsFromAccountToModule and CallEVM(..., "mint", ...). In case the first one succeeded and the second failed and returned an error, the users will lose the native funds (which will be sent to the erc20 module) and will not receive the erc20 funds, leading to direct fund loss.

When swallowing errors this way, the state should be reverted to what it was right before the operation started, using a cached context.

Proof of Concept

See identical POC in the previous submission for the store revert in swap errors.

Tools Used

IDE.

Recommended Mitigation Steps

A cached context should be used, and the store changes should be comitted only in case the ConvertCoin call returned no error. One good example is Osmosis's cached context wrapper that can be found here https://github.com/osmosis-labs/osmosis/blob/main/osmoutils/cache_ctx.go .

Assessed type

Other

[c4-pre-sort]

JeffCX marked the issue as low quality report

[JeffCX]

the code already returns "ack", don't think the report is valid

[0xean]

I believe this to be expected behavior as outlined here:

However, the onboarding process is non-atomic, meaning that even if the swap or conversion fails, it does not revert IBC transfer and the asset transferred to the Canto network will still remain in the Canto network.

It would be good to have @c4-sponsor confirm

[tkkwon1998]

@0xean confirmed. The code returns the acknowledgement on errors.

[c4-judge]

0xean marked the issue as unsatisfactory: Invalid