Erc721 CT2 invariant failure #770

grandizzy · 2023-04-26T17:46:10Z

forge test -vv --match-test test_regression_erc721_CT2

fix by accounting collateral at bucket 7388 that could be compensated when NFT auction settled

…settled

tests/forge/invariants/ERC721Pool/BasicERC721PoolInvariants.t.sol

ith-harvey · 2023-04-26T18:24:33Z

tests/forge/regression/ERC721Pool/RegressionTestReservesERC721Pool.t.sol

@@ -237,4 +237,21 @@ contract RegressionTestReserveERC721Pool is ReserveERC721PoolInvariants {
        invariant_Lps_B4();
        invariant_Buckets_B2_B3();
    }
+
+    function test_regression_erc721_CT2() external {


Please provide comments that outline what is happening in this test / why it is helpful.

not sure what you mean, was failing and then fixed?

Something of this nature -> https://github.com/ajna-finance/contracts/blob/a5e00c268af3cca9dac047007c1efac91894087c/tests/forge/regression/ERC20Pool/RegressionTestLiquidationERC20Pool.t.sol#L286

two to three lines just describing what was failing and what fix we made to resolve.

added with 9b7eb7b

ith-harvey · 2023-04-26T19:08:04Z

tests/forge/invariants/ERC721Pool/BasicERC721PoolInvariants.t.sol

        uint256[] memory collateralBuckets = IBaseHandler(_handler).getCollateralBuckets();
        for (uint256 i = 0; i < collateralBuckets.length; i++) {
            uint256 bucketIndex = collateralBuckets[i];
-            (, collateral, , , ) = _erc721pool.bucketInfo(bucketIndex);
-            bucketCollateral += collateral;
+            if (bucketIndex != MAX_FENWICK_INDEX) {


I don't fully understand the scenario in which this happens therefore my suggestion may be incorrect. However I believe that if collateral exists in any bucket, including the MAX_FENWICK_INDEX it should be returned by the getCollateralBuckets() call on LN 82 above.

I think this fix ensuring that the max fenwick index bucket is added to collateralBuckets should actually be done somewhere in the handler _settleAuction() logic here -> https://github.com/ajna-finance/contracts/blob/a5e00c268af3cca9dac047007c1efac91894087c/tests/forge/invariants/base/handlers/unbounded/UnboundedLiquidationPoolHandler.sol#L211

invariant should always loop and account collateral from all buckets, this should be independent from any test setup as _settleAuction. getCollateralBuckets() was added to make this faster but miss the bucket 7338

hmm, so why not handle this specific case in the handler and ensure that the bucket 7388 gets added to the collateralBuckets enumerable set like we do here? -> https://github.com/ajna-finance/contracts/blob/a5e00c268af3cca9dac047007c1efac91894087c/tests/forge/invariants/base/handlers/unbounded/UnboundedLiquidationPoolHandler.sol#L291

My concern is that if we create another invariant that expects the collateralBuckets enumerator to include all buckets with collateral we would have to cover this same edge case in that invariant as well versus just doing the fix once in the handler.

Maybe I'm missing something?

the auction can be settled in other actions like take, bucket take, repay debt, pledge collateral. We should avoid putting and duplicating this logic instead just making sure in one place (that is invariant) that we account collateral in that bucket (even when 0). This test was failing because bucket is accounted only in settle and not in bucket take exercised in test.

hmm, so why not handle this specific case in the handler and ensure that the bucket 7388 gets added to the collateralBuckets enumerable set like we do here? ->

https://github.com/ajna-finance/contracts/blob/a5e00c268af3cca9dac047007c1efac91894087c/tests/forge/invariants/base/handlers/unbounded/UnboundedLiquidationPoolHandler.sol#L291

This line should go

hmm, so why not handle this specific case in the handler and ensure that the bucket 7388 gets added to the collateralBuckets enumerable set like we do here? ->
https://github.com/ajna-finance/contracts/blob/a5e00c268af3cca9dac047007c1efac91894087c/tests/forge/invariants/base/handlers/unbounded/UnboundedLiquidationPoolHandler.sol#L291

This line should go

Hmm do you also think these lines should go to? -> https://github.com/ajna-finance/contracts/blob/a5e00c268af3cca9dac047007c1efac91894087c/tests/forge/invariants/base/handlers/unbounded/UnboundedLiquidationPoolHandler.sol#L127

If you think this should be handled in the invariant instead of the handler (which seems to be current convention) then I'm OK with it, not the end of the world. I guess I just see it as breaking convention is all 🤷

hmm, so why not handle this specific case in the handler and ensure that the bucket 7388 gets added to the collateralBuckets enumerable set like we do here? ->
https://github.com/ajna-finance/contracts/blob/a5e00c268af3cca9dac047007c1efac91894087c/tests/forge/invariants/base/handlers/unbounded/UnboundedLiquidationPoolHandler.sol#L291

This line should go

Hmm do you also think these lines should go to? ->

https://github.com/ajna-finance/contracts/blob/a5e00c268af3cca9dac047007c1efac91894087c/tests/forge/invariants/base/handlers/unbounded/UnboundedLiquidationPoolHandler.sol#L127

correct

If you think this should be handled in the invariant instead of the handler (which seems to be current convention) then I'm OK with it, not the end of the world. I guess I just see it as breaking convention is all 🤷

We should keep invariants logic in handler at minimum

If you think this should be handled in the invariant instead of the handler (which seems to be current convention) then I'm OK with it, not the end of the world. I guess I just see it as breaking convention is all shrug

even I am not 💯 onboard with the approach, I reverted the invariant change and fixed for now by recording bucket 7388 in bucketTake see 9b7eb7b

EdNoepel

After some consideration, I'm fine with the test harness manually counting collateral in bucket 7388. The getCollateralBuckets() facility is only there to prevent iteration through the entire pool.

…ed regression test comments.

* changes needed for contract verification (#764) * changes needed for contract verification * gotcha * Make NFT tokens claimable after `take`, `bucketTake`, `settle` actions (#763) * Add test failure * When ERC721.bucketTake always rebalance token ids from borrower to pool claimable array * Consistent take with bucketTake, always rebalance tokens * Check invariants in regression test * Always rebalance tokens on settle (settle could enter with 0 collateral but still with bad debt) * Changed epsilon from 1e16 to 1e17 in F1 and F2 invariants (#765) * Add variable token precision for ERC721Pool invariants (#755) * Add multiple precision of quote token in erc721Pool invariants * Add shebang and set -e option * Invariant framework improvements (#769) * Update forge library, set default env vars using envOr instead reading from env file * Remove InvariantsTestHelpers contract and move invariants helper functions to BaseInvariants * env cleanup for invariants * Update invariant testing scripts * Add skipTime in merge collateral handler * Add fuzzed time skip between kick and take/bucketTake --------- Co-authored-by: grandizzy <[email protected]> * Removing brownie TODOs as these tests won't be further expanded (#767) * ERC20 R1_R2_R3_R4_R5_R6_R7_R8 invariant failure (#771) * Add failing test * Change cutoff for deminimus check to 1 LP in a bucket for exchange rate diffs --------- Co-authored-by: mwc <[email protected]> * Erc721 CT2 invariant failure (#770) * Reproduce CT2 invariant failure * Add collateral that could be compensated in bucket 7388 when auction settled * Make sure MAX bucket not accounted twice. Fix test-regression target * Fix bucketTake invariant logic in place instead in CT2 invariant. Added regression test comments. * Fix CI, lock foundry version until resolution for foundry-rs/foundry#4835 --------- Co-authored-by: Ed Noepel <[email protected]> Co-authored-by: Prateek Gupta <[email protected]> Co-authored-by: mwc <[email protected]>

* initial draft * got initial scaffolding running * included Matts invaraints * added PM1 and PM5 * Reward invariants merged (#773) * changes needed for contract verification (#764) * changes needed for contract verification * gotcha * Make NFT tokens claimable after `take`, `bucketTake`, `settle` actions (#763) * Add test failure * When ERC721.bucketTake always rebalance token ids from borrower to pool claimable array * Consistent take with bucketTake, always rebalance tokens * Check invariants in regression test * Always rebalance tokens on settle (settle could enter with 0 collateral but still with bad debt) * Changed epsilon from 1e16 to 1e17 in F1 and F2 invariants (#765) * Add variable token precision for ERC721Pool invariants (#755) * Add multiple precision of quote token in erc721Pool invariants * Add shebang and set -e option * Invariant framework improvements (#769) * Update forge library, set default env vars using envOr instead reading from env file * Remove InvariantsTestHelpers contract and move invariants helper functions to BaseInvariants * env cleanup for invariants * Update invariant testing scripts * Add skipTime in merge collateral handler * Add fuzzed time skip between kick and take/bucketTake --------- Co-authored-by: grandizzy <[email protected]> * Removing brownie TODOs as these tests won't be further expanded (#767) * ERC20 R1_R2_R3_R4_R5_R6_R7_R8 invariant failure (#771) * Add failing test * Change cutoff for deminimus check to 1 LP in a bucket for exchange rate diffs --------- Co-authored-by: mwc <[email protected]> * Erc721 CT2 invariant failure (#770) * Reproduce CT2 invariant failure * Add collateral that could be compensated in bucket 7388 when auction settled * Make sure MAX bucket not accounted twice. Fix test-regression target * Fix bucketTake invariant logic in place instead in CT2 invariant. Added regression test comments. * Fix CI, lock foundry version until resolution for foundry-rs/foundry#4835 --------- Co-authored-by: Ed Noepel <[email protected]> Co-authored-by: Prateek Gupta <[email protected]> Co-authored-by: mwc <[email protected]> * working through PM3 * wrapped position calls * made getPoolAccumulators a free function & cleaned up logging * included free function * increased logging robustness * working through adding positions without passing positions to baseclass * cleanup * Fix position manager invariant testing setup * Fix EVM reverts * Fix some assertions in moveliquidity handler * Move post action checks in try to run only when transaction is successful * position man initially working * removed redundant invariant check * got rewards structure running * Update Rewards Manager handler structure * Remove redundancy in Rewards Handler * regression PM1 failing * Fix invariant PM1 * Fix invariant RW1 * Add remaining rewards manager handlers * Add drawDebt before kick reserve auction to generate some reserves * Fix drawDebt evm revert in preUnstake * handled broken rewardsCap * Fixed evm revert and PM1 invariant * Add failing regression test * Update make commands for Position and Rewards Manager * added check in moveLiquidity that ensure QT amount for actor doesn't change * cleaned up * revised invariants per Prototech labs suggestion * cleanup invar * added update interest to fix QT diff on moveLiquidity * working through test failure * resolved fee on move liquidity * working through rewards * initial commit * cleanup * low haning fruit feedback * fixed takeReserves in rewards invariant * added NFT position tracking for rewards manager * cleanup * Add comments for failing regression test * responded to Mikes feedback * EOF line addition * cleaned up requires, formally added more PositionManager invar * Quote to lps inc on move (#877) * cleaned up requires, formally added more PositionManager invar * cleaned up requires, formally added more PositionManager invar * added logs * works with diff * cleanup --------- Co-authored-by: Ian Harvey <[email protected]> * cleaned up rewards and position naming * responded to class naming * cleaned up spaced --------- Co-authored-by: Ian Harvey <[email protected]> Co-authored-by: grandizzy <[email protected]> Co-authored-by: Ed Noepel <[email protected]> Co-authored-by: Prateek Gupta <[email protected]> Co-authored-by: mwc <[email protected]> Co-authored-by: prateek105 <[email protected]>

* initial draft * got initial scaffolding running * included Matts invaraints * added PM1 and PM5 * Reward invariants merged (#773) * changes needed for contract verification (#764) * changes needed for contract verification * gotcha * Make NFT tokens claimable after `take`, `bucketTake`, `settle` actions (#763) * Add test failure * When ERC721.bucketTake always rebalance token ids from borrower to pool claimable array * Consistent take with bucketTake, always rebalance tokens * Check invariants in regression test * Always rebalance tokens on settle (settle could enter with 0 collateral but still with bad debt) * Changed epsilon from 1e16 to 1e17 in F1 and F2 invariants (#765) * Add variable token precision for ERC721Pool invariants (#755) * Add multiple precision of quote token in erc721Pool invariants * Add shebang and set -e option * Invariant framework improvements (#769) * Update forge library, set default env vars using envOr instead reading from env file * Remove InvariantsTestHelpers contract and move invariants helper functions to BaseInvariants * env cleanup for invariants * Update invariant testing scripts * Add skipTime in merge collateral handler * Add fuzzed time skip between kick and take/bucketTake --------- Co-authored-by: grandizzy <[email protected]> * Removing brownie TODOs as these tests won't be further expanded (#767) * ERC20 R1_R2_R3_R4_R5_R6_R7_R8 invariant failure (#771) * Add failing test * Change cutoff for deminimus check to 1 LP in a bucket for exchange rate diffs --------- Co-authored-by: mwc <[email protected]> * Erc721 CT2 invariant failure (#770) * Reproduce CT2 invariant failure * Add collateral that could be compensated in bucket 7388 when auction settled * Make sure MAX bucket not accounted twice. Fix test-regression target * Fix bucketTake invariant logic in place instead in CT2 invariant. Added regression test comments. * Fix CI, lock foundry version until resolution for foundry-rs/foundry#4835 --------- Co-authored-by: Ed Noepel <[email protected]> Co-authored-by: Prateek Gupta <[email protected]> Co-authored-by: mwc <[email protected]> * working through PM3 * wrapped position calls * made getPoolAccumulators a free function & cleaned up logging * included free function * increased logging robustness * working through adding positions without passing positions to baseclass * cleanup * Fix position manager invariant testing setup * Fix EVM reverts * Fix some assertions in moveliquidity handler * Move post action checks in try to run only when transaction is successful * position man initially working * removed redundant invariant check * got rewards structure running * Update Rewards Manager handler structure * Remove redundancy in Rewards Handler * regression PM1 failing * Fix invariant PM1 * Fix invariant RW1 * Add remaining rewards manager handlers * Add drawDebt before kick reserve auction to generate some reserves * Fix drawDebt evm revert in preUnstake * handled broken rewardsCap * Fixed evm revert and PM1 invariant * Add failing regression test * Update make commands for Position and Rewards Manager * added check in moveLiquidity that ensure QT amount for actor doesn't change * cleaned up * revised invariants per Prototech labs suggestion * cleanup invar * added update interest to fix QT diff on moveLiquidity * working through test failure * resolved fee on move liquidity * working through rewards * initial commit * cleanup * low haning fruit feedback * fixed takeReserves in rewards invariant * added NFT position tracking for rewards manager * cleanup * Add comments for failing regression test * responded to Mikes feedback * EOF line addition * added * cleaned up requires, formally added more PositionManager invar * Quote to lps inc on move (#877) * cleaned up requires, formally added more PositionManager invar * cleaned up requires, formally added more PositionManager invar * added logs * works with diff * cleanup --------- Co-authored-by: Ian Harvey <[email protected]> * cleaned up rewards and position naming * responded to class naming * cleaned up spaced * working through rewards, tests passing * working through invariants * rewards working cleanly * clean up before looking into RW5 * finnished adding RW6 * cleanup * comment cleanup * updated with emergency unstake * cleaned up error handling --------- Co-authored-by: Ian Harvey <[email protected]> Co-authored-by: grandizzy <[email protected]> Co-authored-by: Ed Noepel <[email protected]> Co-authored-by: Prateek Gupta <[email protected]> Co-authored-by: mwc <[email protected]> Co-authored-by: prateek105 <[email protected]>

Reproduce CT2 invariant failure

2e2d2f6

grandizzy added Priority: Critical Status: Work In Progress Scope: Bug fix labels Apr 26, 2023

Add collateral that could be compensated in bucket 7388 when auction …

0177e20

…settled

grandizzy added Scope: Tests and removed Scope: Bug fix labels Apr 26, 2023

grandizzy requested review from ith-harvey, mattcushman and EdNoepel April 26, 2023 18:17

EdNoepel reviewed Apr 26, 2023

View reviewed changes

tests/forge/invariants/ERC721Pool/BasicERC721PoolInvariants.t.sol Outdated Show resolved Hide resolved

Make sure MAX bucket not accounted twice. Fix test-regression target

385c32c

grandizzy requested a review from EdNoepel April 26, 2023 18:40

ith-harvey reviewed Apr 26, 2023

View reviewed changes

grandizzy added Status: Ready for Review and removed Status: Work In Progress labels Apr 26, 2023

EdNoepel approved these changes Apr 26, 2023

View reviewed changes

grandizzy added 2 commits April 27, 2023 08:51

Merge branch 'develop' into erc721-CT2-failure

54732f2

Fic bucketTake invariant logic in place instead in CT2 invariant. Add…

9b7eb7b

…ed regression test comments.

grandizzy merged commit 951a658 into develop Apr 27, 2023

grandizzy deleted the erc721-CT2-failure branch April 27, 2023 06:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Erc721 CT2 invariant failure #770

Erc721 CT2 invariant failure #770

grandizzy commented Apr 26, 2023 •

edited

Loading

ith-harvey Apr 26, 2023

grandizzy Apr 26, 2023

ith-harvey Apr 26, 2023

grandizzy Apr 27, 2023

ith-harvey Apr 26, 2023 •

edited

Loading

grandizzy Apr 26, 2023

ith-harvey Apr 26, 2023

grandizzy Apr 26, 2023 •

edited

Loading

grandizzy Apr 26, 2023

ith-harvey Apr 26, 2023

ith-harvey Apr 26, 2023

grandizzy Apr 26, 2023

grandizzy Apr 26, 2023

grandizzy Apr 27, 2023

EdNoepel left a comment

Erc721 CT2 invariant failure #770

Erc721 CT2 invariant failure #770

Conversation

grandizzy commented Apr 26, 2023 • 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

ith-harvey Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grandizzy Apr 26, 2023 • 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

EdNoepel left a comment

Choose a reason for hiding this comment

grandizzy commented Apr 26, 2023 •

edited

Loading

ith-harvey Apr 26, 2023 •

edited

Loading

grandizzy Apr 26, 2023 •

edited

Loading