token-swap: Add fuzzer for swap / withdraw / deposit #875

joncinque · 2020-11-19T20:33:53Z

Using libfuzzer / cargo-fuzz (https://github.com/rust-fuzz/cargo-fuzz), this adds fuzz instruction testing in token-swap, following the example over at serum (https://github.com/project-serum/serum-dex/tree/master/dex/fuzz).

Still left to do:

add limited runs to CI using the -max_total_time flag
debug owner mismatch crashes, not sure what's there yet

mvines · 2020-11-19T20:43:41Z

token-swap/program/src/instruction.rs

-        /// else that may be required
-        swap_curve: SwapCurve,
-    },
+    Initialize(Initialize),


Kinda sad to see this. Arbitrary doesn't work over enums?

I totally agree. This is actually long-standing rust issue that enum variants don't count as types. I didn't follow the whole discussion over here, but I know there were efforts to make that happen: rust-lang/rust#40666

the other option is to have the fuzz instructions live separately, but I preferred having them re-use the existing instructions. I can be overruled on that though

Separately sounds a lot worse. Maybe there'll be other options when using Borsh encoding since there we have the ability to programmatically figure the types, even from an enum, and could use that information to construct the fuzzer inputs

Oh yeah that's a great point. I'll look into possibilities for how to integrate borsh with the libfuzzer inputs and ditch Arbitrary

sweet, feature-proposal is already borshed up and its instruction interface is really small. It'd be a good guinea pig for Borsh-based fuzzing. (cc: https://github.com/solana-labs/solana-program-library/blob/master/feature-proposal/program/src/borsh_utils.rs). Might be slightly quicker to iterate on than token-swap for experiments.

good point, I'll take a look at it next then :-)

joncinque · 2020-11-20T19:04:22Z

I'll have this run somewhere over the weekend, but so far, results are good. Here's 4 threads running for nearly 3 hours without any problems:

stat::number_of_executed_units: 39009
stat::average_exec_per_sec:     243
stat::new_units_added:          405
stat::slowest_unit_time_sec:    0
stat::peak_rss_mb:              451
INFO: exiting: 2 time: 9609s

joncinque · 2020-11-20T19:18:59Z

token-swap/program/fuzz/fuzz_targets/instructions.rs

+const INITIAL_SWAP_TOKEN_A_AMOUNT: u64 = 100_000_000_000;
+const INITIAL_SWAP_TOKEN_B_AMOUNT: u64 = 300_000_000_000;
+
+const INITIAL_USER_TOKEN_A_AMOUNT: u64 = 1_000_000_000;
+const INITIAL_USER_TOKEN_B_AMOUNT: u64 = 3_000_000_000;


I'm not sure if these numbers should be higher to allow for more extensive testing -- when doing random numbers for swapping and depositing, they're much higher than these

joncinque · 2020-11-20T19:19:36Z

token-swap/program/fuzz/fuzz_targets/instructions.rs

+    let trade_fee_numerator = 25;
+    let trade_fee_denominator = 10000;
+    let owner_trade_fee_numerator = 5;
+    let owner_trade_fee_denominator = 10000;
+    let owner_withdraw_fee_numerator = 30;
+    let owner_withdraw_fee_denominator = 10000;
+    let host_fee_numerator = 1;
+    let host_fee_denominator = 5;
+    let swap_curve = SwapCurve {
+        curve_type: CurveType::ConstantProduct,
+        calculator: Box::new(ConstantProductCurve {
+            trade_fee_numerator,
+            trade_fee_denominator,
+            owner_trade_fee_numerator,
+            owner_trade_fee_denominator,
+            owner_withdraw_fee_numerator,
+            owner_withdraw_fee_denominator,
+            host_fee_numerator,
+            host_fee_denominator,
+        }),
+    };


We can eventually add more curves or even fuzz the curves, but that would be an initialize test as well

joncinque · 2020-11-20T19:23:04Z

token-swap/program/fuzz/fuzz_targets/instructions.rs

+            if e == SwapError::CalculationFailure.into() {
+            } else if e == SwapError::ConversionFailure.into() {
+            } else if e == SwapError::FeeCalculationFailure.into() {
+            } else if e == SwapError::ExceededSlippage.into() {
+            } else if e == SwapError::ZeroTradingTokens.into() {
+            } else if e == TokenError::InsufficientFunds.into() {
+            } else {
+                Err(e).unwrap()
+            }


This allows certain errors to slide, since they don't mean anything truly wrong has happened. Note that these checks do run the risk of overlap, since ProgramError::Custom(u32) can re-use the same value for TokenError and SwapError. We should consider changing it to ProgramError::Custom(Pubkey, u32) to separate better

joncinque · 2020-11-20T19:28:43Z

token-swap/program/fuzz/src/lib.rs

+impl program_stubs::SyscallStubs for TestSyscallStubs {
+    fn sol_invoke_signed(
+        &self,
+        instruction: &Instruction,
+        account_infos: &[AccountInfo],
+        signers_seeds: &[&[&[u8]]],
+    ) -> ProgramResult {
+        let mut new_account_infos = vec![];
+
+        // mimic check for token program in accounts
+        if !account_infos.iter().any(|x| *x.key == spl_token::id()) {
+            return Err(ProgramError::InvalidAccountData);
+        }
+
+        for meta in instruction.accounts.iter() {
+            for account_info in account_infos.iter() {
+                if meta.pubkey == *account_info.key {
+                    let mut new_account_info = account_info.clone();
+                    for seeds in signers_seeds.iter() {
+                        let signer =
+                            Pubkey::create_program_address(&seeds, &spl_token_swap::id()).unwrap();
+                        if *account_info.key == signer {
+                            new_account_info.is_signer = true;
+                        }
+                    }
+                    new_account_infos.push(new_account_info);
+                }
+            }
+        }
+
+        spl_token::processor::Processor::process(
+            &instruction.program_id,
+            &new_account_infos,
+            &instruction.data,
+        )
+    }
+}


This is lifted directly from the processor test code, not sure how we can share this better.

joncinque · 2020-11-20T19:29:24Z

token-swap/program/fuzz/src/lib.rs

+#[derive(Clone)]
+pub struct AccountData {
+    pub key: Pubkey,
+    pub lamports: u64,
+    pub data: Vec<u8>,
+    pub program_id: Pubkey,
+    pub is_signer: bool,
+}


This is a bit hacky, but encompasses account data in an easy-to-use way. Is there a better solution for this?

joncinque · 2020-11-20T20:59:53Z

The concept of these fuzzing tests is to run fuzzed withdraw / swap / deposit instructions on a hard-coded curve, allow for certain errors (like Insufficient funds and calculation errors), and at the end check that we haven't created or deleted any tokens, and that we can also withdraw everything from the token swap. also tagging @aeyakovenko if he has any additional ideas for conditions to check.

joncinque · 2020-11-20T21:00:40Z

I'd like to leave the CI for a follow-up PR, and I'll keep running the fuzzing over the weekend

joncinque · 2020-11-23T12:18:16Z

After running this over the whole weekend, it only caught an error in the fuzzer logic!

joncinque · 2020-11-23T17:49:58Z

I've tried getting this to run in bpf, but we'll need nightly for that, specifically for Arbitrary, which uses the non_exhaustive feature for its errors

joncinque · 2020-11-23T19:38:57Z

I've tried using honggfuzz in 2997203. Specifically, a version that doesn't use #[non_exhaustive] from arbitrary, which trips up the current bpf builder. There would still need to be some clever work to make a fuzz-bpf command though. I'll do some performance diffs between libfuzzer and honggfuzz (current and old version) to see which is the winner.

joncinque · 2020-11-23T20:28:49Z

Converting back to draft as I try honggfuzz some more

joncinque · 2020-11-24T17:14:27Z

ci/install-build-deps.sh

+sudo apt-get install -y binutils-dev
+sudo apt-get install -y libunwind-dev


these can also get added to fuzz.sh if preferred

Na, this is fine IMO

joncinque · 2020-11-24T17:14:38Z

ci/install-program-deps.sh

@@ -9,6 +9,7 @@ set -x

 cargo --version
 cargo install rustfilt || true
+cargo install honggfuzz || true


this can get added to fuzz.sh as well

joncinque · 2020-11-25T12:52:08Z

A nice note for on.schedule workflows at https://docs.github.com/en/free-pro-team@latest/actions/managing-workflow-runs/disabling-and-enabling-a-workflow:

Warning: To prevent unnecessary workflow runs, scheduled workflows may be disabled automatically. When a public repository is forked, scheduled workflows are disabled by default. In a public repository, scheduled workflows are automatically disabled when no repository activity has occurred in 60 days.

Meaning people who have forked the repo won't run the nightly fuzzer

joncinque · 2020-11-25T13:30:42Z

shared-memory/program/tests/shared-memory.rs

@@ -26,7 +25,7 @@ fn run_program(
    program_id: &Pubkey,
    parameter_accounts: &[KeyedAccount],
    instruction_data: &[u8],
-) -> Result<u64, InstructionError> {
+) -> u64 {


This came from a new clippy error

mvines

Looks good overall! I have some concerns about the amount of boilerplate that seems to be required to fuzz but as a v1 there's no need for this PR to continue to dangle. This'll be a good example for other programs at least, and hopefully inspiration for future improvements to streamline fuzzing.

Errr, so did we find any actual bugs in token-swap though?

mvines · 2020-11-30T04:56:15Z

ci/install-build-deps.sh

+sudo apt-get install -y binutils-dev
+sudo apt-get install -y libunwind-dev


Na, this is fine IMO

mvines · 2020-11-30T04:57:47Z

token-swap/program/fuzz/src/instructions.rs

+    native_account_data::NativeAccountData, native_token::get_token_balance,
+    native_token_swap::NativeTokenSwap,
+};
+


nit: try remove whitespace between successive use statements

mvines · 2020-11-30T05:00:23Z

token-swap/program/fuzz/src/instructions.rs

+    }
+}
+
+fn run_fuzz_instructions(fuzz_instructions: Vec<FuzzInstruction>) {


nit: avoid requiring a Vec when a slice will do

Suggested change

fn run_fuzz_instructions(fuzz_instructions: Vec<FuzzInstruction>) {

fn run_fuzz_instructions(fuzz_instructions: &[FuzzInstruction]) {

mvines · 2020-11-30T05:04:33Z

token-swap/program/src/processor.rs

@@ -550,7 +553,8 @@ impl Processor {
                to_u128(token_a.amount)?,
            )
            .ok_or(SwapError::ZeroTradingTokens)?;
-        if a_amount < to_u128(minimum_token_a_amount)? {
+        let a_amount = to_u64(a_amount)?;


Was this a bug fix as a result of fuzzing?

It actually wasn't, just an inconsistency in the code from when we transitioned to u128

joncinque · 2020-11-30T10:47:05Z

@mvines thanks for the review. I ended up starting on the banks version, and we can definitely refactor some logic that's also used in the stake pool end-to-end testing in a token/client library. In the end, no bugs were discovered in token-swap from a few days of fuzzing, but maybe the nightly runs will discover something!

Pull request has been modified.

joncinque added 2 commits November 19, 2020 21:27

token-swap: Add fuzzer for swap / withdraw / deposit

3d2cd37

Run cargo fmt

8ea173b

mvines reviewed Nov 19, 2020

View reviewed changes

joncinque added 2 commits November 20, 2020 19:42

Make end-to-end testing work

e863941

Fix test failures

7725118

joncinque commented Nov 20, 2020

View reviewed changes

joncinque added 3 commits November 20, 2020 20:32

Cleanup for review

51691ca

Remove Clone trait for Initialize

2b1a720

Fix building fuzz targets

334cba9

joncinque requested a review from mvines November 20, 2020 20:57

joncinque marked this pull request as ready for review November 20, 2020 20:57

joncinque changed the title ~~WIP token-swap: Add fuzzer for swap / withdraw / deposit~~ token-swap: Add fuzzer for swap / withdraw / deposit Nov 20, 2020

Fix fuzz withdraw logic to avoid ZeroTradingTokens error

2a3635e

joncinque added 4 commits November 23, 2020 13:19

Cargo fmt

cc29484

Clippy / fmt again

c86994e

Integrate fuzzer in main workspace to share clippy / fmt

497301a

Fix clippy

1a10c39

Switch to stable honggfuzz that could work with BPF

2997203

joncinque marked this pull request as draft November 23, 2020 20:28

joncinque added 5 commits November 24, 2020 12:18

Update to most recent honggfuzz

4040f9d

Add build / run requirements

7d51580

Add fuzz CI and cleanup for honggfuzz

8344491

Revert to using fuzz feature to integrate with workspace

d9d616c

Make fuzz script executable

d69b439

joncinque marked this pull request as ready for review November 24, 2020 17:13

joncinque commented Nov 24, 2020

View reviewed changes

joncinque added 4 commits November 25, 2020 13:00

Refactor and cleanup

9e9f1dd

Add nightly run

7814356

Fix workflow typo

a46ec5f

Add runtime for pull request fuzz workflow

5c8ba05

Fix unrelated new clippy errors

4580432

joncinque commented Nov 25, 2020

View reviewed changes

mvines previously approved these changes Nov 30, 2020

View reviewed changes

Merge branch 'master' into ts-fuzz

5ce9168

joncinque merged commit b40e0dd into solana-labs:master Nov 30, 2020

joncinque deleted the ts-fuzz branch December 3, 2020 11:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

token-swap: Add fuzzer for swap / withdraw / deposit #875

token-swap: Add fuzzer for swap / withdraw / deposit #875

joncinque commented Nov 19, 2020 •

edited

Loading

mvines Nov 19, 2020

joncinque Nov 19, 2020

joncinque Nov 19, 2020

mvines Nov 19, 2020

joncinque Nov 19, 2020

mvines Nov 19, 2020

joncinque Nov 19, 2020

joncinque commented Nov 20, 2020

joncinque Nov 20, 2020

joncinque Nov 20, 2020

joncinque Nov 20, 2020

joncinque Nov 20, 2020

joncinque Nov 20, 2020

joncinque commented Nov 20, 2020

joncinque commented Nov 20, 2020

joncinque commented Nov 23, 2020

joncinque commented Nov 23, 2020

joncinque commented Nov 23, 2020

joncinque commented Nov 23, 2020

joncinque Nov 24, 2020

mvines Nov 30, 2020

joncinque Nov 24, 2020

joncinque commented Nov 25, 2020 •

edited

Loading

joncinque Nov 25, 2020

mvines left a comment

mvines Nov 30, 2020

mvines Nov 30, 2020

mvines Nov 30, 2020

mvines Nov 30, 2020

joncinque Nov 30, 2020

joncinque commented Nov 30, 2020

		sudo apt-get install -y binutils-dev
		sudo apt-get install -y libunwind-dev

	fn run_fuzz_instructions(fuzz_instructions: Vec<FuzzInstruction>) {
	fn run_fuzz_instructions(fuzz_instructions: &[FuzzInstruction]) {

token-swap: Add fuzzer for swap / withdraw / deposit #875

token-swap: Add fuzzer for swap / withdraw / deposit #875

Conversation

joncinque commented Nov 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joncinque commented Nov 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joncinque commented Nov 20, 2020

joncinque commented Nov 20, 2020

joncinque commented Nov 23, 2020

joncinque commented Nov 23, 2020

joncinque commented Nov 23, 2020

joncinque commented Nov 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joncinque commented Nov 25, 2020 • edited Loading

Choose a reason for hiding this comment

mvines left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joncinque commented Nov 30, 2020

joncinque commented Nov 19, 2020 •

edited

Loading

joncinque commented Nov 25, 2020 •

edited

Loading