#Opt In Reentry PR

1 messages · Page 1 of 1 (latest)

gaunt fog
#

cc @neat wave @warped shale and @pastel marlin (was it you that asked for the feature?)

vital merlin
#

thanks, this looks fine on the high level. we still need a CAP though.
also, the implementation of ContractReentryMode is not well specified and not well tested. I'm not sure if just setting it to allowed will work properly right now. but that's probably on us to figure out (and CAP will help as well)

#

I think when a contract does a call_reentrant call, it should only allow reentrance into itself. that's not the case with the current implementation. so e.g. it's possible to do A--(call)->B--(reentrant_call)-->A. I don't think this should be allowed

gaunt fog
#

ContractReentryMode is not well specified and not well tested. I'm not sure if just setting it to allowed will work properly right now

The implementation works correctly, the test I made is really simple but it's a fairly straightforward implementation code-wise, at least in terms of logic. I agree there should be more thorough testing though.

it's possible to do A--(call)->B--(reentrant_call)-->A. I don't think this should be allowed

@vital merlin yeah I specified this in the PR. The caller contract has no power allowing or not allowing reentrancy on it. The reason for this implementation was to follow the current implementation were it's the call itself that specifies whether it's reentrant or not, having to decide if it's reentrant or not in the previous call on the stack seems not to fit well with the current design if we want a simple implementation that is not a maintenance burden.

I still think that anyways the base of the CAP should be this, i.e having a separate function that indicates whether the call can or cannot be re-entrant. I think a good way to avoid A--(call)->B--(reentrant_call)-->A is to simply add a condition linker-level. Since with streamlined linking we're going over each of the module's imports, we can easily catch in contract B (the one which performs the reentrant call) is indeed using reentry, so if contract A has in some way specified that they do not want to allow re-entry the context for B won't even link. There are many ways where A can infer to the vm that they don't want reentrant calls: custom section (not sure it's the right idea though), extra dedicated import (e.g a hf such as enable_reentry, I actually think this might be the way), or an instance level configuration (I also don't think this is the right idea).

Again, I think that a sound impl could be to keep the current reentrant mode as is and have the instantiation decide whether the called contracts are safe or not given the current configs.

gaunt fog
# gaunt fog > ContractReentryMode is not well specified and not well tested. I'm not sure if...

e.g a hf such as enable_reentry, I actually think this might be the way
...
and have the instantiation decide whether the called contracts are safe or not given the current configs

About this, it could either be a binary level flag where it's the instantiation that decides if the "B contract" is safe, or a new field in the context that can be switched on and off. Option 1 is likely faster, but 2 is more customizable.

#

we still need a CAP though.

Happy to write one, just opened the discussion first to see if it was worth it to bring forward as a CAP

gaunt fog
#

actually, let me try and add this to the pr and see how it pans out

gaunt fog
#

Ok I restructued a bit how the instantiation works, mainly:

  1. a new reentry_guard parameter is needed when instantiating the vm. This will specify whether the host should make sure that the reentry is enabled before linking potentially dangerous funcitons (i.e d 1 and d 2). So for contract the guard is not needed. But for calls creating the linker looks at a new field in the store context (enable_reentrant).
  2. a new host function was added to arbitrarily set enable_reentrant
#

Practically, this means that the caller contract will look like this to enable reentry:

#![no_std]
use soroban_sdk::{contract, contractimpl, Env, Address, Symbol, TryIntoVal, Vec, Val};

#[link(wasm_import_module = "d")]
extern "C" {
    #[allow(improper_ctypes)]
    #[link_name = "_"]
    pub fn call_contract(contract: i64, func: i64, args: i64, ) -> i64;

    #[allow(improper_ctypes)]
    #[link_name = "3"]
    pub fn set_reentrant(enabled: i64, ) -> i64;
}

#[contract]
pub struct Contract;

#[contractimpl]
impl Contract {
    pub fn test_reentry(env: Env, called: Address) {
        // ...

        let set_reentrant_flag = Val::from_bool(true).as_val().get_payload() as i64;
        
        unsafe {
            set_reentrant(set_reentrant_flag);
            call_contract(called_val, func_val, args_val);
        };
    }

    // ...
}
neat wave
#

IMO it should not be up to the caller if something can/should be reentrant or not, it should be up to the developer.

The way I would expect something like this to work, is as a contract developer, I can choose to allow reentrant calls on a given function. If any reentrant call is made on that function, it works, and any reentrant call made to any other function would fail.

IE:

#[contract]
pub struct Contract;

#[contractimpl]
impl Contract {
    #[reentrant]
    pub fn my_reentrant_func(stuff) {}
}

This is also slightly more in-line with other smart contract frameworks, where functions are explicitly opt-out of reentrancy via some reentrancy guard

function myFunc(stuff) external nonReentrant { }
vital merlin
#

The implementation works correctly,
it doesn't work correctly until we specify what 'correctly' means

. The reason for this implementation was to follow the current implementation
I think the current implementation only makes sense for prohibiting reentrance. we haven't put much thought into allowed mode, and after giving it a bit more thought it does seem like we need to change the implementation.

is to simply add a condition linker-level.
I'm not sure I agree. I think a safe default is 'don't allow to reenter me, unless I said otherwise'. doing it the opposite way is a massive footgun and even the footgun aside, it changes the protocol behavior for the existing contracts unexpectedly, which is what we generally want to avoid.

#

before we jump into implementation details, I want to understand if there is anything wrong with my initial suggestion, i.e. making it so that call_reentrant from a contract allows reentry into this specific contract from any call down the stack (but not up the stack). it might seem limiting, but OTOH I struggle to see why is it ever safe to allow reentry into contracts that you don't own. for additional safety we could also mark the functions as supporting/not supporting reentry. what I'm interested in is the cases where this is not sufficient

#

I think it would be generally useful to identify the 'legitimate' reentrance scenarios in terms of the call stacks. this will help us figuring out the requirements and thus the design.
but I think a general requirement should be that it is not possible to reenter any existing contract on-chain.

vital merlin
#

the option in the PR where we enable reentry explicitly is also fine (we don't even need the new host fns for calls in that case), but again, this should be limited to reentry into the current contract

gaunt fog
#

@vital merlin @neat wave just for clarification the current implementation in the pr makes it so that:

  • by default, contracts opt out of reentrancy. The change won’t affect any existing contracts on chain.
  • it’s the developer that will allow reentrancy (I.e sdk wise it could be a macro as @neat wave suggested)

What in particular about the current proposed is that you’d change? This looks quite simple to me: there are functions that can be reentrant, and the caller can arbitrarily decide if they want to allow such functions to be even linked during a directly adjacent cross contract call. Is the linker condition a bit too prohibiting?

vital merlin
#

well, yeah, I'm not sure if messing with the linker is necessary. but even before we get into that, I'm not sure I understand the semantics. e.g. imagine A->B->A call stack. contract B doesn't have to worry about reentrancy here, does it? basically what I'm suggesting is that contract A has to explicitly allow the reentrance into itself from the call it does, or in the limited scope (if we go enable/disable reentrance route). I don't think any contract should have a possibility to globally control the reentrance, but OTOH it should be possible for a contract to reenter itself even from several calls deep that don't necessarily know about reentrance

gaunt fog
#

Ah so you’re suggesting that the reentrancy should be enabled in general for contract a if they want to, even if it’s deeper down the stack? (I.e A allows reentrancy, calls B, b calls C, c calls A should be allowed) And also not having contract be use special host functions for their reentrant call?

vital merlin
#

basicallyh in order to reenter into any given contract A, you need to have it somewhere up the stack (or it's not reentrance), thus why not benefit from that and not make the reentrance switch at runtime? again, function marking is ok in addition to that or as the main SDK interface. the runtime switch is more flexible though, e.g. you may allow something like specifically reentering function A::foo from specifically a certain code path of A::bar

gaunt fog
#

Fwiw in current pr a contract cannot contro the reentrancy globally, only if the call next down the stack can perform reentrancy

vital merlin
#

even if it’s deeper down the stack? (
well, we kind of need that don't we? otherwise we're limited to self reentrance, or reentrance with a single hop, which seems generally limiting. I think if you expect reentrance it shouldn't matter from where does it happen. I could be wrong though, which is why I'm looking for some examples. and also we can make things very granular if necessary, e.g. we can allow reentrance up to a certain depth.

And also not having contract be use special host functions for their reentrant call?
that's just an option; in either case we need 2 new host fns. the switch has a benefit of extending the reentrance-enabled scope past a single call, but ultimately it's a minor implementation detail.

only if the call next down the stack can perform reentrancy
sure, but the issue is that it can perform reentrancy into any contract up the stack, and that's a problem, as none of the contracts up the stack besides yours have approved reentrance

gaunt fog
vital merlin
#

and that's totally a flaw in the current env implementation

#

but the impl becomes more complex there since the call stack should have a record on whether the reentry is enabled for a certain contract
that's ok, we can handle that

gaunt fog
#

Yeah multi hop reentry does seem better

#

Yeah also true about being able to reentry on other contracts in the stack

#

Changing the call stack to hold the guard flag shouldn’t actually be much of an impl burden tbh

vital merlin
#

I think we really need to gather some requirements. I think we can be incredibly fancy and fine-grained in terms of reentrance control. like we could go as far as explicitly defining the expected call stack, similarly to what env.authorize_as_current_contract does for auth. I'm not sure if that has the best ergonomics though

gaunt fog
#

I like this idea tbh, ergonomics can always be handled by sdk most of the times

vital merlin
#

if we go for depth limit, I think the most useful options would likely be 1 (self-reentrance), 2 (1 hop) and infinity (any number of hops). anything in-between seems too convoluted to use in practice

#

I mean, the ergonmics issue is rather that you make your call stack rather static and dependent on implementation of the dowstream contracts (even in auth you can skip nodes without require_auth, e.g. for proxies).

gaunt fog
#

Why generalizing the infinity here though? Might have to be changed in the future

vital merlin
#

but maybe we could have both; exact call stack for when you know what's going on exactly and want to be as safe as possible, and depth limit/generic switch when you need something more dynamic

vital merlin
gaunt fog
#

How do we check that the call stack is respecting the downstream contract’s rules? Iirc there’s not way to relay a contexts field to another vm context in the current codebase

#

Oh nvm I believe that new vms are using the same host? I haven’t looked at that part of the code yet

gaunt fog
vital merlin
#

Oh nvm I believe that new vms are using the same host? I haven’t looked at that part of the code yet
yeah, host manages everything.

. I would also be fine with just option 2,
well, that's still up to debate, more options == more work, more edge cases etc. so I'd go with a minimal satisfactory option.

#

(and if it does involve both option, then we'd need to go with both, but otherwise I'd go with the simplest one)

#

FWIW I'll be out for the following couple of weeks, so someone else might want to chime in here, or maybe we could just continue when I'm back (there is plenty of time before p23)

gaunt fog
#

I think the best way is to be as granular as possible host-side, and then if we notice that guest having control over the stack is necessary we can add that on top of the existing host-driven stack-based reentrancy guard implementation.

gaunt fog
# vital merlin FWIW I'll be out for the following couple of weeks, so someone else might want t...

sure. Enjoy the time out of office! (assuming it's vacation). I think I can try taking a stab at thinking about the implementation to be stack-based guard if I can set aside some time.

Ideally, if the contract control the reentrancy from the stack we don't even need the two additional host functions unless the stack definition is to be added inside the call functions. I'd still not go full guest-side controlled granularity because it also means that we have to manage stack path conflicts unless we want to end up potentially executing un-needed instructions.

fathom timber
#

I left some comments on the PR https://github.com/stellar/rs-soroban-env/pull/1491.

Looking purely at the SDK layer, I don't think new call_* functions that allow for reentry will work that well in all situations.

When working with libraries a contract may wish to allow library code to be reentrant. The library code may make calls to other contracts without knowing they need to be reentrant or not. So the SDK functionality should allow for a scope of code to be specified as allowing reentry, instead of a single call.

An interface something like as follows would be most flexible:

env.allow_reentry(|| {
    // ...
    client.call(...);
    library::function(...);
    // ...
});

And the most efficient (I think) way to implement that will be with a set_reentrant(true) called at the start and a set_reentrant(false) called at the end. i.e. only add one new host function.

We technically could store state inside the env while inside allow_reentry so that any client created knows reentry is allowed, and then the client would call a different function, but this would introduce a new branch into every client, and would cause env to be a non-zero sized value which would I think increase the size of most programs.

gaunt fog
#

@fathom timber yeah the PR needs lots of work still. I think I agree with your proposed sdk interface. The duplicate fn is not be needed if we switch to context-dependant reentry configs as the function that allows reentry is enough to mark sdk-level that a binary is using reentrancy.

#

wdyt about marking sdk-levlel in the contractimport! macro if the called contract is using reentrancy? I feel like it might be a good precaution, but is definitely not too informative:

  1. the contract might be reentrant on other contracts.
  2. the imported contract might be calling contracts that are reentrant.

so I feel it might be a bit redundant.

fathom timber
# gaunt fog wdyt about marking sdk-levlel in the `contractimport!` macro if the called contr...

wdyt about marking sdk-levlel in the contractimport! macro if the called contract is using reentrancy?
There's a couple places something like that could be surfaced.

Could be surfaced there on the contractimport!, explicitly. An importer could be required to say allow-reentry = true on the import. It's also a little odd, because the imports are used for a variety of things, like testing and deploying, where this matters a lot less.

Another place would be a contract audit command on the stellar-cli that folks could use to inspect wasms. Or maybe an auditing linter tool of sorts (anybody?). Or a cargo-deny-like linter of sorts that errors on the use of dependencies that carry a feature like reentry that aren't flagged in a separate file as being allowed.

vital merlin
#

the contract might be reentrant on other contracts.
not sure I understand what this means.
the imported contract might be calling contracts that are reentrant.
is that really an issue? the reentrant contracts should be responsible for their logic, right? while it might be useful to generally allow only 'curated' contract implementations to be called (like e.g. only interact with some well-known token impls), I think it's out of scope of the reentrancy discussion - I don't think it's any more special than e.g. bad checks or whatever other logical issues that might be present in downstream contracts

vital merlin
#

Or a cargo-deny-like linter of sorts that errors on the use of dependencies that carry a feature like reentry that aren't flagged in a separate file as being allowed.
let's say we guard the reentry env fn with a feature. can we ensure that libraries that are being imported have the feature disabled?

#

now that I think about this, passing env into libraries is generally risky. having any cross-contract calls from a library is risky (it can just transfer the contract balance anywhere)

gaunt fog
vital merlin
#

yeah, I'm not sure there is a feasible signaling mechanism and I'm not sure we need it at all

gaunt fog
#

agreed

sly tapir
#

Apologies for the late input. A key question is whether reentrancy is truly needed; many ecosystems, like Move chains or Solana, have been performing just fine without it, and there hasn't been such a need.

If reentrancy is required, a possible approach could be using macros to mark contracts - or even specific functions for more flexibility - as reentrant, rather than introducing different call types. Imo this is easier to understand as an opt-in pattern and it also reduces blind spots where devs could make wrong assumptions, as a contract/function is either reentrant or not based on the macros.

vital merlin
#

there are cases where reentrance simplifies things (I can't quickly find the thread, but there are was at least one protocol where reentrance was beneficial, even if not mandatory). generally it doesn't seem like having it as opt-in would hurt, as long as the feature doesn't complicate the protocol.
limiting the scope of reentrance is IMO much safer than a blanket 'reentrant function' annotation. e.g. reentrance from require_auth doesn't seem too useful, but is a footgun that's easy to miss even if you think you know what you're doing.

fathom timber
#

🗣️ Opened a discussion to collect use cases for reentry: stellar-protocol#1594. Please post use cases there so we have a reference point with all the use cases and then everyone exploring the topic has a better understanding of the need.

GitHub
Use cases for reentry · stellar stellar-protocol · Discussion #1594

Hi all! There's been some discussion in Discord and elsewhere about reentry. It'd be helpful to have a good understanding of how reentry would be used if the feature was added. Please post ...