Cosmos-sdk: Draft a brief for auditors describing focus areas of base-app

[robert-zaremba]

Summary

We need to create ½ page brief for BaseApp audit. The goals is to help auditors with pointing areas of BaseApp where they should focus so they can work up a proposal & estimate.

---

For Admin Use

• [x] Not duplicate issue
• [x] Appropriate labels applied
• [x] Appropriate contributors tagged
• [x] Contributor assigned/self-assigned

[robert-zaremba]

• Interchain Foundation - Staking, Slashing, Distribution -- written spec, refactor, and TLA+ spec coming next year -- working with Runtime Verification, Interchain GmbH, Informal
• Crypto.com suggestions: Bank, auth
• Alessio: recovery middleware, way that we mount stores, guaranteeing isolation between modules

[alexanderbez]

For BaseApp, we certainly want to consider runTx and runMsgs as highest priority, specifically in terms of how things are cached, when they're persisted and all error modes, because DeliverTx and CheckTx essentially just call these two methods.

[robert-zaremba]

Question: who is handling Antehandler? Will it be part of the x/auth?

[alessio]

What do you mean by Question: who is handling Antehandler?

[alessio]

For what concerns BaseApp, anteHandler is just a method that the client type that embeds BaseApp can set via SetAnteHandler. BaseApp.anteHandler is called by runTx and only once.

[alexanderbez]

Question: who is handling Antehandler? Will it be part of the x/auth?

Yeah so the AnteHandler is technically outside the scope of the BaseApp, since it's app-specific. Although I do recommend that the auditors review x/auth as well since it's so integral to the tx and msg execution flow.

[hxrts]

We have some flexibility to go outside of BaseApp, however the scope should be very clear for auditors and our informal commitment from a budget allocation perspective is that this will be a BaseApp-centered audit.

[robert-zaremba]

We need to be sure that Antehandler is reviewed locally (inside it's package) and globally - as this is used in every runTx (optionally as @alessio pointed out, but I'm not aware about any app which is not using it). So it can't be reviewed in isolation.
So, we need to be sure that this will be properly handled through crypto.com part or our part.

[alessio]

I do recommend that the auditors review x/auth

I agree with that.

optionally as @alessio pointed out, but I'm not aware about any app which is not using it

The point is not whether apps use antehandler - obviously they do. The point is that antehandler as provided by BaseApp is just an interface. Antehandlers are app-specific and require to be treated (and audited) as such.

The SDK comes with a modular mechanism for building antehandlers pipelines, see types.{AnteHandler,AnteDecorator,ChainAnteDecorators}. That is worth looking into. I'll drop some explanations in the brief.

[hxrts]

Also worth mentioning this will likely be a time-bound audit per crypto.com's request, so some notion of precedence could be useful.

[alexanderbez]

We need to be sure that Antehandler is reviewed locally (inside it's package) and globally - as this is used in every runTx (optionally as @alessio pointed out, but I'm not aware about any app which is not using it). So it can't be reviewed in isolation.

I don't understand how this is possible? Do you mean BaseApp should be audited assuming the default x/auth AnteHandler is used?

[alessio]

I've extended the brief with an explanation of how antehandler and antehandler decorators work. That seems in scope of this audit

[robert-zaremba]

I don't understand how this is possible? Do you mean BaseApp should be audited assuming the default x/auth AnteHandler is used?

I'm saying that AnteHandler must not be audited in isolation.

[alexanderbez]

Correct. The standard one provided by x/auth should be reviewed in conjunction with BaseApp per https://github.com/cosmos/cosmos-sdk/issues/8078#issuecomment-739925947

[alessio]

I suspect this can be closed. I'd defer the decision to close this to @hxrts and @okwme though

[hxrts]

Yes, however the brief does link to this issue. So for posterity, I'll note that closure here is in reference to creation of the brief, not completion of the audit.