From 2e2d2f68219d94263f257231eb0c6057a2f5e118 Mon Sep 17 00:00:00 2001 From: grandizzy Date: Wed, 26 Apr 2023 20:44:57 +0300 Subject: [PATCH 1/4] Reproduce CT2 invariant failure --- ...> RegressionTestLiquidationERC721Pool.t.sol} | 0 .../RegressionTestReservesERC721Pool.t.sol | 17 +++++++++++++++++ 2 files changed, 17 insertions(+) rename tests/forge/regression/ERC721Pool/{RegressionTestLiquidationERC20Pool.t.sol => RegressionTestLiquidationERC721Pool.t.sol} (100%) diff --git a/tests/forge/regression/ERC721Pool/RegressionTestLiquidationERC20Pool.t.sol b/tests/forge/regression/ERC721Pool/RegressionTestLiquidationERC721Pool.t.sol similarity index 100% rename from tests/forge/regression/ERC721Pool/RegressionTestLiquidationERC20Pool.t.sol rename to tests/forge/regression/ERC721Pool/RegressionTestLiquidationERC721Pool.t.sol diff --git a/tests/forge/regression/ERC721Pool/RegressionTestReservesERC721Pool.t.sol b/tests/forge/regression/ERC721Pool/RegressionTestReservesERC721Pool.t.sol index 713dbc32f..e87b38c6f 100644 --- a/tests/forge/regression/ERC721Pool/RegressionTestReservesERC721Pool.t.sol +++ b/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 { + _reserveERC721PoolHandler.settleAuction(13652854, 3, 22274361584262295180502534344873136686717874240, 77611568702503302987473072664549443425918559); + _reserveERC721PoolHandler.withdrawBonds(707, 12156087, 19174970663707445513928200315780515094988880044); + _reserveERC721PoolHandler.kickWithDeposit(3, 3, 672444647); + _reserveERC721PoolHandler.kickAuction(56848152111578191493999238385381542863095352, 58529940235731531982925635292876828548122574070883324158957672865569214, 899039491413, 107401523435280391282671144); + _reserveERC721PoolHandler.settleAuction(19634294495748734616428837200, 4937, 77655590346650144951112602856523781688846543008934920669778106922357739827346, 994969230863047393940054601942); + _reserveERC721PoolHandler.removeQuoteToken(115792089237316195423570985008687907853269984665640564039457584007913129639934, 71512, 2, 3); + _reserveERC721PoolHandler.moveQuoteToken(15905680501786579444933931057811252717108353959172, 115792089237316195423570985008687907853269984665640564039457584007913129639934, 271318302293, 0, 40); + _reserveERC721PoolHandler.kickAuction(115792089237316195423570985008687907853269984665640564039457584007913129639935, 1, 273285540430841592066075105397763679903422015958, 266767292382552685109896561555); + _reserveERC721PoolHandler.moveQuoteToken(19067960621863745617958471469, 2999999999999999996834739158156220923161820262, 20085132340471043072465796726, 92001987806333700856071384682550468910212704266158266358190575554223580054768, 8420); + _reserveERC721PoolHandler.pledgeCollateral(107821936054956412679567988, 1727154709158370, 3); + _reserveERC721PoolHandler.mergeCollateral(742, 36731090122697131118614904036939339014023299978771437847280286066139902285955); + _reserveERC721PoolHandler.bucketTake(115792089237316195423570985008687907853269984665640564039457584007913129639933, 115792089237316195423570985008687907853269984665640564039457584007913129639932, true, 115792089237316195423570985008687907853269984665640564039457584007913129639933, 4); + + invariant_CT2(); + } } From 0177e20b92848b24e2b6eecc3a57b4471ccfae9d Mon Sep 17 00:00:00 2001 From: grandizzy Date: Wed, 26 Apr 2023 21:14:46 +0300 Subject: [PATCH 2/4] Add collateral that could be compensated in bucket 7388 when auction settled --- .../invariants/ERC721Pool/BasicERC721PoolInvariants.t.sol | 3 +++ tests/forge/unit/PositionManager.t.sol | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/forge/invariants/ERC721Pool/BasicERC721PoolInvariants.t.sol b/tests/forge/invariants/ERC721Pool/BasicERC721PoolInvariants.t.sol index adcdaa04b..ce7fe42c8 100644 --- a/tests/forge/invariants/ERC721Pool/BasicERC721PoolInvariants.t.sol +++ b/tests/forge/invariants/ERC721Pool/BasicERC721PoolInvariants.t.sol @@ -80,6 +80,9 @@ contract BasicERC721PoolInvariants is BasicInvariants { (, collateral, , , ) = _erc721pool.bucketInfo(bucketIndex); bucketCollateral += collateral; } + // add collateral that could be compensated at MAX_FENWICK_INDEX when auction settled + (, collateral, , , ) = _erc721pool.bucketInfo(7388); + bucketCollateral += collateral; assertEq(collateralBalance, bucketCollateral + _erc721pool.pledgedCollateral(), "Collateral Invariant CT2"); } diff --git a/tests/forge/unit/PositionManager.t.sol b/tests/forge/unit/PositionManager.t.sol index bf3aa409b..f2cc9a7b1 100644 --- a/tests/forge/unit/PositionManager.t.sol +++ b/tests/forge/unit/PositionManager.t.sol @@ -53,7 +53,7 @@ abstract contract PositionManagerERC20PoolHelperContract is ERC20HelperContract uint256 tokenId_, uint256 deadline_, uint256 ownerPrivateKey_ - ) internal returns (uint8 v, bytes32 r, bytes32 s) { + ) internal view returns (uint8 v, bytes32 r, bytes32 s) { return vm.sign( ownerPrivateKey_, keccak256( From 385c32cc46348d0968144d4608ffe2fe3188b500 Mon Sep 17 00:00:00 2001 From: grandizzy Date: Wed, 26 Apr 2023 21:38:40 +0300 Subject: [PATCH 3/4] Make sure MAX bucket not accounted twice. Fix test-regression target --- Makefile | 2 +- .../ERC721Pool/BasicERC721PoolInvariants.t.sol | 18 +++++++++++------- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/Makefile b/Makefile index 6ab572774..e973f16f3 100644 --- a/Makefile +++ b/Makefile @@ -20,7 +20,7 @@ test-load :; forge test --match-test testLoad --gas-report test-invariant :; forge t --mt invariant --nmc RegressionTest test-invariant-erc20 :; forge t --mt invariant --nmc RegressionTest --mc ERC20 test-invariant-erc721 :; forge t --mt invariant --nmc RegressionTest --mc ERC721 -test-regression :; forge t --mt test_regression +test-regression : test-regression-erc20 test-regression-erc721 test-regression-erc20 :; forge t --mt test_regression --mc ERC20 test-regression-erc721 :; forge t --mt test_regression --mc ERC721 coverage :; forge coverage --no-match-test "testLoad|invariant" diff --git a/tests/forge/invariants/ERC721Pool/BasicERC721PoolInvariants.t.sol b/tests/forge/invariants/ERC721Pool/BasicERC721PoolInvariants.t.sol index ce7fe42c8..dc5d4b194 100644 --- a/tests/forge/invariants/ERC721Pool/BasicERC721PoolInvariants.t.sol +++ b/tests/forge/invariants/ERC721Pool/BasicERC721PoolInvariants.t.sol @@ -4,10 +4,11 @@ pragma solidity 0.8.14; import "@std/console.sol"; -import { Pool } from 'src/base/Pool.sol'; +import { Pool } from 'src/base/Pool.sol'; import { ERC721Pool } from 'src/ERC721Pool.sol'; import { ERC721PoolFactory } from 'src/ERC721PoolFactory.sol'; -import { Maths } from 'src/libraries/internal/Maths.sol'; +import { Maths } from 'src/libraries/internal/Maths.sol'; +import { MAX_FENWICK_INDEX } from 'src/libraries/helpers/PoolHelper.sol'; import { NFTCollateralToken } from '../../utils/Tokens.sol'; @@ -74,15 +75,18 @@ contract BasicERC721PoolInvariants is BasicInvariants { uint256 bucketCollateral; uint256 collateral; + // always account collateral at MAX_FENWICK_INDEX (since there could be collateral compensated when auction settled) + (, collateral, , , ) = _erc721pool.bucketInfo(MAX_FENWICK_INDEX); + bucketCollateral += collateral; + 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) { + (, collateral, , , ) = _erc721pool.bucketInfo(bucketIndex); + bucketCollateral += collateral; + } } - // add collateral that could be compensated at MAX_FENWICK_INDEX when auction settled - (, collateral, , , ) = _erc721pool.bucketInfo(7388); - bucketCollateral += collateral; assertEq(collateralBalance, bucketCollateral + _erc721pool.pledgedCollateral(), "Collateral Invariant CT2"); } From 9b7eb7b959486591784469ac8d2d009858787fd9 Mon Sep 17 00:00:00 2001 From: grandizzy Date: Thu, 27 Apr 2023 09:05:54 +0300 Subject: [PATCH 4/4] Fic bucketTake invariant logic in place instead in CT2 invariant. Added regression test comments. --- .../ERC721Pool/BasicERC721PoolInvariants.t.sol | 15 ++++----------- .../unbounded/UnboundedLiquidationPoolHandler.sol | 5 +++++ .../RegressionTestReservesERC721Pool.t.sol | 11 ++++++++++- 3 files changed, 19 insertions(+), 12 deletions(-) diff --git a/tests/forge/invariants/ERC721Pool/BasicERC721PoolInvariants.t.sol b/tests/forge/invariants/ERC721Pool/BasicERC721PoolInvariants.t.sol index dc5d4b194..adcdaa04b 100644 --- a/tests/forge/invariants/ERC721Pool/BasicERC721PoolInvariants.t.sol +++ b/tests/forge/invariants/ERC721Pool/BasicERC721PoolInvariants.t.sol @@ -4,11 +4,10 @@ pragma solidity 0.8.14; import "@std/console.sol"; -import { Pool } from 'src/base/Pool.sol'; +import { Pool } from 'src/base/Pool.sol'; import { ERC721Pool } from 'src/ERC721Pool.sol'; import { ERC721PoolFactory } from 'src/ERC721PoolFactory.sol'; -import { Maths } from 'src/libraries/internal/Maths.sol'; -import { MAX_FENWICK_INDEX } from 'src/libraries/helpers/PoolHelper.sol'; +import { Maths } from 'src/libraries/internal/Maths.sol'; import { NFTCollateralToken } from '../../utils/Tokens.sol'; @@ -75,17 +74,11 @@ contract BasicERC721PoolInvariants is BasicInvariants { uint256 bucketCollateral; uint256 collateral; - // always account collateral at MAX_FENWICK_INDEX (since there could be collateral compensated when auction settled) - (, collateral, , , ) = _erc721pool.bucketInfo(MAX_FENWICK_INDEX); - bucketCollateral += collateral; - uint256[] memory collateralBuckets = IBaseHandler(_handler).getCollateralBuckets(); for (uint256 i = 0; i < collateralBuckets.length; i++) { uint256 bucketIndex = collateralBuckets[i]; - if (bucketIndex != MAX_FENWICK_INDEX) { - (, collateral, , , ) = _erc721pool.bucketInfo(bucketIndex); - bucketCollateral += collateral; - } + (, collateral, , , ) = _erc721pool.bucketInfo(bucketIndex); + bucketCollateral += collateral; } assertEq(collateralBalance, bucketCollateral + _erc721pool.pledgedCollateral(), "Collateral Invariant CT2"); diff --git a/tests/forge/invariants/base/handlers/unbounded/UnboundedLiquidationPoolHandler.sol b/tests/forge/invariants/base/handlers/unbounded/UnboundedLiquidationPoolHandler.sol index 646404cad..8b2aeaab6 100644 --- a/tests/forge/invariants/base/handlers/unbounded/UnboundedLiquidationPoolHandler.sol +++ b/tests/forge/invariants/base/handlers/unbounded/UnboundedLiquidationPoolHandler.sol @@ -188,6 +188,11 @@ abstract contract UnboundedLiquidationPoolHandler is BaseHandler { // **CT2**: Keep track of bucketIndex when borrower is removed from auction to check collateral added into that bucket (, , , uint256 kickTime, , , , , , ) = _pool.auctionInfo(borrower_); if (kickTime == 0 && _pool.poolType() == 1) { + if (auctionPrice < MIN_PRICE) { + collateralBuckets.add(7388); + } else if (auctionPrice > MAX_PRICE) { + collateralBuckets.add(0); + } collateralBuckets.add(auctionBucketIndex); if (beforeBucketTakeVars.borrowerLps < afterBucketTakeVars.borrowerLps) { lenderDepositTime[borrower_][auctionBucketIndex] = block.timestamp; diff --git a/tests/forge/regression/ERC721Pool/RegressionTestReservesERC721Pool.t.sol b/tests/forge/regression/ERC721Pool/RegressionTestReservesERC721Pool.t.sol index e87b38c6f..5ccb59d12 100644 --- a/tests/forge/regression/ERC721Pool/RegressionTestReservesERC721Pool.t.sol +++ b/tests/forge/regression/ERC721Pool/RegressionTestReservesERC721Pool.t.sol @@ -96,6 +96,11 @@ contract RegressionTestReserveERC721Pool is ReserveERC721PoolInvariants { invariant_Bucket_deposit_time_B5_B6_B7(); } + /* + Test was failing due to collateral not made claimable after actions that can reduce borrower pledged collateral (bucketTake, settle). + In this scenario `settle` was called on a loan with no collateral but bad debt. + Fixed by always making tokens claimable after take, bucketTake and settle actions. + */ function test_regression_erc721_evm_revert_1() external { _reserveERC721PoolHandler.settleAuction(9989243900619820637977810558874905516372668734956884150787421704623, 18550308766242836156918, 185813535265204352484610945242967379275287026502359577631531764507799333257, 0); _reserveERC721PoolHandler.settleAuction(3978325917508522510207263223865211237976, 7790053814939864208425264498, 999999999999999996743786245260429581471869387, 0); @@ -238,7 +243,11 @@ contract RegressionTestReserveERC721Pool is ReserveERC721PoolInvariants { invariant_Buckets_B2_B3(); } - function test_regression_erc721_CT2() external { + /* + Test was failing due to collateral in bucket 7388 not accounted when totaling buckets collateral. + Fixed by adding bucket 7388 in `collateralBuckets` array also when bucket take settles the auction. + */ + function test_regression_CT2_3() external { _reserveERC721PoolHandler.settleAuction(13652854, 3, 22274361584262295180502534344873136686717874240, 77611568702503302987473072664549443425918559); _reserveERC721PoolHandler.withdrawBonds(707, 12156087, 19174970663707445513928200315780515094988880044); _reserveERC721PoolHandler.kickWithDeposit(3, 3, 672444647);