search

ubiquity / ubiquity-dollar

Ubiquity Dollar (uAD) smart contracts and user interface.
https://uad.ubq.fi
Apache License 2.0
31 stars 82 forks source link

feat: UbiquityPoolFacet fuzz tests #940

Closed gitcoindev closed 1 week ago

gitcoindev commented 1 month ago

Resolves https://github.com/ubiquity/ubiquity-dollar/issues/925

Skeleton based on UbiquityPoolFacet tests.

gitcoindev commented 1 month ago

This pull request is still a draft. It contains a skeleton for fuzz tests, I will keep updating and hopefully make as ready for review during or after the weekend.

ubiquibot-continuous-deploys[bot] commented 1 month ago
809f149
eaa29b3
222355d
e9e0e6c
60651d5
478e4df
ab85a33
320f710
7dce7be
565aaa6
gitcoindev commented 1 month ago

I am playing with the fuzzer a lot and discovered an interesting case. When redeem threshold is set to $0.99 , the curve price must be larger than 990000999999999999 due to conversion between 6 digit and 18 digit curve prices in getDollarPriceUsd() <= poolStorage.redeemPriceThreshold comparison, i.e

vm.assume(dollarPriceUsd > 990000999999999999); // 0.99e18 , greater than redeem threshold

I was stuck for some time with the failing fuzz test but after debugging found the edge value. Fuzzer is great, as setting mocked price to 990000999999999998 it almost immediately finds this and reports the case.

gitcoindev commented 1 month ago

I will finish with the whole test suite till Friday EOD and set the pr as ready for review.

rndquu commented 4 weeks ago

I am playing with the fuzzer a lot and discovered an interesting case. When redeem threshold is set to $0.99 , the curve price must be larger than 990000999999999999 due to conversion between 6 digit and 18 digit curve prices in getDollarPriceUsd() <= poolStorage.redeemPriceThreshold comparison, i.e

vm.assume(dollarPriceUsd > 990000999999999999); // 0.99e18 , greater than redeem threshold

I was stuck for some time with the failing fuzz test but after debugging found the edge value. Fuzzer is great, as setting mocked price to 990000999999999998 it almost immediately finds this and reports the case.

This seems to be a low severity issue but keeping in mind that nobody from the Sherlock audit have reported this bug it seems that nobody fuzzed the UbiquityPoolFacet contract extensively.

gitcoindev commented 4 weeks ago

I pushed the remaining fuzz tests also for redemption delay, setting as ready for review now.

rndquu commented 4 weeks ago

@gitcoindev

  1. Could you add 2 more fuzz tests: a) If user mints X Dollars he gets exactly X Dollars (minus fee) b) If user redeems X Dollars exactly X Dollars are subtracted from user balance
  2. Pls notice the original issue spec also describe setting up CI

Overall the PR looks great, we will iterate on the fuzzing tests later

gitcoindev commented 4 weeks ago

@gitcoindev

  1. Could you add 2 more fuzz tests: a) If user mints X Dollars he gets exactly X Dollars (minus fee) b) If user redeems X Dollars exactly X Dollars are subtracted from user balance
  2. Pls notice the original issue spec also describe setting up CI

Overall the PR looks great, we will iterate on the fuzzing tests later

Sure, thank you for the feedback.

gitcoindev commented 2 weeks ago

Hi @rndquu @0x4007 @molecula451 the PR is ready to review, I executed QA against my fork: https://github.com/gitcoindev/ubiquity-dollar/actions/runs/9534134896/job/26278201422 . The CI setup is ready and according to the original spec. Fuzz tests run by default with 256 inputs on PR commits and the 'deep fuzz' workflow with 100000 inputs https://github.com/ubiquity/ubiquity-dollar/blob/development/.github/workflows/deep-fuzz.yml is executed after merge to the development branch.

molecula451 commented 2 weeks ago

hi @gitcoindev can we instead organize the tests inside a "fuzz" folder,

packages/contracts/test/fuzz/diamond/facets/UbiquityPoolFacet.fuzz.t.sol or something similar

i think this is our initial sketching/drawing for fuzzing inside the repo, besides the SMTChecker we've done before, but untouching the .sol files, this is the first so it would be nice the addition of the folder for future development

gitcoindev commented 2 weeks ago

@molecula451 I moved fuzz tests as requested, could you please have a look once again now?

rndquu commented 1 week ago

@gitcoindev Did you experiment with how long the deep fuzz workflow runs for, let's say, 1_000_000 runs ? I want to find out a value so that deep fuzz workflow could run for the amount of hours close to github max CI limit (6 hours).

0x4007 commented 1 week ago

@gitcoindev Did you experiment with how long the deep fuzz workflow runs for, let's say, 1_000_000 runs ? I want to find out a value so that deep fuzz workflow could run for the amount of hours close to github max CI limit (6 hours).

Can try a binary search and just push a few commits.

molecula451 commented 1 week ago

i think in part we're going to see the runs with new upcoming PRs and improve the addition workflow from there

gitcoindev commented 1 week ago

@gitcoindev Did you experiment with how long the deep fuzz workflow runs for, let's say, 1_000_000 runs ? I want to find out a value so that deep fuzz workflow could run for the amount of hours close to github max CI limit (6 hours).

Can try a binary search and just push a few commits.

During my experiments and tests implementation I figured out that 100 000 was more than enough to find all edge cases for this Ubiquity pool lib. Before pushing the commit I modified e.g. vm.assume, expected values or lib implementation, with very small values or large values and fuzzer always found the problem with less than 10 000 iterations. I set deep fuzz to 100 000 to be sure that it will find new issues, but of course we can experiment and increase to full 6 hrs and check if it brings any value.

rndquu commented 1 week ago

@gitcoindev Did you experiment with how long the deep fuzz workflow runs for, let's say, 1_000_000 runs ? I want to find out a value so that deep fuzz workflow could run for the amount of hours close to github max CI limit (6 hours).

Can try a binary search and just push a few commits.

During my experiments and tests implementation I figured out that 100 000 was more than enough to find all edge cases for this Ubiquity pool lib. Before pushing the commit I modified e.g. vm.assume, expected values or lib implementation, with very small values or large values and fuzzer always found the problem with less than 10 000 iterations. I set deep fuzz to 100 000 to be sure that it will find new issues, but of course we can experiment and increase to full 6 hrs and check if it brings any value.

https://github.com/ubiquity/ubiquity-dollar/issues/943