From 21dd79a908bfe670cbb8be9f2bb0fd651a3fe814 Mon Sep 17 00:00:00 2001 From: Feltchy Date: Thu, 25 Jun 2026 17:13:03 +1000 Subject: [PATCH] feat: add ERC-20 approval race + low-level call checks (closes #35, #36, #37) --- .../detectors/custom_erc20_approval_race.py | 122 +++++++++++++ slither/detectors/custom_low_level_call.py | 124 +++++++++++++ src/checks/ERC20ApprovalRaceCheck.sol | 129 ++++++++++++++ src/checks/LowLevelCallCheck.sol | 122 +++++++++++++ src/examples/SecureERC20Approval.sol | 99 +++++++++++ src/examples/SecureLowLevelCall.sol | 46 +++++ src/examples/VulnerableERC20Approval.sol | 73 ++++++++ src/examples/VulnerableLowLevelCall.sol | 54 ++++++ test/ERC20ApprovalRace.t.sol | 168 ++++++++++++++++++ test/LowLevelCall.t.sol | 123 +++++++++++++ 10 files changed, 1060 insertions(+) create mode 100644 slither/detectors/custom_erc20_approval_race.py create mode 100644 slither/detectors/custom_low_level_call.py create mode 100644 src/checks/ERC20ApprovalRaceCheck.sol create mode 100644 src/checks/LowLevelCallCheck.sol create mode 100644 src/examples/SecureERC20Approval.sol create mode 100644 src/examples/SecureLowLevelCall.sol create mode 100644 src/examples/VulnerableERC20Approval.sol create mode 100644 src/examples/VulnerableLowLevelCall.sol create mode 100644 test/ERC20ApprovalRace.t.sol create mode 100644 test/LowLevelCall.t.sol diff --git a/slither/detectors/custom_erc20_approval_race.py b/slither/detectors/custom_erc20_approval_race.py new file mode 100644 index 0000000..15ead6e --- /dev/null +++ b/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 diff --git a/slither/detectors/custom_low_level_call.py b/slither/detectors/custom_low_level_call.py new file mode 100644 index 0000000..9fa1017 --- /dev/null +++ b/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 diff --git a/src/checks/ERC20ApprovalRaceCheck.sol b/src/checks/ERC20ApprovalRaceCheck.sol new file mode 100644 index 0000000..ad661f9 --- /dev/null +++ b/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" + ); + } + } +} diff --git a/src/checks/LowLevelCallCheck.sol b/src/checks/LowLevelCallCheck.sol new file mode 100644 index 0000000..1f40959 --- /dev/null +++ b/src/checks/LowLevelCallCheck.sol @@ -0,0 +1,122 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.24; + +import "../ChecklistBase.sol"; + +/// @title LowLevelCallCheck — detect unchecked low-level call return values +/// @notice Tests whether the target contract properly checks the success +/// boolean from .call(), .delegatecall(), and .staticcall(). +/// Override `getUncheckedWithdrawCalldata()` to point at the +/// vulnerable withdraw function. +/// @author kcolbchain +abstract contract LowLevelCallCheck is ChecklistBase { + /// @dev Override to return calldata for the unchecked withdraw function + function getUncheckedWithdrawCalldata() + internal + view + virtual + returns (bytes memory); + + /// @dev Override to return the deposit amount + function getDepositAmount() internal view virtual returns (uint256) { + return 1 ether; + } + + /// @dev Override to perform a deposit + function performDeposit(address depositor, uint256 amount) internal virtual; + + /// @notice Test: silent failure on .call() without return value check + /// @dev Deploys a malicious receiver that reverts on receive(), + /// deposits, then calls the unchecked withdraw. If the target + /// clears the balance despite the failed ETH transfer, it's + /// vulnerable. + function test_unchecked_call_silent_failure() public { + address attacker = makeAddr("unchecked_call_attacker"); + uint256 depositAmount = getDepositAmount(); + + // Deploy a contract that reverts on ETH receipt + RevertingReceiver receiver = new RevertingReceiver(targetContract); + + // Fund and deposit + vm.deal(attacker, depositAmount); + performDeposit(attacker, depositAmount); + + uint256 balanceBefore = address(attacker).balance; + + // Call the unchecked withdraw via the attacker + // If the function uses .call() without checking return value, + // the state will be corrupted + vm.prank(attacker); + (bool callSuccess,) = targetContract.call( + getUncheckedWithdrawCalldata() + ); + + // The key test: if the external call succeeded (from the caller's perspective) + // but ETH didn't actually move, there's a silent failure + // We check by looking at whether the attacker lost funds + // without gaining them in another form + if (callSuccess) { + uint256 balanceAfter = address(attacker).balance; + // If caller's balance stayed same but TX succeeded, + // the .call() inside returned false but the function didn't revert + if (balanceAfter == balanceBefore) { + emit log( + unicode"VULNERABILITY: Unchecked .call() return value — TX succeeded but ETH not transferred" + ); + emit log( + unicode"The contract likely discards the .call() success boolean" + ); + fail(); + } + } + } + + /// @notice Test: delegatecall return value unchecked + function test_unchecked_delegatecall() public { + // Deploy a target that makes a delegatecall without checking return + // If the target is known to have such a function, test it + bytes memory delegateCallData = getDelegatecallCalldata(); + + if (delegateCallData.length > 0) { + address zeroAddr = address(0); + + vm.prank(address(this)); + (bool success,) = targetContract.call(delegateCallData); + + if (success) { + emit log( + unicode"WARNING: delegatecall succeeded against potentially zero-address target — " + unicode"check if return value is verified" + ); + // Warning only — context-dependent + } + } + } + + /// @dev Override for delegatecall test + function getDelegatecallCalldata() + internal + view + virtual + returns (bytes memory) + { + return ""; + } +} + +/// @dev Helper: contract that reverts on ETH receipt +contract RevertingReceiver { + address public target; + + constructor(address _target) { + target = _target; + } + + receive() external payable { + revert("RevertingReceiver: intentional revert to test .call() check"); + } + + function checkBalance(address who) external view returns (uint256) { + return who.balance; + } +} diff --git a/src/examples/SecureERC20Approval.sol b/src/examples/SecureERC20Approval.sol new file mode 100644 index 0000000..7c49431 --- /dev/null +++ b/src/examples/SecureERC20Approval.sol @@ -0,0 +1,99 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.24; + +/// @title SecureERC20Approval — mitigates the approval race condition +/// @notice Uses the increaseAllowance/decreaseAllowance pattern to safely +/// adjust allowances without the front-running risk. +/// @author kcolbchain +contract SecureERC20Approval { + mapping(address => uint256) public balanceOf; + mapping(address => mapping(address => uint256)) public allowance; + + uint256 public totalSupply; + + string public name = "SecureToken"; + string public symbol = "SECURE"; + uint8 public decimals = 18; + + event Transfer(address indexed from, address indexed to, uint256 value); + event Approval( + address indexed owner, + address indexed spender, + uint256 value + ); + + function mint(address to, uint256 amount) external { + balanceOf[to] += amount; + totalSupply += amount; + emit Transfer(address(0), to, amount); + } + + /// @notice FIXED: approve() requires old allowance to be zero first + function approve(address spender, uint256 value) external returns (bool) { + require( + allowance[msg.sender][spender] == 0 || value == 0, + "ERC20: approve from non-zero to non-zero allowance" + ); + allowance[msg.sender][spender] = value; + emit Approval(msg.sender, spender, value); + return true; + } + + /// @notice FIXED: increaseAllowance safely adds to existing allowance + function increaseAllowance( + address spender, + uint256 addedValue + ) external returns (bool) { + allowance[msg.sender][spender] += addedValue; + emit Approval( + msg.sender, + spender, + allowance[msg.sender][spender] + ); + return true; + } + + /// @notice FIXED: decreaseAllowance safely subtracts from existing allowance + function decreaseAllowance( + address spender, + uint256 subtractedValue + ) external returns (bool) { + uint256 currentAllowance = allowance[msg.sender][spender]; + require( + currentAllowance >= subtractedValue, + "ERC20: decreased allowance below zero" + ); + allowance[msg.sender][spender] = currentAllowance - subtractedValue; + emit Approval( + msg.sender, + spender, + allowance[msg.sender][spender] + ); + return true; + } + + function transfer( + address to, + uint256 value + ) external returns (bool) { + require(balanceOf[msg.sender] >= value, "insufficient balance"); + balanceOf[msg.sender] -= value; + balanceOf[to] += value; + emit Transfer(msg.sender, to, value); + return true; + } + + function transferFrom( + address from, + address to, + uint256 value + ) external returns (bool) { + require(balanceOf[from] >= value, "insufficient balance"); + require(allowance[from][msg.sender] >= value, "insufficient allowance"); + allowance[from][msg.sender] -= value; + balanceOf[from] -= value; + balanceOf[to] += value; + emit Transfer(from, to, value); + return true; + } +} diff --git a/src/examples/SecureLowLevelCall.sol b/src/examples/SecureLowLevelCall.sol new file mode 100644 index 0000000..29d49a1 --- /dev/null +++ b/src/examples/SecureLowLevelCall.sol @@ -0,0 +1,46 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.24; + +/// @title SecureLowLevelCall — properly checks .call() return values +/// @notice Fixes all three vulnerabilities from VulnerableLowLevelCall by +/// requiring success on every low-level call. +/// @author kcolbchain +contract SecureLowLevelCall { + mapping(address => uint256) public balances; + + function deposit() external payable { + balances[msg.sender] += msg.value; + } + + /// @notice FIXED: .call() return value is checked via require() + function withdrawChecked() external { + uint256 amount = balances[msg.sender]; + require(amount > 0, "No balance"); + + // Effects first (Checks-Effects-Interactions, but simple case) + balances[msg.sender] = 0; + + // FIXED: require the call succeeds + (bool success,) = msg.sender.call{value: amount}(""); + require(success, "ETH transfer failed"); + } + + /// @notice FIXED: delegatecall() return value checked + function upgradeToChecked(address newImpl) external { + // FIXED: require delegatecall success + (bool success,) = newImpl.delegatecall( + abi.encodeWithSignature("initialize()") + ); + require(success, "delegatecall failed"); + } + + /// @notice FIXED: staticcall() return value checked + function getValueChecked(address target) external view returns (uint256) { + // FIXED: require staticcall success + (bool success, bytes memory data) = target.staticcall( + abi.encodeWithSignature("getValue()") + ); + require(success, "staticcall failed"); + return abi.decode(data, (uint256)); + } +} diff --git a/src/examples/VulnerableERC20Approval.sol b/src/examples/VulnerableERC20Approval.sol new file mode 100644 index 0000000..e023289 --- /dev/null +++ b/src/examples/VulnerableERC20Approval.sol @@ -0,0 +1,73 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.24; + +import {IERC20} from "forge-std/interfaces/IERC20.sol"; + +/// @title VulnerableERC20Approval — demonstrates the approval race condition +/// @notice DO NOT USE IN PRODUCTION. +/// The approve() function allows a spender to front-run a new approval +/// and spend both the old + new allowance (double-spend). +/// @author kcolbchain +contract VulnerableERC20Approval { + mapping(address => uint256) public balanceOf; + mapping(address => mapping(address => uint256)) public allowance; + + uint256 public totalSupply; + + string public name = "VulnerableToken"; + string public symbol = "VULN"; + uint8 public decimals = 18; + + event Transfer(address indexed from, address indexed to, uint256 value); + event Approval( + address indexed owner, + address indexed spender, + uint256 value + ); + + function mint(address to, uint256 amount) external { + balanceOf[to] += amount; + totalSupply += amount; + emit Transfer(address(0), to, amount); + } + + /// @notice BUG: approve() directly overwrites allowance without safety check + /// @dev An attacker monitoring the mempool can: + /// 1. See approve(spender, 100) in mempool + /// 2. Front-run and spend 100 tokens via transferFrom + /// 3. Owner's approve(spender, 50) executes → new allowance = 50 + /// 4. Attacker now spends ANOTHER 50 tokens + /// Total stolen: 150 tokens from a 100 → 50 decrease + function approve(address spender, uint256 value) external returns (bool) { + // BUG: No require(allowance[msg.sender][spender] == 0 || value == 0) + // The old allowance must be zero before changing to a non-zero value + allowance[msg.sender][spender] = value; + emit Approval(msg.sender, spender, value); + return true; + } + + function transfer( + address to, + uint256 value + ) external returns (bool) { + require(balanceOf[msg.sender] >= value, "insufficient balance"); + balanceOf[msg.sender] -= value; + balanceOf[to] += value; + emit Transfer(msg.sender, to, value); + return true; + } + + function transferFrom( + address from, + address to, + uint256 value + ) external returns (bool) { + require(balanceOf[from] >= value, "insufficient balance"); + require(allowance[from][msg.sender] >= value, "insufficient allowance"); + allowance[from][msg.sender] -= value; + balanceOf[from] -= value; + balanceOf[to] += value; + emit Transfer(from, to, value); + return true; + } +} diff --git a/src/examples/VulnerableLowLevelCall.sol b/src/examples/VulnerableLowLevelCall.sol new file mode 100644 index 0000000..4b101be --- /dev/null +++ b/src/examples/VulnerableLowLevelCall.sol @@ -0,0 +1,54 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.24; + +/// @title VulnerableLowLevelCall — demonstrates unchecked .call() return value +/// @notice DO NOT USE IN PRODUCTION. The withdraw() function ignores the +/// .call() success boolean, leading to silent failures and state +/// inconsistency when the external call reverts or runs out of gas. +/// @author kcolbchain +contract VulnerableLowLevelCall { + mapping(address => uint256) public balances; + + function deposit() external payable { + balances[msg.sender] += msg.value; + } + + /// @notice VULNERABLE: .call() return value is silently discarded + /// @dev If the recipient's receive() reverts (or runs out of gas), + /// the ETH is not transferred but the balance IS cleared — the + /// user permanently loses their funds with no error. + function withdrawUnchecked() external { + uint256 amount = balances[msg.sender]; + require(amount > 0, "No balance"); + + // BUG: return value (bool success) is discarded + // If this call fails silently, balances are still wiped + msg.sender.call{value: amount}(""); + + // State update happens regardless of whether ETH was sent + balances[msg.sender] = 0; + } + + /// @notice VULNERABLE: delegatecall() return value unchecked + /// @dev If the target has no code or reverts, the caller won't know + function upgradeToUnchecked(address newImpl) external { + // BUG: delegatecall return value unchecked + // If newImpl has no code, the proxy silently points to a zero-byte address + newImpl.delegatecall(abi.encodeWithSignature("initialize()")); + } + + /// @notice VULNERABLE: staticcall() return value unchecked + /// @dev Returns stale/zero data when the call fails + function getValueUnchecked(address target) external view returns (uint256) { + // BUG: staticcall return value unchecked + // Returns default 0 instead of indicating failure + (bool success, bytes memory data) = target.staticcall( + abi.encodeWithSignature("getValue()") + ); + // success is unused — if call fails, we return 0 silently + if (data.length >= 32) { + return abi.decode(data, (uint256)); + } + return 0; + } +} diff --git a/test/ERC20ApprovalRace.t.sol b/test/ERC20ApprovalRace.t.sol new file mode 100644 index 0000000..88c4ba3 --- /dev/null +++ b/test/ERC20ApprovalRace.t.sol @@ -0,0 +1,168 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.24; + +import "forge-std/Test.sol"; +import "../src/checks/ERC20ApprovalRaceCheck.sol"; +import "../src/examples/VulnerableERC20Approval.sol"; +import "../src/examples/SecureERC20Approval.sol"; + +/// @title TestERC20ApprovalRaceVulnerable — proves the approval race bug +contract TestERC20ApprovalRaceVulnerable is ERC20ApprovalRaceCheck { + VulnerableERC20Approval token; + + function setUp() public { + token = new VulnerableERC20Approval(); + targetContract = address(token); + } + + function getApproveCalldata( + address spender, + uint256 amount + ) internal pure override returns (bytes memory) { + return abi.encodeWithSignature("approve(address,uint256)", spender, amount); + } + + function getTransferFromCalldata( + address from, + address to, + uint256 amount + ) internal pure override returns (bytes memory) { + return + abi.encodeWithSignature( + "transferFrom(address,address,uint256)", + from, + to, + amount + ); + } + + function setupAllowance( + address owner, + address spender, + uint256 initialAllowance + ) internal override { + // Mint tokens to owner + token.mint(owner, initialAllowance * 2); + // Owner approves spender + vm.prank(owner); + token.approve(spender, initialAllowance); + } + + /// @notice Direct proof: spender double-spends via approval front-running + function test_prove_double_spend_attack() public { + address owner = makeAddr("victim"); + address attacker = makeAddr("attacker"); + address attackerWallet = makeAddr("wallet"); + + uint256 initialApproval = 100 ether; + uint256 reducedApproval = 10 ether; + + // Mint tokens to owner + token.mint(owner, 200 ether); + + // Owner approves attacker for 100 + vm.prank(owner); + token.approve(attacker, initialApproval); + + // ATTACKER front-runs the reduced approval: + // Step 1: Attacker spends the full 100 before reduction + vm.prank(attacker); + token.transferFrom(owner, attackerWallet, initialApproval); + + // Step 2: Owner's reduction TX lands (intended: reduce to 10) + vm.prank(owner); + token.approve(attacker, reducedApproval); + + // Step 3: Attacker spends ANOTHER 10 + vm.prank(attacker); + token.transferFrom(owner, attackerWallet, reducedApproval); + + // VULNERABILITY CONFIRMED: attacker extracted 110 tokens + // Owner intended max 10 tokens from this point + assertEq( + token.balanceOf(attackerWallet), + initialApproval + reducedApproval, + "Attacker double-spent the allowance" + ); + // This assertion fails (and that's the proof): the attacker got 110, not 10 + } +} + +/// @title TestERC20ApprovalRaceSecure — verifies the fixed version +contract TestERC20ApprovalRaceSecure is ERC20ApprovalRaceCheck { + SecureERC20Approval token; + + function setUp() public { + token = new SecureERC20Approval(); + targetContract = address(token); + } + + function getApproveCalldata( + address spender, + uint256 amount + ) internal pure override returns (bytes memory) { + return abi.encodeWithSignature("approve(address,uint256)", spender, amount); + } + + function getTransferFromCalldata( + address from, + address to, + uint256 amount + ) internal pure override returns (bytes memory) { + return + abi.encodeWithSignature( + "transferFrom(address,address,uint256)", + from, + to, + amount + ); + } + + function setupAllowance( + address owner, + address spender, + uint256 initialAllowance + ) internal override { + token.mint(owner, initialAllowance * 2); + vm.prank(owner); + token.approve(spender, initialAllowance); + } + + /// @notice Verify: secure version prevents non-zero-to-non-zero approval change + function test_secure_rejects_dangerous_approval_change() public { + address owner = makeAddr("safe_owner"); + address spender = makeAddr("safe_spender"); + + token.mint(owner, 200 ether); + + // Set initial allowance to 100 + vm.prank(owner); + token.approve(spender, 100 ether); + + // Try to change allowance from 100 → 50 without zeroing first + vm.prank(owner); + vm.expectRevert("ERC20: approve from non-zero to non-zero allowance"); + token.approve(spender, 50 ether); + } + + /// @notice Verify: secure version allows safe decreaseAllowance + function test_secure_decrease_allowance_works() public { + address owner = makeAddr("safe_owner"); + address spender = makeAddr("safe_spender"); + + token.mint(owner, 200 ether); + vm.prank(owner); + token.approve(spender, 100 ether); + + // decreaseAllowance should work (adjusts from existing, no double-spend risk) + vm.prank(owner); + token.decreaseAllowance(spender, 40 ether); + + // Spender can now only spend 60 + address wallet = makeAddr("wallet"); + vm.prank(spender); + token.transferFrom(owner, wallet, 60 ether); + + assertEq(token.balanceOf(wallet), 60 ether); + } +} diff --git a/test/LowLevelCall.t.sol b/test/LowLevelCall.t.sol new file mode 100644 index 0000000..6558d45 --- /dev/null +++ b/test/LowLevelCall.t.sol @@ -0,0 +1,123 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.24; + +import "forge-std/Test.sol"; +import "../src/checks/LowLevelCallCheck.sol"; +import "../src/examples/VulnerableLowLevelCall.sol"; +import "../src/examples/SecureLowLevelCall.sol"; + +/// @title LowLevelCallAudit — demonstrates the unchecked-call check +/// @notice Run: forge test --match-contract LowLevelCallAudit -vvv +contract TestLowLevelCallVulnerable is LowLevelCallCheck { + VulnerableLowLevelCall vulnerable; + + function setUp() public { + vulnerable = new VulnerableLowLevelCall(); + targetContract = address(vulnerable); + } + + function getUncheckedWithdrawCalldata() + internal + pure + override + returns (bytes memory) + { + return abi.encodeWithSignature("withdrawUnchecked()"); + } + + function performDeposit(address depositor, uint256 amount) internal override { + vm.prank(depositor); + vulnerable.deposit{value: amount}(); + } + + /// @notice Prove the vulnerability: deposit, withdraw via reverting receiver, + /// verify funds lost with no revert + function test_prove_unchecked_call_vulnerability() public { + address user = makeAddr("user"); + uint256 amount = 1 ether; + + vm.deal(user, amount); + vm.prank(user); + vulnerable.deposit{value: amount}(); + + // Deploy a reverting receiver as the user's "wallet" + // and impersonate it as the caller of withdrawUnchecked + RevertingReceiver receiver = new RevertingReceiver(address(vulnerable)); + + // The receiver deposits and then tries to withdraw + vm.deal(address(receiver), amount); + vm.prank(address(receiver)); + vulnerable.deposit{value: amount}(); + + uint256 balBefore = address(receiver).balance; + + // Call withdrawUnchecked — the receiver's receive() will revert, + // but withdrawUnchecked doesn't check the .call() return value + vm.prank(address(receiver)); + vulnerable.withdrawUnchecked(); + + uint256 balAfter = address(receiver).balance; + + // VULNERABILITY CONFIRMED: receiver's ETH balance didn't increase + // but vulnerable contract cleared their balance + assertEq(balAfter, balBefore, "Receiver should have received ETH"); + // This will fail, proving the vulnerability: + // The .call() silently failed but the balance was still wiped + } +} + +/// @title SecureLowLevelCallAudit — verifies the fixed version +contract TestLowLevelCallSecure is LowLevelCallCheck { + SecureLowLevelCall secure; + + function setUp() public { + secure = new SecureLowLevelCall(); + targetContract = address(secure); + } + + function getUncheckedWithdrawCalldata() + internal + pure + override + returns (bytes memory) + { + return abi.encodeWithSignature("withdrawChecked()"); + } + + function performDeposit(address depositor, uint256 amount) internal override { + vm.prank(depositor); + secure.deposit{value: amount}(); + } + + /// @notice Verify the fixed version properly reverts on failed .call() + function test_secure_withdraw_reverts_on_failed_call() public { + // Deploy a reverting receiver + RevertingReceiver receiver = new RevertingReceiver(address(secure)); + + uint256 amount = 1 ether; + vm.deal(address(receiver), amount); + vm.prank(address(receiver)); + secure.deposit{value: amount}(); + + // The fixed version should revert because .call() return value is checked + vm.prank(address(receiver)); + vm.expectRevert("ETH transfer failed"); + secure.withdrawChecked(); + } + + /// @notice Verify secure version works for normal (non-reverting) receivers + function test_secure_withdraw_succeeds_for_normal_user() public { + address user = makeAddr("user"); + uint256 amount = 1 ether; + + vm.deal(user, amount); + vm.prank(user); + secure.deposit{value: amount}(); + + uint256 balBefore = user.balance; + vm.prank(user); + secure.withdrawChecked(); + + assertEq(user.balance, balBefore + amount, "User should receive ETH back"); + } +}