From 771bb3ddfdb9b4e4cf3793829f6dda2c5fbdc3b0 Mon Sep 17 00:00:00 2001 From: nelson-pereira8 <94120714+nican0r@users.noreply.github.com> Date: Mon, 21 Oct 2024 17:16:25 -0300 Subject: [PATCH 1/5] chore: unit tests for broken properties --- .../TestContracts/invariants/Setup.sol | 7 +-- .../foundry_test/ForkToFoundry.t.sol | 50 +++++++++++++++++-- 2 files changed, 51 insertions(+), 6 deletions(-) diff --git a/packages/contracts/contracts/TestContracts/invariants/Setup.sol b/packages/contracts/contracts/TestContracts/invariants/Setup.sol index a9c6ca089..a74ab34b9 100644 --- a/packages/contracts/contracts/TestContracts/invariants/Setup.sol +++ b/packages/contracts/contracts/TestContracts/invariants/Setup.sol @@ -341,12 +341,13 @@ abstract contract Setup is BaseStorageVariables, PropertiesConstants { event Log(string); // This is a fix to allow facilitate dynamic replacement that searches for the `vm.roll` statements. - IHevm constant vm = IHevm(0x7109709ECfa91a80626fF3989D68f67F5b1DD12D); function _setUpFork() internal { + IHevm vm = IHevm(0x7109709ECfa91a80626fF3989D68f67F5b1DD12D); + // Add timestamp and block for Recon dynamic replacement - vm.roll(20770509); - vm.warp(1726578263); + // vm.roll(20996709); + // vm.warp(1729305851); // NOTE: Addresses from: https://gist.github.com/GalloDaSballo/75d77f8d0837821156fe061d0d8687e1 defaultGovernance = address(0xaDDeE229Bd103bb5B10C3CdB595A01c425dd3264); diff --git a/packages/contracts/foundry_test/ForkToFoundry.t.sol b/packages/contracts/foundry_test/ForkToFoundry.t.sol index 0e7454863..88a685196 100644 --- a/packages/contracts/foundry_test/ForkToFoundry.t.sol +++ b/packages/contracts/foundry_test/ForkToFoundry.t.sol @@ -26,9 +26,11 @@ contract ForkToFoundry is BeforeAfterWithLogging { function setUp() public { - vm.createSelectFork("YOUR_RPC_URL", 20777211); + string memory MAINNET_RPC_URL = vm.envString("MAINNET_RPC_URL"); + vm.createSelectFork(MAINNET_RPC_URL, 20996709); // NOTE: changed block to match coverage report _setUpFork(); - _setUpActors(); + // _setUpActors(); + _setUpActorsFork(); actor = actors[address(USER1)]; // If the accounting hasn't been synced since the last rebase @@ -56,10 +58,52 @@ contract ForkToFoundry is // forge test --match-test test_asserts_GENERAL_12_0 -vv function test_asserts_GENERAL_12_0() public { - vm.roll(block.number + 4963); vm.warp(block.timestamp + 50417); asserts_GENERAL_12(); + } + + // forge test --match-test test_asserts_GENERAL_12_1 -vv + function test_asserts_GENERAL_12_1() public { + // NOTE: from reproducer test immediately breaks but when asserts_test_fail is commented it doesn't + // vm.roll(block.number + 60364); + // vm.warp(block.timestamp + 11077); + // asserts_active_pool_invariant_5(); + + // vm.roll(block.number + 1984); + // vm.warp(block.timestamp + 322370); + // asserts_test_fail(); + + // vm.roll(block.number + 33560); + // vm.warp(block.timestamp + 95); + // asserts_GENERAL_12(); + // ======================== + + // NOTE: from shrunken logs breaks immediately + vm.roll(block.number + 1); + vm.warp(block.timestamp + 2973); + asserts_GENERAL_12(); + } + + // forge test --match-test test_asserts_GENERAL_13_2 -vv + // @audit this fails for all blocks/timestamps, including the initial block/timestamp setup was forked from + function test_asserts_GENERAL_13_2() public { + // from shrunken logs + // vm.roll(block.number + 1); + // vm.warp(block.timestamp + 2963); + + // from reproducer + // vm.roll(block.number + 60471); + // vm.warp(block.timestamp + 6401); + + // fails for the initial fork block and all others + console2.log("block.number == 20996709: ", block.number == 20996709); + console2.log("block.timestamp == 1729305851: ", block.timestamp == 1729305851); + + asserts_GENERAL_13(); + } + function test_asserts_GENERAL_14() public { + asserts_GENERAL_14(); } } From 3ad19c54ac9dd9d4e98ddb876709605d4d6e8b4b Mon Sep 17 00:00:00 2001 From: nelson-pereira8 <94120714+nican0r@users.noreply.github.com> Date: Tue, 22 Oct 2024 09:02:08 -0300 Subject: [PATCH 2/5] chore: fixing setup for ForkToFoundry to replicate EchidnaForkTester --- .../foundry_test/ForkToFoundry.t.sol | 43 ++++++++++--------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/packages/contracts/foundry_test/ForkToFoundry.t.sol b/packages/contracts/foundry_test/ForkToFoundry.t.sol index 88a685196..5dae86c7e 100644 --- a/packages/contracts/foundry_test/ForkToFoundry.t.sol +++ b/packages/contracts/foundry_test/ForkToFoundry.t.sol @@ -44,7 +44,11 @@ contract ForkToFoundry is // Previous cumulative CDPs per each rebase // Will need to be adjusted - vars.cumulativeCdpsAtTimeOfRebase = 200; + // @audit removed because inconsistent with EchidnaForkTester setup + // vars.cumulativeCdpsAtTimeOfRebase = 200; + + // NOTE: this is essential to get reproducer tests to work correctly + _setUpCdpFork(); } // forge test --match-test test_asserts_GENERAL_13_1 -vv @@ -56,7 +60,7 @@ contract ForkToFoundry is } - // forge test --match-test test_asserts_GENERAL_12_0 -vv + // forge test --match-test test_asserts_GENERAL_12_0 -vv function test_asserts_GENERAL_12_0() public { vm.roll(block.number + 4963); vm.warp(block.timestamp + 50417); @@ -64,45 +68,44 @@ contract ForkToFoundry is } // forge test --match-test test_asserts_GENERAL_12_1 -vv + // @audit passes without warping function test_asserts_GENERAL_12_1() public { // NOTE: from reproducer test immediately breaks but when asserts_test_fail is commented it doesn't - // vm.roll(block.number + 60364); - // vm.warp(block.timestamp + 11077); - // asserts_active_pool_invariant_5(); + vm.roll(block.number + 60364); + vm.warp(block.timestamp + 11077); + asserts_active_pool_invariant_5(); + // NOTE: removing this assertion and warp causes a failure // vm.roll(block.number + 1984); // vm.warp(block.timestamp + 322370); // asserts_test_fail(); - // vm.roll(block.number + 33560); - // vm.warp(block.timestamp + 95); - // asserts_GENERAL_12(); + vm.roll(block.number + 33560); + vm.warp(block.timestamp + 95); + asserts_GENERAL_12(); // ======================== // NOTE: from shrunken logs breaks immediately - vm.roll(block.number + 1); - vm.warp(block.timestamp + 2973); - asserts_GENERAL_12(); + // vm.roll(block.number + 1); + // vm.warp(block.timestamp + 2973); + // asserts_GENERAL_12(); } // forge test --match-test test_asserts_GENERAL_13_2 -vv - // @audit this fails for all blocks/timestamps, including the initial block/timestamp setup was forked from + // @audit this fails only for blocks/timestamps at which echidna tests fail function test_asserts_GENERAL_13_2() public { - // from shrunken logs + // NOTE: from shrunken logs // vm.roll(block.number + 1); // vm.warp(block.timestamp + 2963); - // from reproducer - // vm.roll(block.number + 60471); - // vm.warp(block.timestamp + 6401); - - // fails for the initial fork block and all others - console2.log("block.number == 20996709: ", block.number == 20996709); - console2.log("block.timestamp == 1729305851: ", block.timestamp == 1729305851); + // NOTE: from reproducer + vm.roll(block.number + 60471); + vm.warp(block.timestamp + 6401); asserts_GENERAL_13(); } + // NOTE: sanity check function test_asserts_GENERAL_14() public { asserts_GENERAL_14(); } From 159808d1cbe3a464ce8f6c4160c18d1c5ba7ad5b Mon Sep 17 00:00:00 2001 From: nelson-pereira8 <94120714+nican0r@users.noreply.github.com> Date: Tue, 22 Oct 2024 11:07:55 -0300 Subject: [PATCH 3/5] fix: use fetchPrice to get price in properties --- .../TestContracts/invariants/Properties.sol | 6 ++-- .../foundry_test/ForkToFoundry.t.sol | 33 ++++++++----------- 2 files changed, 16 insertions(+), 23 deletions(-) diff --git a/packages/contracts/contracts/TestContracts/invariants/Properties.sol b/packages/contracts/contracts/TestContracts/invariants/Properties.sol index e4c88be9c..696bfe658 100644 --- a/packages/contracts/contracts/TestContracts/invariants/Properties.sol +++ b/packages/contracts/contracts/TestContracts/invariants/Properties.sol @@ -466,8 +466,8 @@ abstract contract Properties is BeforeAfter, PropertiesDescriptions, Asserts, Pr PriceFeedTestnet priceFeedMock, CRLens crLens ) internal returns (bool) { - uint256 curentPrice = priceFeedMock.lastGoodPrice(); - return crLens.quoteRealTCR() == cdpManager.getSyncedTCR(curentPrice); + uint256 currentPrice = priceFeedMock.fetchPrice(); + return crLens.quoteRealTCR() == cdpManager.getSyncedTCR(currentPrice); } function invariant_GENERAL_13( @@ -478,7 +478,7 @@ abstract contract Properties is BeforeAfter, PropertiesDescriptions, Asserts, Pr ) internal returns (bool) { bytes32 currentCdp = sortedCdps.getFirst(); - uint256 _price = priceFeedMock.lastGoodPrice(); + uint256 _price = priceFeedMock.fetchPrice(); // Compare synched with quote for all Cdps while (currentCdp != bytes32(0)) { diff --git a/packages/contracts/foundry_test/ForkToFoundry.t.sol b/packages/contracts/foundry_test/ForkToFoundry.t.sol index 5dae86c7e..0eb21b592 100644 --- a/packages/contracts/foundry_test/ForkToFoundry.t.sol +++ b/packages/contracts/foundry_test/ForkToFoundry.t.sol @@ -68,31 +68,29 @@ contract ForkToFoundry is } // forge test --match-test test_asserts_GENERAL_12_1 -vv - // @audit passes without warping function test_asserts_GENERAL_12_1() public { // NOTE: from reproducer test immediately breaks but when asserts_test_fail is commented it doesn't - vm.roll(block.number + 60364); - vm.warp(block.timestamp + 11077); - asserts_active_pool_invariant_5(); + // vm.roll(block.number + 60364); + // vm.warp(block.timestamp + 11077); + // asserts_active_pool_invariant_5(); - // NOTE: removing this assertion and warp causes a failure - // vm.roll(block.number + 1984); - // vm.warp(block.timestamp + 322370); - // asserts_test_fail(); + // // NOTE: removing this assertion and warp causes a failure + // // vm.roll(block.number + 1984); + // // vm.warp(block.timestamp + 322370); + // // asserts_test_fail(); - vm.roll(block.number + 33560); - vm.warp(block.timestamp + 95); - asserts_GENERAL_12(); + // vm.roll(block.number + 33560); + // vm.warp(block.timestamp + 95); + // asserts_GENERAL_12(); // ======================== // NOTE: from shrunken logs breaks immediately - // vm.roll(block.number + 1); - // vm.warp(block.timestamp + 2973); - // asserts_GENERAL_12(); + vm.roll(block.number + 1); + vm.warp(block.timestamp + 2973); + asserts_GENERAL_12(); } // forge test --match-test test_asserts_GENERAL_13_2 -vv - // @audit this fails only for blocks/timestamps at which echidna tests fail function test_asserts_GENERAL_13_2() public { // NOTE: from shrunken logs // vm.roll(block.number + 1); @@ -104,9 +102,4 @@ contract ForkToFoundry is asserts_GENERAL_13(); } - - // NOTE: sanity check - function test_asserts_GENERAL_14() public { - asserts_GENERAL_14(); - } } From c20de21a2dfcb6e7d59b8b2ac23fa78ea6cd2e88 Mon Sep 17 00:00:00 2001 From: nelson-pereira8 <94120714+nican0r@users.noreply.github.com> Date: Tue, 22 Oct 2024 11:13:49 -0300 Subject: [PATCH 4/5] chore: cleanup setup and add notes about testing locally --- .../contracts/contracts/TestContracts/invariants/Setup.sol | 4 ++-- packages/contracts/foundry_test/ForkToFoundry.t.sol | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/contracts/contracts/TestContracts/invariants/Setup.sol b/packages/contracts/contracts/TestContracts/invariants/Setup.sol index a74ab34b9..91d8ec515 100644 --- a/packages/contracts/contracts/TestContracts/invariants/Setup.sol +++ b/packages/contracts/contracts/TestContracts/invariants/Setup.sol @@ -346,8 +346,8 @@ abstract contract Setup is BaseStorageVariables, PropertiesConstants { IHevm vm = IHevm(0x7109709ECfa91a80626fF3989D68f67F5b1DD12D); // Add timestamp and block for Recon dynamic replacement - // vm.roll(20996709); - // vm.warp(1729305851); + vm.roll(20996709); + vm.warp(1729305851); // NOTE: Addresses from: https://gist.github.com/GalloDaSballo/75d77f8d0837821156fe061d0d8687e1 defaultGovernance = address(0xaDDeE229Bd103bb5B10C3CdB595A01c425dd3264); diff --git a/packages/contracts/foundry_test/ForkToFoundry.t.sol b/packages/contracts/foundry_test/ForkToFoundry.t.sol index 0eb21b592..3c0bc02e0 100644 --- a/packages/contracts/foundry_test/ForkToFoundry.t.sol +++ b/packages/contracts/foundry_test/ForkToFoundry.t.sol @@ -27,9 +27,10 @@ contract ForkToFoundry is { function setUp() public { string memory MAINNET_RPC_URL = vm.envString("MAINNET_RPC_URL"); - vm.createSelectFork(MAINNET_RPC_URL, 20996709); // NOTE: changed block to match coverage report + // TODO: when testing locally change this block with block from coverage report set inside _setUpFork + vm.createSelectFork(MAINNET_RPC_URL, 20996709); + _setUpFork(); - // _setUpActors(); _setUpActorsFork(); actor = actors[address(USER1)]; @@ -47,7 +48,6 @@ contract ForkToFoundry is // @audit removed because inconsistent with EchidnaForkTester setup // vars.cumulativeCdpsAtTimeOfRebase = 200; - // NOTE: this is essential to get reproducer tests to work correctly _setUpCdpFork(); } From 56e1dc978179a55715b844fdaa09ce11ba9dbc47 Mon Sep 17 00:00:00 2001 From: nelson-pereira8 <94120714+nican0r@users.noreply.github.com> Date: Wed, 23 Oct 2024 09:41:12 -0300 Subject: [PATCH 5/5] chore: updating Foundry readme for info about running broken property reproducers --- packages/contracts/.env.example | 1 + packages/contracts/README.md | 16 ++++++++++++++-- 2 files changed, 15 insertions(+), 2 deletions(-) create mode 100644 packages/contracts/.env.example diff --git a/packages/contracts/.env.example b/packages/contracts/.env.example new file mode 100644 index 000000000..7559d0124 --- /dev/null +++ b/packages/contracts/.env.example @@ -0,0 +1 @@ +MAINNET_RPC_URL="" \ No newline at end of file diff --git a/packages/contracts/README.md b/packages/contracts/README.md index 708cc153c..9ae825404 100644 --- a/packages/contracts/README.md +++ b/packages/contracts/README.md @@ -4,11 +4,22 @@ Use this guide to install foundry: https://book.getfoundry.sh/getting-started/installation -## Running foundry tests +## Running Foundry tests - Simply `cd` into target test package: - `cd packages/contracts` - Run `forge test` +## Running Foundry fork tests for broken property reproducers +To run Foundry fork tests for reproducers: +- Add the reproducer test for the broken property to the `ForkToFoundry` contract +- Change the block number in the call to `vm.createSelectFork` to the block number in the coverage report that gets dynamically replaced in the [`Setup`](https://github.com/ebtc-protocol/ebtc/blob/925073f04bdfe5a6b594898d6491950087eee23b/packages/contracts/contracts/TestContracts/invariants/Setup.sol#L348) contract. + - The block number shown in the coverage report for the run will be the block at which Echidna forked mainnet from for your run. +- Rename the `.env.example` file to `.env` and add your rpc url. +- `cd` into the target test package: + - `cd packages/contracts` +- Run `forge test --mt ` + + ## Remappings: Foundry test configuration is using existing hardhat dependencies, such as @openzeppelin etc. They are declated in `remappings.txt`. @@ -26,4 +37,5 @@ Then from root of the project run: yarn install ``` -And everything should work \ No newline at end of file +And everything should work +