Harden contract, add ERC-20 recovery, and improve tooling

.github/workflows/ci.yml

@@ -20,8 +20,24 @@ jobs:
 
       - run: npm ci
 
+      - name: Lint Solidity
+        run: npm run lint
+
       - run: npx hardhat compile
 
       - run: npx hardhat test
 
       - run: npx hardhat coverage
+
+  static-analysis:
+    runs-on: ubuntu-latest
+
+    steps:
+      - uses: actions/checkout@v4
+
+      - name: Run Slither
+        uses: crytic/slither-action@v0.4.0
+        with:
+          node-version: 20
+          fail-on: high
+          slither-config: slither.config.json

.solhint.json

@@ -0,0 +1,12 @@
+{
+  "extends": "solhint:recommended",
+  "rules": {
+    "compiler-version": ["error", "0.8.20"],
+    "func-visibility": ["error", { "ignoreConstructors": true }],
+    "not-rely-on-time": "off",
+    "gas-custom-errors": "error",
+    "no-empty-blocks": "off",
+    "no-global-import": "off",
+    "no-complex-fallback": "off"
+  }
+}

.solhintignore

@@ -0,0 +1,3 @@
+node_modules/
+artifacts/
+cache/

README.md

@@ -7,8 +7,8 @@
 [![Solidity](https://img.shields.io/badge/Solidity-0.8.20-363636?logo=solidity)](https://soliditylang.org/)
 [![Hardhat](https://img.shields.io/badge/Built%20with-Hardhat-yellow)](https://hardhat.org/)
 [![OpenZeppelin](https://img.shields.io/badge/OpenZeppelin-v5.0.2-4E5EE4?logo=openzeppelin)](https://www.openzeppelin.com/contracts)
-[![License: ISC](https://img.shields.io/badge/License-ISC-blue.svg)](https://opensource.org/licenses/ISC)
-[![Tests](https://img.shields.io/badge/Tests-49%20passing-brightgreen)]()
+[![License: MIT](https://img.shields.io/badge/License-MIT-blue.svg)](https://opensource.org/licenses/MIT)
+[![Tests](https://img.shields.io/badge/Tests-54%20passing-brightgreen)]()
 [![Coverage](https://img.shields.io/badge/Coverage-100%25%20lines-brightgreen)]()
 
 </div>
@@ -50,7 +50,9 @@ Sender ──► [ FundForwarder Contract ]                      │
 | **Two-step Ownership (Ownable2Step)** | Ownership transfers require explicit acceptance, preventing irreversible mistakes |
 | **Pausable** | Owner can halt all deposits and flushes during emergencies or maintenance |
 | **Emergency Recovery** | Owner can withdraw any ETH stuck in the contract (works even when paused) |
+| **ERC-20 Recovery** | Owner can sweep any ERC-20 tokens accidentally sent to the contract |
 | **Reentrancy Protection** | Guards against reentrancy on deposit, flush, and recovery paths |
+| **Custom Errors** | Reverts use custom errors instead of strings for lower deploy and revert gas |
 
 ---
 
@@ -61,7 +63,8 @@ contracts/
 ├── FundForwarder.sol          # Main contract
 └── test/
     ├── ForceFeeder.sol        # Test helper — force-sends ETH
-    └── Reverter.sol           # Test helper — always-reverting receiver
+    ├── Reverter.sol           # Test helper — always-reverting receiver
+    └── MockERC20.sol          # Test helper — minimal ERC-20 token
 ```
 
 ### Contract Inheritance
@@ -86,6 +89,7 @@ Ownable ─► Ownable2Step ─┐
 | `updateMinDeposit()` | Owner | Update the minimum deposit threshold |
 | `pause()` / `unpause()` | Owner | Toggle deposit and flush acceptance |
 | `recoverFunds()` | Owner | Withdraw stuck ETH to the owner (works when paused) |
+| `recoverERC20()` | Owner | Sweep ERC-20 tokens accidentally sent to the contract |
 | `transferOwnership()` | Owner | Initiate two-step ownership transfer |
 | `acceptOwnership()` | Pending Owner | Accept ownership transfer |
 
@@ -176,7 +180,7 @@ The deploy script reads `TARGET_WALLET` from `.env` and automatically verifies t
 
 ## Test Suite
 
-49 tests across 11 categories with **100% line and function coverage**.
+54 tests across 12 categories with **100% line and function coverage**.
 
 ```
   FundForwarder
@@ -185,16 +189,25 @@ The deploy script reads `TARGET_WALLET` from `.env` and automatically verifies t
     Wallet Change Flow (9 tests)
     MinDeposit Updates (4 tests)
     Pause / Unpause (3 tests)
-    Recovery (3 tests)
+    Recovery (4 tests)
+    ERC-20 Recovery (4 tests)
     Ownable2Step (2 tests)
     Fallback (1 test)
     Deposit History (4 tests)
     Batch Mode (11 tests)
     Edge Cases (2 tests)
 
-  49 passing
+  54 passing
 ```
 
+### Linting
+
+```bash
+npm run lint
+```
+
+Solidity sources are linted with [solhint](https://protofire.github.io/solhint/) (`solhint:recommended`). Both linting and [Slither](https://github.com/crytic/slither) static analysis run automatically in CI.
+
 ---
 
 ## Security Considerations
@@ -209,12 +222,12 @@ The deploy script reads `TARGET_WALLET` from `.env` and automatically verifies t
 
 ### Known Limitations
 
-- The contract does not support ERC-20 token forwarding — ETH only.
+- The contract does not *forward* ERC-20 tokens — it handles ETH only. Tokens accidentally sent to the contract can, however, be swept by the owner via `recoverERC20()`.
 - `selfdestruct`-based force-sends can deposit ETH below `minDeposit` without triggering forwarding. The `recoverFunds()` function handles this case.
 - In batch mode, if the target wallet is changed before calling `flush()`, accumulated funds will be sent to the **new** target wallet, not the one active when deposits were made.
 
 ---
 
 ## License
 
-This project is licensed under the [ISC License](https://opensource.org/licenses/ISC).
+This project is licensed under the [MIT License](https://opensource.org/licenses/MIT) — see the [LICENSE](LICENSE) file for details.

contracts/FundForwarder.sol

@@ -4,6 +4,8 @@ pragma solidity 0.8.20;
 import "@openzeppelin/contracts/access/Ownable2Step.sol";
 import "@openzeppelin/contracts/utils/Pausable.sol";
 import "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
+import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
 
 /**
  * @title FundForwarder
@@ -17,8 +19,48 @@ import "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
  *      - ReentrancyGuard: Protects the deposit and recovery paths against reentrancy attacks.
  *      - Time-locked wallet changes: A mandatory 24-hour delay on target wallet updates
  *        gives stakeholders time to react to potentially malicious changes.
+ *      - Custom errors: Used throughout for lower deploy and revert gas costs.
  */
 contract FundForwarder is Ownable2Step, Pausable, ReentrancyGuard {
+    using SafeERC20 for IERC20;
+
+    // ──────────────────────────────────────────────
+    //  Custom Errors
+    // ──────────────────────────────────────────────
+
+    /// @notice Thrown when a zero address is supplied where a valid address is required.
+    error ZeroAddress();
+
+    /// @notice Thrown when a minimum deposit of zero is supplied.
+    error MinDepositZero();
+
+    /// @notice Thrown when a minimum deposit above `MAX_MIN_DEPOSIT` is supplied.
+    error MinDepositExceedsMax();
+
+    /// @notice Thrown when an incoming deposit is below the configured `minDeposit`.
+    error DepositBelowMinimum();
+
+    /// @notice Thrown when the contract is called with non-empty calldata.
+    error UnexpectedCalldata();
+
+    /// @notice Thrown when an ETH transfer (forward, flush, or recovery) fails.
+    error TransferFailed();
+
+    /// @notice Thrown when a wallet change is requested for the already-active target.
+    error WalletUnchanged();
+
+    /// @notice Thrown when finalizing or cancelling with no pending wallet change.
+    error NoPendingWalletChange();
+
+    /// @notice Thrown when finalizing a wallet change before the time lock expires.
+    error TimelockActive();
+
+    /// @notice Thrown when setting batch mode to its current value.
+    error BatchModeUnchanged();
+
+    /// @notice Thrown when a flush or recovery is attempted with no balance available.
+    error NoBalance();
+
     // ──────────────────────────────────────────────
     //  State Variables
     // ──────────────────────────────────────────────
@@ -67,7 +109,7 @@ contract FundForwarder is Ownable2Step, Pausable, ReentrancyGuard {
     event Deposit(address indexed sender, uint256 amount);
 
     /// @notice Emitted when ETH is successfully forwarded to the target wallet.
-    /// @param to The recipient address (target wallet or owner during recovery).
+    /// @param to The recipient address (the target wallet).
     /// @param amount The forwarded amount in wei.
     event Forwarded(address indexed to, uint256 amount);
 
@@ -100,6 +142,17 @@ contract FundForwarder is Ownable2Step, Pausable, ReentrancyGuard {
     /// @param amount The total amount flushed in wei.
     event Flushed(address indexed to, uint256 amount);
 
+    /// @notice Emitted when the owner recovers stuck ETH to their own address.
+    /// @param to The owner address that received the recovered ETH.
+    /// @param amount The recovered amount in wei.
+    event FundsRecovered(address indexed to, uint256 amount);
+
+    /// @notice Emitted when the owner recovers ERC-20 tokens sent to the contract.
+    /// @param token The ERC-20 token contract address.
+    /// @param to The owner address that received the recovered tokens.
+    /// @param amount The recovered token amount in the token's smallest unit.
+    event ERC20Recovered(address indexed token, address indexed to, uint256 amount);
+
     // ──────────────────────────────────────────────
     //  Constructor
     // ──────────────────────────────────────────────
@@ -113,9 +166,9 @@ contract FundForwarder is Ownable2Step, Pausable, ReentrancyGuard {
      *        Must be > 0 and <= MAX_MIN_DEPOSIT (10 ETH).
      */
     constructor(address _targetWallet, uint256 _minDeposit) Ownable(msg.sender) {
-        require(_targetWallet != address(0), "Target wallet cannot be zero address");
-        require(_minDeposit > 0, "Minimum deposit must be greater than 0");
-        require(_minDeposit <= MAX_MIN_DEPOSIT, "Minimum deposit exceeds maximum");
+        if (_targetWallet == address(0)) revert ZeroAddress();
+        if (_minDeposit == 0) revert MinDepositZero();
+        if (_minDeposit > MAX_MIN_DEPOSIT) revert MinDepositExceedsMax();
         targetWallet = _targetWallet;
         minDeposit = _minDeposit;
     }
@@ -133,7 +186,7 @@ contract FundForwarder is Ownable2Step, Pausable, ReentrancyGuard {
      *      to guard against reentrancy via the external call in `forwardFunds()`.
      */
     receive() external payable whenNotPaused nonReentrant {
-        require(msg.value >= minDeposit, "Deposit below minimum amount");
+        if (msg.value < minDeposit) revert DepositBelowMinimum();
 
         // Track per-depositor and global totals
         depositHistory[msg.sender] += msg.value;
@@ -152,7 +205,7 @@ contract FundForwarder is Ownable2Step, Pausable, ReentrancyGuard {
      * @dev Only plain ETH transfers (no data) are accepted via `receive()`.
      */
     fallback() external payable {
-        revert("Use direct ETH transfer");
+        revert UnexpectedCalldata();
     }
 
     // ──────────────────────────────────────────────
@@ -168,8 +221,12 @@ contract FundForwarder is Ownable2Step, Pausable, ReentrancyGuard {
         uint256 amount = address(this).balance;
         if (amount == 0) return;
 
+        // `targetWallet` is an owner-managed, validated, time-lock-protected address
+        // (see requestTargetWalletChange) — not a user-supplied destination. Forwarding
+        // the balance to it is the contract's core intended behavior.
+        // slither-disable-next-line arbitrary-send-eth
         (bool success, ) = targetWallet.call{value: amount}("");
-        require(success, "Forwarding failed");
+        if (!success) revert TransferFailed();
 
         emit Forwarded(targetWallet, amount);
     }
@@ -185,8 +242,8 @@ contract FundForwarder is Ownable2Step, Pausable, ReentrancyGuard {
      * @param _newWallet The proposed new target wallet address. Must be non-zero.
      */
     function requestTargetWalletChange(address _newWallet) external onlyOwner {
-        require(_newWallet != address(0), "New wallet cannot be zero address");
-        require(_newWallet != targetWallet, "New wallet is already the target");
+        if (_newWallet == address(0)) revert ZeroAddress();
+        if (_newWallet == targetWallet) revert WalletUnchanged();
         pendingTargetWallet = _newWallet;
         walletChangeUnlockTime = block.timestamp + WALLET_CHANGE_DELAY;
         emit TargetWalletChangeRequested(targetWallet, _newWallet, walletChangeUnlockTime);
@@ -198,8 +255,8 @@ contract FundForwarder is Ownable2Step, Pausable, ReentrancyGuard {
      *      Only callable by the owner.
      */
     function finalizeTargetWalletChange() external onlyOwner {
-        require(pendingTargetWallet != address(0), "No pending wallet change");
-        require(block.timestamp >= walletChangeUnlockTime, "Time lock not yet expired");
+        if (pendingTargetWallet == address(0)) revert NoPendingWalletChange();
+        if (block.timestamp < walletChangeUnlockTime) revert TimelockActive();
 
         address oldWallet = targetWallet;
         targetWallet = pendingTargetWallet;
@@ -217,7 +274,7 @@ contract FundForwarder is Ownable2Step, Pausable, ReentrancyGuard {
      *      or the request was made in error. Only callable by the owner.
      */
     function cancelTargetWalletChange() external onlyOwner {
-        require(pendingTargetWallet != address(0), "No pending wallet change");
+        if (pendingTargetWallet == address(0)) revert NoPendingWalletChange();
         address cancelled = pendingTargetWallet;
         pendingTargetWallet = address(0);
         walletChangeUnlockTime = 0;
@@ -235,8 +292,8 @@ contract FundForwarder is Ownable2Step, Pausable, ReentrancyGuard {
      * @param _newMinDeposit The new minimum deposit amount in wei. Must be > 0 and <= 10 ETH.
      */
     function updateMinDeposit(uint256 _newMinDeposit) external onlyOwner {
-        require(_newMinDeposit > 0, "Minimum deposit must be greater than 0");
-        require(_newMinDeposit <= MAX_MIN_DEPOSIT, "Minimum deposit exceeds maximum");
+        if (_newMinDeposit == 0) revert MinDepositZero();
+        if (_newMinDeposit > MAX_MIN_DEPOSIT) revert MinDepositExceedsMax();
         uint256 oldMinDeposit = minDeposit;
         minDeposit = _newMinDeposit;
         emit MinDepositUpdated(oldMinDeposit, _newMinDeposit);
@@ -254,7 +311,7 @@ contract FundForwarder is Ownable2Step, Pausable, ReentrancyGuard {
      * @param _enabled True to enable batch mode, false to resume instant forwarding.
      */
     function setBatchMode(bool _enabled) external onlyOwner {
-        require(batchMode != _enabled, "Batch mode already set");
+        if (batchMode == _enabled) revert BatchModeUnchanged();
         batchMode = _enabled;
         emit BatchModeUpdated(_enabled);
     }
@@ -269,10 +326,10 @@ contract FundForwarder is Ownable2Step, Pausable, ReentrancyGuard {
      */
     function flush() external onlyOwner whenNotPaused nonReentrant {
         uint256 amount = address(this).balance;
-        require(amount > 0, "No funds to flush");
+        if (amount == 0) revert NoBalance();
 
         (bool success, ) = targetWallet.call{value: amount}("");
-        require(success, "Flush failed");
+        if (!success) revert TransferFailed();
 
         emit Flushed(targetWallet, amount);
     }
@@ -312,11 +369,30 @@ contract FundForwarder is Ownable2Step, Pausable, ReentrancyGuard {
      */
     function recoverFunds() external onlyOwner nonReentrant {
         uint256 amount = address(this).balance;
-        require(amount > 0, "No funds to recover");
+        if (amount == 0) revert NoBalance();
 
         (bool success, ) = msg.sender.call{value: amount}("");
-        require(success, "Recovery failed");
+        if (!success) revert TransferFailed();
+
+        emit FundsRecovered(msg.sender, amount);
+    }
+
+    /**
+     * @notice Allows the owner to recover ERC-20 tokens accidentally sent to the contract.
+     * @dev The contract only handles ETH; any ERC-20 tokens transferred to it would
+     *      otherwise be permanently stuck. Tokens are sent to `msg.sender` (the owner).
+     *      Uses `SafeERC20` to support non-standard tokens that do not return a bool.
+     *      Protected by `nonReentrant` to guard against reentrancy via ERC-777-style hooks.
+     * @param token The ERC-20 token contract address to recover. Must be non-zero.
+     */
+    function recoverERC20(address token) external onlyOwner nonReentrant {
+        if (token == address(0)) revert ZeroAddress();
+
+        uint256 amount = IERC20(token).balanceOf(address(this));
+        if (amount == 0) revert NoBalance();
+
+        IERC20(token).safeTransfer(msg.sender, amount);
 
-        emit Forwarded(msg.sender, amount);
+        emit ERC20Recovered(token, msg.sender, amount);
     }
 }

contracts/test/MockERC20.sol

@@ -0,0 +1,16 @@
+// SPDX-License-Identifier: MIT
+pragma solidity 0.8.20;
+
+import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
+
+/**
+ * @title MockERC20
+ * @notice Minimal ERC-20 token used in tests to verify `FundForwarder.recoverERC20()`.
+ * @dev Mints the full initial supply to the deployer. Not intended for production use.
+ */
+contract MockERC20 is ERC20 {
+    /// @param initialSupply The token amount minted to the deployer at construction.
+    constructor(uint256 initialSupply) ERC20("Mock Token", "MOCK") {
+        _mint(msg.sender, initialSupply);
+    }
+}

contracts/test/Reverter.sol

@@ -8,8 +8,11 @@ pragma solidity 0.8.20;
  *      the contract correctly propagates the revert when `forwardFunds()` fails.
  */
 contract Reverter {
+    /// @notice Thrown unconditionally on any incoming ETH transfer.
+    error AlwaysReverts();
+
     /// @dev Always reverts to simulate an uncooperative recipient.
     receive() external payable {
-        revert("I always revert");
+        revert AlwaysReverts();
     }
 }