[Bug-Candidate]: Corpus replay does not work as expected when the target contract changes

[aviggiano]

Describe the issue:

I am not sure if this is a "real" issue or just "bad luck", but echidna's corpus replay is not behaving as expected.

I am trying to improve the code coverage of my tests (Tester.sol), and during the process of adding assertions on the "not happy path", sometimes I stumble upon wrongly created asserts [1].

In order to better debug what's going on, I add "log" events on the "target contract" (UniswapV2Pair.sol) [2], but when I restart echidna, the failure does not trigger anymore [3]!

This is unexpected, as I would like the corpus replay to reuse the same previous transactions that found an issue just a moment ago.

Code example to reproduce the issue:

Local repo inspired by https://github.com/crytic/echidna-streaming-series/tree/main/part4/contracts, will be open sourced soon

Version:

$ echidna --version
Echidna 2.2.0

$ slither --version
0.9.2

Relevant log output:

[1] While trying to improve the code coverage of my tests, and after writing a wrong assertion, I get an error after only a few transactions.

diff --git a/protocols/uniswap-v2/test/Tester.sol b/protocols/uniswap-v2/test/Tester.sol
index d252bc8..e1940eb 100644
--- a/protocols/uniswap-v2/test/Tester.sol
+++ b/protocols/uniswap-v2/test/Tester.sol
@@ -334,6 +334,17 @@ abstract contract Tester is Setup, Asserts {
                 feeTouserLpBalanceBefore,
                 "Swapping does not decrease `feeTo` LP tokens balance"
             );
+        } else {
+            uint[] memory amounts = UniswapV2Library.getAmountsOut(
+                address(factory),
+                swapAmountIn,
+                path
+            );
+            assertGt(
+                amounts[1],
+                reserve2Before,
+                "Swapping should not fail if there's enough liquidity"
+            );
         }
     }

$ echidna protocols/uniswap-v2 --contract EchidnaTester --config protocols/uniswap-v2/test/config.yaml --workers 4 --test-limit 100000
[2023-06-29 11:22:25.92] Compiling protocols/uniswap-v2... Done! (5.883408s)
Analyzing contract: /Users/user/fuzzer-evaluation/protocols/uniswap-v2/test/EchidnaTester.sol:EchidnaTester
[2023-06-29 11:22:32.60] Running slither on protocols/uniswap-v2... Done! (13.005103s)
Loaded 6 transaction sequences from corpus/reproducers
Loaded 27 transaction sequences from corpus/coverage
swapExactTokensForTokens(uint256): failed!💥
  Call sequence, shrinking 642/5000:
    addLiquidity(2194894,2)
    removeLiquidity(9954955145375539039793190139635783659890494)
    swapExactTokensForTokens(0)

[3] Then, I try to understand what's going on with the "target contract" (UniswapV2Pair.sol), and I introduce the following log:

diff --git a/protocols/uniswap-v2/src/UniswapV2Pair.sol b/protocols/uniswap-v2/src/UniswapV2Pair.sol
index 83cfddc..24a66a2 100644
--- a/protocols/uniswap-v2/src/UniswapV2Pair.sol
+++ b/protocols/uniswap-v2/src/UniswapV2Pair.sol
@@ -198,6 +198,8 @@ import "./interfaces/IUniswapV2Callee.sol";
         emit Burn(msg.sender, amount0, amount1, to);
     }
 
+    event L4(uint, uint, uint, uint);
+
     // this low-level function should be called from a contract which performs important safety checks
     function swap(
         uint amount0Out,
@@ -210,6 +212,7 @@ import "./interfaces/IUniswapV2Callee.sol";
             "UniswapV2: INSUFFICIENT_OUTPUT_AMOUNT"
         );
         (uint112 _reserve0, uint112 _reserve1, ) = getReserves(); // gas savings
+        emit L4(_reserve0, _reserve1, amount0Out, amount1Out);
         require(
             amount0Out < _reserve0 && amount1Out < _reserve1,
             "UniswapV2: INSUFFICIENT_LIQUIDITY"

3. Now, the revert does not trigger anymore

$ echidna protocols/uniswap-v2 --contract EchidnaTester --config protocols/uniswap-v2/test/config.yaml --workers 4 --test-limit 100000
[2023-06-29 11:24:19.70] Compiling protocols/uniswap-v2... Done! (5.535581s)
Analyzing contract: /Users/user/fuzzer-evaluation/protocols/uniswap-v2/test/EchidnaTester.sol:EchidnaTester
[2023-06-29 11:24:25.96] Running slither on protocols/uniswap-v2... Done! (11.87923s)
Loaded 7 transaction sequences from corpus/reproducers
Loaded 27 transaction sequences from corpus/coverage
swapExactTokensForTokens(uint256): passing
removeLiquidity(uint256): passing
addLiquidity(uint256,uint256): passing
swapExactTokensForTokensPathIndependence(uint256): passing
AssertionFailed(..): passing

[ggrieco-tob]

Hi Antonio!

Which contract are you modifying exactly? Do you have a test contract that deploys uniswap pairs?

[aviggiano]

Hey,

Yes, Tester.sol is the contract echidna is testing, and it is deploying Uniswap v2 on its constructor. Then, if I change UniswapV2Pair, the bug does not trigger anymore.

Sorry for not having an MWE available, I'll send an update here once I push it to github.

[ggrieco-tob]

The uniswap pair that you deploy is going to be end in different addresses if you modify the code. In order to avoid that, you will need to use a different approach, try using the echidna option: deployContracts: [["0x42", "A"]] will will always deploy contract A in address 0x42.

[ggrieco-tob]

Another option, is just clean the corpus at each UniuswapV2Pair modification. The modifications of Tester are (mostly) ok since the address will never change (but if you change the interface the corpus will be stale 😱 )

[aviggiano]

Thanks, I did not know that. Using the deployContracts configuration fixes my problem!

So this is indeed a non-issue.

Maybe an improvement would be to automatically redeploy contracts at the same address in order to benefit from the corpus replay by default if the interface does not change?

[ggrieco-tob]

Redeploying automatically everything that was already deployed will be difficult: we will need to track arbitrary changes to bytecode that the user could be doing. Instead, we will add better documentation for this case.

[aviggiano]

I'm not sure how this works under the hood, but I was assuming that, since slither is run before echidna, echidna would have the IR of all contracts deployed on the constructor, in addition to their interfaces. So in theory it could deterministically redeploy them to the same address if their interfaces do not change without looking into the bytecode.

[ggrieco-tob]

Perhaps, however, the contracts are not usually deployed directly by echidna, but using the bytecode, which will require us to hook the CREATE/CREATE2 instruction from hevm execution. I'm afraid that is a large and complicated task for such a small gain.