Skip to content
New issue

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

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

Already on GitHub? # to your account

R4R: Fix ics20 event bug #6269

Merged
merged 1 commit into from
May 22, 2020
Merged

R4R: Fix ics20 event bug #6269

merged 1 commit into from
May 22, 2020

Conversation

SegueII
Copy link
Contributor

@SegueII SegueII commented May 22, 2020

EVENT BUG

This is one of the BUGs that IRISnet team reported to security@cosmosnetwork.dev, but it seems that it has not been fixed. Hacker can use this bug to attack other nodes in the network, thereby improving its ranking in the game (however we did not do so), this bug has a very serious impact on the entire schedule of GoZ!

Bug Description

When a Tx contains multiple ICS-20 messages, the events will be abnormal. The reason is that the following msg events will contain all the previous msg events. You can see the event logs.events.type='send_packet' here: https://gist.github.com/SegueII/401de4d690a0a40038ad21a4ed2536ea
This is an example using one, two and three MSGs in one Tx.

Because of event error,so relayPacketFromQueryResponse(...) can't get the correct result. https://github.com/iqlusioninc/relayer/blob/master/relayer/naive-strategy.go#L320
We fixed this problem in relayer, making it possible to query the packet correctly. But more serious problems were found in the next test: When there are fewer msgs in one Tx, no exception will occur, but if the number of MSGs in a single Tx is big, the event data will be very, very large, and even cause node memory overflow during query.

// tendermint/rpc/client/interface.go

type SignClient interface {
    ...
    TxSearch(query string, prove bool, page, perPage int, orderBy string) (*ctypes.ResultTxSearch, error)
}

// query str => `send_packet.packet_src_channel='<channel-id>' AND send_packet.packet_sequence='<seq>'`

If a Tx contains dozens or even hundreds of MSGs, when using multi-goroutine call TxSearch(...), the following error will be caused, and the node memory will explode. In my PC, gaia's memory usage will exceed 20G.

You can see the err log here: https://gist.github.com/SegueII/d9c543c92cc6dde7d8d5a75e6eae6ba1

In phase-1b, this BUG can be used to attack the public RPC nodes, which will have a greater impact on the game results. Attacks can also be easily launched in phase-2 and phase-3, by sending transactions containing a large number of MSGs to the channels of other teams, making their relayer query timeout and unable to relay packet.

The fix method is very simple, just add ctx = ctx.WithEventManager(sdk.NewEventManager()) to NewHandler() in ICS-20 module.

// NewHandler returns sdk.Handler for IBC token transfer module messages
func NewHandler(k Keeper) sdk.Handler {
    return func(ctx sdk.Context, msg sdk.Msg) (*sdk.Result, error) {

        // ==================================================
        ctx = ctx.WithEventManager(sdk.NewEventManager())
        // ==================================================

        switch msg := msg.(type) {
        case MsgTransfer:
            return handleMsgTransfer(ctx, k, msg)
        default:
            return nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "unrecognized ICS-20 transfer message type: %T", msg)
        }
    }
}

@codecov
Copy link

codecov bot commented May 22, 2020

Codecov Report

Merging #6269 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #6269   +/-   ##
=======================================
  Coverage   55.66%   55.67%           
=======================================
  Files         447      447           
  Lines       26866    26867    +1     
=======================================
+ Hits        14956    14957    +1     
  Misses      10843    10843           
  Partials     1067     1067           

@fedekunze fedekunze merged commit ecbb5ac into cosmos:master May 22, 2020
jackzampolin pushed a commit that referenced this pull request May 22, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants