Skip to content

Commit

Permalink
Refactor: Mark functions that create promises on NEAR as unsafe (#617)
Browse files Browse the repository at this point in the history
  • Loading branch information
@birchmd
birchmd authored Oct 5, 2022
1 parent 0d72cfb commit c46339f
Show file tree
Hide file tree
Showing 7 changed files with 126 additions and 61 deletions.
44 changes: 21 additions & 23 deletions engine-sdk/src/near_runtime.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,10 @@ const CUSTODIAN_ADDRESS: &[u8] = &[
132, 168, 43, 179, 156, 131, 152, 157, 93, 192, 126, 19, 16, 40, 25, 35, 210, 84, 77, 194,
];

macro_rules! unsafe_feature_gated {
macro_rules! feature_gated {
($feature_name:literal, $code:block) => {
if cfg!(feature = $feature_name) {
unsafe { $code }
$code
} else {
unimplemented!("Not implemented without feature {}", $feature_name)
}
Expand Down Expand Up @@ -292,14 +292,14 @@ impl crate::promise::PromiseHandler for Runtime {
}
}

fn promise_create_call(&mut self, args: &PromiseCreateArgs) -> PromiseId {
unsafe fn promise_create_call(&mut self, args: &PromiseCreateArgs) -> PromiseId {
let account_id = args.target_account_id.as_bytes();
let method_name = args.method.as_bytes();
let arguments = args.args.as_slice();
let amount = args.attached_balance.as_u128();
let gas = args.attached_gas.as_u64();

let id = unsafe {
let id = {
exports::promise_create(
account_id.len() as _,
account_id.as_ptr() as _,
Expand All @@ -314,7 +314,7 @@ impl crate::promise::PromiseHandler for Runtime {
PromiseId::new(id)
}

fn promise_attach_callback(
unsafe fn promise_attach_callback(
&mut self,
base: PromiseId,
callback: &PromiseCreateArgs,
Expand All @@ -325,7 +325,7 @@ impl crate::promise::PromiseHandler for Runtime {
let amount = callback.attached_balance.as_u128();
let gas = callback.attached_gas.as_u64();

let id = unsafe {
let id = {
exports::promise_then(
base.raw(),
account_id.len() as _,
Expand All @@ -342,36 +342,34 @@ impl crate::promise::PromiseHandler for Runtime {
PromiseId::new(id)
}

fn promise_create_batch(&mut self, args: &PromiseBatchAction) -> PromiseId {
unsafe fn promise_create_batch(&mut self, args: &PromiseBatchAction) -> PromiseId {
let account_id = args.target_account_id.as_bytes();

let id = unsafe {
exports::promise_batch_create(account_id.len() as _, account_id.as_ptr() as _)
};
let id = { exports::promise_batch_create(account_id.len() as _, account_id.as_ptr() as _) };

for action in args.actions.iter() {
match action {
PromiseAction::CreateAccount => unsafe {
PromiseAction::CreateAccount => {
exports::promise_batch_action_create_account(id);
},
PromiseAction::Transfer { amount } => unsafe {
}
PromiseAction::Transfer { amount } => {
let amount = amount.as_u128();
exports::promise_batch_action_transfer(id, &amount as *const u128 as _);
},
PromiseAction::DeployContract { code } => unsafe {
}
PromiseAction::DeployContract { code } => {
let code = code.as_slice();
exports::promise_batch_action_deploy_contract(
id,
code.len() as _,
code.as_ptr() as _,
);
},
}
PromiseAction::FunctionCall {
name,
gas,
attached_yocto,
args,
} => unsafe {
} => {
let method_name = name.as_bytes();
let arguments = args.as_slice();
let amount = attached_yocto.as_u128();
Expand All @@ -384,9 +382,9 @@ impl crate::promise::PromiseHandler for Runtime {
&amount as *const u128 as _,
gas.as_u64(),
)
},
}
PromiseAction::Stake { amount, public_key } => {
unsafe_feature_gated!("all-promise-actions", {
feature_gated!("all-promise-actions", {
let amount = amount.as_u128();
let pk: RawPublicKey = public_key.into();
let pk_bytes = pk.as_bytes();
Expand All @@ -399,7 +397,7 @@ impl crate::promise::PromiseHandler for Runtime {
});
}
PromiseAction::AddFullAccessKey { public_key, nonce } => {
unsafe_feature_gated!("all-promise-actions", {
feature_gated!("all-promise-actions", {
let pk: RawPublicKey = public_key.into();
let pk_bytes = pk.as_bytes();
exports::promise_batch_action_add_key_with_full_access(
Expand All @@ -417,7 +415,7 @@ impl crate::promise::PromiseHandler for Runtime {
receiver_id,
function_names,
} => {
unsafe_feature_gated!("all-promise-actions", {
feature_gated!("all-promise-actions", {
let pk: RawPublicKey = public_key.into();
let pk_bytes = pk.as_bytes();
let allowance = allowance.as_u128();
Expand All @@ -437,7 +435,7 @@ impl crate::promise::PromiseHandler for Runtime {
});
}
PromiseAction::DeleteKey { public_key } => {
unsafe_feature_gated!("all-promise-actions", {
feature_gated!("all-promise-actions", {
let pk: RawPublicKey = public_key.into();
let pk_bytes = pk.as_bytes();
exports::promise_batch_action_delete_key(
Expand All @@ -448,7 +446,7 @@ impl crate::promise::PromiseHandler for Runtime {
});
}
PromiseAction::DeleteAccount { beneficiary_id } => {
unsafe_feature_gated!("all-promise-actions", {
feature_gated!("all-promise-actions", {
let beneficiary_id = beneficiary_id.as_bytes();
exports::promise_batch_action_delete_key(
id,
Expand Down
31 changes: 24 additions & 7 deletions engine-sdk/src/promise.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,33 @@ pub trait PromiseHandler {
fn promise_results_count(&self) -> u64;
fn promise_result(&self, index: u64) -> Option<PromiseResult>;

fn promise_create_call(&mut self, args: &PromiseCreateArgs) -> PromiseId;
fn promise_attach_callback(
/// # Safety
/// Creating calls to other contracts using the Engine account is dangerous because
/// it has special admin privileges (especially with itself), for example minting
/// bridged tokens. Therefore, this function must be used with extreme caution to prevent
/// security vulnerabilities. In particular, it must not be possible for users to execute
/// arbitrary calls using the Engine.
unsafe fn promise_create_call(&mut self, args: &PromiseCreateArgs) -> PromiseId;

/// # Safety
/// See note on `promise_create_call`.
unsafe fn promise_attach_callback(
&mut self,
base: PromiseId,
callback: &PromiseCreateArgs,
) -> PromiseId;
fn promise_create_batch(&mut self, args: &PromiseBatchAction) -> PromiseId;

/// # Safety
/// See note on `promise_create_call`. Promise batches in particular must be used very
/// carefully because they can take destructive actions such as deploying new contract
/// code or adding/removing access keys.
unsafe fn promise_create_batch(&mut self, args: &PromiseBatchAction) -> PromiseId;

fn promise_return(&mut self, promise: PromiseId);

fn promise_create_with_callback(&mut self, args: &PromiseWithCallbackArgs) -> PromiseId {
/// # Safety
/// See note on `promise_create_call`.
unsafe fn promise_create_with_callback(&mut self, args: &PromiseWithCallbackArgs) -> PromiseId {
let base = self.promise_create_call(&args.base);
self.promise_attach_callback(base, &args.callback)
}
Expand Down Expand Up @@ -69,19 +86,19 @@ impl PromiseHandler for Noop {
None
}

fn promise_create_call(&mut self, _args: &PromiseCreateArgs) -> PromiseId {
unsafe fn promise_create_call(&mut self, _args: &PromiseCreateArgs) -> PromiseId {
PromiseId::new(0)
}

fn promise_attach_callback(
unsafe fn promise_attach_callback(
&mut self,
_base: PromiseId,
_callback: &PromiseCreateArgs,
) -> PromiseId {
PromiseId::new(0)
}

fn promise_create_batch(&mut self, _args: &PromiseBatchAction) -> PromiseId {
unsafe fn promise_create_batch(&mut self, _args: &PromiseBatchAction) -> PromiseId {
PromiseId::new(0)
}

Expand Down
6 changes: 3 additions & 3 deletions engine-standalone-storage/src/promise.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,19 +29,19 @@ impl<'a> PromiseHandler for NoScheduler<'a> {
Some(result)
}

fn promise_create_call(&mut self, _args: &PromiseCreateArgs) -> PromiseId {
unsafe fn promise_create_call(&mut self, _args: &PromiseCreateArgs) -> PromiseId {
PromiseId::new(0)
}

fn promise_attach_callback(
unsafe fn promise_attach_callback(
&mut self,
_base: PromiseId,
_callback: &PromiseCreateArgs,
) -> PromiseId {
PromiseId::new(0)
}

fn promise_create_batch(&mut self, _args: &PromiseBatchAction) -> PromiseId {
unsafe fn promise_create_batch(&mut self, _args: &PromiseBatchAction) -> PromiseId {
PromiseId::new(0)
}

Expand Down
6 changes: 3 additions & 3 deletions engine-test-doubles/src/promise.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,14 @@ impl PromiseHandler for PromiseTracker {
self.promise_results.get(index as usize).cloned()
}

fn promise_create_call(&mut self, args: &PromiseCreateArgs) -> PromiseId {
unsafe fn promise_create_call(&mut self, args: &PromiseCreateArgs) -> PromiseId {
let id = self.take_id();
self.scheduled_promises
.insert(id, PromiseArgs::Create(args.clone()));
PromiseId::new(id)
}

fn promise_attach_callback(
unsafe fn promise_attach_callback(
&mut self,
base: PromiseId,
callback: &PromiseCreateArgs,
Expand All @@ -66,7 +66,7 @@ impl PromiseHandler for PromiseTracker {
PromiseId::new(id)
}

fn promise_create_batch(&mut self, args: &PromiseBatchAction) -> PromiseId {
unsafe fn promise_create_batch(&mut self, args: &PromiseBatchAction) -> PromiseId {
let id = self.take_id();
self.scheduled_promises
.insert(id, PromiseArgs::Batch(args.clone()));
Expand Down
22 changes: 17 additions & 5 deletions engine/src/engine.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1408,10 +1408,19 @@ where
if log.topics.is_empty() {
if let Ok(promise) = PromiseArgs::try_from_slice(&log.data) {
match promise {
PromiseArgs::Create(promise) => schedule_promise(handler, &promise),
PromiseArgs::Create(promise) => {
// Safety: this promise creation is safe because it does not come from
// users directly. The exit precompiles only create promises which we
// are able to execute without violating any security invariants.
unsafe { schedule_promise(handler, &promise) }
}
PromiseArgs::Callback(promise) => {
let base_id = schedule_promise(handler, &promise.base);
schedule_promise_callback(handler, base_id, &promise.callback)
// Safety: This is safe because the promise data comes from our own
// exit precompiles. See note above.
unsafe {
let base_id = schedule_promise(handler, &promise.base);
schedule_promise_callback(handler, base_id, &promise.callback)
}
}
PromiseArgs::Recursive(_) => {
unreachable!("Exit precompiles do not produce recursive promises")
Expand Down Expand Up @@ -1452,7 +1461,10 @@ where
.collect()
}

fn schedule_promise<P: PromiseHandler>(handler: &mut P, promise: &PromiseCreateArgs) -> PromiseId {
unsafe fn schedule_promise<P: PromiseHandler>(
handler: &mut P,
promise: &PromiseCreateArgs,
) -> PromiseId {
sdk::log!(&crate::prelude::format!(
"call_contract {}.{}",
promise.target_account_id,
Expand All @@ -1461,7 +1473,7 @@ fn schedule_promise<P: PromiseHandler>(handler: &mut P, promise: &PromiseCreateA
handler.promise_create_call(promise)
}

fn schedule_promise_callback<P: PromiseHandler>(
unsafe fn schedule_promise_callback<P: PromiseHandler>(
handler: &mut P,
base_id: PromiseId,
promise: &PromiseCreateArgs,
Expand Down
36 changes: 25 additions & 11 deletions engine/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -603,7 +603,10 @@ mod contract {
.sdk_unwrap()
.deposit(raw_proof, current_account_id, predecessor_account_id)
.sdk_unwrap();
let promise_id = io.promise_create_with_callback(&promise_args);
// Safety: this call is safe because it comes from the eth-connector, not users.
// The call is to verify the user-supplied proof for the deposit, with `finish_deposit`
// as a callback.
let promise_id = unsafe { io.promise_create_with_callback(&promise_args) };
io.promise_return(promise_id);
}

Expand Down Expand Up @@ -640,7 +643,10 @@ mod contract {
.sdk_unwrap();

if let Some(promise_args) = maybe_promise_args {
let promise_id = io.promise_create_with_callback(&promise_args);
// Safety: this call is safe because it comes from the eth-connector, not users.
// The call will be to the Engine's ft_transfer_call`, which is needed as part
// of the bridge flow (if depositing ETH to an Aurora address).
let promise_id = unsafe { io.promise_create_with_callback(&promise_args) };
io.promise_return(promise_id);
}
}
Expand Down Expand Up @@ -757,7 +763,9 @@ mod contract {
io.prepaid_gas(),
)
.sdk_unwrap();
let promise_id = io.promise_create_with_callback(&promise_args);
// Safety: this call is safe. It is required by the NEP-141 spec that `ft_transfer_call`
// creates a call to another contract's `ft_on_transfer` method.
let promise_id = unsafe { io.promise_create_with_callback(&promise_args) };
io.promise_return(promise_id);
}

Expand All @@ -772,7 +780,9 @@ mod contract {
.storage_deposit(predecessor_account_id, amount, args)
.sdk_unwrap();
if let Some(promise) = maybe_promise {
io.promise_create_batch(&promise);
// Safety: This call is safe. It is only a transfer back to the user in the case
// that they over paid for their deposit.
unsafe { io.promise_create_batch(&promise) };
}
}

Expand All @@ -787,7 +797,8 @@ mod contract {
.storage_unregister(predecessor_account_id, force)
.sdk_unwrap();
if let Some(promise) = maybe_promise {
io.promise_create_batch(&promise);
// Safety: This call is safe. It is only a transfer back to the user for their deposit.
unsafe { io.promise_create_batch(&promise) };
}
}

Expand Down Expand Up @@ -941,12 +952,15 @@ mod contract {
attached_balance: ZERO_ATTACHED_BALANCE,
attached_gas: GAS_FOR_FINISH,
};
io.promise_create_with_callback(
&aurora_engine_types::parameters::PromiseWithCallbackArgs {
base: verify_call,
callback: finish_call,
},
);
// Safety: this call is safe because it is only used in integration tests.
unsafe {
io.promise_create_with_callback(
&aurora_engine_types::parameters::PromiseWithCallbackArgs {
base: verify_call,
callback: finish_call,
},
)
};
}

///
Expand Down
Loading

0 comments on commit c46339f

Please sign in to comment.