Skip to content

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

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

Closed
NandyBa opened this issue Apr 4, 2024 · 2 comments
Closed
Assignees
Labels
A-internals Area: internals C-forge Command: forge Cmd-forge-test Command: forge test T-feature Type: feature

Comments

@NandyBa
Copy link

NandyBa commented Apr 4, 2024

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

@NandyBa NandyBa added the T-feature Type: feature label Apr 4, 2024
@zerosnacks zerosnacks added Cmd-forge-test Command: forge test C-forge Command: forge A-internals Area: internals labels Jul 12, 2024
@zerosnacks zerosnacks changed the title Detect used contract that haven't been initialised and return an explicit error message feat: emit explicit error message when non-initialized contract is used Jul 12, 2024
@zerosnacks zerosnacks changed the title feat: emit explicit error message when non-initialized contract is used feat: emit explicit warning message when non-initialized contract is used Jul 12, 2024
@zerosnacks
Copy link
Member

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)

@zerosnacks zerosnacks added this to the v1.0.0 milestone Jul 26, 2024
@grandizzy grandizzy self-assigned this Oct 3, 2024
@grandizzy grandizzy removed this from the v1.0.0 milestone Oct 3, 2024
@github-project-automation github-project-automation bot moved this to Todo in Foundry Oct 3, 2024
@grandizzy
Copy link
Collaborator

This is a dupe of #4141, going to close it and track the impl there. Thank you!

@grandizzy grandizzy closed this as not planned Won't fix, can't repro, duplicate, stale Nov 17, 2024
@github-project-automation github-project-automation bot moved this from Todo to Done in Foundry Nov 17, 2024
@grandizzy grandizzy moved this from Done to Completed in Foundry Nov 18, 2024
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-feature Type: feature
Projects
Status: Completed
Development

No branches or pull requests

3 participants