OnRecvPacket can lead to loss of funds when swapping and converting due to lack of rollback/poor error handling

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-05-canto/blob/d1d51b2293d4689f467b8b1c82bba84f8f7ea008/canto-main/x/onboarding/keeper/ibc_callbacks.go#L136-L140

Vulnerability details

Impact

Loss of funds

Proof of Concept

In general, the failure of any operation in the execution of a tx is considered a failure of that tx and it isn't commited.

In onRecvPacket, the operation has already succeeded and does not care whether subsequent operations succeed or fail, because the asset will exist in the canto network in any case.

So, when OnRecvPacket of the Onboarding module is called, then it calls coinswapKeeper. TradeInputForExactOutput and erc20Keeper.ConvertCoin.

However, if the execution of these two functions fails in the middle, for example, the user's token has been deducted, but the execution fails when mint is executed, the transaction will not be rolled back, which will cause the loss of user funds.

But if the execution of one of the calls fail then maybe the token balance of the user will be deducted, but the minting will fail.

So, in this case the transaction won’t be rolled back and there is a possibility that subsequent operations will fail with only the user's token deducting.

When called through a RPC, the x/keeper will return an error. When that happens the SDK handles the failure by rolling back.

But the problem here is because OnRecvPacket calls the keeper function directly, and the error returned by the keeper function is not passed.

func (k Keeper) OnRecvPacket(
    ctx sdk.Context,
    packet channeltypes.Packet,
    ack exported.Acknowledgement,
) exported.Acknowledgement {
    // return original acknowledgement
    return ack
}

As demonstrated, OnRecvPacket issues an acknowledgment (ack) regardless of results. This means that in situations like a failure in erc20Keeper.ConvertCoin, there isn't proper error management. Further examining ConvertCoin, the operation initially deducts the balance and then attempts to mint them. Given that there are numerous situations where K.CallEVM might be invoked, the potential risk arises when, after deducting a user’s balance, K.CallEVM fails to execute successfully. This will lead to the irreversible loss of the deducted funds.

Tools Used

Eyes

Recommended Mitigation Steps

In OnRecvPacket check if transaction failed and rollback if so

Assessed type

Other

[poorphd]

• Labels
  • sponsor confirmed
• reasoning
  • If an error is returned from call.EVM(), it is possible to reproduce the situation where the user's fund is escrowed in the module account and lost, as raised in the issue.
  • The ibc callback functions implemented in the onboarding module do not operate at the transaction level, but directly call the keeper methods, so the operation may not be atomic unlike transactions. In other words, if some keeper methods succeed and some keeper methods fail, a mechanism is needed to properly rollback the changed state.
• Severity
  • High → Mid
  • Logically, it was possible to reproduce the issue, but the chances of an EVM call failing in reality are low. If a realistic attack scenario or PoC is provided, it is judged that the severity can be readjusted to high.
• Patch
  • We will patch this before v0.50 production release.
  • We plan to patch in a direction that does not roll back the IBC transfer even if the convert coin fails, by using a cached context when calling erc20Keeper.ConvertCoin in the OnRecvPacket of the onboarding middleware.

[3docSec]

The H impact could be justified only with a concrete scenario that would cause erc20Keeper.ConvertCoin to fail after coinswapKeeper.TradeInputForExactOutput succeeded.

I am provisionally downgrading this to QA; the wardens are welcome to submit during PJQA a convincing explanation of how this scenario could happen without the protocol admins making a configuration error.

[c4-judge]

3docSec changed the severity to QA (Quality Assurance)

[c4-judge]

3docSec marked the issue as grade-b

[zhaojio]

The H impact could be justified only with a concrete scenario that would cause erc20Keeper.ConvertCoin to fail after coinswapKeeper.TradeInputForExactOutput succeeded.

I am provisionally downgrading this to QA; the wardens are welcome to submit during PJQA a convincing explanation of how this scenario could happen without the protocol admins making a configuration error.

I think there may be some misunderstanding here

The question is not about:

erc20Keeper.ConvertCoin to fail after coinswapKeeper.TradeInputForExactOutput succeeded.

Instead: erc20Keeper.ConvertCoin failed to execute internally and the error it returned was not handled, When ConvertCoin is executed, if the user's token is deducted, but mint or the transfer function does not execute successfully, it will usually roll back, but not in the OnRecvPacket callback function, which will result in the loss of the user's funds.

mint/transfer fails in the following cases:

1. The contract has the pause function. The administrator suspends the contract.
2. When the transfer function was executed, the number of tokens held in the contract was insufficient.

There are also many cases where functions in EVM fail to execute, and it's possible that the contract is published by a third party, and we don't know how the function is implemented. In the callback function of OnRecvPacket , error handling for ConvertCoin/TradeInputForExactOutput is broken.

I think this issue is the most worthy of attention, my report #3 is a duplicate of this.

[3docSec]

Instead: erc20Keeper.ConvertCoin failed to execute internally and the error it returned was not handled

That was clear indeed - I am not sure why you thought this was a misunderstanding: to get to the point where the error is not handled (L139) you have to be in the situation where coinswapKeeper.TradeInputForExactOutput succeeds and erc20Keeper.ConvertCoin fails.

The reason for invalidation is, as stated in https://github.com/code-423n4/2024-05-canto-findings/issues/19#issuecomment-2191603629, that we miss proof of concrete situations for this scenario happening.

You provided these:

mint/transfer fails in the following cases:

• The contract has the pause function. The administrator suspends the contract.
• When the transfer function was executed, the number of tokens held in the contract was insufficient.

• Having an ERC20 contract paused, and at the same time configured for swapping (maxSwapAmount > 0) is a configuration error, so this is a L (QA) scenario.
• there is no evidence to support the possibility of a lack tokens to transfer, because the tokens are "taken" from the amount that was just received via IBC (so they can't be not available), and then "minted":

        // canto-main/x/erc20/keeper/msg_server.go:L163
    // Escrow coins on module account
    err := k.bankKeeper.SendCoinsFromAccountToModule(ctx, sender, types.ModuleName, coins)
    if err != nil {
        return nil, errorsmod.Wrap(err, "failed to escrow coins")
    }

    // Mint tokens and send to receiver
    _, err = k.CallEVM(ctx, erc20, types.ModuleAddress, contract, true, "mint", receiver, msg.Coin.Amount.BigInt())
    if err != nil {
        return nil, err
    }

Also, the CallEVM( ... "mint") call follows a defensive MintingEnabled check, so even in case of misconfiguration, the user remains with the IBC tokens in their possession.

About this:

There are also many cases where functions in EVM fail to execute, and it's possible that the contract is published by a third party, and we don't know how the function is implemented.

That's not how it works on Ethermint: unlike in mainstream EVM networks, contract deployment is permissioned by default, so the team and/or governance has full control over the ERC20 contracts deployed in the Canto network.

[zhaojio]

@3docSec
You are right, In theory, the ConvertCoin function will not fail to execute mint/transfer after deducting the user's token.

But the failure of the error handling mechanism of the ConvertCoin function is an unexpected behavior:

The ConvertCoin function checks the user's balance after convert, which is invalid here because the returned error was not handled. This is explained in my report #3

Assuming that the contract developer assumes that this check must be valid, code in the contract (possibly code for future upgrades) depends on this check, but in fact this check is invalid in the OnRecvPacket callback, which can cause unknown problems.

Can the severity be M? In the comments, sponsor also thinks that the severity is M.

[3docSec]

Hi @zhaojio I think we all agree on the validity of the issue highlighted. However, for this finding to be judged M (or H even), we need to:

• identify at least one scenario where this error may happen
• identify a precise impact

This report falls short on both aspects, and you said it yourself by using the can cause unknown problems wording; this doesn't mean the finding is "wrong", but rather that it doesn't meet the criteria for an M finding.

[zhaojio]

Hi @zhaojio I think we all agree on the validity of the issue highlighted. However, for this finding to be judged M (or H even), we need to:

• identify at least one scenario where this error may happen
• identify a precise impact

This report falls short on both aspects, and you said it yourself by using the can cause unknown problems wording; this doesn't mean the finding is "wrong", but rather that it doesn't meet the criteria for an M finding.

that ok, sometimes an issue like this will be identified as M and ultimately it's up to you to decide