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

feat: prevent fswap and fbridge module in Submessages #123

Closed
wants to merge 5 commits into from

Conversation

da1suk8
Copy link
Member

@da1suk8 da1suk8 commented Apr 25, 2024

Description

This PR implements a filtering mechanism for specific message types within Submessages.
Specifically, messages related to fswap and fbridge are targeted by this filter, and any attempts to execute operations based on these message types will result in an error.

FYI: Finschia/finschia-sdk#1336, Finschia/finschia-sdk#1340

Motivation and context

How has this been tested?

Screenshots (if appropriate):

Checklist:

  • I followed the contributing guidelines and code of conduct.
  • I have added a relevant changelog to CHANGELOG.md
  • I have added tests to cover my changes.
  • I have updated the documentation accordingly.
  • I have updated API documentation client/docs/swagger-ui/swagger.yaml

@da1suk8 da1suk8 marked this pull request as ready for review April 25, 2024 03:36
@da1suk8 da1suk8 requested a review from zemyblue April 25, 2024 03:36
x/wasm/keeper/msg_dispatcher.go Outdated Show resolved Hide resolved
x/wasm/keeper/msg_dispatcher.go Outdated Show resolved Hide resolved
@Mdaiki0730
Copy link
Member

Hello, I have two questions.

@170210
Copy link

170210 commented Apr 25, 2024

@Mdaiki0730

How do you filter when trying to send fswap or fbredge messages as Custom?

From this pr (#118 ), custom querier has been disabled.

You are filtering within MessageDispatcher.DispatchSubmessages, but is it not necessary to consider filtering in cases where dispatch is performed from MessageDispatcher.DispatchMessage?

This pr looks like only filter fswap/fbridge in submessage not all messages. Is it correct?

Alternative queries for our original modules are going to be opened with the stargate query feature in later versions.

@da1suk8 But we haven't opened our original modules' Stargate Querier yet. If we remove this PR, can the tests still pass?

@da1suk8
Copy link
Member Author

da1suk8 commented Apr 25, 2024

@Mdaiki0730 @170210

This PR prevents the use of fbridge and fswap within submessages, not in all messages.
Thank you for your comments.

@da1suk8
Copy link
Member Author

da1suk8 commented Apr 25, 2024

@170210

But we haven't opened our original modules' Stargate Querier yet. If we remove this PR, can the tests still pass?

Sorry I didn't quite understand, please explain it again.

loloicci
loloicci previously approved these changes Apr 25, 2024
@Mdaiki0730
Copy link
Member

@170210 @da1suk8

From this pr (#118 ), custom querier has been disabled.

thank you for your reply. The querier's Custom has been changed to not handle, but what I'm wondering about is the submessage's Custom.
querier: https://github.com/Finschia/wasmvm/blob/main/types/queries.go#L85
submessage: https://github.com/Finschia/wasmvm/blob/main/types/msg.go#L101

@Mdaiki0730
Copy link
Member

Mdaiki0730 commented Apr 25, 2024

@da1suk8

This PR prevents the use of fbridge and fswap within submessages, not in all messages.

I'm worried about whether this is the right place to implement it.
Currently, you are filtering using MessageDispatcher.DispatchSubmessages, but since messenger.DispatchMsg is public, it will not be filtered if it is called without going through MessageDispatcher.DispatchSubmessages.
Please don't bother if the scope of this PR doesn't require us to consider this.

@da1suk8
Copy link
Member Author

da1suk8 commented Apr 26, 2024

@Mdaiki0730

To clarify my understanding, in the context of a Submessage, MessageDispatcher.DispatchSubmessages is invoked, and have applied a filter there.

Does this mean that messenger.DispatchMsg is also called?
Or are you suggesting that it might be better to apply a filter within messenger.DispatchMsg itself?

@Mdaiki0730
Copy link
Member

Mdaiki0730 commented Apr 26, 2024

Or are you suggesting that it might be better to apply a filter within messenger.DispatchMsg itself?

This is my opinion. But it doesn't seem to be a problem at the moment, so you can skip it.

Copy link
Member

@zemyblue zemyblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

func (h SDKMessageHandler) handleSdkMessage(ctx sdk.Context, contractAddr sdk.Address, msg sdk.Msg) (*sdk.Result, error) {
if err := msg.ValidateBasic(); err != nil {
return nil, err
}
// make sure this account can send it
for _, acct := range msg.GetSigners() {
if !acct.Equals(contractAddr) {
return nil, sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "contract doesn't have permission")
}
}
// find the handler and execute it
if handler := h.router.Handler(msg); handler != nil {
// ADR 031 request type routing
msgResult, err := handler(ctx, msg)
return msgResult, err
}
// legacy sdk.Msg routing
// Assuming that the app developer has migrated all their Msgs to
// proto messages and has registered all `Msg services`, then this
// path should never be called, because all those Msgs should be
// registered within the `msgServiceRouter` already.
return nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "can't route message %+v", msg)
}

Is there any case where it's executed through here?

@da1suk8
Copy link
Member Author

da1suk8 commented May 2, 2024

@Mdaiki0730

I will handle another PR
Thank you.

@da1suk8
Copy link
Member Author

da1suk8 commented May 2, 2024

@zemyblue

Is there any case where it's executed through here?

The relevant functions are executed in the order of 1, 2, 3, as shown below. Therefore, this PR is sufficient.
Thank you.

  1. events, data, err = d.messenger.DispatchMsg(subCtx, contractAddr, ibcPort, msg.Msg)
  2. events, data, err := h.DispatchMsg(ctx, contractAddr, contractIBCPortID, msg)
  3. func (h SDKMessageHandler) DispatchMsg(ctx sdk.Context, contractAddr sdk.AccAddress, contractIBCPortID string, msg wasmvmtypes.CosmosMsg) (events []sdk.Event, data [][]byte, err error) {
    sdkMsgs, err := h.encoders.Encode(ctx, contractAddr, contractIBCPortID, msg)
    if err != nil {
    return nil, nil, err
    }
    for _, sdkMsg := range sdkMsgs {
    res, err := h.handleSdkMessage(ctx, contractAddr, sdkMsg)
    if err != nil {
    return nil, nil, err
    }
    // append data
    data = append(data, res.Data)
    // append events
    sdkEvents := make([]sdk.Event, len(res.Events))
    for i := range res.Events {
    sdkEvents[i] = sdk.Event(res.Events[i])
    }
    events = append(events, sdkEvents...)
    }
    return
    }

@da1suk8 da1suk8 requested a review from zemyblue May 2, 2024 14:39
Copy link
Member

@zemyblue zemyblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes is not need, I added another PR to do same feature in finschia witouth this wasmd change. Finschia/finschia#352

@da1suk8
Copy link
Member Author

da1suk8 commented May 8, 2024

Closed via Finschia/finschia#352

@da1suk8 da1suk8 closed this May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants