From f7e22a75e1be7c72d2b7d3a094899521b6f31b01 Mon Sep 17 00:00:00 2001 From: alpharush <0xalpharush@protonmail.com> Date: Tue, 9 Aug 2022 16:27:35 -0500 Subject: [PATCH] implement detector for erc20 tokens that have function collision with DOMAIN_SEPARATOR --- .github/workflows/features.yml | 1 + slither/detectors/all_detectors.py | 1 + .../permit_domain_signature_collision.py | 55 +++++ tests/mock_permit_domain_collision.sol | 208 ++++++++++++++++++ tests/test_detectors_mocked_contract.py | 21 ++ 5 files changed, 286 insertions(+) create mode 100644 slither/detectors/functions/permit_domain_signature_collision.py create mode 100644 tests/mock_permit_domain_collision.sol create mode 100644 tests/test_detectors_mocked_contract.py diff --git a/.github/workflows/features.yml b/.github/workflows/features.yml index 7414e9ebe6..8c7126dcc3 100644 --- a/.github/workflows/features.yml +++ b/.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 diff --git a/slither/detectors/all_detectors.py b/slither/detectors/all_detectors.py index a79dcaf51a..2c8d244281 100644 --- a/slither/detectors/all_detectors.py +++ b/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 diff --git a/slither/detectors/functions/permit_domain_signature_collision.py b/slither/detectors/functions/permit_domain_signature_collision.py new file mode 100644 index 0000000000..95f9f658e3 --- /dev/null +++ b/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 diff --git a/tests/mock_permit_domain_collision.sol b/tests/mock_permit_domain_collision.sol new file mode 100644 index 0000000000..d766eb0bdc --- /dev/null +++ b/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) {} diff --git a/tests/test_detectors_mocked_contract.py b/tests/test_detectors_mocked_contract.py new file mode 100644 index 0000000000..8d8d5faa87 --- /dev/null +++ b/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