web3-bug-classes

# BUG CLASSES — DeFi Smart Contract Vulnerabilities 10 bug classes. Each one with root cause, vulnerable code, fix, grep patterns, and real paid examples. --- ## 1. ACCOUNTING STATE DESYNCHRONIZATION > #1 Critical bug class — 28% of all Criticals on Immunefi. > Real protocols: Yeet, Alchemix V3, Folks Finance, ResupplyFi, MetaPool ### What It Is Two state variables are supposed to stay in sync. One code path updates variable A but forgets variable B. Later code reads both and makes decisions based on the stale B. ``` Real Value = A - B If A is updated but B isn't → Real Value appears larger than it is → phantom value ``` ### Root Cause Pattern ```solidity // BEFORE (correct state): // aToken.balanceOf(this) = 1000 (principal + yield) // totalSupply = 1000 (only principal) // yield = 1000 - 1000 = 0 ✓ correct // Attacker triggers startUnstake: totalSupply -= amount; // decremented BEFORE transfer // totalSupply = 900 now // aToken.balanceOf still = 1000 // yield appears = 1000 - 900 = 100 (PHANTOM) // Now harvest(): yieldAmount = aToken.balanceOf(this) - totalSupply; // = 1000 - 900 = 100 (phantom yield — no real yield was earned) // Protocol harvests 100 of principal and distributes as "yield" ``` ### Variants **Variant 1: Phantom Yield** — totalSupply decremented before transfer ```solidity // Yeet protocol (35 duplicate reports): function startUnstake(uint256 amount) external { totalSupply -= amount; // decremented here, transfer happens later // balanceOf(this) - totalSupply now shows phantom yield } ``` **Variant 2: Fast Path Skips State Update** — early return bypasses critical updates ```solidity // Alchemix V3 claimRedemption: function claimRedemption(uint256 tokenId) external { if (transmuter.balance >= amount) { transmuter.transfer(user, amount); _burn(tokenId); return; // EARLY RETURN — cumulativeEarmarked, _redemptionWeight, totalDebt never updated } // SLOW PATH: updates all state vars correctly alchemist.redeem(...); } ``` **Variant 3: Rewards Accrue to Wrong Accumulator** ```solidity // Folks Finance Liquid Staking: function addRewards(uint256 amount) external { algoBalance += amount; // rewards go here // MISSING: TOTAL_ACTIVE_STAKE += amount } function withdraw(uint256 shares) external { uint256 myAmount = (shares * TOTAL_ACTIVE_STAKE) / totalSupply; // TOTAL_ACTIVE_STAKE never got rewards → underflow → freeze } ``` **Variant 4: Update Happens in Wrong Order** ```solidity // Alchemix: function deposit(uint256 amount) external { _shares = (amount * totalShares) / totalAssets; // calculated BEFORE deposit totalAssets += amount; // assets added AFTER shares calculated totalShares += _shares; // shares calculation used stale totalAssets → wrong rate } ``` ### Grep Patterns ```bash # List all balance/supply variables grep -rn "totalSupply\|totalShares\|totalAssets\|totalDebt\|totalCollateral\|cumulativeReward\|rewardPerShare" contracts/ | grep -v "//\|test" # Find ALL writes to key variables grep -rn "totalSupply\s*[-+*]=[^=]\|totalSupply\s*=" contracts/ grep -rn "cumulativeRewardPerShare\s*[-+*]=" contracts/ # Find all early returns in claim/redeem functions grep -rn "\breturn\b" contracts/ -B3 | grep -B3 "if\b" # For each early return: which state updates are in the normal path but not this one? ``` ### Kill Signals - Only one variable is involved (no pair to desync) - Both paths update all state vars identically - Transfer happens AFTER state update in every path (correct CEI) - Single-transaction atomicity prevents the window (no intermediate state visible) ### Real Paid Examples | Protocol | Root Cause | |----------|-----------| | Yeet | `startUnstake` decrements totalSupply before transfer → phantom yield | | Alchemix V3 | `claimRedemption` fast path skips 3 state updates → phantom collateral | | Folks Finance | Rewards accrue to `algoBalance` not `TOTAL_ACTIVE_STAKE` → underflow | | ResupplyFi | ERC4626 near-empty vault exchange rate manipulation | | MetaPool | `mint()` skipped receipt check from `_deposit()` | --- ## 2. ACCESS CONTROL > #2 Critical bug class — 19% of all Criticals. $953M lost in 2024 alone. > Real protocols: Wormhole ($10M), ZeroLend, Flare FAssets, Parity ($150M frozen) ### What It Is A function that should be restricted is callable by anyone. Or a function checks the wrong condition (existence vs. ownership). Or a modifier uses `if` instead of `require` and silently does nothing for non-admins. ### Root Cause Patterns **Variant 1: Missing Modifier on Sibling Function** ```solidity function vote(uint256 tokenId) external onlyNewEpoch(tokenId) { // guarded function reset(uint256 tokenId) external onlyNewEpoch(tokenId) { // guarded function poke(uint256 tokenId) external { // NO GUARD // Anyone calls poke() unlimited times per epoch // poke() distributes FLUX rewards → infinite inflation } ``` **Variant 2: Wrong Check — Existence vs. Ownership** ```solidity // ZeroLend split() — anyone can steal victim's tokens: function split(uint256 tokenId, uint256 amount) external { _requireOwned(tokenId); // checks if token EXISTS, not if caller OWNS it _burn(tokenId); _mint(msg.sender, amount); // attacker gets tokens they don't own } ``` **Variant 3: Tautology in Require** ```solidity // Flare FAssets — proof validation always passes: require( sourceAddressesRoot == sourceAddressesRoot, // always true! comparing to itself "Invalid" ); ``` **Variant 4: Silent Modifier (if vs require)** ```solidity // VULNERABLE — non-admin silently gets through: modifier onlyAdmin() { if (msg.sender == admin) { _; // only executes body for admin } // non-admin: modifier body skipped, function STILL EXECUTES } // CORRECT: modifier onlyAdmin() { require(msg.sender == admin, "Not admin"); _; } ``` **Variant 5: Uninitialized Proxy — initialize() Callable by Anyone** ```solidity contract Vault { address public owner; function initialize(address _owner) public { // MISSING: initializer modifier owner = _owner; // anyone can call this and become owner } } // Fix: constructor() { _disableInitializers(); } ``` ### Grep Patterns ```bash # Find sibling function families — do ALL have the same modifier set? grep -rn "function vote\|function poke\|function reset\|function update\|function claim\|function harvest" contracts/ -A2 # Ownership check pattern — existence vs ownership? grep -rn "_requireOwned\|ownerOf\|_isApprovedOrOwner\|_checkAuthorized" contracts/ -B5 -A5 # Silent modifiers using if without revert grep -rn "modifier\b" contracts/ -A8 | grep -B3 "if (" | grep -v "require\|revert\|else.*revert" # Uninitialized initializer grep -rn "function initialize\b" contracts/ -A3 grep -rn "_disableInitializers()" contracts/ # Missing access control on critical functions grep -rn "function mint\b\|function burn\b\|function emergencyWithdraw\b\|function upgradeTo\b" contracts/ -A3 ``` ### Roles Audit Checklist ``` For every privileged role: □ Who can GRANT this role? □ Who can REVOKE this role? □ Is the initial role granted in constructor to the correct address? □ Can the same address grant itself additional roles? □ Is there a timelock on role transfers? □ What happens if this role address is address(0)? □ Are all roles actually granted that are referenced in the code? ``` ### Kill Signals - Function has correct modifier AND modifier uses `require` (not silent `if`) - Upgrade functions have `onlyOwner` or role check in `_authorizeUpgrade` - `_disableInitializers()` is present in implementation constructor - All roles referenced in `onlyRole()` are actually granted in constructor or initializer ### Real Paid Examples | Protocol | Payout | Bug | |----------|--------|-----| | Wormhole | $10M | Uninitialized UUPS proxy → anyone calls initialize() | | ZeroLend | n/a | split() uses existence check not ownership check | | Alchemix | n/a | poke() missing onlyNewEpoch → infinite FLUX inflation | | Flare | n/a | Tautology in require → proof always passes | | Parity | $150M frozen | No access control on initWallet() in library | --- ## 3. INCOMPLETE CODE PATH > #3 Critical bug class — 17% of Criticals. > Real protocols: Plume, Puffer, ThunderNFT, Alchemix V3, MetaPool, LI.FI ### What It Is The happy path (deposit, create, place) handles tokens correctly. An alternate path (update, partial fill, fast path, zero amount) either moves tokens WITHOUT updating accounting, or updates accounting WITHOUT moving tokens, or deletes state regardless of whether the operation succeeded. ### Root Cause Patterns **Variant 1: Update Function Missing Refund** ```solidity // ThunderNFT — place_order takes tokens, update_order doesn't refund: function place_order(OrderInput calldata order) external { token.safeTransferFrom(msg.sender, address(this), order.price); // takes tokens orders[orderId] = order; } function update_order(OrderInput calldata updatedOrder) external { if (updatedOrder.price < existingOrder.price) { uint256 refund = existingOrder.price - updatedOrder.price; // BUG: NO REFUND for sell orders → tokens permanently stuck } orders[orderId] = updatedOrder; } ``` **Variant 2: Partial Fill — Token Stuck** ```solidity // Plume — refund handles ETH only, not ERC20: function swapForETH(uint256 amountIn) external { token.safeTransferFrom(msg.sender, address(this), amountIn); uint256 filled = dex.swap(amountIn); // partial fill possible _refundExcessEth(amountIn - filled); // BUG: refunds ETH only // If token is ERC20: remaining tokens NEVER refunded } ``` **Variant 3: Queue Entry Deleted on Failure** ```solidity // Puffer — delete happens before execution, in batch where one failure corrupts all: function executeTransaction(bytes32 txHash) external { Transaction memory tx = queue[txHash]; delete queue[txHash]; // deleted BEFORE execution (bool success,) = tx.target.call{value: tx.value}(tx.data); // In batch: failure of one element corrupted state for whole batch } ``` **Variant 4: safeApprove Without Cleanup** ```solidity // Plume — residual approval blocks second swap: function executeSwap(uint256 amount) external { token.safeApprove(router, amount); // approve full amount uint256 used = router.swap(amount); // partial fill: used < amount // remaining approval (amount - used) never cleared // Next call: safeApprove(router, newAmount) → REVERTS (current allowance != 0) } // Fix: token.safeApprove(router, 0); before approving ``` **Variant 5: mint() Skips Receipt Check That deposit() Has** ```solidity // MetaPool — mint() bypasses the check enforced by _deposit(): function deposit(uint256 assets, address receiver) public override returns (uint256 shares) { shares = _deposit(assets, receiver); // includes receipt validation } function mint(uint256 shares, address receiver) public override returns (uint256 assets) { assets = convertToAssets(shares); _mint(receiver, shares); // BUG: directly mints without _deposit() validation // _deposit() has: require(actualReceived >= expectedAmount, "Insufficient") // mint() skips this → mints without receiving actual assets } ``` ### The Function Family Comparison Test For every pair of functions that do similar things: ``` 1. List all state changes in function A (deposit/place/create) 2. List all state changes in function B (withdraw/update/cancel) 3. For each state change in A: does B have the corresponding reverse? 4. For each token transfer in A: does B have the corresponding refund? 5. For each event in A: does B emit a corresponding event? If A does X but B doesn't do the reverse of X → BUG. ``` ### Grep Patterns ```bash # Find create/place/add vs update/modify function pairs grep -rn "function place_\|function create_\|function add_\|function open_" contracts/ -A5 grep -rn "function update_\|function modify_\|function edit_\|function change_" contracts/ -A5 # Find refund logic — does it handle both ETH and ERC20? grep -rn "_refundExcess\|refundTokens\|refundAmount\|remainder" contracts/ -A10 # safeApprove without zero-reset before grep -rn "safeApprove\b" contracts/ # delete before operation completes grep -rn "delete\b" contracts/ -B5 -A5 # ERC4626: compare deposit() vs mint(), withdraw() vs redeem() grep -rn "function deposit\|function mint\|function withdraw\|function redeem" contracts/ -A10 ``` ### Kill Signals - update/cancel functions explicitly handle token transfers in all cases - Partial fills refund both ETH and ERC20 paths - `safeApprove(router, 0)` present before every `safeApprove(router, amount)` - `deposit()` and `mint()` both call the same internal `_deposit()` function ### Real Paid Examples | Protocol | Root Cause | |----------|-----------| | Plume | `_refundExcessEth` handles ETH only → ERC20 partial fill stuck | | Plume | `safeApprove` without cleanup → second swap reverts | | ThunderNFT | `update_order` missing refund for sell orders | | Puffer | `executeTransaction` deletes queue entry on failure | | LI.FI | $1.7M — library skips whitelist → arbitrary external call | | MetaPool | `mint()` bypasses receipt check that `deposit()` has | --- ## 4. OFF-BY-ONE & BOUNDARY CONDITIONS > #4 High bug class — 22% of Highs. Single character change. Massive impact. > Real protocols: VeChain Stargate, Alchemix, Flare, Shardeum ### What It Is At a boundary condition (period end, epoch transition, time == deadline), the wrong comparison operator routes to the wrong code branch. The "equal case" is the bug — `>` misses it, `>=` catches it. ### Root Cause Pattern ```solidity // VeChain Stargate — post-exit drain: function _claimableDelegationPeriods(address delegator) internal view returns (uint256) { uint256 endPeriod = userInfo[delegator].exitPeriod; // BUG: when block.period == endPeriod (exactly at exit), condition is FALSE if (endPeriod > nextClaimablePeriod) { return 0; // exited users get nothing — correct for this case } // WRONG: endPeriod == nextClaimablePeriod lands here return nextClaimablePeriod - lastClaimedPeriod; // → returns rewards for the period after exit → infinite post-exit drain // FIX: // if (endPeriod >= nextClaimablePeriod) { return 0; } } ``` ### The 6 Boundary Locations to Check **1. Period / Epoch Boundaries** ```bash grep -rn "period\|epoch\|round" contracts/ -i | grep "[<>][^=]" # Every > should be questioned: should it be >=? ``` **2. Time-Based Locks**