From 864645e9b04d4becede22b9d9b3888ab5a60458e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Costin=20Caraba=C8=99?= Date: Mon, 7 Oct 2024 23:39:01 +0300 Subject: [PATCH 1/4] paymaster: Audit fixes --- contracts/paymaster/src/forward_call.rs | 1 - contracts/paymaster/src/paymaster.rs | 20 ++++++++++++++++---- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/contracts/paymaster/src/forward_call.rs b/contracts/paymaster/src/forward_call.rs index c0efa34d..f516b0e2 100644 --- a/contracts/paymaster/src/forward_call.rs +++ b/contracts/paymaster/src/forward_call.rs @@ -30,7 +30,6 @@ pub trait ForwardCall { original_caller: ManagedAddress, #[call_result] result: ManagedAsyncCallResult>, ) -> MultiValueEncoded { - // TODO: use ManagedGetBackTransfers once rc1.6 is activated let back_transfers = self.blockchain().get_back_transfers(); // Send the original input tokens back to the original caller diff --git a/contracts/paymaster/src/paymaster.rs b/contracts/paymaster/src/paymaster.rs index 19c02be3..a458aac5 100644 --- a/contracts/paymaster/src/paymaster.rs +++ b/contracts/paymaster/src/paymaster.rs @@ -4,13 +4,16 @@ use multiversx_sc::imports::*; pub mod forward_call; pub mod paymaster_proxy; -const FEE_PAYMENT: usize = 0; +const FEE_PAYMENT_INDEX: usize = 0; #[multiversx_sc::contract] pub trait PaymasterContract: forward_call::ForwardCall { #[init] fn init(&self) {} + #[upgrade] + fn upgrade(&self) {} + #[endpoint(forwardExecution)] #[payable("*")] fn forward_execution( @@ -20,21 +23,30 @@ pub trait PaymasterContract: forward_call::ForwardCall { endpoint_name: ManagedBuffer, endpoint_args: MultiValueEncoded, ) { + let original_caller = self.blockchain().get_caller(); + let own_sc_address = self.blockchain().get_sc_address(); + let own_shard = self.blockchain().get_shard_of_address(&own_sc_address); + let original_caller_shard = self.blockchain().get_shard_of_address(&original_caller); + require!( + own_shard == original_caller_shard, + "Caller must be in the same shard" + ); + let payments = self.call_value().all_esdt_transfers(); require!(!payments.is_empty(), "There is no fee for payment!"); - let fee_payment = payments.get(FEE_PAYMENT); + let fee_payment = payments.get(FEE_PAYMENT_INDEX); self.tx() .to(&relayer_addr) .payment(EsdtTokenPayment::new( fee_payment.token_identifier, - 0, + fee_payment.token_nonce, fee_payment.amount, )) .transfer(); let mut payments_without_fee = payments.clone_value(); - payments_without_fee.remove(FEE_PAYMENT); + payments_without_fee.remove(FEE_PAYMENT_INDEX); self.forward_call(dest, endpoint_name, payments_without_fee, endpoint_args); } From 17e7101d06e5d6f0b31e9c0b16ddba2044b8c069 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Costin=20Caraba=C8=99?= Date: Wed, 9 Oct 2024 09:46:22 +0300 Subject: [PATCH 2/4] paymaster: add min_gas for execution --- .../interactor/src/interactor_main.rs | 8 ++++- contracts/paymaster/interactor/src/proxy.rs | 30 ++++++++++++++++--- contracts/paymaster/src/forward_call.rs | 6 ++++ contracts/paymaster/src/paymaster.rs | 9 +++++- contracts/paymaster/src/paymaster_proxy.rs | 30 ++++++++++++++++--- .../tests/paymaster_blackbox_test.rs | 6 ++++ contracts/paymaster/wasm/src/lib.rs | 4 ++- 7 files changed, 82 insertions(+), 11 deletions(-) diff --git a/contracts/paymaster/interactor/src/interactor_main.rs b/contracts/paymaster/interactor/src/interactor_main.rs index ab16d482..c32d4b34 100644 --- a/contracts/paymaster/interactor/src/interactor_main.rs +++ b/contracts/paymaster/interactor/src/interactor_main.rs @@ -162,7 +162,13 @@ impl ContractInteract { .to(self.state.current_address()) .gas(30_000_000u64) .typed(proxy::PaymasterContractProxy) - .forward_execution(relayer_addr, dest, endpoint_name, endpoint_args) + .forward_execution( + relayer_addr, + dest, + 1_000_000u64, + endpoint_name, + endpoint_args, + ) .payment(payments) .returns(ReturnsResultUnmanaged) .prepare_async() diff --git a/contracts/paymaster/interactor/src/proxy.rs b/contracts/paymaster/interactor/src/proxy.rs index 9bb7df68..6c4e298d 100644 --- a/contracts/paymaster/interactor/src/proxy.rs +++ b/contracts/paymaster/interactor/src/proxy.rs @@ -53,6 +53,25 @@ where } } +#[rustfmt::skip] +impl PaymasterContractProxyMethods +where + Env: TxEnv, + Env::Api: VMApi, + From: TxFrom, + To: TxTo, + Gas: TxGas, +{ + pub fn upgrade( + self, + ) -> TxTypedUpgrade { + self.wrapped_tx + .payment(NotPayable) + .raw_upgrade() + .original_result() + } +} + #[rustfmt::skip] impl PaymasterContractProxyMethods where @@ -65,19 +84,22 @@ where pub fn forward_execution< Arg0: ProxyArg>, Arg1: ProxyArg>, - Arg2: ProxyArg>, - Arg3: ProxyArg>>, + Arg2: ProxyArg, + Arg3: ProxyArg>, + Arg4: ProxyArg>>, >( self, relayer_addr: Arg0, dest: Arg1, - endpoint_name: Arg2, - endpoint_args: Arg3, + min_gas_limit: Arg2, + endpoint_name: Arg3, + endpoint_args: Arg4, ) -> TxTypedCall { self.wrapped_tx .raw_call("forwardExecution") .argument(&relayer_addr) .argument(&dest) + .argument(&min_gas_limit) .argument(&endpoint_name) .argument(&endpoint_args) .original_result() diff --git a/contracts/paymaster/src/forward_call.rs b/contracts/paymaster/src/forward_call.rs index f516b0e2..354f72c7 100644 --- a/contracts/paymaster/src/forward_call.rs +++ b/contracts/paymaster/src/forward_call.rs @@ -9,10 +9,16 @@ pub trait ForwardCall { fn forward_call( &self, dest: ManagedAddress, + min_gas_limit: u64, endpoint_name: ManagedBuffer, payments: PaymentsVec, endpoint_args: MultiValueEncoded, ) { + let gas_left = self.blockchain().get_gas_left(); + require!( + gas_left >= min_gas_limit, + "Minimum required gas not provided" + ); let original_caller = self.blockchain().get_caller(); self.tx() diff --git a/contracts/paymaster/src/paymaster.rs b/contracts/paymaster/src/paymaster.rs index a458aac5..11a909d8 100644 --- a/contracts/paymaster/src/paymaster.rs +++ b/contracts/paymaster/src/paymaster.rs @@ -20,6 +20,7 @@ pub trait PaymasterContract: forward_call::ForwardCall { &self, relayer_addr: ManagedAddress, dest: ManagedAddress, + min_gas_limit: u64, endpoint_name: ManagedBuffer, endpoint_args: MultiValueEncoded, ) { @@ -48,6 +49,12 @@ pub trait PaymasterContract: forward_call::ForwardCall { let mut payments_without_fee = payments.clone_value(); payments_without_fee.remove(FEE_PAYMENT_INDEX); - self.forward_call(dest, endpoint_name, payments_without_fee, endpoint_args); + self.forward_call( + dest, + min_gas_limit, + endpoint_name, + payments_without_fee, + endpoint_args, + ); } } diff --git a/contracts/paymaster/src/paymaster_proxy.rs b/contracts/paymaster/src/paymaster_proxy.rs index 9bb7df68..6c4e298d 100644 --- a/contracts/paymaster/src/paymaster_proxy.rs +++ b/contracts/paymaster/src/paymaster_proxy.rs @@ -53,6 +53,25 @@ where } } +#[rustfmt::skip] +impl PaymasterContractProxyMethods +where + Env: TxEnv, + Env::Api: VMApi, + From: TxFrom, + To: TxTo, + Gas: TxGas, +{ + pub fn upgrade( + self, + ) -> TxTypedUpgrade { + self.wrapped_tx + .payment(NotPayable) + .raw_upgrade() + .original_result() + } +} + #[rustfmt::skip] impl PaymasterContractProxyMethods where @@ -65,19 +84,22 @@ where pub fn forward_execution< Arg0: ProxyArg>, Arg1: ProxyArg>, - Arg2: ProxyArg>, - Arg3: ProxyArg>>, + Arg2: ProxyArg, + Arg3: ProxyArg>, + Arg4: ProxyArg>>, >( self, relayer_addr: Arg0, dest: Arg1, - endpoint_name: Arg2, - endpoint_args: Arg3, + min_gas_limit: Arg2, + endpoint_name: Arg3, + endpoint_args: Arg4, ) -> TxTypedCall { self.wrapped_tx .raw_call("forwardExecution") .argument(&relayer_addr) .argument(&dest) + .argument(&min_gas_limit) .argument(&endpoint_name) .argument(&endpoint_args) .original_result() diff --git a/contracts/paymaster/tests/paymaster_blackbox_test.rs b/contracts/paymaster/tests/paymaster_blackbox_test.rs index f35298d5..b61d6487 100644 --- a/contracts/paymaster/tests/paymaster_blackbox_test.rs +++ b/contracts/paymaster/tests/paymaster_blackbox_test.rs @@ -160,6 +160,7 @@ fn test_forward_call_no_fee_payment() { .forward_execution( RELAYER_ADDRESS_EXPR, CALLEE_USER_ADDRESS_EXPR, + 0u64, b"add", MultiValueVec::>::new(), ) @@ -181,6 +182,7 @@ fn test_forward_call_user() { .forward_execution( RELAYER_ADDRESS_EXPR, CALLEE_USER_ADDRESS_EXPR, + 0u64, b"add", MultiValueVec::>::new(), ) @@ -215,6 +217,7 @@ fn test_forward_call_sc_adder() { .forward_execution( RELAYER_ADDRESS_EXPR, CALLEE_SC_ADDER_ADDRESS_EXPR, + 0u64, b"add", MultiValueVec::from([top_encode_to_vec_u8_or_panic(&ADDITIONAL_ADD_VALUE)]), ) @@ -256,6 +259,7 @@ fn test_forward_call_sc_adder_with_relayer_address() { .forward_execution( RELAYER_ADDRESS_EXPR, CALLEE_SC_ADDER_ADDRESS_EXPR, + 0u64, b"add", MultiValueVec::from([top_encode_to_vec_u8_or_panic(&ADDITIONAL_ADD_VALUE)]), ) @@ -310,6 +314,7 @@ fn test_forward_call_wegld() { .forward_execution( RELAYER_ADDRESS_EXPR, CALLEE_SC_WEGLD_ADDRESS_EXPR, + 0u64, UNWRAP_ENDPOINT_NAME, MultiValueEncoded::new(), ) @@ -358,6 +363,7 @@ fn test_forward_call_fails_wegld_0_amount() { .forward_execution( RELAYER_ADDRESS_EXPR, CALLEE_SC_WEGLD_ADDRESS_EXPR, + 0u64, UNWRAP_ENDPOINT_NAME, MultiValueEncoded::new(), ) diff --git a/contracts/paymaster/wasm/src/lib.rs b/contracts/paymaster/wasm/src/lib.rs index c2d83536..3ef696be 100644 --- a/contracts/paymaster/wasm/src/lib.rs +++ b/contracts/paymaster/wasm/src/lib.rs @@ -5,9 +5,10 @@ //////////////////////////////////////////////////// // Init: 1 +// Upgrade: 1 // Endpoints: 1 // Async Callback: 1 -// Total number of exported functions: 3 +// Total number of exported functions: 4 #![no_std] @@ -18,6 +19,7 @@ multiversx_sc_wasm_adapter::endpoints! { paymaster ( init => init + upgrade => upgrade forwardExecution => forward_execution ) } From 5fd028601768ea8cd165984125fc17f88cc1348a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Costin=20Caraba=C8=99?= Date: Tue, 15 Oct 2024 14:23:17 +0300 Subject: [PATCH 3/4] paymaster: same shard fix --- contracts/paymaster/src/paymaster.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/contracts/paymaster/src/paymaster.rs b/contracts/paymaster/src/paymaster.rs index 11a909d8..17310059 100644 --- a/contracts/paymaster/src/paymaster.rs +++ b/contracts/paymaster/src/paymaster.rs @@ -24,13 +24,12 @@ pub trait PaymasterContract: forward_call::ForwardCall { endpoint_name: ManagedBuffer, endpoint_args: MultiValueEncoded, ) { - let original_caller = self.blockchain().get_caller(); let own_sc_address = self.blockchain().get_sc_address(); let own_shard = self.blockchain().get_shard_of_address(&own_sc_address); - let original_caller_shard = self.blockchain().get_shard_of_address(&original_caller); + let dest_shard = self.blockchain().get_shard_of_address(&dest); require!( - own_shard == original_caller_shard, - "Caller must be in the same shard" + own_shard == dest_shard, + "Destination must be in the same shard" ); let payments = self.call_value().all_esdt_transfers(); From 9c8ff9e22703c82db68221d8ec9a1dca4d35f47e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Costin=20Caraba=C8=99?= Date: Tue, 15 Oct 2024 14:43:25 +0300 Subject: [PATCH 4/4] paymaster: add require in different func --- contracts/paymaster/src/forward_call.rs | 14 +++++++++----- contracts/paymaster/src/paymaster.rs | 19 +++++++++++-------- 2 files changed, 20 insertions(+), 13 deletions(-) diff --git a/contracts/paymaster/src/forward_call.rs b/contracts/paymaster/src/forward_call.rs index 354f72c7..3e22d30d 100644 --- a/contracts/paymaster/src/forward_call.rs +++ b/contracts/paymaster/src/forward_call.rs @@ -14,11 +14,7 @@ pub trait ForwardCall { payments: PaymentsVec, endpoint_args: MultiValueEncoded, ) { - let gas_left = self.blockchain().get_gas_left(); - require!( - gas_left >= min_gas_limit, - "Minimum required gas not provided" - ); + self.require_min_gas_limit(min_gas_limit); let original_caller = self.blockchain().get_caller(); self.tx() @@ -66,4 +62,12 @@ pub trait ForwardCall { } } } + + fn require_min_gas_limit(&self, min_gas_limit: u64) { + let gas_left = self.blockchain().get_gas_left(); + require!( + gas_left >= min_gas_limit, + "Minimum required gas not provided" + ); + } } diff --git a/contracts/paymaster/src/paymaster.rs b/contracts/paymaster/src/paymaster.rs index 17310059..d17cea64 100644 --- a/contracts/paymaster/src/paymaster.rs +++ b/contracts/paymaster/src/paymaster.rs @@ -24,14 +24,7 @@ pub trait PaymasterContract: forward_call::ForwardCall { endpoint_name: ManagedBuffer, endpoint_args: MultiValueEncoded, ) { - let own_sc_address = self.blockchain().get_sc_address(); - let own_shard = self.blockchain().get_shard_of_address(&own_sc_address); - let dest_shard = self.blockchain().get_shard_of_address(&dest); - require!( - own_shard == dest_shard, - "Destination must be in the same shard" - ); - + self.require_dest_same_shard(&dest); let payments = self.call_value().all_esdt_transfers(); require!(!payments.is_empty(), "There is no fee for payment!"); @@ -56,4 +49,14 @@ pub trait PaymasterContract: forward_call::ForwardCall { endpoint_args, ); } + + fn require_dest_same_shard(&self, dest: &ManagedAddress) { + let own_sc_address = self.blockchain().get_sc_address(); + let own_shard = self.blockchain().get_shard_of_address(&own_sc_address); + let dest_shard = self.blockchain().get_shard_of_address(dest); + require!( + own_shard == dest_shard, + "Destination must be in the same shard" + ); + } }