search

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

1 stars 0 forks source link

Missing store revert in case of swap error can lead to loss of funds #69

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/onboarding/keeper/ibc_callbacks.go#L93-L96

Vulnerability details

Impact

The module is expected to have no state changes in case a swap failed, and continue to the conversion phase. It was implemented by swallowing the error with a log and continuing with the flow (erc20 conversion, etc). This is the relevant code section:

swappedAmount, err = k.coinswapKeeper.TradeInputForExactOutput(ctx, coinswaptypes.Input{Coin: transferredCoin, Address: recipient.String()}, coinswaptypes.Output{Coin: swapCoins, Address: recipient.String()})
        if err != nil {
            logger.Error("failed to swap coins", "error", err)
        }

In reality, the TradeInputForExactOutput function might change the store state and the caller will not revert it in case of an error.

For example, the internal swapCoins function calls SendCoins twice. In case the first one succeeded and the second failed and returned an error, the users will lose the in funds (which will be sent to the pool) and will not receive the out 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

  1. Apply this patch. It will cause swap to error after some state changes happened (one side of the swap):

    
diff --git a/Canto/x/coinswap/keeper/swap.go b/Canto/x/coinswap/keeper/swap.go
index 67e04ef..21129f7 100644
--- a/Canto/x/coinswap/keeper/swap.go
+++ b/Canto/x/coinswap/keeper/swap.go
@@ -2,6 +2,7 @@ package keeper

import (
    "fmt"
+
    sdk "github.com/cosmos/cosmos-sdk/types"
    sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"

@@ -19,11 +20,7 @@ func (k Keeper) swapCoins(ctx sdk.Context, sender, recipient sdk.AccAddress, coi return err }

  1. Add this test to x/onboarding/keeper/ibc_callbacks_test.go : 
    {
"swap fails after state change / convert remaining ibc token - some ibc tokens are lost to the pool, the rest are not converted",
func() {
    transferAmount = sdk.NewIntWithDecimal(25, 6)
    transfer := transfertypes.NewFungibleTokenPacketData(denom, transferAmount.String(), secpAddrCosmos, ethsecpAddrcanto)
    bz := transfertypes.ModuleCdc.MustMarshalJSON(&transfer)
    packet = channeltypes.NewPacket(bz, 100, transfertypes.PortID, sourceChannel, transfertypes.PortID, cantoChannel, timeoutHeight, 0)
},
true,
sdk.NewCoins(sdk.NewCoin("acanto", sdk.NewIntWithDecimal(3, 18))),
sdk.NewCoin("acanto", sdk.NewIntWithDecimal(3, 18)),
sdk.NewCoin(uusdcIbcdenom, sdk.NewIntFromUint64(20998399)),
sdk.NewInt(0),
},

It will pass, and we can see that the token conversion failed as well (because the math of the remaining tokens to transfer from the escrow address was too high). It means that (artificial!) swap failure can potentially cause loss of funds and disable the conversion.

Tools Used

IDE.

Recommended Mitigation Steps

A cached context should be used, and the store changes should be committed only in case the TradeInputForExactOutput 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 commented 1 year ago

JeffCX marked the issue as duplicate of #5

c4-pre-sort commented 1 year ago

JeffCX marked the issue as duplicate of #80

c4-judge commented 1 year ago

0xean marked the issue as satisfactory