Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

blockedAddrs can bypass #10

Open
c4-bot-6 opened this issue Jun 16, 2024 · 4 comments
Open

blockedAddrs can bypass #10

c4-bot-6 opened this issue Jun 16, 2024 · 4 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-b insufficient quality report This report is not of sufficient quality Q-11 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_primary AI based primary recommendation sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue

Comments

@c4-bot-6
Copy link
Contributor

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

@c4-bot-6 c4-bot-6 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Jun 16, 2024
c4-bot-2 added a commit that referenced this issue Jun 16, 2024
@c4-bot-11 c4-bot-11 added the 🤖_primary AI based primary recommendation label Jun 20, 2024
@howlbot-integration howlbot-integration bot added the insufficient quality report This report is not of sufficient quality label Jun 21, 2024
@3docSec
Copy link

3docSec commented Jun 27, 2024

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 c4-judge added downgraded by judge Judge downgraded the risk level of this issue and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Jun 27, 2024
@c4-judge
Copy link

3docSec changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added the QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax label Jun 27, 2024
@c4-judge c4-judge reopened this Jun 27, 2024
@c4-judge
Copy link

3docSec marked the issue as grade-b

@poorphd
Copy link

poorphd commented Jul 3, 2024

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

@poorphd poorphd added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Jul 3, 2024
@C4-Staff C4-Staff added the Q-11 label Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-b insufficient quality report This report is not of sufficient quality Q-11 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_primary AI based primary recommendation sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue
Projects
None yet
Development

No branches or pull requests

6 participants