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

Paymaster audit fixes #33

merged 16 commits into from
Nov 28, 2023

Conversation

CostinCarabas
Copy link
Contributor

No description provided.

@github-actions
Copy link

github-actions bot commented Oct 10, 2023

Contract comparison - from 3ec6ea1 to 55d4dae

Path                                                                                             size                  has-allocator                     has-format
mx-contracts-rs/contracts
- adder/adder.wasm 685 No No
- bonding-curve-contract/bonding-curve-contract.wasm 15688 No No
- check-pause/check-pause.wasm 1227 No No
- crowdfunding-esdt/crowdfunding-esdt.wasm 3612 No No
- crypto-bubbles/crypto-bubbles.wasm 2364 No No
- crypto-zombies/crypto-zombies.wasm 11131 No No
- digital-cash/digital-cash.wasm 7370 No No
- empty/empty.wasm 232 No No
- esdt-transfer-with-fee/esdt-transfer-with-fee.wasm 9094 No No
- factorial/factorial.wasm 567 No No
- fractional-nfts/fractional-nfts.wasm 8712 No No
- liquid-locking/liquid-locking.wasm 10239 No No
- lottery-esdt/lottery-esdt.wasm 11385 No No
- multisig
- - multisig.wasm 15306 No No
- - multisig-full.wasm 17310 No No
- - multisig-view.wasm 7699 No No
- mvx-game-sc/mvx-game-sc.wasm 11137 No No
- mystery-box/mystery-box.wasm 13804 No No
- nft-escrow/nft-escrow.wasm 7327 No No
- nft-minter/nft-minter.wasm 10237 No No
- nft-storage-prepay/nft-storage-prepay.wasm 2293 No No
- paymaster/paymaster.wasm 7400 ➡️ 7509 🔴 No No
- ping-pong-egld/ping-pong-egld.wasm 6696 No No
- price-aggregator/multiversx-price-aggregator-sc.wasm 20328 No No
- proxy-pause/proxy-pause.wasm 5757 No No
- rewards-distribution/rewards-distribution.wasm 11259 No No
- seed-nft-minter/seed-nft-minter.wasm 15343 No No
- token-release/token-release.wasm 8466 No No
- wegld-swap/multiversx-wegld-swap-sc.wasm 4382 No No
mx-contracts-rs/contracts/crypto-kitties
- kitty-auction/kitty-auction.wasm 11134 No No
- kitty-genetic-alg/kitty-genetic-alg.wasm 3584 No No
- kitty-ownership/kitty-ownership.wasm 14005 No No
mx-contracts-rs/contracts/order-book
- factory/order-book-factory.wasm 4995 No No
- pair/order-book-pair.wasm 16426 No No

sasurobert
sasurobert previously approved these changes Oct 10, 2023
@@ -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.

@@ -5,7 +5,6 @@ multiversx_sc::imports!();
pub mod forward_call;
const FEE_PAYMENT: usize = 0;

/// 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.

@sasurobert sasurobert merged commit 21717d7 into main Nov 28, 2023
5 checks passed
@CostinCarabas CostinCarabas deleted the paymaster_audit_fixes branch December 4, 2023 07:30
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

Successfully merging this pull request may close these issues.

3 participants