Add ERC4626 standard property tests

[daejunpark]

This PR adds the ERC4626 property tests for standard conformance: https://github.com/a16z/erc4626-tests

The idea is to maintain a common test suite that is applicable for any ERC4626 implementations, to help detect their unintentional discrepancies, if any. More details can be found in this post.

Any feedback or suggestions (e.g., adding more test cases, or improving fuzzing speed, etc.) would be greatly appreciated!

PR Checklist

• Tests
• Documentation
• Changelog entry

[frangio]

Hard to overstate how awesome this is. Thank you! We'll review the tests before merging.

[Amxx]

Run the tests, everything looks good.

Should we review in details what is in this submodule?

[daejunpark]

Awesome! Please let me know if you have any questions or concerns while reviewing the tests!

BTW, fuzzing this new test currently takes ~3m on the github actions. Would it be problematic?

[daejunpark]

Should we review in details what is in this submodule?

@Amxx you're more than welcome to review them if you want!

• If you want to see what kind of test cases (properties) are there, you can take a look this: https://github.com/a16z/erc4626-tests/blob/main/ERC4626.prop.sol
• If you want to review the fuzzing setup: here it is: https://github.com/a16z/erc4626-tests/blob/main/ERC4626.test.sol

[Amxx]

LGTM

[Amxx]

Any reason why _vaultMayBeEmpty is false. Do check fail with true? If so, should we try to fix that ?

[Amxx]

If I do _unlimitedAmount = true, I get an error

[FAIL. Reason: Too many global rejects] test_mint((address[4],uint256[4],uint256[4],int256),uint256,uint256) (runs: 8916, μ: 495072, ~: 497478)

Is that a timeout?

[daejunpark]

Any reason why _vaultMayBeEmpty is false. Do check fail with true? If so, should we try to fix that ?

I'm glad you bring this up, which I wish I have a chance to discuss with you!

Indeed, if you are lucky(?), you will get fuzz inputs that fail some "round-trip" property tests when this flag is set to true.

For example, consider the following property, which says that when you withdraw some amount of assets and then immediately deposit the same amount again, the number of shares burned for the previous withdrawal must not be smaller than the number of shares minted for the deposit; because, otherwise, you can make a free profit by repeating withdrawal and deposit back and force. (Hence, called a round-trip property, and all the combinations are included in the tests.)

    // s = withdraw(a)
    // s' = deposit(a)
    // s' <= s
    function prop_RT_withdraw_deposit(address caller, uint assets) public {
        vm.prank(caller); uint shares1 = vault_withdraw(assets, caller, caller);
        if (!_vaultMayBeEmpty) vm.assume(IERC20(_vault_).totalSupply() > 0);
        vm.prank(caller); uint shares2 = vault_deposit(assets, caller);
        assertApproxLeAbs(shares2, shares1, _delta_);
    }

The main motivation behind this round-trip property is to check whether the standard-preferred rounding direction is employed by the vault implementations.

Since your ERC4626 implementation follows the preferred rounding direction (which I appreciate!), all the round-trip property tests should pass...... as long as the vault is never emptied in the middle.

For example, consider a vault where the current total assets is 20, and total supply (shares) is 10, and suppose that you hold all the shares. Now, suppose you withdraw 20 assets, and then deposit them back immediately (i.e., assets == 20 in the above code). In this case, the number of shares burned by the withdrawal is 10 (i.e., shares1 == 10), but the number of shares minted by the deposit is 20 (i.e., shares2 == 20). This is because the vault became emptied after the first withdrawal, which resets the share price to the default initial price 1.

Now, the question is whether this behavior could be exploited or not. What concerns me is that, as you can see in the above example, there is a seemingly unintentional way to drastically decrease the share price (or increase depending on the context). Would it be too much to worry about some scenarios, say that adversaries create a vault, quickly increase the share price by "donating" assets, deceive some lending protocols to believe the high share price, and then atomically withdraw all the assets from the vault to reset the price to 1, mint a lot of shares with the initial price, and use them as collateral to borrow much more from the lending protocols while they still believe the earlier high share price?

Even if my concern is valid, I'm not sure if this is something that needs to be addressed by this library contract (e.g., not allowing the vault to be emptied, asking some dust leaved to keep track of the share price), or just pitfalls that devs/users should be aware of and operate carefully. Would love to hear from you on this!

[daejunpark]

If I do _unlimitedAmount = true, I get an error

[FAIL. Reason: Too many global rejects] test_mint((address[4],uint256[4],uint256[4],int256),uint256,uint256) (runs: 8916, μ: 495072, ~: 497478)

Is that a timeout?

It is not timeout but similar; you can set a higher value for the max_test_rejects config variable or the FOUNDRY_FUZZ_MAX_TEST_REJECTS environment variable. The default value is 65536, and I used to generously set it to ~100x of the number of fuzz runs (which is currently set to 10,000 in your config file). https://book.getfoundry.sh/reference/config/testing?highlight=reject#max_test_rejects

Our tests need a higher value to cover edge cases.

[Amxx]

It is not timeout but similar; you can set a higher value for the max_test_rejects config variable or the FOUNDRY_FUZZ_MAX_TEST_REJECTS environment variable. The default value is 65536, and I used to generously set it to ~100x of the number of fuzz runs (which is currently set to 10,000 in your config file). https://book.getfoundry.sh/reference/config/testing?highlight=reject#max_test_rejects

Our tests need a higher value to cover edge cases.

Should we set both flags to true and increase the max_test_rejects ? Would that give us more guarantees, or is it not worth the extra check time ?

[daejunpark]

Should we set both flags to true and increase the max_test_rejects ? Would that give us more guarantees, or is it not worth the extra check time ?

Setting _unlimitedAmount to true with increasing max_test_rejects will provide a better test coverage, so I recommend it if you're OK with spending a couple more minutes on CI.

However, _vaultMayBeEmpty should remain false at this moment, otherwise you will see intermittent failure in the foundry fuzz tests. Please find my previous comment that explains more details why it happens and my concern with the current ERC4626 behavior. If you think my concern is valid, then you can add an extra logic to your ERC4626 implementation, and set the _vaultMayBeEmpty flag true to ensure the new behavior. Otherwise, you are better to leave the _vaultMayBeEmpty flag set to false.

Please let me know if you want to discuss further!

[Amxx]

We have plans to slightly modify ERC4626 to address (already known and documented) security concerns.
We may want to work on supporting _vaultMayBeEmpty at the same time if changes are required.

[daejunpark]

Oh, that sounds great! In that case, I will open an issue for that for more discussion there.

For now, I set _unlimitedAmount true and increase max_test_rejects to 1M. Please let me know if there is anything else I can help you with merging this!

[frangio]

This seems like a high value. Do you know if it's mainly due to vm.assume(false) which is used in various places?

[frangio]

I see this was mentioned in a previous comment in the PR. I would like to understand more why there are so many rejections, as keeping rejections low is important to keep fuzzing efficient.

If it's due to vm.assume(false) there may be other ways to implement the same thing, or to prevent the vault call from reverting beforehand.

[daejunpark]

Yes, that's my understanding based on the document: https://book.getfoundry.sh/cheatcodes/assume

Indeed, it can be reduced to 100K for the current tests (the default is 65536), if you prefer it to be tighter. Please let me know!

[frangio]

If the default is 65536, bumping it to 100k doesn't sound like that big of a deal. I'm still wondering if there's a better way to implement this that doesn't rely on vm.assume(false) so much. It would need to be a way of avoiding triggering reverts, instead of catching them and discarding the run after the fact.

[daejunpark]

Since the tests are written to be agnostic to the implementation details (so that they are applicable to various implementations), it is hard to precisely find the input range that never reverts for various conditions.

An option is to bound inputs for, e.g., withdraw() using previewWithdraw() or convertToAssets(), but as you know, the preview or convert functions do not need to be precise and they may be conservative, so relying on them may exclude some edge cases. (Indeed, setting _unlimitedAmount flag to false will exactly do this.)

I haven't found a better way to not exclude edge cases without the trial-and-error approach, but please let me know if we have an idea!

[frangio]

I see, that makes a lot of sense. Let's lower it to 100k then.

[daejunpark]

Yep, now it's 100K.

[daejunpark]

FYI, I opened an issue #3800 for more discussion on my concern that I mentioned above.

If the issue is addressed, _vaultMayBeEmpty can be set to true.

[gitpoap-bot]

Congrats, your important contribution to this open-source project has earned you a GitPOAP!

GitPOAP: 2022 OpenZeppelin Contracts Contributor:

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.