implement detector for erc20 tokens that have function collision with…

… DOMAIN_SEPARATOR

.github/workflows/features.yml

@@ -50,3 +50,4 @@ jobs:
           pytest tests/test_functions_ids.py
           pytest tests/test_function.py
           pytest tests/test_source_mapping.py
+          pytest tests/test_detectors_mocked_contract.py

slither/detectors/all_detectors.py

@@ -84,3 +84,4 @@
 from .statements.msg_value_in_loop import MsgValueInLoop
 from .statements.delegatecall_in_loop import DelegatecallInLoop
 from .functions.protected_variable import ProtectedVariables
+from .functions.permit_domain_signature_collision import DomainSeparatorCollision

slither/detectors/functions/permit_domain_signature_collision.py

@@ -0,0 +1,55 @@
+"""
+Module detecting EIP-2612 domain separator collision
+"""
+from slither.utils.function import get_function_id
+from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
+
+
+class DomainSeparatorCollision(AbstractDetector):
+    """
+    Domain separator collision
+    """
+
+    ARGUMENT = "domain-separator-collision"
+    HELP = "Detects ERC20 tokens that have a function whose signature collides with EIP-2612's DOMAIN_SEPARATOR()"
+    IMPACT = DetectorClassification.MEDIUM
+    CONFIDENCE = DetectorClassification.HIGH
+
+    WIKI = (
+        "https://github.com/crytic/slither/wiki/Detector-Documentation#domain-separator-collision"
+    )
+
+    WIKI_TITLE = "Domain separator collision"
+    WIKI_DESCRIPTION = "An ERC20 token has a function whose signature collides with EIP-2612's DOMAIN_SEPARATOR(), causing unanticipated behavior for contracts using `permit` functionality."
+
+    # region wiki_exploit_scenario
+    WIKI_EXPLOIT_SCENARIO = """
+```solidity
+contract Contract{
+    function some_collisions() external() {}
+}
+```
+`some_collision` clashes with EIP-2612's DOMAIN_SEPARATOR() and will interfere with contract's using `permit`."""
+    # endregion wiki_exploit_scenario
+
+    WIKI_RECOMMENDATION = "Remove or rename the function that collides with DOMAIN_SEPARATOR()."
+
+    def _detect(self):
+        results = []
+        domain_sig = get_function_id("DOMAIN_SEPARATOR()")
+        for contract in self.compilation_unit.contracts_derived:
+            if contract.is_erc20():
+                for func in contract.functions:
+                    if (
+                        func.name != "DOMAIN_SEPARATOR"
+                        and get_function_id(func.solidity_signature) == domain_sig
+                    ):
+                        info = [
+                            func,
+                            "'s function signature collides with DOMAIN_SEPARATOR and should be renamed or removed.\n",
+                        ]
+                        res = self.generate_result(info)
+                        results.append(res)
+                        break
+
+        return results

tests/mock_permit_domain_collision.sol

@@ -0,0 +1,208 @@
+// SPDX-License-Identifier: AGPL-3.0-only
+pragma solidity >=0.8.0;
+
+/// @notice Modern and gas efficient ERC20 + EIP-2612 implementation.
+/// @author Solmate (https://github.com/transmissions11/solmate/blob/main/src/tokens/ERC20.sol)
+/// @author Modified from Uniswap (https://github.com/Uniswap/uniswap-v2-core/blob/master/contracts/UniswapV2ERC20.sol)
+/// @dev Do not manually set balances without updating totalSupply, as the sum of all user balances must not exceed it.
+abstract contract ERC20 {
+    /*//////////////////////////////////////////////////////////////
+                                 EVENTS
+    //////////////////////////////////////////////////////////////*/
+
+    event Transfer(address indexed from, address indexed to, uint256 amount);
+
+    event Approval(address indexed owner, address indexed spender, uint256 amount);
+
+    /*//////////////////////////////////////////////////////////////
+                            METADATA STORAGE
+    //////////////////////////////////////////////////////////////*/
+
+    string public name;
+
+    string public symbol;
+
+    uint8 public immutable decimals;
+
+    /*//////////////////////////////////////////////////////////////
+                              ERC20 STORAGE
+    //////////////////////////////////////////////////////////////*/
+
+    uint256 public totalSupply;
+
+    mapping(address => uint256) public balanceOf;
+
+    mapping(address => mapping(address => uint256)) public allowance;
+
+    /*//////////////////////////////////////////////////////////////
+                            EIP-2612 STORAGE
+    //////////////////////////////////////////////////////////////*/
+
+    uint256 internal immutable INITIAL_CHAIN_ID;
+
+    bytes32 internal immutable INITIAL_DOMAIN_SEPARATOR;
+
+    mapping(address => uint256) public nonces;
+
+    /*//////////////////////////////////////////////////////////////
+                               CONSTRUCTOR
+    //////////////////////////////////////////////////////////////*/
+
+    constructor(
+        string memory _name,
+        string memory _symbol,
+        uint8 _decimals
+    ) {
+        name = _name;
+        symbol = _symbol;
+        decimals = _decimals;
+
+        INITIAL_CHAIN_ID = block.chainid;
+        INITIAL_DOMAIN_SEPARATOR = computeDomainSeparator();
+    }
+
+    /*//////////////////////////////////////////////////////////////
+                               ERC20 LOGIC
+    //////////////////////////////////////////////////////////////*/
+
+    function approve(address spender, uint256 amount) public virtual returns (bool) {
+        allowance[msg.sender][spender] = amount;
+
+        emit Approval(msg.sender, spender, amount);
+
+        return true;
+    }
+
+    function transfer(address to, uint256 amount) public virtual returns (bool) {
+        balanceOf[msg.sender] -= amount;
+
+        // Cannot overflow because the sum of all user
+        // balances can't exceed the max uint256 value.
+        unchecked {
+            balanceOf[to] += amount;
+        }
+
+        emit Transfer(msg.sender, to, amount);
+
+        return true;
+    }
+
+    function transferFrom(
+        address from,
+        address to,
+        uint256 amount
+    ) public virtual returns (bool) {
+        uint256 allowed = allowance[from][msg.sender]; // Saves gas for limited approvals.
+
+        if (allowed != type(uint256).max) allowance[from][msg.sender] = allowed - amount;
+
+        balanceOf[from] -= amount;
+
+        // Cannot overflow because the sum of all user
+        // balances can't exceed the max uint256 value.
+        unchecked {
+            balanceOf[to] += amount;
+        }
+
+        emit Transfer(from, to, amount);
+
+        return true;
+    }
+
+    /*//////////////////////////////////////////////////////////////
+                             EIP-2612 LOGIC
+    //////////////////////////////////////////////////////////////*/
+
+    function permit(
+        address owner,
+        address spender,
+        uint256 value,
+        uint256 deadline,
+        uint8 v,
+        bytes32 r,
+        bytes32 s
+    ) public virtual {
+        require(deadline >= block.timestamp, "PERMIT_DEADLINE_EXPIRED");
+
+        // Unchecked because the only math done is incrementing
+        // the owner's nonce which cannot realistically overflow.
+        unchecked {
+            address recoveredAddress = ecrecover(
+                keccak256(
+                    abi.encodePacked(
+                        "\x19\x01",
+                        DOMAIN_SEPARATOR(),
+                        keccak256(
+                            abi.encode(
+                                keccak256(
+                                    "Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)"
+                                ),
+                                owner,
+                                spender,
+                                value,
+                                nonces[owner]++,
+                                deadline
+                            )
+                        )
+                    )
+                ),
+                v,
+                r,
+                s
+            );
+
+            require(recoveredAddress != address(0) && recoveredAddress == owner, "INVALID_SIGNER");
+
+            allowance[recoveredAddress][spender] = value;
+        }
+
+        emit Approval(owner, spender, value);
+    }
+
+    function DOMAIN_SEPARATOR() public view virtual returns (bytes32) {
+        return block.chainid == INITIAL_CHAIN_ID ? INITIAL_DOMAIN_SEPARATOR : computeDomainSeparator();
+    }
+
+    function computeDomainSeparator() internal view virtual returns (bytes32) {
+        return
+            keccak256(
+                abi.encode(
+                    keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"),
+                    keccak256(bytes(name)),
+                    keccak256("1"),
+                    block.chainid,
+                    address(this)
+                )
+            );
+    }
+
+    /*//////////////////////////////////////////////////////////////
+                        INTERNAL MINT/BURN LOGIC
+    //////////////////////////////////////////////////////////////*/
+
+    function _mint(address to, uint256 amount) internal virtual {
+        totalSupply += amount;
+
+        // Cannot overflow because the sum of all user
+        // balances can't exceed the max uint256 value.
+        unchecked {
+            balanceOf[to] += amount;
+        }
+
+        emit Transfer(address(0), to, amount);
+    }
+
+    function _burn(address from, uint256 amount) internal virtual {
+        balanceOf[from] -= amount;
+
+        // Cannot underflow because a user's balance
+        // will never be larger than the total supply.
+        unchecked {
+            totalSupply -= amount;
+        }
+
+        emit Transfer(from, address(0), amount);
+    }
+}
+
+contract Test is ERC20("TEST", "TEST", 18) {}

tests/test_detectors_mocked_contract.py

@@ -0,0 +1,21 @@
+from solc_select import solc_select
+from slither import Slither
+from slither.detectors.all_detectors import DomainSeparatorCollision
+
+
+def test_permit_domain_collision():
+    """There is not a (known) collision for this function signature
+    so this test mutates a function's name to mock it.
+    """
+    solc_select.switch_global_version("0.8.0", always_install=True)
+    sl = Slither("tests/mock_permit_domain_collision.sol")
+    assert len(sl.contracts_derived) == 1
+    contract = sl.contracts_derived[0]
+    # This will memoize the solidity signature and mutating the name
+    # won't change the function selector calculated
+    func = contract.get_function_from_signature("DOMAIN_SEPARATOR()")
+    # Change the name to mock
+    func.name = "MOCK_COLLISION"
+    sl.register_detector(DomainSeparatorCollision)
+    results = sl.run_detectors()
+    assert len(results) == 1