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

Add Echidna Tests #16

Merged
merged 49 commits into from
May 14, 2021
Merged

Add Echidna Tests #16

merged 49 commits into from
May 14, 2021

Conversation

naszam
Copy link
Contributor

@naszam naszam commented Apr 30, 2021

Description:

  • Add Echidna Tests for DssVest

https://app.clubhouse.io/scdomaincommunityteam/story/11462/add-echidna-tests-for-dss-vest

COB Team Name or Author(s): @naszam

Contribution Checklist

  • PR title ends with (SC-<TICKET_NUMBER>)
  • Link to clubhouse ticket
  • Code approved
  • Tests approved
  • CI Tests pass

@brianmcmichael
Copy link
Contributor

Thanks, can you add some additional docs to the readme with general setup for this?

@naszam
Copy link
Contributor Author

naszam commented May 3, 2021

I've added the fuzz docs as well as GitHub Actions Fuzz CI and badge 🙂

}

function test_init_params(uint256 _amt, uint256 _bgn, uint256 _tau, uint256 _clf, uint256 _pmt, address _mgr) public {
_amt = 1 * WAD + _amt % uint128(-1);
Copy link

Choose a reason for hiding this comment

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

this can exceed uint128(-1), which I assume will produce a violation since init will revert

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just set _amt and _pmt min seed value to zero

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this should be done.

}

function test_vest(uint256 _amt, uint256 _bgn, uint256 _tau, uint256 _clf, uint256 _pmt, address _mgr, uint256 _tick) public {
_amt = 1 * WAD + _amt % uint128(-1);
Copy link

Choose a reason for hiding this comment

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

same comment as above

Copy link
Contributor Author

@naszam naszam May 5, 2021

Choose a reason for hiding this comment

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

Done.

_amt = 1 * WAD + _amt % uint128(-1);
_bgn = block.timestamp + _bgn % vest.MAX_VEST_PERIOD();
_tau = 1 + _tau % vest.MAX_VEST_PERIOD();
_clf = 1 + _clf % _tau;
Copy link

Choose a reason for hiding this comment

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

Why not allow _clf ==0? Seems like a valid case.

Copy link
Contributor Author

@naszam naszam May 5, 2021

Choose a reason for hiding this comment

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

Done.

_amt = 1 * WAD + _amt % uint128(-1);
_bgn = block.timestamp + _bgn % vest.MAX_VEST_PERIOD();
_tau = 1 + _tau % vest.MAX_VEST_PERIOD();
_clf = 1 + _clf % _tau;
Copy link

Choose a reason for hiding this comment

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

same comment as above

Copy link
Contributor Author

@naszam naszam May 5, 2021

Choose a reason for hiding this comment

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

Done.

_tau = 1 + _tau % vest.MAX_VEST_PERIOD();
_clf = 1 + _clf % _tau;
_pmt = 1 * WAD + _pmt % _amt;
_tick = block.timestamp + _tick % uint128(-1);
Copy link

Choose a reason for hiding this comment

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

Okay I need some education on echidna...how does the value of _tick affect the behavior of the the vest call? I guess what I'm expecting is some sort of special instruction that creates a relation between _tick and block.timestamp before the call to vest, similar to hevm.warp, but I'm not seeing it.

Copy link
Contributor Author

@naszam naszam May 5, 2021

Choose a reason for hiding this comment

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

I've added _tick as a seed to simulate time for time dependant scenarios.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just removed _tick as echidna should increase block.timestamp when fuzzing.
Also fixed the check in test_vest with block.timestamp >= fin.
Let me know if it's ok.

}

function test_init_ids(uint256 _amt, uint256 _bgn, uint256 _tau, uint256 _clf, uint256 _pmt, address _mgr) public {
_amt = 0 + _amt % uint128(-1);
Copy link

Choose a reason for hiding this comment

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

I kind of think _amt = 0 is an uninteresting case--I'd recommend enforcing a reasonable minimum like you were before, just in a way that won't exceed the uint128(-1) limit. For example, here's one option:

_amt = _amt % uint128(-1);
if (_amt < WAD) _amt *= WAD;

Copy link

Choose a reason for hiding this comment

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

That will never cause a uint128 overflow since WAD * (WAD - 1) < 2^128 - 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

Copy link

Choose a reason for hiding this comment

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

Oh yeah...we want to avoid _amt == 0...so we need something like:

_amt = _amt % uint128(-1);
if (_amt < WAD) _amt = (1 + _amt) * WAD;

Copy link
Contributor Author

@naszam naszam May 5, 2021

Choose a reason for hiding this comment

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

Done.

_pmt = _pmt % _amt;
uint256 prevId = vest.ids();
uint256 id = vest.init(address(this), _amt, _bgn, _tau, _clf, _pmt, _mgr);
assert(vest.ids() == add(prevId, id));
Copy link

Choose a reason for hiding this comment

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

This won't hold in general. Suggest replacing with:

        assert(vest.ids() == add(prevId, 1));
        assert(vest.ids() == id);

Copy link
Contributor Author

@naszam naszam May 5, 2021

Choose a reason for hiding this comment

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

Done.

require((z = x - y) <= x);
}

function test_init_ids(uint256 _amt, uint256 _bgn, uint256 _tau, uint256 _clf, uint256 _pmt, address _mgr) public {
Copy link

Choose a reason for hiding this comment

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

If we're bothering to pass _mgr as a fuzz parameter, we should at least verify that it's being set correctly in the award (same for the other values).

Copy link
Contributor Author

@naszam naszam May 5, 2021

Choose a reason for hiding this comment

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

this should be done in test_init_params

Copy link

Choose a reason for hiding this comment

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

Actually I'd suggest to combine test_init_ids and test_init_params into a single test named test_init. And it's probably unnecessary to pass _mgr in test_vest; alternatively, test_vest could be modified to randomly call vest on a previously-initialized award.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the _mgr seed and replace with echidna_mgr

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@brianmcmichael brianmcmichael left a comment

Choose a reason for hiding this comment

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

Fuzzing!

assert(z == x);
}

function test_init(uint256 _tot, uint256 _bgn, uint256 _tau, uint256 _clf, uint256 _end) public {
Copy link
Contributor

Choose a reason for hiding this comment

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

might also be worthwhile to have a test that hevm warps to some random point during or after init and check that payouts and accrued and unpaid values all line up.

Copy link
Contributor Author

@naszam naszam May 12, 2021

Choose a reason for hiding this comment

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

yeah, I think @kmbarry1 will make some dapp fuzz tests as well leveraging on hevm.warp to fuzz time-dependant logic and compare the two approaches.

Choose a reason for hiding this comment

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

yeah I was kind of waiting for all the churn to settle down in the code itself

}

function test_vest(uint256 id) internal {
vest.vest(id);

Choose a reason for hiding this comment

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

So, the issue here is that the award will almost never exist if id is a random uint256. You'll need to mod it into the appropriate range:
id = 1 + id % vest.ids();
to effectively test already-created vests.

Copy link
Contributor Author

@naszam naszam May 13, 2021

Choose a reason for hiding this comment

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

test_vest and test_yank are called inside test_init with an existing valid id.
I can fuzz them independently leveraging on the preserved state option eventually

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we shouldn't fuzz the ids cause they're checked in all the functions on dss-vest, if the award exists, so declaring both test_vest and test_yank internal allows to check for asserts on test_init seeds.

Choose a reason for hiding this comment

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

I think this approach is good enough. The only downside is every award gets yanked before any more are created, so you're never testing with multiple awards in existence. However, given how the contract is written, there's very little chance of bugs that only surface when multiple awards exist.

}

function test_yank(uint256 id, uint256 end) internal {
( , , , uint48 _fin, , , ) = vest.awards(id);

Choose a reason for hiding this comment

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

You'll have a similar issue here as above.

Copy link
Contributor Author

@naszam naszam May 13, 2021

Choose a reason for hiding this comment

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

In this case test_yank, that is called inside test_init, will use an existing valid id plus an _end seed from test_init.

Copy link

@kmbarry1 kmbarry1 left a comment

Choose a reason for hiding this comment

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

LGTM

@brianmcmichael brianmcmichael merged commit 9b8b37b into makerdao:master May 14, 2021
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.

4 participants