contracts: ReentranceDenied shouldn't trap the caller but return an error code from seal_call

[xgreenx]

At the moment if one of the sub-contract doesn't allow reentrance it throws a ReentranceDenied that breaks the execution of the full call stack.

But it is the same level of error as CalleeTrapped. So we don't need to break all execution, we can return CalleeReentranceDenied and the caller can decide what to do with that error.

[bkchr]

CC @athei

[athei]

My rationale was that basically any case of unintended re-entrancy would constitute an attack or at least a bug. This is why I opted to revert the call stack instead of returning an error.

Do you think it there is a use case where you actually want to handle this error?

[xgreenx]

Do you think it there is a use case where you actually want to handle this error?

Yep, for example:

During the transfer of fungible tokens, the PSP22 calls the before_received on the callee contract to be sure that he is expecting or not expecting some tokens. If the callee contract implements that method and returns PSP22ReceiverError::TransferRejected -> he doesn't expect any tokens. But if the result of execution is NotCallable or CalleeReentranceDenied then the callee contract doesn't implement that method and it is a positive scenario and we can transfer tokens.

[athei]

Why would you want to allow transfer of token when you get CalleeReentranceDenied? When you get this error it could very well be that the callee implements before_received with a PSP22ReceiverError::TransferRejected but never got the chance to execute it.

[xgreenx]

Why would you want to allow transfer of token when you get CalleeReentranceDenied? When you get this error it could very well be that the callee implements before_received with a PSP22ReceiverError::TransferRejected but never got the chance to execute it.

I agree with you that CalleeReentranceDenied means two cases:

• before_received is implemented and the developer of the contract forgot to allow reentrancy during the cross-contract call.
• before_received is not implemented and the developer didn't plan to allow reentrancy for the PSP22Receiver trait.

The idea of the PSP22Receiver trait is that if you plan to reject the transfer of unexpected tokens, you should say about it via PSP22ReceiverError::TransferRejected. For that you need to worry about reentrancy because it is a common case where:

• Someone calls contract A
• Contract A does a PSP22::tranfer_from from caller to itself
• PSP22 calls PSP22Receiver::before_receive of the contract A
• In most cases PSP22Receiver is not implemented and it was not planned to allow reentrancy, it is why we want to make it the default flow.

[athei]

In most cases PSP22Receiver is not implemented and it was not planned to allow reentrancy, it is why we want to make it the default flow.

I don't think it is sound to do that. As already discussed: You get exactly zero information whether tokens should be received or not. You should not do the transfer in this case.

This is clearly a case where you want re-entrancy. So why not call transfer_from with re-entrancy enabled?

[xgreenx]

This is clearly a case where you want re-entrancy. So why not call transfer_from with re-entrancy enabled?

Because that means that if I don't plan to implement PSP22Receiver I still should allow reentrancy to return the CalleeTraped or NotCallable. If I allow it I need to handle cases of reentrancy in other functions of my contract. But I don't want to do it because I only need a simple transfer of tokens I don't plan to fight with reentrancy=) (I describe the use cases from the developer side).

It is the most common case.

The problem is that if the contract doesn't implement PSP22Receiver the execution will fail with ReentrancyDenied and it is not fair. It is okay to throw a ReentrancyDenied error if PSP22Receiver is implemented and reentrancy is not allowed.

[athei]

If I allow it I need to handle cases of reentrancy in other functions of my contract. But I don't want to do it because I only need a simple transfer of tokens I don't plan to fight with reentrancy=)

If you want to send yourself tokens you know that you need to call the PSP22 contract with reentrancy enabled. I see no problem with that. If you don't do that you just have a bug in your contract that you will fix. Cause you know you should test contracts before deploying them to production. Not transferring the funds is a sound default for that kind of programming error.

It is okay to throw a ReentrancyDenied error if PSP22Receiver is implemented and reentrancy is not allowed.

But you can't know if PSP22Receiver is implemented with disabled reentrancy.

[xgreenx]

If you want to send yourself tokens you know that you need to call the PSP22 contract with reentrancy enabled. I see no problem with that

The problem is that I don't plan to implement PSP22Receiver but I still need to allow reentrancy to not fail the whole execution. Allowing reentrancy forcing me to worry about it in other functions and so on. But okay, no matter=)

The idea of the issue is that this error can be handled by the caller contract and the caller decides how to process it. In most cases, it will be CallerTrapped because of unwrap. But if the developer wants to handle it I think it is better to provide that ability=)

But you can't know if PSP22Receiver is implemented with disabled reentrancy.

Will be cool to pass reentrancy_state into the callee contract(via seal_reentrancy_state for example) and decide about allowance on contract level instead(like for payment). In that case the user can specify #[ink(message, reentrancy = true)]

[athei]

The problem is that I don't plan to implement PSP22Receiver but I still need to allow reentrancy to not fail the whole execution. Allowing reentrancy forcing me to worry about it in other functions and so on. But okay, no matter=)

Yeah we digressed a bit. I just had the feeling you intent to use that feature in an unsafe way. But I am not opposed to make this error non fatal.

Will be cool to pass reentrancy_state into the callee contract(via seal_reentrancy_state for example) and decide about allowance on contract level instead(like for payment). In that case the user can specify #[ink(message, reentrancy = true)]

I don't fully get what you mean. But shouldn't be discussed here. If you have a good idea there please open another issue where you explain it fully :)

[xgreenx]

I don't fully get what you mean. But shouldn't be discussed here. If you have a good idea there please open another issue where you explain it fully :)

I describe it here because maybe it will cancel that issue.

At the moment contract-pallet controls the reentrancy logic. What do you think about the idea to move the control of that to the contract level?

The contract-palet can provide the seal_is_reentrancy method that return true if it is reentrancy call. Or maybe another information, for example, the deep of the reentrancy and something like this(we can discuss that).

The contract can call that method and decide by itself: panic, or not. It can be implemented in the ink! the same way as payable.

By default, reentrancy is not allowed(as payment for payable). But the developer can allow it and ink! will not include the check for reentrancy.

#[ink(message, payable, allow_reetrancy)]
pub fn deposit(&self) {
    // ...
}

[athei]

It is essentially the question who controls the reentrancy. In both cases it is the contract which is in control (by specifying the call flags). Caller vs Callee: Right now we have a caller controlled restriction (you specify it on call).

We can have of course both. Adding a seal_is_reentrant function should be easy enough.

[xgreenx]

We can have of course both. Adding a seal_is_reentrant function should be easy enough.

If the callee contract controls the reentrancy -> contract-pallet doesn't control it(doesn't throw a ReentranceDenied error) -> The caller contract can't control that because the final decision is up to the callee contract.

[athei]

Yeah I meant that "some contract" is in control. In that case it is the callee. Which is fine. We should support both cases.

[xgreenx]

So, what is our final decision regarding that issue?

For me, the most usable solution is:

1. Remove the ALLOW_REENTRY flag from CallFlags.
2. Remove the ReentranceDenied error.
3. Add new method seal_is_reentrant to contract-pallet.
4. On the ink side we introduce a new attribute allow_reentry. All methods in the contract by default throw ContractTrapped if seal_is_reentrant is true and the method is not marked as allow_reentry.

Do we need to add something?

If you want to have an additional way how to control it from the caller's side too(it is an additional way because the caller can use seal_is_reentrant to deny reentry), then we can rename ALLOW_REENTRY into DENY_REENTRY. In that case, reentry is allowed by default but if the developer wants, he can deny it.

[athei]

Remove the ReentranceDenied error.

Why would we remove that? There might be reasons the caller want to disallow reentrancy.

If you want to have an additional way how to control it from the caller's side too(it is an additional way because the caller can use seal_is_reentrant to deny reentry), then we can rename ALLOW_REENTRY into DENY_REENTRY. In that case, reentry is allowed by default but if the developer wants, he can deny it.

No need to rename anything here. ink! can just pass ALLOW_REENTRY it by default if necessary.

[xgreenx]

No need to rename anything here. ink! can just pass ALLOW_REENTRY it by default if necessary.

ink! can, but it will increase the size of the contract because each cross-contract call will specify that flag. If we agree that the default way to handle reentry is on the callee side, then better to adapt the whole flow for that. It means that reentrancy is allowed on the contract-pallet level by default because the callee contract decides is that allowed or not.

Remove the ReentranceDenied error.

I think it is better to remove that because caller == callee in the scope of reentry. ReentranceDenied is redundant here because:

If it is expected reentry, then the developer should mark the function with allow_reentry otherwise it fails. It doesn't make sense to mark it allow_reentry and enable ALLOW_REENTRY at the same time(If one of them is false the execution fails).

I see only a small sense to have DENY_REENTRY, but I can't imagine the use case at the moment and it is why I think that simpler will remove that from CallFlags at all.

It is more clear for the developer.

[athei]

ink! can, but it will increase the size of the contract because each cross-contract call will specify that flag.

Sorry but I am not pushing user space functionality to the pallet in order to save setting one bit in a bitfield. Somewhere a line needs to be drawn.

It means that reentrancy is allowed on the contract-pallet level by default because the callee contract decides is that allowed or not.

This is not about pallet vs. contract. It is about caller vs. callee. The pallet merely enforces the callers decision of not setting ALLOW_REENTRY. Just as it enforces the callees decision to trap if it detects that it is re entrant. I don't see how it follows from your argument that one is better than the other. Sometimes the caller wants to make sure that it won't be reentered.

There is a huge barrier to changing existing functionality because we need to be backwards compatible to existing contracts. If it is solvable in user space we solve it there.

[xgreenx]

Sorry but I am not pushing user space functionality to the pallet in order to save setting one bit in a bitfield. Somewhere a line needs to be drawn.

And it will not affect the size, I forgot that it is a bit=D

There is a huge barrier to changing existing functionality because we need to be backwards compatible to existing contracts. If it is solvable in user space we solve it there.

You are right. It can be changed on the ink! side. ALLOW_REENTRY will always be true and the user, if wants can off that flag via CallFlags::deny_reentry.

[athei]

I described my reasoning for having this error being fatal in my first reply. This discussion didn't convince me that we want to change that. Rather we should to these things:
#11082
use-ink/ink#1207