diff --git a/README.md b/README.md index 50bb335..f16aac3 100644 --- a/README.md +++ b/README.md @@ -173,6 +173,7 @@ or open [`web/index.html`](web/index.html) directly. | `OracleCheck` | Spot price reads (manipulable), missing TWAP, single-source oracles | | `UpgradeCheck` | Storage layout collisions in proxies, uninitialized implementation contracts | | `FlashLoanCheck` | Functions vulnerable to flash-loan-powered price/state manipulation | +| `ERC20ApprovalRaceCheck` | Unsafe ERC-20 allowance overwrites that enable approve front-running | ## Architecture @@ -192,6 +193,10 @@ test/ └── Example.t.sol — Full example audit against VulnerableVault ``` +The ERC-20 approval race check includes `src/checks/ERC20ApprovalRaceCheck.sol`, +`src/examples/VulnerableERC20Approval.sol`, `test/ERC20ApprovalRace.t.sol`, and +the companion Slither heuristic in `slither/detectors/custom_erc20_approval_race.py`. + ## License MIT diff --git a/slither/detectors/custom_erc20_approval_race.py b/slither/detectors/custom_erc20_approval_race.py new file mode 100644 index 0000000..1682184 --- /dev/null +++ b/slither/detectors/custom_erc20_approval_race.py @@ -0,0 +1,103 @@ +"""Custom Slither detector for ERC-20 approval race hazards. + +Detects token-like contracts whose approve(address,uint256) implementation +directly overwrites allowance state without an obvious zero-first or +delta-based allowance guard. This flags the classic allowance front-running +pattern where a spender can consume the old allowance before a lower approval +lands, then consume the new allowance as well. +""" +try: + from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification +except ModuleNotFoundError: + class DetectorClassification: + MEDIUM = "medium" + + class AbstractDetector: + pass + + +ALLOWANCE_NAMES = ("allowance", "allowances", "_allowances") +SAFE_GUARD_TERMS = ( + "increaseallowance", + "decreaseallowance", + "safeincreaseallowance", + "safedecreaseallowance", +) + + +def _name_of(item) -> str: + return str(getattr(item, "name", item)).lower() + + +def is_approve_function(function) -> bool: + if _name_of(function) != "approve": + return False + parameters = list(getattr(function, "parameters", []) or []) + return len(parameters) == 2 + + +def function_writes_allowance(function) -> bool: + for node in getattr(function, "nodes", []) or []: + for ir in getattr(node, "irs", []) or []: + writes = getattr(ir, "writes", None) or [] + for write in writes: + variable = write[0] if isinstance(write, tuple) and write else write + if _name_of(variable) in ALLOWANCE_NAMES: + return True + text = str(getattr(function, "source_mapping", "")) + " " + str(getattr(function, "nodes", "")) + lowered = text.lower() + return any(name in lowered for name in ALLOWANCE_NAMES) + + +def has_allowance_race_guard(function) -> bool: + text = " ".join(str(item).lower() for item in getattr(function, "nodes", []) or []) + if any(term in text for term in SAFE_GUARD_TERMS): + return True + zero_guard_patterns = ( + "amount == 0", + "amount==0", + "value == 0", + "value==0", + "current == 0", + "current==0", + "allowance[msg.sender][spender] == 0", + "_allowances[msg.sender][spender] == 0", + ) + return any(pattern in text for pattern in zero_guard_patterns) + + +def find_approval_race_functions(contract) -> list: + if getattr(contract, "is_interface", False): + return [] + + findings = [] + for function in getattr(contract, "functions", []) or []: + if not getattr(function, "is_implemented", True): + continue + if is_approve_function(function) and function_writes_allowance(function) and not has_allowance_race_guard(function): + findings.append(function) + return findings + + +class CustomERC20ApprovalRaceDetector(AbstractDetector): + ARGUMENT = "custom-erc20-approval-race" + HELP = "Detects ERC-20 approve implementations vulnerable to allowance race conditions" + IMPACT = DetectorClassification.MEDIUM + CONFIDENCE = DetectorClassification.MEDIUM + + WIKI = "https://github.com/kcolbchain/audit-checklist/wiki/ERC20-Approval-Race" + WIKI_TITLE = "Custom ERC20 Approval Race Detector" + + def _detect(self) -> list: + results = [] + for contract in self.compilation_unit.contracts: + for function in find_approval_race_functions(contract): + info = [ + "ERC20 approve may overwrite allowance unsafely: ", + f"{function.name}() in {contract.name}\n", + "\nPrefer increase/decrease allowance flows or reject direct non-zero allowance changes.\n", + ] + res = self.generate_result(info) + res.add(function) + results.append(res) + return results diff --git a/slither/tests/test_custom_erc20_approval_race.py b/slither/tests/test_custom_erc20_approval_race.py new file mode 100644 index 0000000..a749479 --- /dev/null +++ b/slither/tests/test_custom_erc20_approval_race.py @@ -0,0 +1,76 @@ +import importlib.util +import pathlib +import types +import unittest + + +ROOT = pathlib.Path(__file__).resolve().parents[2] +DETECTOR_PATH = ROOT / "slither" / "detectors" / "custom_erc20_approval_race.py" + +spec = importlib.util.spec_from_file_location("custom_erc20_approval_race", DETECTOR_PATH) +custom_erc20_approval_race = importlib.util.module_from_spec(spec) +assert spec.loader is not None +spec.loader.exec_module(custom_erc20_approval_race) + + +def ir_writing(name): + return types.SimpleNamespace(writes=[types.SimpleNamespace(name=name)]) + + +def node(text, writes=None): + return types.SimpleNamespace(irs=writes or [], __str__=lambda self: text) + + +class TextNode: + def __init__(self, text, writes=None): + self.text = text + self.irs = writes or [] + + def __str__(self): + return self.text + + +def function(name, nodes): + return types.SimpleNamespace( + name=name, + parameters=[types.SimpleNamespace(name="spender"), types.SimpleNamespace(name="amount")], + nodes=nodes, + is_implemented=True, + ) + + +def contract(functions): + return types.SimpleNamespace( + name="Token", + functions=functions, + is_interface=False, + ) + + +class CustomERC20ApprovalRaceDetectorTest(unittest.TestCase): + def test_flags_approve_that_writes_allowance_without_guard(self): + approve = function("approve", [TextNode("allowance[msg.sender][spender] = amount", [ir_writing("allowance")])]) + + findings = custom_erc20_approval_race.find_approval_race_functions(contract([approve])) + + self.assertEqual(findings, [approve]) + + def test_ignores_approve_with_zero_guard(self): + approve = function( + "approve", + [ + TextNode("require(current == 0 || amount == 0)"), + TextNode("allowance[msg.sender][spender] = amount", [ir_writing("allowance")]), + ], + ) + + self.assertEqual(custom_erc20_approval_race.find_approval_race_functions(contract([approve])), []) + + def test_ignores_non_approve_function(self): + transfer = function("transferFrom", [TextNode("allowance[from][msg.sender] -= amount", [ir_writing("allowance")])]) + + self.assertEqual(custom_erc20_approval_race.find_approval_race_functions(contract([transfer])), []) + + +if __name__ == "__main__": + unittest.main() diff --git a/src/checks/ERC20ApprovalRaceCheck.sol b/src/checks/ERC20ApprovalRaceCheck.sol new file mode 100644 index 0000000..6780d24 --- /dev/null +++ b/src/checks/ERC20ApprovalRaceCheck.sol @@ -0,0 +1,87 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.24; + +import "../ChecklistBase.sol"; + +interface IERC20ApprovalRaceTarget { + function approve(address spender, uint256 amount) external returns (bool); + function transferFrom(address from, address to, uint256 amount) external returns (bool); + function allowance(address owner, address spender) external view returns (uint256); + function balanceOf(address account) external view returns (uint256); +} + +/// @title ERC20ApprovalRaceCheck - detect unsafe non-zero allowance overwrites +/// @notice Models the classic ERC-20 approve race: an owner tries to lower a +/// non-zero allowance, the spender uses the old allowance first, and +/// then receives the new allowance as well. Tokens should prefer +/// increase/decrease allowance flows or reject direct non-zero to +/// non-zero approval changes. +/// @author kcolbchain +abstract contract ERC20ApprovalRaceCheck is ChecklistBase { + function getApprovalOwner() internal view virtual returns (address) { + return address(0xA11CE); + } + + function getApprovalSpender() internal view virtual returns (address) { + return address(0xB0B); + } + + function getApprovalRecipient() internal view virtual returns (address) { + return address(0xCAFE); + } + + function getInitialApprovalAmount() internal view virtual returns (uint256) { + return 100 ether; + } + + function getReducedApprovalAmount() internal view virtual returns (uint256) { + return 50 ether; + } + + /// @dev Seed enough owner balance for the front-run old allowance spend + /// and the later reduced allowance spend. + function seedOwnerBalance(address owner, uint256 amount) internal virtual; + + function test_erc20_approval_change_cannot_be_double_spent() public { + IERC20ApprovalRaceTarget token = IERC20ApprovalRaceTarget(targetContract); + address owner = getApprovalOwner(); + address spender = getApprovalSpender(); + address recipient = getApprovalRecipient(); + uint256 initialAmount = getInitialApprovalAmount(); + uint256 reducedAmount = getReducedApprovalAmount(); + + require(initialAmount > reducedAmount, "initial must exceed reduced"); + seedOwnerBalance(owner, initialAmount + reducedAmount); + + vm.prank(owner); + bool initialApproved = token.approve(spender, initialAmount); + assertTrue(initialApproved, "initial approve failed"); + + vm.prank(spender); + bool oldAllowanceSpent = token.transferFrom(owner, recipient, initialAmount); + assertTrue(oldAllowanceSpent, "spender could not spend initial allowance"); + + vm.prank(owner); + try token.approve(spender, reducedAmount) returns (bool reducedApproved) { + if (!reducedApproved) { + return; + } + } catch { + return; + } + + uint256 recipientBefore = token.balanceOf(recipient); + vm.prank(spender); + try token.transferFrom(owner, recipient, reducedAmount) returns (bool spentReduced) { + if (spentReduced && token.balanceOf(recipient) > recipientBefore) { + emit log_named_uint( + "VULNERABILITY: spender extracted extra allowance after approval race", + reducedAmount + ); + assertLe(token.balanceOf(recipient), recipientBefore, "approval race allowed double spend"); + } + } catch { + return; + } + } +} diff --git a/src/examples/VulnerableERC20Approval.sol b/src/examples/VulnerableERC20Approval.sol new file mode 100644 index 0000000..ce00d66 --- /dev/null +++ b/src/examples/VulnerableERC20Approval.sol @@ -0,0 +1,67 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.24; + +/// @title VulnerableERC20Approval - ERC-20 fixture with unsafe approve overwrite +/// @notice DO NOT USE IN PRODUCTION. This token intentionally allows direct +/// non-zero to non-zero allowance changes, which demonstrates the +/// classic ERC-20 approval race. +/// @author kcolbchain +contract VulnerableERC20Approval { + string public name = "Vulnerable Approval Token"; + string public symbol = "VAT"; + uint8 public constant decimals = 18; + + mapping(address => uint256) public balanceOf; + mapping(address => mapping(address => uint256)) public allowance; + + function mint(address to, uint256 amount) external { + balanceOf[to] += amount; + } + + function approve(address spender, uint256 amount) public virtual returns (bool) { + allowance[msg.sender][spender] = amount; + return true; + } + + function transferFrom(address from, address to, uint256 amount) external returns (bool) { + uint256 allowed = allowance[from][msg.sender]; + require(allowed >= amount, "insufficient allowance"); + require(balanceOf[from] >= amount, "insufficient balance"); + + allowance[from][msg.sender] = allowed - amount; + balanceOf[from] -= amount; + balanceOf[to] += amount; + return true; + } +} + +/// @title SafeERC20Approval - fixture that supports delta-based allowance changes +/// @notice Rejects direct non-zero to non-zero approve changes and exposes +/// increase/decrease allowance helpers for safe adjustment. +contract SafeERC20Approval is VulnerableERC20Approval { + mapping(address => mapping(address => bool)) public hasApprovedSpender; + + function approve(address spender, uint256 amount) public override returns (bool) { + uint256 current = allowance[msg.sender][spender]; + require(!hasApprovedSpender[msg.sender][spender] || amount == 0, "use increase/decrease allowance"); + require(current == 0 || amount == 0, "use increase/decrease allowance"); + allowance[msg.sender][spender] = amount; + if (amount > 0) { + hasApprovedSpender[msg.sender][spender] = true; + } + return true; + } + + function increaseAllowance(address spender, uint256 addedValue) external returns (bool) { + hasApprovedSpender[msg.sender][spender] = true; + allowance[msg.sender][spender] += addedValue; + return true; + } + + function decreaseAllowance(address spender, uint256 subtractedValue) external returns (bool) { + uint256 current = allowance[msg.sender][spender]; + require(current >= subtractedValue, "decreased allowance below zero"); + allowance[msg.sender][spender] = current - subtractedValue; + return true; + } +} diff --git a/test/ERC20ApprovalRace.t.sol b/test/ERC20ApprovalRace.t.sol new file mode 100644 index 0000000..28930bf --- /dev/null +++ b/test/ERC20ApprovalRace.t.sol @@ -0,0 +1,71 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.24; + +import "forge-std/Test.sol"; +import "../src/checks/ERC20ApprovalRaceCheck.sol"; +import "../src/examples/VulnerableERC20Approval.sol"; + +contract ExampleERC20ApprovalRaceAudit is ERC20ApprovalRaceCheck { + VulnerableERC20Approval token; + + function setUp() public { + token = new VulnerableERC20Approval(); + targetContract = address(token); + } + + function seedOwnerBalance(address owner, uint256 amount) internal override { + token.mint(owner, amount); + } +} + +contract SafeERC20ApprovalRaceAudit is ERC20ApprovalRaceCheck { + SafeERC20Approval token; + + function setUp() public { + token = new SafeERC20Approval(); + targetContract = address(token); + } + + function seedOwnerBalance(address owner, uint256 amount) internal override { + token.mint(owner, amount); + } +} + +contract ERC20ApprovalRaceTest is Test { + function test_detects_direct_approve_race() public { + ExampleERC20ApprovalRaceAudit audit = new ExampleERC20ApprovalRaceAudit(); + audit.setUp(); + + vm.expectRevert(); + audit.test_erc20_approval_change_cannot_be_double_spent(); + } + + function test_safe_token_rejects_direct_nonzero_allowance_change() public { + SafeERC20ApprovalRaceAudit audit = new SafeERC20ApprovalRaceAudit(); + audit.setUp(); + + audit.test_erc20_approval_change_cannot_be_double_spent(); + } + + function test_decrease_allowance_prevents_front_run_second_spend() public { + SafeERC20Approval token = new SafeERC20Approval(); + address owner = address(0xA11CE); + address spender = address(0xB0B); + address recipient = address(0xCAFE); + + token.mint(owner, 150 ether); + + vm.prank(owner); + token.increaseAllowance(spender, 100 ether); + + vm.prank(spender); + token.transferFrom(owner, recipient, 100 ether); + + vm.prank(owner); + vm.expectRevert(bytes("decreased allowance below zero")); + token.decreaseAllowance(spender, 50 ether); + + assertEq(token.balanceOf(recipient), 100 ether); + assertEq(token.allowance(owner, spender), 0); + } +}