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

allow_reentrancy modifier for messages #1285

Open
xgreenx opened this issue Jun 10, 2022 · 1 comment
Open

allow_reentrancy modifier for messages #1285

xgreenx opened this issue Jun 10, 2022 · 1 comment
Assignees

Comments

@xgreenx
Copy link
Collaborator

xgreenx commented Jun 10, 2022

To prevent developers from making reentrancy vulnerabilities in the contract ink! doesn't allow reentrancy during cross-contract calls by default. It is controlled via CallFlags.allow_reentry(false by default) during the creation of a cross-contract call(CallBuilder).

/// The flags used to change the behavior of a contract call.
#[must_use]
#[derive(Copy, Clone, Debug, Default)]
pub struct CallFlags {
    forward_input: bool,
    clone_input: bool,
    tail_call: bool,
    allow_reentry: bool,
}

If the contract already exists in the stack of calls, the contract-pallet immediately finishes executing the transaction and throws the ReentranceDenied error. The caller contract can't handle it, and it is hard to debug(discussion). Also, with that workflow, we can't have some functions that allow reentrancy and some that deny it.

To improve the usability and add control on which methods accept reentrancy and which do not, we want to introduce a new modifier for the message allow_reentrancy and change the default behavior of the CallFlags.

A new definition looks like:

/// The flags used to change the behavior of a contract call.
#[must_use]
#[derive(Copy, Clone, Debug, Default)]
pub struct CallFlags {
    forward_input: bool,
    clone_input: bool,
    tail_call: bool,
    deny_reentry: bool, // reverse behavior
}

So it is a breaking change because the allow_reentry function will be renamed to deny_reentry(We can keep empty allow_reentry, but better inform all users about the new API).

The allow_reentrancy behaves as a payable modifier but checks the output of seal_reentrant_count. All methods don't allow reentrancy by default. But the developer can mark some methods with the allow_reentrancy modifier to allow it.

/// Transfers `amount` from caller to `to`.
#[ink(message, allow_reentrancy)]
fn transfer(&mut self, to: AccountId, amount: Balance);

The implementation in codegen is the same as for payable so we need only repeat the logic.

@xgreenx
Copy link
Collaborator Author

xgreenx commented Jun 10, 2022

We plan to create a pull request into ink! after merging paritytech/substrate#11539.

Are we okay with a new API and behavior? The level of security is the same. The caller contract controls the reentrancy during the execution of the cross-contract call(if we enter again into the same contract) than during instantiating the cross-contract call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants