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

RunTx() and runCheckTxConcurrently() should contain existing handler msgs check #20

Open
dalmirel opened this issue Jun 22, 2023 · 0 comments
Labels
informal audit Issues surfaced during the Informal Systems audit

Comments

@dalmirel
Copy link

The following issue was reported during the Informal Systems audit of dYdX cosmos-sdk fork codebase at commit hash: f6e7e7a.

Classification of the issue:

Type Severity Impact Exploitability
Implementation Low Medium Low

Involved artifacts:

Description
A thorough analysis of Cosmos SDK’s RunTx() for modes: runTxModeCheck, runTxModeReCheck
and newly added dYdX Cosmos SDK's fork runCheckTxConcurrently() functions is performed in order to confirm that all necessary checks are being executed prior to adding the transaction to the mempool.

It has been noticed that RunMsgs() is missing a check whether the application's msgServiceRouter contains a handler for each of the messages contained in the tx, which is expected to be part of this function by the Cosmos SDK documentation.

Problem Scenarios
Without the check listed above, there is a possibility of invalid (non-executable) transactions from the mempool reaching to the proposed blocks and taking slots that could be filled with valid, executable transactions.

Recommendation
runTx() and runCheckTxConcurrently() should contain the following check:

for i, msg := range msgs {
		
  handler := app.msgServiceRouter.Handler(msg)
  if handler == nil {
		err = sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "can't route message %+v", msg)
  }
}

The check could be placed above the critical section part here, which would be inline with the solution Cosmos SDK team applied here.

@dalmirel dalmirel added the informal audit Issues surfaced during the Informal Systems audit label Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
informal audit Issues surfaced during the Informal Systems audit
Development

No branches or pull requests

1 participant