Smart Contract Audit — Found 3 Vulnerabilities Before Deployment

Me: @thom audit contracts/StakingPool.sol

Thom: 🔍 Starting security audit of StakingPool.sol...
  Contract: StakingPool (387 lines)
  Solidity version: ^0.8.19
  Imports: OpenZeppelin ReentrancyGuard, IERC20, Ownable

  Running analysis...

AUDIT_CHECKS = [
    "reentrancy",           # State changes after external calls
    "unchecked_returns",    # Ignored return values from calls
    "integer_overflow",     # Arithmetic without SafeMath (pre-0.8)
    "access_control",       # Missing modifiers, public functions
    "front_running",        # Transactions vulnerable to MEV
    "gas_optimization",     # Storage patterns, loop efficiency
    "logic_errors",         # Business logic flaws
    "denial_of_service",    # Unbounded loops, block gas limit
    "oracle_manipulation",  # Price feed dependencies
    "upgrade_safety"        # Proxy pattern issues
]

🔒 Security Audit: StakingPool.sol
  Findings: 3 vulnerabilities

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
🔴 CRITICAL — Reentrancy in withdraw()

  Location: Line 147-162

  Vulnerable code:

function withdraw(uint256 amount) external {
    require(stakes[msg.sender] >= amount, "Insufficient stake");

    // ❌ External call BEFORE state update
    (bool success, ) = msg.sender.call{value: amount}("");
    require(success, "Transfer failed");

    // State updated AFTER external call — reentrancy window
    stakes[msg.sender] -= amount;
    totalStaked -= amount;
}

  Exploit scenario:
  Attacker deploys contract with fallback function that
  re-calls withdraw(). On each re-entry, stakes[msg.sender]
  hasn't been decremented yet, so the require() passes again.
  Attacker drains the entire pool.

  Fix:

function withdraw(uint256 amount) external nonReentrant {
    require(stakes[msg.sender] >= amount, "Insufficient stake");

    // ✅ State update BEFORE external call
    stakes[msg.sender] -= amount;
    totalStaked -= amount;

    (bool success, ) = msg.sender.call{value: amount}("");
    require(success, "Transfer failed");
}

  Note: Contract imports ReentrancyGuard but doesn't use
  the nonReentrant modifier on this function.

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
🟠 HIGH — Unchecked return value in distributeRewards()

  Location: Line 203

  Vulnerable code:

function distributeRewards() external onlyOwner {
    for (uint i = 0; i < stakers.length; i++) {
        uint256 reward = calculateReward(stakers[i]);
        // ❌ Return value not checked — silent failure
        IERC20(rewardToken).transfer(stakers[i], reward);
    }
}

  Risk: If a transfer fails (blacklisted address, paused token),
  the loop continues silently. Some stakers get rewards, others
  don't, with no record of who was skipped.

  Fix:

function distributeRewards() external onlyOwner {
    for (uint i = 0; i < stakers.length; i++) {
        uint256 reward = calculateReward(stakers[i]);
        // ✅ Use SafeERC20 for checked transfers
        IERC20(rewardToken).safeTransfer(stakers[i], reward);
    }
}

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
🟡 MEDIUM — Gas optimization in getStakerCount()

  Location: Line 89

  Current:

// ❌ Reading from storage array length — costs ~2600 gas
function getStakerCount() public view returns (uint256) {
    return stakers.length;
}

  Optimized:

// ✅ Cache length in a state variable — costs ~200 gas
uint256 public stakerCount;

// Update stakerCount on stake/unstake
function stake() external payable {
    // ... existing logic
    stakerCount++;
}

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

⛽ Gas Report:
| Function          | Gas Cost | Notes                          |
|-------------------|----------|--------------------------------|
| stake()           | 47,230   | Normal — storage write         |
| withdraw()        | 32,450   | Could save ~2K with optimization|
| distributeRewards | 28,100 × N | ⚠️ Unbounded loop — DoS risk at scale |
| getStakerCount    | 2,601    | Optimize to ~200 with caching  |

Overall: Fix distributeRewards loop. If stakers[] exceeds
~500 entries, the function will hit block gas limit.
Consider batch processing or pull-based reward claims.