Fixed expectRevert being applied to calls to cheatcodes

[lisandrasilva]

This PR fixes the expectRevert cheatcode implementation. According to the Foundry documentation:

After calling expectRevert, calls to other cheatcodes before the reverting call are ignored.
This means, for example, we can call prank immediately before the reverting call.

In the previous implementation, the call to the cheatcode was being ignored in the sense that was not being executed. This PR still executes cheatcode calls but does not check if these calls revert or not.

Fixes #993.

[anvacaru]

Does this check also have to be added to the rule [foundry.set.expectrevert.2] below?

[lisandrasilva]

Do you think it's possible/likely to delegate a call to the cheatcode address?

[anvacaru]

I looked into this and wrote two tests, one for delegatecall and one for staticcall calling a cheatcode.

The assertion at the end fails for delegatecall, whereas it does not fail for staticcall.

    function testDelegatecallCheatcodeReverts() external {
        (bool success, ) = VM_ADDRESS.delegatecall(
            abi.encodeWithSignature("roll(uint256)", 1337)
        );
        assertTrue(success);
        assertEq(block.number, 1337);
    }

    function testStaticcallcallCheatcode() external view {
        (bool success, ) = VM_ADDRESS.staticcall(
            abi.encodeWithSignature("roll(uint256)", 1337)
        );
        assertTrue(success);
        assertEq(block.number, 1337);
    }

So maybe it's worth adding the rule to cover staticcall for now. Next, in another issue/PR, we should check that Kontrol has the same behavior and ensure that delegating into VM_ADDRESS does not execute the cheat code.

@palinatolmach what do you think?

[palinatolmach]

I think it sounds good! Let's add a rule for staticcall here, and ensure that Kontrol behaves similarly wrt delegatecall as a follow-up.

[lisandrasilva]

Will do it!
I also tried the following tests with Foundry:

function testIncrementAsNotOwner_2() public {
        vm.expectRevert(Unauthorized.selector);
        (bool success, ) = VM_ADDRESS.delegatecall(
            abi.encodeWithSignature("roll(uint256)", 1337)
        );
        upOnly.increment();
    }

And this test is passing when it shouldn't, since the function does not revert as expected. On the other hand, the following test fails:

function testIncrementAsNotOwner_3() public {
        vm.expectRevert(Unauthorized.selector);
        (bool success, ) = VM_ADDRESS.delegatecall(
            abi.encodeWithSignature("roll(uint256)", 1337)
        );
        vm.prank(address(0));
        upOnly.increment();
    }

It seems that delegatecall to VM_ADDRESS clears the expectRevert cheatcode.

[lisandrasilva]

@anvacaru and @palinatolmach, I added this rule, and now Kontrol mimics the same behavior as Foundry for the expectRevert cheatcode.
We still have to address cheatcodes being ignored when called through DELEGATECALL in another PR.