ink icon indicating copy to clipboard operation
ink copied to clipboard

`allow_reentrancy` modifier for messages

Open xgreenx opened this issue 2 years ago • 1 comments

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 avatar Jun 10 '22 14:06 xgreenx

We plan to create a pull request into ink! after merging https://github.com/paritytech/substrate/pull/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.

xgreenx avatar Jun 10 '22 14:06 xgreenx