feat: add ERC-20 approval race + low-level call checks

slither/detectors/custom_erc20_approval_race.py

@@ -0,0 +1,122 @@
+"""Custom Slither detector for ERC-20 approval race condition.
+
+Detects: approve() functions that overwrite non-zero allowances with
+non-zero values, enabling front-running double-spend attacks.
+
+The classic attack: Alice approves Bob for 100, then changes to 50.
+Bob front-runs the second approve() and spends 100, then spends
+the new 50 = 150 total from an intended max of 50.
+
+Mitigation: require(old_allowance == 0 || new_value == 0) in approve(),
+or use increaseAllowance/decreaseAllowance pattern.
+"""
+from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
+from slither.slithir.operations import InternalCall, HighLevelCall
+from slither.core.declarations import Function
+
+
+class CustomERC20ApprovalRaceDetector(AbstractDetector):
+    ARGUMENT = "custom-erc20-approval-race"
+    HELP = "Detects ERC-20 approve() functions vulnerable to approval race condition"
+    IMPACT = DetectorClassification.MEDIUM
+    CONFIDENCE = DetectorClassification.MEDIUM
+
+    WIKI = (
+        "https://github.com/kcolbchain/audit-checklist/wiki/ERC20-Approval-Race-Detector"
+    )
+    WIKI_TITLE = "Custom ERC-20 Approval Race Detector"
+
+    def _detect(self) -> list:
+        results = []
+
+        for contract in self.compilation_unit.contracts:
+            # Find the approve function
+            approve_func = self._find_approve_function(contract)
+            if approve_func is None:
+                continue
+
+            # Check if the approve function has the race-condition mitigation
+            has_protection = self._has_approval_race_protection(approve_func, contract)
+
+            # Check if increaseAllowance / decreaseAllowance exist
+            has_increase = self._has_function(contract, "increaseAllowance")
+            has_decrease = self._has_function(contract, "decreaseAllowance")
+
+            if not has_protection and not (has_increase and has_decrease):
+                info = [
+                    "ERC-20 approval race condition in ",
+                    f"{contract.name}.approve()\n",
+                    f"  The approve() function overwrites allowances without checking\n",
+                    f"  that the old allowance is zero. A spender can front-run the\n",
+                    f"  approve() transaction and double-spend.\n",
+                    "\nMitigation options:\n",
+                    "  1. Add: require(allowance[msg.sender][spender] == 0 || value == 0)\n",
+                    "  2. Or implement increaseAllowance() / decreaseAllowance()\n",
+                ]
+                res = self.generate_result(info)
+                if approve_func.entry_point:
+                    res.add(approve_func.entry_point)
+                results.append(res)
+
+            # Also check: does the token have increaseAllowance but not decreaseAllowance?
+            if has_increase and not has_decrease:
+                info = [
+                    "Incomplete allowance safety in ",
+                    f"{contract.name}\n",
+                    f"  increaseAllowance() exists but decreaseAllowance() is missing.\n",
+                    f"  Without decreaseAllowance, users may still use approve() directly.\n",
+                ]
+                res = self.generate_result(info)
+                results.append(res)
+
+        return results
+
+    def _find_approve_function(self, contract):
+        """Find the approve(address,uint256) function."""
+        for function in contract.functions:
+            if function.name == "approve" and function.is_implemented:
+                params = function.parameters
+                if len(params) == 2:
+                    # Check signature: approve(address,uint256)
+                    if (params[0].type == "address" and
+                            "uint" in params[1].type):
+                        return function
+        return None
+
+    def _has_approval_race_protection(self, func, contract) -> bool:
+        """Check if approve() has non-zero-to-non-zero protection.
+
+        Looks for require statements that check old_allowance == 0
+        or new_value == 0 before setting the allowance.
+        """
+        source = str(func).lower()
+        # Common protection patterns
+        protection_patterns = [
+            "allowance[msg.sender][spender] == 0",
+            "allowance[msg.sender][_spender] == 0",
+            "currentallowance == 0",
+            "oldallowance == 0",
+        ]
+
+        for pattern in protection_patterns:
+            if pattern in source:
+                return True
+
+        # Check for OpenZeppelin's _approve internal function pattern
+        # which has a separate _spendAllowance
+        has_spend_allowance = self._has_function(contract, "_spendAllowance")
+        has_internal_approve = self._has_function(contract, "_approve")
+
+        if has_spend_allowance and has_internal_approve:
+            # OpenZeppelin v4+ pattern — likely has protection
+            # Still flag if approve() is public and has no require
+            return True
+
+        return False
+
+    def _has_function(self, contract, name: str) -> bool:
+        """Check if a function with the given name exists."""
+        for function in contract.functions:
+            if function.name == name and function.is_implemented:
+                return True
+        return False

slither/detectors/custom_low_level_call.py

@@ -0,0 +1,124 @@
+"""Custom Slither detector for unchecked low-level call return values.
+
+Detects: .call(), .delegatecall(), .staticcall(), and .send() where the
+return boolean is not checked, leading to silent failures.
+
+Common vulnerability patterns:
+- address.call{value: x}("") without checking success
+- target.delegatecall(data) without checking success
+- target.staticcall(data) without checking success
+- address.send(amount) without checking return (send returns bool)
+"""
+from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
+from slither.slithir.operations import LowLevelCall, Send, Transfer
+
+
+class CustomLowLevelCallDetector(AbstractDetector):
+    ARGUMENT = "custom-low-level-call"
+    HELP = "Detects unchecked low-level call return values (.call, .delegatecall, .staticcall, .send)"
+    IMPACT = DetectorClassification.MEDIUM
+    CONFIDENCE = DetectorClassification.HIGH
+
+    WIKI = (
+        "https://github.com/kcolbchain/audit-checklist/wiki/Low-Level-Call-Detector"
+    )
+    WIKI_TITLE = "Custom Low-Level Call Return Value Detector"
+
+    def _detect(self) -> list:
+        results = []
+
+        for contract in self.compilation_unit.contracts:
+            for function in contract.functions_and_modifiers:
+                if not function.is_implemented:
+                    continue
+
+                for node in function.nodes:
+                    for ir in node.irs:
+                        self._check_low_level_call(ir, node, function, contract, results)
+                        self._check_send(ir, node, function, contract, results)
+
+        return results
+
+    def _check_low_level_call(self, ir, node, function, contract, results):
+        """Check .call(), .delegatecall(), .staticcall() return values."""
+        if not isinstance(ir, LowLevelCall):
+            return
+
+        call_type = ir.call_type  # "call", "delegatecall", "staticcall"
+
+        # Check if the return value (success, returndata) is used
+        # LowLevelCall typically assigns to (success, returndata) = ...
+        # If the LVALUE is empty or discarded, it's unchecked
+        if ir.lvalue is None:
+            # No assignment at all — definitely unchecked
+            info = [
+                f"Unchecked {call_type}() return value in ",
+                f"{function.name}() of {contract.name}\n",
+                f"  At {node.source_mapping}\n",
+                f"  The {call_type}() return boolean is not captured.\n",
+                f"  If the call fails silently, execution continues as if it succeeded.\n",
+                "\nFix: capture the success boolean and require/revert on failure.\n",
+                "  (bool success, ) = target.call{value: amount}(\"\");\n",
+                "  require(success, \"Call failed\");\n",
+            ]
+            res = self.generate_result(info)
+            res.add(node)
+            results.append(res)
+            return
+
+        # Check if the success variable is actually used after the call
+        if isinstance(ir.lvalue, tuple) and len(ir.lvalue) >= 1:
+            success_var = ir.lvalue[0]
+            # Check if success_var is used in any subsequent operation
+            used_after = self._is_variable_used_after(node, function, success_var)
+            if not used_after:
+                info = [
+                    f"Unchecked {call_type}() return value in ",
+                    f"{function.name}() of {contract.name}\n",
+                    f"  At {node.source_mapping}\n",
+                    f"  The success boolean is captured but never checked.\n",
+                    f"  If the call fails silently, execution continues as if it succeeded.\n",
+                    "\nFix: require(success, \"...\") or if (!success) revert(...)\n",
+                ]
+                res = self.generate_result(info)
+                res.add(node)
+                results.append(res)
+
+    def _check_send(self, ir, node, function, contract, results):
+        """Check .send() return values (send returns bool, unlike transfer)."""
+        if not isinstance(ir, Send):
+            return
+
+        if ir.lvalue is None:
+            info = [
+                f"Unchecked .send() return value in ",
+                f"{function.name}() of {contract.name}\n",
+                f"  At {node.source_mapping}\n",
+                f"  .send() returns a bool indicating success.\n",
+                f"  If send fails (e.g. 2300 gas stipend exceeded), it returns false silently.\n",
+                "\nFix: use .call{value: amount}(\"\") with require, or check .send() return.\n",
+            ]
+            res = self.generate_result(info)
+            res.add(node)
+            results.append(res)
+
+    def _is_variable_used_after(self, start_node, function, target_var) -> bool:
+        """Check if target_var is used (read/referenced) in any node
+        after start_node within the same function."""
+        found_start = False
+        for node in function.nodes:
+            if node == start_node:
+                found_start = True
+                continue
+            if not found_start:
+                continue
+            for ir in node.irs:
+                # Check if target_var appears in any read variables
+                if hasattr(ir, "read") and ir.read:
+                    for read_var in ir.read:
+                        if read_var == target_var:
+                            return True
+                # Check if used in condition
+                if hasattr(ir, "variable") and ir.variable == target_var:
+                    return True
+        return False

src/checks/ERC20ApprovalRaceCheck.sol

@@ -0,0 +1,129 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.24;
+
+import "../ChecklistBase.sol";
+
+/// @title ERC20ApprovalRaceCheck — detect approval race condition
+/// @notice Tests whether the target ERC20 token is vulnerable to the
+///         classic approve() front-running attack (double-spend via
+///         non-zero-to-non-zero allowance change).
+/// @author kcolbchain
+abstract contract ERC20ApprovalRaceCheck is ChecklistBase {
+    /// @dev Override to return calldata for approve(spender, amount)
+    function getApproveCalldata(
+        address spender,
+        uint256 amount
+    ) internal view virtual returns (bytes memory);
+
+    /// @dev Override to return calldata for transferFrom(from, to, amount)
+    function getTransferFromCalldata(
+        address from,
+        address to,
+        uint256 amount
+    ) internal view virtual returns (bytes memory);
+
+    /// @dev Override to set up initial balances/allowances for the test
+    function setupAllowance(
+        address owner,
+        address spender,
+        uint256 initialAllowance
+    ) internal virtual;
+
+    /// @notice Test: simulate the approval race condition attack
+    /// @dev 1. Owner approves spender for 100 tokens
+    ///      2. Owner wants to reduce to 50 — calls approve(spender, 50)
+    ///      3. Spender front-runs and spends 100 before approve(50) lands
+    ///      4. After approve(50), spender spends another 50
+    ///      5. Total spent: 150 from intended 50 limit
+    function test_approval_race_double_spend() public {
+        address owner = makeAddr("token_owner");
+        address spender = makeAddr("token_spender");
+        address recipient = makeAddr("recipient");
+
+        uint256 initialAllowance = 100 ether;
+        uint256 reducedAllowance = 50 ether;
+
+        // Step 1: Owner gives spender initial allowance of 100
+        setupAllowance(owner, spender, initialAllowance);
+
+        // Step 2: Spender front-runs the reduction and spends 100
+        vm.prank(spender);
+        (bool success1,) = targetContract.call(
+            getTransferFromCalldata(owner, recipient, initialAllowance)
+        );
+        require(success1, "Step 2 failed: first transferFrom should succeed");
+
+        // Step 3: Owner's reduce-allowance TX lands — sets allowance to 50
+        vm.prank(owner);
+        (bool success2,) = targetContract.call(
+            getApproveCalldata(spender, reducedAllowance)
+        );
+        require(success2, "Step 3 failed: second approve should succeed");
+
+        // Step 4: Spender spends ANOTHER 50 (total = 150, but intended cap was 50)
+        vm.prank(spender);
+        (bool success3,) = targetContract.call(
+            getTransferFromCalldata(owner, recipient, reducedAllowance)
+        );
+
+        // WARNING: if the second transferFrom succeeds,
+        // the spender has spent 150 tokens despite the owner
+        // intending to cap them at 50.
+        // This is a warning, not automatic failure — mitigation depends
+        // on whether the token has non-zero-to-non-zero protection.
+        if (success3) {
+            emit log(
+                unicode"WARNING: ERC-20 approval race condition — "
+                unicode"spender can double-spend by front-running approve()"
+            );
+            emit log(
+                unicode"Total extracted: 150 tokens (100 + 50) vs expected max 50"
+            );
+            emit log(
+                unicode"Mitigation: use increaseAllowance/decreaseAllowance, "
+                unicode"or require(old_allowance == 0 || value == 0) in approve()"
+            );
+        }
+    }
+
+    /// @notice Test: verify decreaseAllowance/increaseAllowance pattern
+    /// @dev If the token supports increaseAllowance, verify it's safe
+    function test_safe_allowance_adjustment() public {
+        address owner = makeAddr("safe_owner");
+        address spender = makeAddr("safe_spender");
+        address recipient = makeAddr("safe_recipient");
+
+        uint256 initialAllowance = 100 ether;
+
+        setupAllowance(owner, spender, initialAllowance);
+
+        // Try increaseAllowance (should check for this function)
+        bytes memory increaseCalldata = abi.encodeWithSignature(
+            "increaseAllowance(address,uint256)",
+            spender,
+            50 ether
+        );
+        vm.prank(owner);
+        (bool hasIncrease,) = targetContract.call(increaseCalldata);
+
+        // Try decreaseAllowance
+        bytes memory decreaseCalldata = abi.encodeWithSignature(
+            "decreaseAllowance(address,uint256)",
+            spender,
+            30 ether
+        );
+        vm.prank(owner);
+        (bool hasDecrease,) = targetContract.call(decreaseCalldata);
+
+        if (hasIncrease || hasDecrease) {
+            emit log(
+                unicode"Token supports increase/decrease allowance — safe pattern in use"
+            );
+        } else {
+            emit log(
+                unicode"WARNING: Token lacks increaseAllowance/decreaseAllowance — "
+                unicode"users must approve(0) before setting new allowance"
+            );
+        }
+    }
+}