Invariant violation not found on simple 2 step owner transfer test

[karmacoma-eth]

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 (ec3f9bd 2023-09-19T13:48:52.205431000Z)

What command(s) is the bug in?

forge test

Operating System

macOS (Intel)

Describe the bug

take the following src contract:

contract Owned {
    address public owner;
    address private ownerCandidate;

    constructor() {
        owner = msg.sender;
    }

    modifier onlyOwner() {
        require(msg.sender == owner);
        _;
    }

    modifier onlyOwnerCandidate() {
        require(msg.sender == ownerCandidate);
        _;
    }

   function transferOwnership(address candidate) external onlyOwner {
        ownerCandidate = candidate;
    }

    function acceptOwnership() external onlyOwnerCandidate {
        owner = ownerCandidate;
    }
}

and the following test:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.15;
import "forge-std/Test.sol";

contract Handler is Test {
    Owned owned;

    constructor(Owned _owned) {
        owned = _owned;
    }

    function transferOwnership(address sender, address candidate) external {
        vm.assume(sender != address(0));
        vm.prank(sender);
        owned.transferOwnership(candidate);
    }

    function acceptOwnership(address sender) external {
        vm.assume(sender != address(0));
        vm.prank(sender);
        owned.acceptOwnership();
    }
}

contract TwoStepOwnershipTestFoundry is Test {
    address owner;
    Owned owned;
    Handler handler;

    function setUp() public {
        owner = address(this);
        owned = new Owned();

        handler = new Handler(owned);
        targetContract(address(handler));

        bytes4[] memory selectors = new bytes4[](2);
        selectors[0] = Handler.transferOwnership.selector;
        selectors[1] = Handler.acceptOwnership.selector;
        targetSelector(FuzzSelector(address(handler), selectors));
    }

    function test_successful_transfer(address newOwner) public {
        handler.transferOwnership(owner, newOwner);
        handler.acceptOwnership(newOwner);

        assertEq(owned.owner(), newOwner);
    }

    function invariant_owner_never_changes_this_is_bad_lol() public returns(bool cond) {
        cond = (owned.owner() == owner);
        assertEq(owned.owner(), owner);
    }
}

There is no trick here, this is a kind of sanity check for invariant testing. Note the test_successful_transfer test, all that is needed to violate the invariant is to chain these two call:

        // must pass the current owner as the first arg
        handler.transferOwnership(owner, newOwner);

        // must pass the same newOwner as in the previous call
        handler.acceptOwnership(newOwner);

However:

• running forge test --mc TwoStepOwnershipTest yields no violation
• same thing with 10k fuzz runs
• tried with 100k fuzz runs but I ended up killing forge after 20 min

cc @horsefacts who helped investigate (https://twitter.com/eth_call/status/1704304001292914782)
cc @mds1 who asked for the issue

[Evalir]

Wonder if we're now able to catch this with #6530 cc @brockelmore

[brockelmore]

Nope the shrinking change is only relevant post-finding a counterexample

[grandizzy]

fails right away here with fail_on_revert set to true, @karmacoma-eth can you confirm you run with this option on?

[karmacoma-eth]

@grandizzy I run with fail_on_revert = false

failing on revert doesn't make sense for this invariant test, I am trying to see if benchmark can successfully find the sequence of 2 calls that actually transfer ownership.

It is expected that most runs will revert (because of the onlyOwner and onlyOwnerCandidate modifiers

[grandizzy]

@karmacoma-eth got it, thanks, that makes sense. I assume you tweaked invariant depth and dictionary_weight too?

[karmacoma-eth]

I did not, I run it with defaults

[grandizzy]

gotcha, I should have some time this week to try more variations here too and debug, there's also the the targetSender function that can be used to narrow down addresses participating in scenario, not sure though if you want to do this in your invariant test?

[grandizzy]

managed to make some tests, pls see #7240

[grandizzy]

@karmacoma-eth this is now merged, pls retest by setting the number of random addresses to recycle (in addition to known state addresses as handlers) e.g.

[invariant]
max_calldata_fuzz_dictionary_addresses=100

If not specified or set to 0 then current behavior is applied, (unbounded number of fuzzed addresses)

[grandizzy]

in latest builds this is caught with default settings but run depth increased (max_calldata_fuzz_dictionary_addresses setting was retired since we now support fixtures https://book.getfoundry.sh/forge/fuzz-testing#fuzz-test-fixtures), e.g. with a depth of 500

[FAIL. Reason: assertion failed]
        [Sequence]
                sender=0x2C03070E8B998F0177C5B9A64342862a2EECbb98 addr=[test/Owned.t.sol:Handler]0x2e234DAe75C793f67A35089C9d99245E1C58470b calldata=transferOwnership(address,address) args=[0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496, 0x000000000000000000000000000000006d435422]
                sender=0x422ec71B1b9c4b35c6b9AcB44773F962078dbe8D addr=[test/Owned.t.sol:Handler]0x2e234DAe75C793f67A35089C9d99245E1C58470b calldata=acceptOwnership(address) args=[0x000000000000000000000000000000006d435422]
 invariant_owner_never_changes_this_is_bad() (runs: 256, calls: 127959, reverts: 127939)
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 19.14s (19.11s CPU time)

Ran 1 test suite in 19.14s (19.14s CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)

I think this one can be closed, @mds1 wdyt?

[mds1]

From my testing, trying to flesh out depth vs. runs relationship a bit:

• Default fuzz settings do not catch it, regardless of number of runs (I only tried up to 10k runs)
• Setting depth=500 catches it consistently with the default number of runs (256)
• Setting depth=500 with runs=100 occasionally catches it
• Setting depth=10000 with runs=1 occasionally catches it, but seems to be a bit less often than prior bullet

So:

• I think we can close this as it's now catchable with the proper config
• But we probably need a higher default depth than 15, as the depth seems very import, and should reconsider our default invariant config

[grandizzy]

Agree, we should revisit all defaults and update them to more relevant values, like

• depth to 500 (For example echidna uses a default seqLen: 100 and testLimit: 50000 which is like 500 runs with depth of 100 in foundry terms)
• shrink_run_limit defaults now to 2^18 which is not realistic anymore with our new shrinking mechanism (echidna uses shrinkLimit: 5000). From tests done in perf(invariant): sequentially shrink failed sequence #7756 we can shrink a sequence of 5000 calls in 215 seconds, so I think that default is acceptable
• maybe default senders to 3 known addresses as echidna does with 0x1, 0x2, 0x3 addresses
• others

Worth mentioning that this kind of failure will be identified must faster when we implement per type fuzzing from state, so new owner address will be exercised right away.

[mds1]

Awesome, created #7848 to track and will close this one