code-423n4 / 2024-05-canto-findings

0 stars 0 forks source link

blockedAddrs can bypass #10

Open c4-bot-6 opened 3 months ago

c4-bot-6 commented 3 months ago

Lines of code

https://github.com/code-423n4/2024-05-canto/blob/d1d51b2293d4689f467b8b1c82bba84f8f7ea008/canto-main/x/coinswap/keeper/msg_server.go#L144 https://github.com/code-423n4/2024-05-canto/blob/d1d51b2293d4689f467b8b1c82bba84f8f7ea008/canto-main/x/onboarding/keeper/ibc_callbacks.go#L96

Vulnerability details

Impact

Bypass the blacklist

Proof of Concept

coinswap module SwapCoin function to validate the input parameters Then call Swap function, and then call TradeInputForExactOutput/TradeExactInputForOutput

SwapCoin -> Input verification -> Swap -> TradeInputForExactOutput/TradeExactInputForOutput

However, the onboarding module's OnRecvPacket callback function directly calls the TradeInputForExactOutput function without verifying the input.

OnRecvPacket -> TradeInputForExactOutput

In this report let's look at the validation of blockedAddrs:

func (m msgServer) SwapCoin(goCtx context.Context, msg *types.MsgSwapOrder) (*types.MsgSwapCoinResponse, error) {
    ....
    if m.Keeper.blockedAddrs[msg.Output.Address] {
        return nil, errorsmod.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to receive external funds", msg.Output.Address)
    }
    ....
}

There is no verification of Output.Address in OnRecvPacket, so an attacker can bypass blockedAddrs detection using the onboarding module.

Tools Used

vscode, manual

Recommended Mitigation Steps

-    OnRecvPacket -> TradeInputForExactOutput
+    OnRecvPacket -> SwapCoin

Assessed type

Invalid Validation

3docSec commented 2 months ago

The onboarding module only swaps gas (very little) amounts, and it does it only once, so it's not clear how an H/M impact can be caused by this finding.

c4-judge commented 2 months ago

3docSec changed the severity to QA (Quality Assurance)

c4-judge commented 2 months ago

3docSec marked the issue as grade-b

poorphd commented 2 months ago

Send to blocked address is already handled by IBC transfer module. Once IBC transfer ack has error, coinswap logic doesn't initiate and we have test case for that. https://github.com/code-423n4/2024-05-canto/blob/main/canto-main/x/onboarding/ibc_module_test.go#L175-L191