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

Paymaster audit fixes #33

Merged
merged 16 commits into from
Nov 28, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions contracts/paymaster/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,6 @@ version = "0.43.4"

[dev-dependencies.adder]
path = "../adder"

[dev-dependencies.multiversx-wegld-swap-sc]
path = "../wegld-swap"
17 changes: 12 additions & 5 deletions contracts/paymaster/src/forward_call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ pub trait ForwardCall {
.with_callback(self.callbacks().transfer_callback(original_caller))
.call_and_exit();
}

#[callback]
fn transfer_callback(
&self,
Expand All @@ -33,18 +33,25 @@ pub trait ForwardCall {
let initial_payments = self.call_value().all_esdt_transfers();
CostinCarabas marked this conversation as resolved.
Show resolved Hide resolved

match result {
ManagedAsyncCallResult::Ok(return_values) => return_values,
ManagedAsyncCallResult::Ok(return_values) => {
// Send the resulted tokens to the original caller
if !initial_payments.is_empty() {
self.send()
.direct_multi(&original_caller, &initial_payments);
}

return_values
}
ManagedAsyncCallResult::Err(err) => {
// Send the original input tokens back to the original caller
if !initial_payments.is_empty() {
self.send()
.direct_multi(&original_caller, &initial_payments);
}

let mut err_result = MultiValueEncoded::new();
err_result.push(ManagedBuffer::new_from_bytes(ERR_CALLBACK_MSG));
err_result.push(err.err_msg.clone());

sc_print!("{}", err.err_msg);
err_result.push(err.err_msg);

err_result
}
Expand Down
1 change: 0 additions & 1 deletion contracts/paymaster/src/paymaster.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ multiversx_sc::imports!();
pub mod forward_call;
const FEE_PAYMENT: usize = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rename to FEE_PAYMENT_INDEX for more clarity.


/// An empty contract. To be used as a template when starting a new contract from scratch.
#[multiversx_sc::contract]
pub trait PaymasterContract: forward_call::ForwardCall {
#[init]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't add a comment on the unmodified code, but to have a safer approach regarding the fees in the forwardExecution endpoint, I would change the fee_payment nonce for the relayer_addr to fee_payment.token_nonce, instead of hardcoding the value 0, as the code would crash (without having a custom error) if the user would send an SFT. Now, if you want to allow only fungible tokens, do the check that the payment's nonce is 0. But still, I would keep the token_nonce approach.

Expand Down
127 changes: 118 additions & 9 deletions contracts/paymaster/tests/paymaster_blackbox_test.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use multiversx_sc::{
codec::{multi_types::MultiValueVec, top_encode_to_vec_u8_or_panic},
storage::mappers::SingleValue,
types::{Address, BigUint},
types::{Address, BigUint, MultiValueEncoded},
};
use multiversx_sc_scenario::{
api::StaticApi,
Expand All @@ -13,35 +13,42 @@ use multiversx_sc_scenario::{
};

use adder::ProxyTrait as _;
use multiversx_wegld_swap_sc::ProxyTrait as _;
use paymaster::ProxyTrait as _;

const PAYMASTER_ADDRESS_EXPR: &str = "sc:paymaster";
const RELAYER_ADDRESS_EXPR: &str = "address:relayer";
const CALLEE_SC_ADDER_ADDRESS_EXPR: &str = "sc:adder";
const CALLEE_SC_WEGLD_ADDRESS_EXPR: &str = "sc:wegld";
const PAYMASTER_PATH_EXPR: &str = "file:output/paymaster.wasm";
const ADDER_PATH_EXPR: &str = "file:tests/test-contracts/adder.wasm";
const WEGLD_PATH_EXPR: &str = "file:tests/test-contracts/multiversx-wegld-swap-sc.wasm.wasm";
const CALLER_ADDRESS_EXPR: &str = "address:caller";
const CALLEE_USER_ADDRESS_EXPR: &str = "address:callee_user";
const OWNER_ADDRESS_EXPR: &str = "address:owner";
const BALANCE: &str = "100,000,000";
const PAYMASTER_TOKEN_ID_EXPR: &str = "str:PAYMSTR-123456";
const WEGLD_TOKEN_ID_EXPR: &str = "str:WEGLD-123456";
const WEGLD_TOKEN_ID: &[u8] = b"WEGLD-123456";
const FEE_TOKEN_ID_EXPR: &str = "str:FEE-123456";
const ADDITIONAL_TOKEN_ID_EXPR: &str = "str:ADDIT-123456";
const FEE_AMOUNT: &str = "20,000";
const INITIAL_ADD_VALUE: u64 = 5;
const ADDITIONAL_ADD_VALUE: u64 = 5;


const UNWRAP_ENDPOINT_NAME: &[u8] = b"unwrap";

type PaymasterContract = ContractInfo<paymaster::Proxy<StaticApi>>;
type AdderContract = ContractInfo<adder::Proxy<StaticApi>>;
type WegldContract = ContractInfo<multiversx_wegld_swap_sc::Proxy<StaticApi>>;


fn world() -> ScenarioWorld {
let mut blockchain = ScenarioWorld::new();
blockchain.set_current_dir_from_workspace("contracts/paymaster");

blockchain.register_contract(PAYMASTER_PATH_EXPR, paymaster::ContractBuilder);
blockchain.register_contract(ADDER_PATH_EXPR, adder::ContractBuilder);
blockchain.register_contract(WEGLD_PATH_EXPR, multiversx_wegld_swap_sc::ContractBuilder);

blockchain
}
Expand All @@ -52,6 +59,7 @@ struct PaymasterTestState {
paymaster_contract: PaymasterContract,
relayer_address: Address,
callee_sc_adder_contract: AdderContract,
callee_sc_wegld_address: WegldContract,
}

impl PaymasterTestState {
Expand All @@ -66,6 +74,7 @@ impl PaymasterTestState {
.nonce(1)
.balance(BALANCE)
.esdt_balance(PAYMASTER_TOKEN_ID_EXPR, BALANCE)
.esdt_balance(WEGLD_TOKEN_ID_EXPR, BALANCE)
.esdt_balance(FEE_TOKEN_ID_EXPR, BALANCE)
.esdt_balance(ADDITIONAL_TOKEN_ID_EXPR, BALANCE),
)
Expand All @@ -81,14 +90,15 @@ impl PaymasterTestState {
let relayer_address = AddressValue::from(RELAYER_ADDRESS_EXPR).to_address();
let paymaster_contract = PaymasterContract::new(PAYMASTER_ADDRESS_EXPR);
let callee_sc_adder_contract = AdderContract::new(CALLEE_SC_ADDER_ADDRESS_EXPR);
// let callee_sc_adder_address = AddressValue::from(CALLEE_SC_ADDER_ADDRESS_EXPR).to_address();
let callee_sc_wegld_address = WegldContract::new(CALLEE_SC_WEGLD_ADDRESS_EXPR);

Self {
world,
callee_user_address,
paymaster_contract,
relayer_address,
callee_sc_adder_contract,
callee_sc_wegld_address,
}
}

Expand Down Expand Up @@ -130,6 +140,25 @@ impl PaymasterTestState {
self
}

fn deploy_wegld_contract(&mut self) -> &mut Self {
let wegld_code = self.world.code_expression(WEGLD_PATH_EXPR);

self.world
.set_state_step(SetStateStep::new().new_address(
OWNER_ADDRESS_EXPR,
3,
CALLEE_SC_WEGLD_ADDRESS_EXPR,
))
.sc_deploy(
ScDeployStep::new()
.from(OWNER_ADDRESS_EXPR)
.code(wegld_code)
.call(self.callee_sc_wegld_address.init(WEGLD_TOKEN_ID)),
);

self
}

fn check_esdt_balance(
&mut self,
address_expr: &str,
Expand All @@ -144,13 +173,27 @@ impl PaymasterTestState {

self
}
fn check_egld_balance(
&mut self,
address_expr: &str,
balance_expr: &str,
) -> &mut Self {
self.world
.check_state_step(CheckStateStep::new().put_account(
address_expr,
CheckAccount::new().balance(balance_expr),
));

self
}
}

#[test]
fn test_deploy_paymasters() {
let mut state = PaymasterTestState::new();
state.deploy_paymaster_contract();
state.deploy_adder_contract();
state.deploy_wegld_contract();
}

#[test]
Expand All @@ -164,7 +207,7 @@ fn test_forward_call_no_fee_payment() {
.call(state.paymaster_contract.forward_execution(
state.relayer_address.clone(),
state.callee_user_address.clone(),
b"add" ,
b"add",
MultiValueVec::<Vec<u8>>::new(),
))
.expect(TxExpect::user_error("str:There is no fee for payment!")),
Expand All @@ -187,7 +230,7 @@ fn test_forward_call_user() {
b"add",
MultiValueVec::<Vec<u8>>::new(),
))
.esdt_transfer(FEE_TOKEN_ID_EXPR, 0, FEE_AMOUNT)
.esdt_transfer(FEE_TOKEN_ID_EXPR, 0, FEE_AMOUNT),
)
.check_state_step(CheckStateStep::new().put_account(
RELAYER_ADDRESS_EXPR,
Expand All @@ -214,7 +257,7 @@ fn test_forward_call_sc_adder() {
state.callee_sc_adder_contract.to_address(),
b"add",
MultiValueVec::from([top_encode_to_vec_u8_or_panic(&ADDITIONAL_ADD_VALUE)]),
))
)),
);

let expected_adder_sum = INITIAL_ADD_VALUE + ADDITIONAL_ADD_VALUE;
Expand All @@ -231,7 +274,6 @@ fn test_forward_call_sc_adder() {
);
}


#[test]
fn test_forward_call_sc_adder_multiple_payments() {
let mut state = PaymasterTestState::new();
Expand All @@ -252,7 +294,7 @@ fn test_forward_call_sc_adder_multiple_payments() {
state.callee_sc_adder_contract.to_address(),
b"add",
MultiValueVec::from([top_encode_to_vec_u8_or_panic(&ADDITIONAL_ADD_VALUE)]),
))
)),
);

let expected_adder_sum = INITIAL_ADD_VALUE + ADDITIONAL_ADD_VALUE;
Expand All @@ -273,3 +315,70 @@ fn test_forward_call_sc_adder_multiple_payments() {
FEE_AMOUNT,
);
}

#[test]
fn test_forward_call_wegld() {
let mut state = PaymasterTestState::new();
state.deploy_paymaster_contract();
state.deploy_adder_contract();

state.check_esdt_balance(CALLER_ADDRESS_EXPR, FEE_TOKEN_ID_EXPR, BALANCE);
state.check_esdt_balance(CALLER_ADDRESS_EXPR, WEGLD_TOKEN_ID_EXPR, BALANCE);

// Call fails because unwrap amount is 0
state.world.sc_call(
ScCallStep::new()
.from(CALLER_ADDRESS_EXPR)
.esdt_transfer(FEE_TOKEN_ID_EXPR, 0, FEE_AMOUNT)
.esdt_transfer(WEGLD_TOKEN_ID_EXPR, 0, BALANCE)
.call(state.paymaster_contract.forward_execution(
state.relayer_address.clone(),
state.callee_sc_wegld_address.to_address(),
UNWRAP_ENDPOINT_NAME,
MultiValueEncoded::new(),
))
);

// Fee is kept by the relayer
let new_fee_amount: &str = "99980000";
state.check_esdt_balance(RELAYER_ADDRESS_EXPR, FEE_TOKEN_ID_EXPR, FEE_AMOUNT);
state.check_esdt_balance(CALLER_ADDRESS_EXPR, FEE_TOKEN_ID_EXPR, new_fee_amount);

// Caller has the original balance
state.check_egld_balance(CALLER_ADDRESS_EXPR, BALANCE);
}


#[test]
fn test_forward_call_fails_wegld_0_amount() {
let mut state = PaymasterTestState::new();
state.deploy_paymaster_contract();
state.deploy_adder_contract();

state.check_esdt_balance(CALLER_ADDRESS_EXPR, FEE_TOKEN_ID_EXPR, BALANCE);
state.check_esdt_balance(CALLER_ADDRESS_EXPR, WEGLD_TOKEN_ID_EXPR, BALANCE);

let failling_amount = 0u64;

// Call fails because unwrap amount is 0
state.world.sc_call(
ScCallStep::new()
.from(CALLER_ADDRESS_EXPR)
.esdt_transfer(FEE_TOKEN_ID_EXPR, 0, FEE_AMOUNT)
.esdt_transfer(WEGLD_TOKEN_ID_EXPR, 0, failling_amount)
.call(state.paymaster_contract.forward_execution(
state.relayer_address.clone(),
state.callee_sc_wegld_address.to_address(),
UNWRAP_ENDPOINT_NAME,
MultiValueEncoded::new(),
))
);

// Fee is kept by the relayer
let new_fee_amount: &str = "99980000";
state.check_esdt_balance(RELAYER_ADDRESS_EXPR, FEE_TOKEN_ID_EXPR, FEE_AMOUNT);
state.check_esdt_balance(CALLER_ADDRESS_EXPR, FEE_TOKEN_ID_EXPR, new_fee_amount);

// Caller has the original balance
state.check_esdt_balance(CALLER_ADDRESS_EXPR, WEGLD_TOKEN_ID_EXPR, BALANCE);
}
Binary file not shown.
Loading