foundry-rs / foundry

Foundry is a blazing fast, portable and modular toolkit for Ethereum application development written in Rust.
https://getfoundry.sh
Apache License 2.0
8.06k stars 1.65k forks source link

feat: emit explicit warning message when non-initialized contract is used #7562

Open NandyBa opened 4 months ago

NandyBa commented 4 months ago

Component

Forge

Describe the feature you would like

Proposal: Enhanced Error Handling for Calls to Uninitialized Contracts

Managing multiple contracts can occasionally lead to an oversight where one contract is not initialized. This oversight typically results in a EvmError: Revert; error, which is often detected only at the end of the execution stack. This not only makes debugging challenging but also time-consuming.

I propose to implement a mechanism that specifically catches this type of error and, in response, provides a more explicit and informative error message, such as Error: Contract not initialized. By doing so, we aim to alert developers to the precise nature of the issue more quickly, making it easier to identify and rectify the oversight of not initializing a contract.

Additional context

Here's a revised version of your message for clarity and correctness:


Additional Context

During ETH Oxford, I encountered an issue that took me a while to resolve due to the lack of an explicit error message.

How it Occurred: The issue arises when executing forge test and calling a function within your contract that triggers another contract. If you specify the contract but forget to initialize it, you encounter a non-explicit error: EvmError: Revert;.

Reproducible Error:

Screenshot 2024-04-04 at 15 56 40
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";

contract Wrapper {
    IERC20 usdc;
    mapping(address => uint256) public balances;

    constructor(address _usdc) {
        usdc = IERC20(_usdc);
    }

    function supply(uint256 amount) external {
        IERC20(address(usdc)).transferFrom(msg.sender, address(this), amount);
        balances[msg.sender] += amount;
    }
}
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";

contract TestToken is ERC20 {
    constructor(uint256 initialSupply) ERC20("TestToken", "TTK") {
        _mint(msg.sender, initialSupply);
    }
}

contract USDC is ERC20 {
    constructor(uint256 initialSupply) ERC20("USDC","USDC"){
        _mint(msg.sender, initialSupply);
    }
}

```Wrapper.t.sol
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "forge-std/Test.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import { Wrapper } from '../src/Wrapper.sol';

contract WrapperTest is Test{

    Wrapper wrapToken;
    IERC20 usdc;

    function setUp() public {
        wrapToken = new Wrapper(address(usdc));
    }

    function testSupply() public {
        uint256 supplyAmount = 100e18;
        wrapToken.supply(supplyAmount);
    }
}

Note: I have simplified the implementation to get a short reproductible code

zerosnacks commented 1 month ago

It agree it is quite easy to miss, one of the clear indications is when the trace shows that you are calling methods on address(0)

I'm open to having improvements for this but I don't know how technically feasible this would be and what the rate of false positives would be like in a forking scenario. (cc @klkvr)