Invariant test does not revert though fail_on_revert is set to true

[0xmp]

Component

Forge

Have you ensured that all of these are up to date?

• Foundry
• Foundryup

What version of Foundry are you on?

forge 0.2.0 (2bcb4a1 2024-01-02T00:17:30.395578000Z)

What command(s) is the bug in?

forge test

Describe the bug

I am running an invariant test through a handler and have a weird issue where fail_on_revert is set to true in my config (confirmed by running forge config), one of the calls revert (confirmed by looking at traces with high verbosity) but the test passes. I believe fail_on_revert is not respected when using vm.warp and/or vm.roll. I do not use --via-ir. It is problematic as it means some inner calls can fail silently without you noticing (which is what initially happened in my case).

I managed to create a small self-contained repro:

Reproduction code

Foundry.toml

[...]
[invariant]
runs = 256
depth = 100
fail_on_revert = true
dictionary_weight = 20

DummyInvariant.t.sol

pragma solidity ^0.8.19;

import "@forge-std/Test.sol";
import "@forge-std/console2.sol";
import {DummyHandler} from "./DummyHandler.sol";

contract DummyContract {
    constructor() {}
    function dummyRevert() public {
        revert();
    }
}

contract DummyInvariantTest is Test {
    DummyContract myContract;
    DummyHandler handler;

    function setUp() public {
        myContract = new DummyContract();
        handler = new DummyHandler(myContract);

        vm.label({ account: address(handler), newLabel: "DummyHandler"});

        // Target only the handler
        targetContract(address(handler));

        bytes4[] memory selectors = new bytes4[](2);
        selectors[0] = handler.thisFunctionReverts.selector;
        selectors[1] = handler.advanceTime.selector;

        targetSelector(FuzzSelector({
            addr: address(handler),
            selectors: selectors
        }));
    }

    function invariant_DummyInvariant() public {
        assertTrue(true);
    }
}

DummyHandler.sol

pragma solidity ^0.8.19;

import "@forge-std/Test.sol";
import "@forge-std/console2.sol";
import {DummyContract} from "./DummyInvariant.t.sol";

contract DummyHandler is Test {
    DummyContract myContract;

    constructor(DummyContract contract_) {
        myContract = contract_;
    }

    function thisFunctionReverts() public {
        if (block.number < 20) {
        } else {
            myContract.dummyRevert();
        }
    }

    function advanceTime(uint256 blocks) external {
        blocks = blocks % 10;
        
        vm.roll(block.number + blocks);
        vm.warp(block.timestamp + blocks * 12);
    }
}

In the above, the handler has two functions:

1. advanceTime increments the block number and the timestamp
2. thisFunctionReverts reverts if block.number >= 20.

It should be trivial to make the above revert with a stateful test. However, if I run the above with forge test --match-contract DummyInvariantTest -vvvvv, I can see that calls to thisFunctionReverts do revert in the logs but the test passes successfully.

Running 1 test for test/foundry/DummyInvariant.t.sol:DummyInvariantTest
[PASS] invariant_DummyInvariant() (runs: 256, calls: 25600, reverts: 0)
Traces:
[...]
  [5313] MessageServicesHandler::thisFunctionReverts()
    ├─ [107] DummyContract::dummyRevert()
    │   └─ ← EvmError: Revert
    └─ ← EvmError: Revert
[...]
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.41s

Importantly, if I comment out vm.warp & vm.roll in the advanceTime function and just call vm.roll (to roll to block 21) directly in setUp, the test reverts successfully. This points to the issue being when the handler calls vm.roll or vm.warp.

[keyneom]

I'm not positive but I think I have a similar issue. I have an assert that fails but the test still indicates that it passed. This is very concerning to me as I would not have realized things were broken if I hadn't been running with high verbosity for another issue. I can't seem to figure out what would cause it but I am somewhat new to using foundry for tests.

I believe I have something pretty compact and reproducible running with the following settings and code.

forge test --mt statefulFuzz_testHasTimePassed -vvvvv

foundry.toml

[profile.default]
src = "src"
out = "out"
libs = ["lib"]
ffi = true
optimizer = true
evm_version = 'shanghai'
fs_permissions = [{ access = "read", path = "./" }]

[fuzz]
runs = 256
seed = '0x3'

[invariant]
runs = 64
depth = 32
fail_on_revert = true

AssertTests.t.sol

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.20;

import {Test, console2} from "forge-std/Test.sol";
import {StdInvariant} from "forge-std/StdInvariant.sol";

contract ContractTestSuite is StdInvariant, Test {
    Mock mock;
    MockHandler mockHandler;

    uint256 id = 0;

    function setUp() public {
        mock = new Mock();
        mockHandler = new MockHandler(address(mock));

        targetContract(address(mockHandler));
        bytes4[] memory selectors = new bytes4[](2);
        selectors[0] = mockHandler.setTimeById.selector;
        selectors[1] = mockHandler.hasRequiredTimePassed.selector;
        
        targetSelector(FuzzSelector({addr: address(mockHandler), selectors: selectors}));
    }

    function statefulFuzz_testHasTimePassed() public {
        assert(mock.hasRequiredTimePassed(id));
    }
}

contract Mock {
    uint256 public constant REQUIRED_TIME = 1 days;

    mapping(uint256 id => uint256 timeStamp) public idsToTimestamps;

    constructor() {}

    function setTimeById(uint256 id) external {
        idsToTimestamps[id] = block.timestamp;
    }

    function hasRequiredTimePassed(uint256 id) external view returns (bool) {
        if (idsToTimestamps[id] <= block.timestamp - REQUIRED_TIME) {
            return false;
        }
        return true;
    }
}

contract MockHandler is Test {
    address private originalContract;
    uint256 counter = 0;

    constructor(address _originalContract) {
        originalContract = _originalContract;
        vm.warp(623095235);
        Mock(originalContract).setTimeById(0);
    }

    function setTimeById() external {
        ++counter;
        uint256 rollForwardBy = counter % 2 == 0 ? 36 hours : 12 hours;
        vm.warp(vm.getBlockTimestamp()+rollForwardBy);
        vm.roll(vm.getBlockNumber()+1);
        Mock(originalContract).setTimeById(0);
    }

    function hasRequiredTimePassed() external returns (bool) {
        ++counter;
        uint256 rollForwardBy = counter % 2 == 0 ? 36 hours : 12 hours;
        vm.warp(vm.getBlockTimestamp()+rollForwardBy);
        vm.roll(vm.getBlockNumber()+1);
        return Mock(originalContract).hasRequiredTimePassed(0);
    }
}

Output:

 . . . 
  [9920] ContractTestSuite::statefulFuzz_testHasTimePassed()
    ├─ [2579] Mock::hasRequiredTimePassed(0) [staticcall]
    │   └─ ← false
    └─ ← panic: assertion failed (0x01)

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 163.58ms
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

If I apply the following diff to the stateful fuzz test it appears as if the failed assertion is properly recognized:

    function statefulFuzz_testHasTimePassed() public {
-        assert(mock.hasRequiredTimePassed(id));
+        assert(mockHandler.hasRequiredTimePassed());
    }

Output:

 . . . 
  [22629] ContractTestSuite::statefulFuzz_testHasTimePassed()
    ├─ [17384] MockHandler::hasRequiredTimePassed()
    │   ├─ [0] VM::getBlockTimestamp() [staticcall]
    │   │   └─ ← 623138435 [6.231e8]
    │   ├─ [0] VM::warp(623268035 [6.232e8])
    │   │   └─ ← ()
    │   ├─ [0] VM::getBlockNumber() [staticcall]
    │   │   └─ ← 2
    │   ├─ [0] VM::roll(3)
    │   │   └─ ← ()
    │   ├─ [2579] Mock::hasRequiredTimePassed(0) [staticcall]
    │   │   └─ ← false
    │   └─ ← false
    └─ ← panic: assertion failed (0x01)

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 171.51ms
 
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/AssertTests.t.sol:ContractTestSuite
[FAIL. Reason: panic: assertion failed (0x01)]
        [Sequence]
                sender=0x0000000000000000000000000000000000000308 addr=[test/AssertTests.t.sol:MockHandler]0x2e234DAe75C793f67A35089C9d99245E1C58470b calldata=setTimeById() args=[]
 statefulFuzz_testHasTimePassed() (runs: 64, calls: 2017, reverts: 0)

Encountered a total of 1 failing tests, 0 tests succeeded

[0xmp]

@keyneom Yes, I think we have the same issue.

This is very concerning to me as I would not have realized things were broken if I hadn't been running with high verbosity for another issue.

Same here, from what I see fail_on_revert doesn't work when the invariant handler uses vm.roll or vm.warp. I think this is a pretty common situation so there may be a few projects not realizing that their invariant test suite is actually not reverting. For example, Sablier (considered as a good standard in terms of clean tests) relies on invariant tests with fail_on_revert and does use vm.warp.

@Evalir Really apologize for the tag, but not sure how else to draw attention to this issue and am a bit worried by the possibility of codebases relying on fail_on_revert

[simplyoptimistic]

+1, would like to request this gets fixed too

[keyneom]

@0xMelkor I saw you have assigned #3411 for invariant fuzz test benchmarks. I just don't think benchmarks will be very meaningful if the tests aren't reporting failures correctly and I'd like to draw attention to this issue as it appears there are more and more people that are noticing its impact on them. I just want to make sure you all are aware of it since I know you've all got a ton of issues open right now.

[grandizzy]

this is something that at some point made us consider switching away from foundry, but glad we stayed with it :)
The thing is that in the 1st phase of invariant testing the state after calling handler selector is not preserved, so thisFunctionReverts doesn't revert because block.number won't get to 20 (as roll in advanceTime is not committed to db).
What is displayed in traces is collected in the 2nd phase of invariant testing, when 1st stage is replayed (if no failure) but this time with state committed, hence revert happens and properly shows up in traces.

What we ended up in our project was to save chain state in handler and reapply on each call, e.g.

pragma solidity ^0.8.19;

import "forge-std/Test.sol";
import "forge-std/console2.sol";

contract DummyInvariantOkTest is Test {
    uint256 curBlock;
    bytes4[] internal selectors;

    modifier roll(uint256 blocks) {
        blocks = blocks % 10;

        vm.roll(curBlock + blocks);
        _;

        curBlock = block.number;
    }

    function setUp() public {
        vm.label({account: address(this), newLabel: "DummyHandler"});

        selectors.push(this.thisFunctionReverts.selector);

        targetSelector(
            FuzzSelector({addr: address(this), selectors: selectors})
        );
    }

    function invariant_DummyInvariant() public {
        assertTrue(true);
    }

    function thisFunctionReverts(uint256 blocks) public roll(blocks) {
        if (block.number < 20) {} else {
            revert();
        }
    }
}

Totally agree the behavior is unintuitive and bad ux so I am going to craft a PR with a new invariant var preserve_state which in conjunction with fail_on_revert will persist state after the call, disabled by default as adds performance penalty

[grandizzy]

made a draft PR #7219 probably not the best solution for this issue @mattsse @Evalir @DaniPopes any thoughts on this really appreciated. thank you

[mds1]

I believe this issue was closed by #7219, but can reopen if I misunderstood

[0xmp]

A bit late, but thank you for the fix @grandizzy ! I confirm it fixes the issue on my end. Just to summarize if anyone stumbles here with an invariant test not reverting, make sure your foundry.toml includes:

[invariant]
preserve_state = true
fail_on_revert = true