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

feat(forge): add forge sig-collision cmd #4973

Closed
wants to merge 2 commits into from

Conversation

lmanini
Copy link
Contributor

@lmanini lmanini commented May 18, 2023

Motivation

Easily check for colliding function selectors between two different contracts, especially useful when working with contracts that rely con delegating calls to others.

Solution

add forge sig-collision command to scan two contracts and look for matches between their functions' selectors.

example

forge sig-collision ./proxy.sol:Proxy ./implementation.sol:Token

where proxy.sol:Proxy is:

pragma solidity ^0.8.0;

contract Proxy {
    uint256 public initializer;
    mapping(address => uint256) private _balances;
    uint256 private _totalSupply;

    string public name;
    string public symbol;

    address public proxyOwner;
    address public implementation;
    int lock;

    constructor(address implementationaddr) {
        proxyOwner = msg.sender;
        _setImplementation(implementationaddr);
    }

    modifier onlyProxyOwner() {
        require(msg.sender == proxyOwner);
        _;
    }

    function upgrade(address newimplementation) external onlyProxyOwner {
        _setImplementation(newimplementation);
    }

    function _setImplementation(address imp) private {
        implementation = imp;
    }

    fallback () external {
        address impl = implementation;

        assembly {
            calldatacopy(0, 0, calldatasize())
            let result := delegatecall(gas(), impl, 0, calldatasize(), 0, 0)
            returndatacopy(0, 0, returndatasize())

            switch result
            case 0 { revert(0, returndatasize()) }
            default { return(0, returndatasize()) }
        }
    }

    function compile_performance_mint(bytes16) external {
        implementation.delegatecall(abi.encodeWithSignature(
            "mint(address,uint256)", proxyOwner, 1000
        ));
    }

    function create_alternative_balance(bytes16) external {
        implementation.delegatecall(abi.encodeWithSignature(
            "balanceOf(address)", proxyOwner
        ));
    }

    function collate_propagate_storage(bytes16) external {
        implementation.delegatecall(abi.encodeWithSignature(
            "transfer(address,uint256)", proxyOwner, 1000
        ));
    }
}

and implementation.sol:Token is:

pragma solidity ^0.8.0;

contract Token {
    uint256 public initializer;
    mapping(address => uint256) private _balances;
    uint256 private _totalSupply;

    string public name;
    string public symbol;

    event Transfer(address indexed from, address indexed to, uint256 value);

    constructor() {
        name = "ABM frens token";
        symbol = "ABM";
        _totalSupply = 300000000000000;
    }

    function initialize(
        string memory _name,
        string memory _symbol,
        uint256 initialSupply,
        address _bob,
        address _mallory
    )
        public
    {
        require(initializer == 0);
        initializer += 1;
        name = _name;
        symbol = _symbol;
        _mint(msg.sender, initialSupply/3);
        _mint(_bob, initialSupply/3);
        _mint(_mallory, initialSupply/3);
    }

    function balanceOf(address account) public view virtual returns (uint256) {
        return _balances[account];
    }

    function _mint(address account, uint256 amount) internal virtual {
        require(account != address(0), "ERC20: mint to the zero address");

        _totalSupply += amount;
        unchecked {
            _balances[account] += amount;
        }
        emit Transfer(address(0), account, amount);
    }

    function _transfer(
        address from,
        address to,
        uint256 amount
    ) internal virtual {
        require(from != address(0), "ERC20: transfer from the zero address");
        require(to != address(0), "ERC20: transfer to the zero address");

        uint256 fromBalance = _balances[from];
        require(fromBalance >= amount, "ERC20: transfer amount exceeds balance");
        unchecked {
            _balances[from] = fromBalance - amount;
            _balances[to] += amount;
        }

        emit Transfer(from, to, amount);
    }

    function transfer(address to, uint256 amount) public virtual returns (bool) {
        address owner = msg.sender;
        _transfer(owner, to, amount);
        return true;
    }

    function burn(uint256 amount) public virtual {
        _burn(msg.sender, amount);
    }

    function _burn(address account, uint256 amount) internal virtual {
        require(account != address(0), "ERC20: burn from the zero address");

        uint256 accountBalance = _balances[account];
        require(accountBalance >= amount, "ERC20: burn amount exceeds balance");
        unchecked {
            _balances[account] = accountBalance - amount;
            // Overflow not possible: amount <= accountBalance <= totalSupply.
            _totalSupply -= amount;
        }

        emit Transfer(account, address(0), amount);
    }
}

outputs >

./proxy.sol ./implementation.sol
The two contracts have the following methods whose signatures clash: [
    (
        "collate_propagate_storage(bytes16)",
        "burn(uint256)",
    ),
    (
        "initializer()",
        "initializer()",
    ),
    (
        "name()",
        "name()",
    ),
    (
        "symbol()",
        "symbol()",
    ),
]

@mds1
Copy link
Collaborator

mds1 commented May 19, 2023

I haven't looked closely at this PR yet, but generally supportive of it as this is useful. Some thoughts:

  • We're checking selector collisions, not signatures, so we might want to rename things
  • A popular request is a command to list all selectors in a project, so we might instead want something like forge selectors which can have subcommands such as:
    • forge selectors collision for what this PR does
    • forge selectors list <options> to list function selectors and/or error selectors and/or event sigs for certain/all files in a project
    • forge selectors upload would replace the existing forge upload-selectors

@lmanini
Copy link
Contributor Author

lmanini commented May 19, 2023

I didn't know of all the requests regarding selectors, should we open an issue to collect desired functionalities for subcommands of forge selectors?
I can rewrite this to correct the naming of stuff and make it a subcommand.
Glad to work on other subcommands as well!

@Evalir Evalir self-requested a review May 19, 2023 21:41
@mds1
Copy link
Collaborator

mds1 commented May 22, 2023

That sgtm, thanks!

Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will also need to run cargo fmt --all and cargo clippy -p foundry-cli --fix with the nightly toolchain.

Comment on lines +18 to +28
#[clap(
help = "The first of the two contracts for which to look method signature collisions in the form `(<path>:)?<contractname>`.",
value_name = "FIRST_CONTRACT"
)]
pub first_contract: ContractInfo,

#[clap(
help = "The second of the two contracts for which to look method signature collisions in the form `(<path>:)?<contractname>`.",
value_name = "SECOND_CONTRACT"
)]
pub second_contract: ContractInfo,
Copy link
Member

@DaniPopes DaniPopes May 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please adhere to the style of the other Clap Parser structs? This would mean the help and about are turned into doc comments, and value_name is removed as it's redundant.

@lmanini
Copy link
Contributor Author

lmanini commented May 23, 2023

Great guys! I'll scrap this PR and make an issue (#5012) to track desired functionalities for forge selectors.

@lmanini lmanini closed this May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants