Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

debt: deprecate testFail* #4437

Open
Tracked by #8574
rafales opened this issue Feb 27, 2023 · 10 comments · May be fixed by #9574
Open
Tracked by #8574

debt: deprecate testFail* #4437

rafales opened this issue Feb 27, 2023 · 10 comments · May be fixed by #9574
Labels
A-internals Area: internals C-forge Command: forge Cmd-forge-test Command: forge test T-debt Type: code debt T-likely-breaking Type: requires changes that can be breaking
Milestone

Comments

@rafales
Copy link

rafales commented Feb 27, 2023

Component

Forge

Describe the feature you would like

Hey. I just ran into a problem that wasted more time that I'm willing to admit.

Foundry has a feature where tests named testFailSomething are expected to fail. It's easy to forget about it.

So I found myself in a situation where test is failing for some unknown reason and all I get is this message:

[FAIL. Reason: Assertion failed.]

It would be good if this particular failure had a more user-friendly message, pointing to the reason behind the failure. It's easy to come up with a test name like testFailsWhenCalledContractFailed and not know about the testFailXXX feature.

Message like FAIL. Reason: test was expected to revert, but it did not. or something similar.

Additional context

No response

@rafales rafales added the T-feature Type: feature label Feb 27, 2023
@gakonst gakonst added this to Foundry Feb 27, 2023
@github-project-automation github-project-automation bot moved this to Todo in Foundry Feb 27, 2023
@mds1
Copy link
Collaborator

mds1 commented Feb 27, 2023

@gakonst We should probably just remove testFail at this point, things like this have happened a few times and I don't think we need support for it anymore. Similar to #3153 (comment), we should look for tests named testFail* and print an issue/warning to inform users not to use this test name/format.

@rafales In the meantime I'd suggest following the naming conventions in https://book.getfoundry.sh/tutorials/best-practices, which are test_RevertIf_Condition

@mds1 mds1 added Cmd-forge-test Command: forge test C-forge Command: forge T-debt Type: code debt labels Feb 27, 2023
@abhiram11
Copy link

We can prefer to use vm.expectRevert() in the function itself, rather than naming a function that starts with testFail. I feel it is possible that testFail may be deprecated so it's better to change this developer practice asap.

@zerosnacks zerosnacks added this to the v1.0.0 milestone Jul 26, 2024
@zerosnacks zerosnacks changed the title Better message when testFailXXX test doesn't fail chore: raise warning upon encountering a test function with the testFail* prefix Sep 16, 2024
@zerosnacks zerosnacks added A-internals Area: internals and removed T-feature Type: feature labels Sep 16, 2024
@zerosnacks
Copy link
Member

Related: foundry-rs/book#813

@grandizzy grandizzy added the T-likely-breaking Type: requires changes that can be breaking label Nov 21, 2024
@zerosnacks zerosnacks changed the title chore: raise warning upon encountering a test function with the testFail* prefix chore: deprecate testFail Nov 21, 2024
@zerosnacks
Copy link
Member

Renamed the title, we are intending to deprecate testFail* in a breaking way for v1.0

@zerosnacks zerosnacks changed the title chore: deprecate testFail chore: deprecate testFail* Nov 21, 2024
@zerosnacks zerosnacks changed the title chore: deprecate testFail* debt: deprecate testFail* Nov 21, 2024
@yash-atreya yash-atreya linked a pull request Dec 18, 2024 that will close this issue
@escottalexander
Copy link

escottalexander commented Jan 6, 2025

Removing TestFail altogether presents a problem for my use case. I have a test that checks if a contract can be exploited by reentrancy. Students will submit contracts that must satisfy the requirements so there is no way to determine exactly what revert to expect. TestFail has come in handy since any part of the test can fail based on any number of student contract designs and that proves reentrancy is not possible.

How should I accomplish this generalized testing in the absence of testFail?

@zerosnacks
Copy link
Member

Hi @escottalexander

Thanks for raising this, would it be possible to share a minimal example of how you are using testFail*?

We are likely able to provide a solution that wouldn't require testFail* through other means (e.g. with vm.expectRevert())

@escottalexander
Copy link

escottalexander commented Jan 7, 2025

Thanks @zerosnacks,

Here is a minimalist example:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test, console} from "../lib/forge-std/src/Test.sol";

contract StoreFunds {
    mapping(address => uint256) public balances;

    function storeFunds() public payable {
        balances[msg.sender] += msg.value;
    }

    function getBalance(address addr) public view returns (uint256) {
        return balances[addr];
    }

    function withdraw(uint256 amount) public {
        // Set to false to use the other example
        if (true) {
            withdrawExampleOne(amount);
        } else {
            withdrawExampleTwo(amount);
        }
    }

    function withdrawExampleOne(uint256 amount) public {
        require(balances[msg.sender] >= amount, "Insufficient balance");
        balances[msg.sender] -= amount;
        (bool success, ) = payable(msg.sender).call{value: amount}("");
        // require(success, "Transfer failed"); // Poorly written and not checking for success yet still reentrancy safe
    }

    mapping(bytes32 => bool) public enteredThisBlock; // Only used in example two

    function withdrawExampleTwo(uint256 amount) public {
        // Demonstrates reentrancy safety even when Checks-Effects-Interactions are not followed
        bytes32 uniqueId = keccak256(abi.encodePacked(block.number, msg.sender));
        require(!enteredThisBlock[uniqueId], "Already entered this block");
        enteredThisBlock[uniqueId] = true;

        require(balances[msg.sender] >= amount, "Insufficient balance");
        
        (bool success, ) = payable(msg.sender).call{value: amount}("");
        // require(success, "Transfer failed"); // Poorly written and not checking for success
        balances[msg.sender] -= amount; // adjusting balance after transfer (typically a bad practice)
    }
}

contract ContractTest is Test {
    StoreFunds public storeFunds;

    function setUp() public {
        storeFunds = new StoreFunds();
    }

    function testFail_WithdrawReentrancySafe() public {
        // Whale deposits 10 ether
        vm.deal(address(0x1), 10 ether);
        vm.prank(address(0x1));
        storeFunds.storeFunds{value: 10 ether}();

        // Set up the exploiter
        ReentrancyExploiter exploiter = (new ReentrancyExploiter){ value: 1 ether }(address(storeFunds));

        // Exploiter withdraws 1 ether
        exploiter.exploit(); // Maybe this reverts

        // Check that the exploiter has more than 1 ether
        assertGt(address(exploiter).balance, 1 ether); // Or maybe this reverts

        // Either way we have proven that the contract is reentrancy safe
    }

    // Attempt without testFail
    function test_RevertsWhenExploited() public {
        // Whale deposits 10 ether
        vm.deal(address(0x1), 10 ether);
        vm.prank(address(0x1));
        storeFunds.storeFunds{value: 10 ether}();

        // Set up the exploiter
        ReentrancyExploiter exploiter = (new ReentrancyExploiter){ value: 1 ether }(address(storeFunds));

        // Exploiter withdraws 1 ether
        vm.expectRevert();
        exploiter.exploit();

        // Maybe that reverts as expected or maybe it doesn't based on the implementation but it is still possible that the contract is reentrancy safe
    }
}

contract ReentrancyExploiter {
    StoreFunds public storeFunds;

    constructor(address _storeFunds) payable {
        storeFunds = StoreFunds(_storeFunds);
        // Deposit funds to the StoreFunds contract
        storeFunds.storeFunds{value: 1 ether}();
    }

    function exploit() public {
        storeFunds.withdraw(1 ether);
    }

    fallback() external payable {
        if (storeFunds.getBalance(address(this)) == 0) {
            revert("Exploit failed because balance is adjusted");
        } else if (address(storeFunds).balance > 0) {
            uint highestValidWithdrawal = storeFunds.getBalance(address(this)) > address(storeFunds).balance ? address(storeFunds).balance : storeFunds.getBalance(address(this));
            storeFunds.withdraw(highestValidWithdrawal);
        }
    }
}

Also you can just use this repo: https://github.com/escottalexander/testFail-example (based on a new hello_foundry build)

This contains two tests, one showing testFail working and one showing a typical expectRevert not working.

This contains two withdraw method design examples (adjust the bool in the StoreFunds.withdraw method to switch between them) that are reentrancy safe yet do not cause the revert in an expected way (due to them reverting after the external call back to the ReentrancyExploiter contract's fallback method. There is no way for me to prove that either the exploit reverts OR the assertion proves that reentrancy was unsuccessful. TestFail accomplishes this but it is not possible with expectRevert.

Reminder that in my case, I can only write the tests and I have no control over the contracts (StoreFunds) designs that are submitted by students.

Thank you for any advice for my specific use case.

@zerosnacks
Copy link
Member

zerosnacks commented Jan 8, 2025

Hi @escottalexander, I think something like this should work, I've tested it for both example solutions:

    function test_RevertsWhenExploited() public {
        // Whale deposits 10 ether
        vm.deal(address(0x1), 10 ether);
        vm.prank(address(0x1));
        storeFunds.storeFunds{value: 10 ether}();

        // Set up the exploiter
        ReentrancyExploiter exploiter = (new ReentrancyExploiter){value: 1 ether}(address(storeFunds));

        // Try to exploit, ignoring if it passes or reverts
        try exploiter.exploit() {} catch {}

        // ...

        // Perform assertions on the final state
        assertLe(address(exploiter).balance, 1 ether, "Exploit succeeded, but balance increased unexpectedly");
        assertGe(address(storeFunds).balance, 10 ether, "StoreFunds contract balance should remain unchanged");
    }

Not directly related but a good read for your students: https://www.nascent.xyz/idea/youre-writing-require-statements-wrong

@escottalexander
Copy link

@zerosnacks Using a try/catch did not occur to me. Thank you for the help. That should work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-internals Area: internals C-forge Command: forge Cmd-forge-test Command: forge test T-debt Type: code debt T-likely-breaking Type: requires changes that can be breaking
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

7 participants
@rafales @mds1 @abhiram11 @escottalexander @grandizzy @zerosnacks and others