ADR-033 Router anti-reentry mechanism

[blushi]

ADR-033 introduced an anti-reentry mechanism that should be part of the router system for security reasons.

But this could be an issue in the context of the group module in the case of executing proposals that contain message requests from the group module itself.

Having a simple exception for the group module doesn't seem like a viable solution. This sort of anti-reentry mechanism is somewhat arbitrary and pretty strict. It would be better if it were designed on clear reasoning of what re-entry is and isn’t acceptable.

cc/ @aaronc

[aaronc]

Re-entry may be more of an issue for user-defined contracts that we need to think about in the context of CosmWasm. So we should probably remove it from the router. I'd love to hear @ethanfrey's thoughts on this if you have a minute.

[ethanfrey]

If you can control the flows somehow, eg. Group votes, reentrancy is potentially an issue for all modules. None of them handle that, e.g no one is prepared for a re entrant call when calling the bank keeper to move tokens.

You can assume that all native modules are safe and will not do this. Or you can code them to be immune. Or you can prohibit it.

CosmWasm took the code immunity approach. I think prohibition is a weak approach.

[hxrts]

Wasn't this one of the primary reasons we opted to restructure the module system, to properly leverage encapsulation and ocaps control flow? https://medium.com/agoric/preventing-reentrancy-attacks-in-smart-contracts-3899bf837f23

[ethanfrey]

I just read that article. Seems like the same general approach of CosmWasm (except we make them more explicit/ugly).

I wonder a bit about their design in non-trivial examples however. But in general, the requirement is the functions are called asynchronously which would be such a major breaking change to the Cosmos SDK that you may as well fork it and re-name it if you try this (or making everything pure functions). Both are useful ideas but so different than the current design you cannot bring all the app developers along.

[fdymylja]

IMHO - message execution should consume gas. This is most likely the best re-entrancy protection we could get.

We'd need to define message execution cost - until we define a clear way on how a module defines its gas costs we could use a default value. (not sure what it should be though)

[fdymylja]

I see where you're coming from robert, IDK then how we can make sure to not limit use-cases whilst at the same time being safe unless we execute in a sandboxed environment similar to wasm.

Although, I'm not understanding something, if the executions are handled from a single point we should be able to consume gas each time a new msg execution is invoked.

The only problem I'm seeing right now is some handler that keeps blocking forever; but this is a problem that you would have even by calling keepers that call other keepers?

[aaronc]

From what I've seen I think we should probably use gas as the primary anti-entry protection for trusted modules - i.e. ones built with Cosmos SDK. And CosmWasm should setup its own re-entrancy protections for user contracts which leverage ADR 033.

[ethanfrey]

And CosmWasm should setup its own re-entrancy protections for user contracts which leverage ADR 033.

We have been re-entrancy safe since day one. Nice design with the actor model. Thanks for that one.

[dtribble]

I know this is late to the party, but how does gas fees prevent reentrancy at all? It just makes it cost a little bit, but if the attack is worth something, that's in the noise. Since billions have been lost to reentrancy attacks, that doesn't seem like much of a deterrent.

[fdymylja]

know this is late to the party, but how does gas fees prevent reentrancy at all? It just makes it cost a little bit, but if the attack is worth something, that's in the noise. Since billions have been lost to reentrancy attacks, that doesn't seem like much of a deterrent.

When I wrote this I meant to avoid for some re-entrant call to lock the execution forever.

With how modules are designed as of now, and since modules are sort of RPC servers, fitting a re-entrancy-safe model such as the one used in CosmWasm would make for a really weird and counter-intuitive API imo...

[aaronc]

Based on the discussion, we're removing explicit re-entry from the ADR 033 router. Marking this as closed.

[robert-zaremba]

General protection for re-entrency attack is to store all updates before giving a control to another module. One way to do it will be with the async calls - the call will happen only after current message processing is finished.

[fdymylja]

I do agree, but I don't see how we can make this model fit into current modules... since execution is done via RPC calls, it would make for a really weird API as it would force MsgServers responses to contain msgs to execute. On top of that it'd require a refactoring of all the modules which depend on other modules...

[ethanfrey]

Yeah, if you go that route, you might as well re-write everything as CosmWasm contracts ;)

But I agree with this point. You cannot make huge breaking changes to how modules are architected and assume everyone downstream will follow and rewrite all their apps. It was hard enough to swap out a codec, changing the whole paradigm of module composition is a bit much.

[robert-zaremba]

Just brainstorming. I was thinking if the async call mechanism @aaronc is exploring would work here. Basically:

• all re-entrence are disabled if they happen in the same "sync" call
• we can schedule async calls - which will receive a new derived context. The async call can only happen after the current call is finished.

Defining this async calls and callbacks for async calls could be a complex thing.