SBET Protocol Deep Audit Report

Description: The EIP-712 domain separator has a space before address — "uint256 chainId, address verifyingContract" has a spurious space after the comma that deviates from the canonical EIP-712 spec. Most wallets compute domains with "uint256 chainId,address verifyingContract" (no space). Frontends that follow the standard format will produce a different domain separator hash, causing all signature verifications to fail or require the frontend to replicate this exact typo.

Impact: Signatures from standard EIP-712 libraries will not match; interop risk if any signer uses canonical spacing.

Fix: Removed spurious spaces in EIP-712 domain type string.

Description: uint32(uint256(price)) silently truncates a Chainlink int256 answer (typically 8 or 18 decimals) to 32 bits (max ~4.29B). A Chainlink price feed with 8 decimals returning e.g. $50,000 = 5,000,000,000,000 would be truncated to garbage. There's also no price > 0 check (negative oracle answers are cast to uint).

Impact: Match finalized at an incorrect/garbage price; irreversible fund loss for all participants.

Fix: Added price ≤ 0 and ≥ MAX_PRICE checks before uint32 cast.

Description: The base virtual claim() at line 39 always emits graderFee: 0 and transfers the full payoff. Only _settlePosition() (used by _executeClaim) deducts grader fees. If the unoverridden claim() path is ever callable (dispatch ambiguity or future refactor), grader fees are bypassed entirely. Currently the override in SBET.sol routes to _executeClaim, but the dead code is a maintenance trap.

Impact: Grader fees could be bypassed; funds drained from the grader fee pool.

Fix: Rerouted claim() through _settlePosition for grader fee deduction.

Description: _executeFinalizeMatchNFTs sets matchFinalized[matchId] = true but never sets matchFinalPrice — P2P positions on the same matchId become unclaimable because _computePayoff uses matchFinalPrice[matchId] which is 0, paying out mulDiv(position, 0, MAX_PRICE) = 0 for longs. NFT bets and P2P bets share the same matchId namespace.

Impact: If a match has both NFT stakes and P2P positions, the NFT finalization path locks up all P2P funds permanently.

Fix: Set matchFinalPrice in NFT finalization for P2P settlement.

Description: _executeFinalizeMatchNFTsBatch always sets matchFinalized = true on every call — After the first batch call, subsequent calls will revert at if (matchFinalized[matchId]) revert MatchIsFinalized() on line 309. The batch design (process slices over multiple txs) is broken — only the first batch runs.

Impact: NFTs beyond the first batch are permanently locked in the contract.

Fix: Batch NFT finalize uses if (!matchFinalized) guard + outcome tamper check.

Description: _markNonceUsed(left.maker, left.nonce) is called inside the loop for each right order. After the first right order fills, the left order's nonce is used. On the second iteration, _unpackOrderSoft doesn't re-check (left was already unpacked), but position state has changed from the first fill, making subsequent tradeCore calls operate on stale position snapshots.

Impact: Incorrect position calculations; potential double-counting of the left order's fill.

Fix: Moved _markNonceUsed(left) out of per-right-order loop.

Description: The "last grader gets remainder" logic depends on claim order, not grader identity. If claimedSoFar == numReporters - 1, the current claimer gets the remainder regardless of whether they're the "last" reporter. Race conditions between graders determine who gets the rounding dust bonus (or a larger-than-expected share if the pool was partially drained by a reentrancy or accounting bug).

Impact: Unfair fee distribution; minor value leakage on rounding.

Fix: Equal shares for all graders, no claim-order privilege.

Description: joinPoolBet checks poolId == 0 but does not validate that the poolId references a valid existing pool before dereferencing storage.

Impact: Out-of-bounds storage access for non-existent pool IDs.

Fix: poolId bounds check before storage dereference.

Description: The check is if (finalPrice > MAX_PRICE), not >= MAX_PRICE. A finalPrice of 0 passes validation. A match finalized at price 0 gives longs a payoff of 0 and shorts their full position. This is exploitable if recoverFunds is called after matchCancelPrice is somehow set to 0. More importantly, the executeFinalization oracle path could yield 0 after truncation.

Impact: Match finalized at price 0 = total loss for all long positions. Combined with C2, this is especially dangerous.

Fix: _finalizeMatch rejects finalPrice == 0.

Description: When payoff < 0, the losing side must send tokens to the contract. This requires the user to have pre-approved the contract for that amount. If they haven't, the claim reverts. This means losers can grief winners by never calling claim — the winning side's tokens are locked because the contract needs the losing side's tokens to fund the payout. The system relies on losers cooperating.

Impact: Winners cannot claim if losers don't have sufficient balance/approval. Funds can be griefed.

Fix: Removed dead safeTransferFrom negative payoff branch.

Description: The EIP-712 domain separator uses a fixed salt. If the contract is deployed to multiple chains or as a proxy, the salt is identical. Combined with chain-specific chainId in the domain, this is likely fine, but the salt adds no entropy over what chainId + verifyingContract already provide.

Impact: Cosmetic; no direct exploit but misleading security assumption.

Status: Acknowledged — salt is redundant but harmless.

Description: if (block.timestamp >= emergencyEndTime) emergencyPaused = false; writes storage on every call after emergency expires. This costs extra gas on the first post-emergency call to any function using this modifier.

Impact: Unexpected gas cost; benign but surprising.

Fix: Explicit clearEmergency() replaces modifier side-effect.

Description: assert(amount >= 0 && price > 0) consumes all remaining gas on failure (unlike require/custom errors which refund). In Solidity 0.8.x, assert is meant for invariant violations only.

Impact: Wasted gas on edge-case failures that could be handled gracefully.

Fix: Custom errors replace assert() in SBETMath.

Description: The virtual trade() at line 86 contains the same logic as _executeTrade() at line 413. Both are complete implementations. The override in SBET.sol calls _executeTrade, making the base trade() unreachable dead code. Same for matchOrders, cancelAll, getAndIncrementNonce, cancelOrderNonce, cancelUpTo.

Impact: Code bloat; maintenance risk if one copy is updated but not the other.

Status: Acknowledged — dead virtual functions are the override pattern; removing them breaks the interface.

Description: Normal settlement only clears userOutcomeStake[msg.sender][p.outcome] but not userStake[msg.sender]. If a user bet on multiple outcomes, userStake retains the total. This allows getUserTotalStake to return stale data post-claim, and getUserActivePools to list pools where the user has already claimed.

Impact: Misleading view function results; no fund loss but confusing UX.

Fix: Zero userStake on normal pool claim.

Description: tradeSize = t.longAmount + t.shortAmount is computed in an unchecked block, then overflow is checked on the next line. This is intentional (check-after pattern), but the unchecked block unnecessarily exposes intermediate overflow. A single checked addition would be clearer and equally gas-efficient in 0.8.x.

Impact: Style issue; the subsequent check catches it, but the pattern is fragile.

Fix: Checked arithmetic replaces unchecked + manual check.

Description: The block if (matchRecoveryDeadline[matchId] == 0) { // No explicit recovery needed } does nothing.

Impact: Dead code.

Fix: Removed dead if-block.

Description: _applyTradeAll allows o.token == address(0) to route to ETH path — But _adjustETH reverts on negative deltas (NegativeETHDeltaNotSupported), making the ETH path broken for any trade where the short side owes. The trade() function validates token != address(0) but matchOrders also validates this. However, the ETH path logic is dead/broken code.

Impact: Broken ETH-native trading path; no exploit if token == address(0) is always rejected upstream.

Status: Acknowledged — ETH path is dead code guarded by upstream token != address(0) checks.

Description: Pool void condition only triggers when outcomeStaked[winningOutcome] == 0. If all outcomes have 0 staked but totalStaked is also 0, it doesn't void (no-op). Fine logically, but worth noting.

Impact: No-op for empty pools.

Status: Acknowledged — correct behavior.

Description: (totalStaked * userStake) / outcomeTotal uses basic division instead of Math.mulDiv, risking overflow on large pools. The main contract uses Math.mulDiv correctly.

Impact: Potential view-function revert on large values.

Fix: Math.mulDiv in SBETQuery pool simulation.

Description: For short positions, the formula produces a negative value but uses uint256(-position) * (MAX_PRICE - currentPrice) / MAX_PRICE which could overflow if currentPrice > MAX_PRICE (no bounds check on the input currentPrice).

Impact: View function revert on out-of-range input.

Fix: currentPrice bounds check + zero-position early return.

Description: The receive() fallback had no pause or lock guard. During an emergency pause or migration lock, ETH could still be deposited into the treasury. If the contract is mid-migration, these deposits would be stranded in the old contract after migration completes.

Impact: ETH permanently lost in a migrated or paused contract.

Fix: Added notPaused modifier and if (locked) revert ContractIsLocked() check to receive().

Description: batchDeposit uses notPaused and nonReentrant but does not check the locked flag. During migration, ERC-20 tokens can still be batch-deposited. These deposits are stranded after executeMigration sweeps the contract.

Impact: Tokens permanently locked in a migrated contract.

Fix: Added if (locked) revert ContractIsLocked() at the start of batchDeposit.

Description: The remaining withdrawal calculation dailyWithdrawalLimit - used underflows if used > dailyWithdrawalLimit (possible when the limit is lowered after withdrawals have already been made in the current period). The same issue exists for the global daily withdrawal limit.

Impact: Revert on all withdrawal attempts until the daily period resets; temporary DoS.

Fix: Safe subtraction: uint256 remaining = used >= dailyWithdrawalLimit ? 0 : dailyWithdrawalLimit - used.

Description: The batchDeposit loop does not reject amounts[i] == 0. Zero-amount deposits generate misleading DepositERC20 events and waste gas while providing no value. Combined with off-chain accounting that trusts deposit events, this can inflate reported metrics.

Impact: Event spam; misleading on-chain accounting; wasted gas.

Fix: Added if (amounts[i] == 0) revert DepositAmountMustBeGreaterThanZero() in the loop.

Description: The payout address verification only applied to EOAs (payout.code.length == 0), meaning any contract address bypassed the verifiedPayoutAddresses check. An attacker could deploy a malicious contract and receive integrator payouts without being verified.

Impact: Unauthorized fund extraction to unverified contract addresses.

Fix: Removed the EOA-only gate — all payout addresses must now be in verifiedPayoutAddresses.

Description: accrueFeeFor performs an ERC-20 safeTransferFrom but is not guarded by nonReentrant. If the token is a callback-enabled ERC-777 or similar, the caller could reenter during the transfer and manipulate fee accounting.

Impact: Reentrancy-based fee inflation if a callback-enabled token is used.

Fix: Added nonReentrant modifier to accrueFeeFor.

Description: emergencyWithdraw updates tokenMetrics[token] unconditionally, even for tokens not in the allowlist. This creates phantom metric entries for arbitrary token addresses, polluting the accounting data.

Impact: Corrupted token metrics; misleading accounting for non-allowlisted tokens.

Fix: Wrapped metrics update in if (_allowedTokens.contains(token)).

Description: All six module setter functions accept address(0) without validation. Setting a module manager to the zero address effectively disables the module and causes fundModule calls to that module to send funds to the burn address.

Impact: Accidental fund loss if a module is set to zero address and then funded.

Fix: Added if (_manager == address(0)) revert ZeroAddressNotAccepted() to all 6 setters.

Description: After pauseEndTime passes, the notPaused modifier stopped blocking but the emergencyPause flag remained true in storage. The emergencyPaused() view function returned true even after the pause expired, causing off-chain systems and UIs to incorrectly display the contract as paused.

Impact: Stale pause state misleads off-chain integrations and frontends.

Fix: notPaused modifier auto-clears expired pause; emergencyPaused() view checks both flag and timestamp.

Description: fundModule transfers tokens out of the treasury but does not update tokenMetrics[token].totalWithdrawn or lastUpdated. This creates a discrepancy between actual balances and tracked metrics, making the treasury appear to hold more funds than it actually does.

Impact: Inaccurate treasury accounting; metrics drift from reality over time.

Fix: Added tokenMetrics[token].totalWithdrawn += amount and lastUpdated = block.timestamp in fundModule.

Description: The fee split calculation returns (toIntegrator, toTreasury) but the event emitted the second return value from a discarded variable. The actual treasury fee amount was not captured, causing the event to log an incorrect value.

Impact: Misleading event data for off-chain analytics and accounting.

Fix: Captured toTreasury return value and used it in the event emission.

Description: The per-token setDailyWithdrawalLimit emits a WithdrawalLimitUpdated event, but the global variant does not. Changes to the global limit are invisible to off-chain monitoring systems.

Impact: Silent configuration change; no audit trail for global limit updates.

Fix: Added WithdrawalLimitUpdated event emission to setGlobalDailyWithdrawalLimit.

Description: batchDeposit only emitted a single BatchDeposit event with arrays. Individual DepositERC20 events were not emitted per token, making it impossible for indexers that filter on single-deposit events to track batch deposits.

Impact: Incomplete event coverage for off-chain indexing.

Fix: Added per-token DepositETH/DepositERC20 events inside the loop.

Description: lockSetTimestamp was declared at line 589 (inside the migration section) instead of with other storage variables. The MigrationLockInterrupted error was similarly declared far from the error section. Scattered declarations make the contract harder to audit and increase the risk of missed state variables.

Impact: Readability and auditability concern; no direct exploit.

Fix: Moved lockSetTimestamp to the storage section and MigrationLockInterrupted to the errors section.

Description: The migration functions use if (!hasRole(ADMIN_ROLE, msg.sender)) revert AccountNotAdmin(msg.sender) instead of the standard OpenZeppelin onlyRole(ADMIN_ROLE) modifier. This creates an inconsistency with the rest of the contract's access control pattern and uses a custom error instead of the standard AccessControlUnauthorizedAccount.

Impact: Style inconsistency; no security impact.

Status: Acknowledged — the onlyRole modifier does not work correctly through the TreasuryFacade inheritance chain (which overrides hasRole for timed roles). The manual pattern is kept for compatibility.

Description: When an Add/Update/Remove operation is queued, validation (recipient cap, total BPS, duplicate check, index bounds) runs at queue time only. By the time the timelock expires and the operation is executed, state may have changed: other ops may have been executed, recipients added/removed, or recipientsTotalBps changed. The Add path doesn't re-check MAX_FEE_RECIPIENTS, BPS cap, or duplicates. The Update and Remove paths don't re-check op.index bounds — if a swap-and-pop removal occurred during the timelock, the index may now point to a different recipient or be out of bounds.

Impact: BPS cap bypass (>100% total fees), array out-of-bounds revert or wrong-recipient modification, duplicate recipients.

Fix: All three operation paths now re-validate constraints at execution time: recipient cap + BPS limit + duplicate check for Add; index bounds + BPS cap for Update; index bounds for Remove.

Description: accumulateFees accepts amount == 0, generating a misleading FeesAccumulated event and (for ERC-20) executing a no-op safeTransferFrom. For ETH, msg.value == 0 also passes.

Impact: Event spam; inflated fee accounting optics.

Fix: Added if (amount == 0) revert BadFeeArgs() at function entry.

Description: Each recipient's share is computed as (available * percentage) / activeTotalBps. Due to integer division, the sum of all shares is less than available. The remainder stays in accumulatedFees but is never explicitly distributed. Over many distributions, dust accumulates and becomes permanently locked since minDistributionAmount can prevent small amounts from being distributed.

Impact: Permanent token lockup of rounding dust; minor value leakage over time.

Fix: Last active recipient receives the remainder (available - distributed) instead of a truncated share.

Description: The swap-and-pop removal moves feeRecipients[lastIndex] to op.index. Any queued operations referencing the old lastIndex position now point to a non-existent slot (popped), and the moved recipient is at a new index. Operations queued before the removal reference stale indices.

Impact: Stale indices in queued ops could modify or remove the wrong recipient.

Fix: Covered by FMH1 — execution-time re-validation of index bounds prevents out-of-bounds access and ensures the operation targets a valid entry.

Description: setRecipientActive toggles a recipient's active flag with immediate effect, only requiring FEE_MANAGER_ROLE. All other recipient modifications (add/update/remove) go through the timelock. Deactivating a recipient zeroes their share in the next distribution — functionally equivalent to removing them, but without the timelock delay.

Impact: Timelock bypass for effectively removing a recipient from distributions.

Fix: Elevated to FEE_ADMIN_ROLE (higher privilege than FEE_MANAGER_ROLE) to match the sensitivity of the operation.

Description: emergencyWithdraw transfers tokens out but does not reduce accumulatedFees[token]. After an emergency withdrawal, distributeFees will attempt to distribute fees that no longer exist in the contract, causing ERC-20 transfers to fail or sending ETH that belongs to other tokens.

Impact: Broken fee distribution after emergency withdrawal; potential fund cross-contamination.

Fix: emergencyWithdraw now reduces accumulatedFees[token] by the withdrawn amount (clamped to zero).

Description: The receive() fallback accepts any ETH sent to the contract but does not update accumulatedFees[ETH_ADDRESS]. This ETH is stranded — held by the contract but not tracked in any accounting variable. Only emergencyWithdraw can recover it.

Impact: ETH permanently untracked; only recoverable via emergency withdrawal.

Fix: receive() now updates accumulatedFees[ETH_ADDRESS] and emits FeesAccumulated.

Description: The duplicate check iterates all recipients including inactive ones. A deactivated recipient cannot be re-added at a new index with different BPS — the only way to change their percentage is via queueUpdateFeeRecipient.

Impact: UX friction; no security issue.

Status: Acknowledged — design choice to prevent duplicate addresses regardless of active status.

Description: Queued operations have an eta (earliest execution time) but no expiration. An operation queued today can be executed months later when state has changed significantly. Combined with stale-state risks, this amplifies the window for unintended execution.

Impact: Operations can be executed in unexpected future states.

Fix: Added OP_EXPIRY_WINDOW (14 days) — operations revert with OpExpired if not executed within 14 days of their ETA.

Description: The notPaused modifier makes external calls to ITreasury(treasury).emergencyPause() and ITreasury(treasury).pauseEndTime() on every guarded function call. If the treasury contract is compromised or self-destructs, all notPaused functions revert permanently (DoS). The treasury is immutable, so it cannot be updated.

Impact: Permanent DoS if treasury becomes unresponsive; dependency coupling risk.

Status: Acknowledged — immutable treasury reference is a deliberate design constraint. The FeeManager is tightly coupled to its treasury by design.

Description: createVesting increments totalVestedByToken and records a vesting schedule without checking whether the contract actually holds enough tokens to cover all outstanding vesting obligations. A VESTING_MANAGER can over-promise tokens, creating schedules whose releases will fail later when the contract runs dry.

Impact: Beneficiaries may be unable to release vested tokens; insolvency risk for the vesting contract.

Fix: Added a post-creation balance check: the contract’s token balance must be ≥ totalVestedByToken[token] - totalReleasedByToken[token]. Reverts with InsufficientVestingBalance if underfunded.

Description: revokeVesting performs up to two external token transfers (vested amount to beneficiary, unvested amount to treasury) but lacks the nonReentrant modifier. If either recipient is a malicious contract, it could re-enter during the transfer.

Impact: Potential reentrancy during revocation if beneficiary or treasury is a contract.

Fix: Added nonReentrant modifier to revokeVesting.

Description: The contract defines a vestingPaused flag and notVestingPaused modifier, but neither releaseVested nor batchRelease use it. The pauseVesting() / unpauseVesting() admin functions have no effect on actual releases.

Impact: Vesting pause mechanism is non-functional; admin cannot halt releases during emergencies.

Fix: Added notVestingPaused modifier to both releaseVested and batchRelease.

Description: depositForVesting accepts zero-amount ERC-20 deposits (wasting gas) and emits no event to confirm the deposit was received. Without an event, off-chain indexers cannot track vesting deposits.

Impact: Wasted gas on zero deposits; no off-chain audit trail for funding.

Fix: Added if (amount == 0) revert InvalidAmount(); check and a new VestingDeposit event emitted on successful deposit.

Description: emergencyWithdraw transfers tokens out but does not reduce totalVestedByToken. After withdrawal, the accounting claims more tokens are owed than the contract holds, causing future createVesting balance checks to fail and releases to revert.

Impact: Stale accounting after emergency withdrawal blocks new schedules and breaks existing release flows.

Fix: emergencyWithdraw now reduces totalVestedByToken by the withdrawn amount (capped at outstanding obligations).

Description: The receive() fallback accepts ETH but emits no event. Direct ETH transfers are invisible to off-chain indexers, making it impossible to track vesting funding via events alone.

Impact: Missing audit trail for direct ETH deposits.

Fix: receive() now emits VestingDeposit(ETH_ADDRESS, msg.sender, msg.value).

Description: The beneficiarySchedules[beneficiary] array grows without bound as new schedules are created. Functions like getActiveVestings and getTotalVestedForBeneficiary iterate the entire array. A beneficiary with many schedules could eventually hit gas limits.

Impact: Theoretical gas limit risk for beneficiaries with very many schedules.

Status: Acknowledged — in practice, vesting schedules are created by privileged roles (VESTING_MANAGER) for a limited set of beneficiaries. The iteration cost is acceptable for the expected usage pattern.

Description: rebalance moves capital between strategies but increments ts.totalDeposited and lifetimeDeposited[token] on the deposit side. Since the tokens were already counted when originally deposited, this double-counts them — inflating global and per-strategy accounting with every rebalance call.

Impact: Accounting corruption — totalDeposited grows unbounded with each rebalance, making withdrawals appear to have negative returns.

Fix: Removed totalDeposited and lifetimeDeposited increments from the rebalance deposit side, and removed the corresponding decrements from the withdraw side. Rebalance now moves capital without affecting deposit/withdrawal accounting.

Description: withdrawFromStrategy pulls tokens from the strategy into the TreasuryYield contract but never transfers them back to the treasury. Tokens accumulate in the yield contract rather than being returned to the treasury where they belong.

Impact: Tokens stranded in yield contract; treasury balance does not reflect actual holdings.

Fix: Added IERC20(token).safeTransfer(treasury, received) after successful withdrawal from the strategy.

Description: The rebalance function withdraws from one strategy and deposits into another, but only checks slippage on the deposit side. The withdrawal from the source strategy has no slippage protection, meaning the source strategy could return significantly fewer tokens than requested without detection.

Impact: Silent value loss if the source strategy returns fewer tokens during rebalance.

Fix: Added slippage check on the withdraw side: if (slip > MAX_SLIPPAGE_BPS) revert SlippageTooHigh().

Description: emergencyWithdrawFromStrategy updates accounting using the requested amount parameter rather than the actual tokens received from the strategy. If the strategy returns fewer tokens (due to slippage, fees, or partial withdrawal), the accounting will be overstated — believing more tokens were withdrawn than actually received.

Impact: Accounting desync — totalDeposited reduced by more than actually received, corrupting strategy metrics.

Fix: Measures actual received tokens via balance-before/after pattern and uses received for all accounting updates.

Description: removeStrategy uses swap-and-pop to remove a strategy from the tokenStrategies array. This changes the order of remaining strategies, which could affect any off-chain systems that rely on strategy array indices.

Impact: Off-chain index references become stale after removal.

Status: Acknowledged — swap-and-pop is O(1) and the standard Solidity pattern. On-chain logic uses strategy addresses (not indices), so ordering is irrelevant for contract correctness.

Description: setTreasury updates the treasury address but emits no event. Off-chain monitoring tools cannot detect when the treasury reference changes, reducing auditability.

Impact: Missing audit trail for treasury address changes.

Fix: Added TreasuryUpdated(oldTreasury, newTreasury) event emission.

Description: createBudget increments totalBudgetedByToken and records a budget without checking whether the contract actually holds enough tokens to cover all outstanding budget obligations. A BUDGET_MANAGER can create budgets far exceeding the contract’s balance, and spend() will revert later when it tries to transfer.

Impact: Budgets can be over-promised; spending will fail when the contract runs dry.

Fix: Added a post-creation balance check: the contract’s token balance must be ≥ totalBudgetedByToken[token] - totalSpentByToken[token]. Reverts with InsufficientBudgetBalance if underfunded.

Description: emergencyWithdraw transfers tokens out but does not reduce totalBudgetedByToken. After withdrawal, the accounting claims more tokens are budgeted than the contract holds, causing future createBudget balance checks to fail and spends to revert.

Impact: Stale accounting after emergency withdrawal blocks new budgets and breaks spending flows.

Fix: emergencyWithdraw now reduces totalBudgetedByToken by the withdrawn amount (capped at outstanding obligations).

Description: depositForBudget accepts zero-amount ERC-20 deposits (wasting gas) and emits no event to confirm the deposit was received. Without an event, off-chain indexers cannot track budget funding.

Impact: Wasted gas on zero deposits; no off-chain audit trail for funding.

Fix: Added if (amount == 0) revert InvalidAmount(); check and a new BudgetDeposit event emitted on successful deposit.

Description: The receive() fallback accepts ETH but emits no event. Direct ETH transfers are invisible to off-chain indexers, making it impossible to track budget funding via events alone.

Impact: Missing audit trail for direct ETH deposits.

Fix: receive() now emits BudgetDeposit(ETH_ADDRESS, msg.sender, msg.value).

Description: extendBudgetPeriod adds additionalTime to endTime without any upper bound check. An admin could extend a budget indefinitely, and additionalTime of 0 is also accepted (no-op with misleading event emission).

Impact: Unbounded extensions; zero-duration extension emits misleading event.

Fix: Added bounds check: additionalTime must be > 0 and ≤ MAX_BUDGET_DURATION.

Description: The BudgetUtilizationAlert event fires inside allocateToDepartment, which does not change budget.spent. The alert triggers based on spending level during allocation changes, which is misleading. It should fire in spend() where spending actually occurs.

Impact: Misleading utilization alerts during allocation, not during actual spending.

Fix: Moved the utilization alert from allocateToDepartment to spend().

Description: allBudgetIds is append-only. Deactivated budgets are never removed. getAllBudgetIds() will eventually hit gas limits if many budgets are created over time.

Impact: Theoretical gas limit risk for view functions iterating all budget IDs.

Status: Acknowledged — budgets are created by privileged roles (BUDGET_MANAGER) and are expected to be limited in number.

Description: Both getAllBudgets() and getAllBudgetIds() return the same allBudgetIds array. Duplicated dead code.

Impact: Code duplication and confusing API surface.

Fix: Removed getAllBudgets(); kept the more descriptive getAllBudgetIds().

Description: The validation newThreshold >= signerCount prevents setting the threshold equal to the number of signers (unanimity). With 3 signers, proposeChangeThreshold(3, ...) reverts. This should be newThreshold > signerCount.

Impact: Cannot configure unanimity requirement; limits governance flexibility.

Fix: Changed >= to > in the threshold validation, allowing threshold == signerCount.

Description: Between proposal creation and execution (up to 7-day expiry), state may have changed. AddSigner does not re-check that the signer doesn’t already exist or that signerCount < MAX_SIGNERS. RemoveSigner does not re-check that the signer still exists or that signerCount > MIN_SIGNERS. ChangeThreshold does not re-validate against current signerCount.

Impact: Stale proposals can corrupt signer tracking or set invalid thresholds.

Fix: Added execution-time re-validation for all three governance proposal types: duplicate/count checks for AddSigner, existence/minimum/threshold checks for RemoveSigner, and bounds validation for ChangeThreshold.

Description: revokeRole decrements signerCount whenever role == SIGNER_ROLE, even if the account did not actually have the role (the parent revokeRole is a no-op in that case). Repeated calls can underflow signerCount, corrupting signer tracking.

Impact: signerCount desync; potential underflow corruption.

Fix: Checks hasRole(SIGNER_ROLE, account) before the parent call; only decrements and removes from list if the account actually held the role.

Description: deposit() accepts tokens and receive() accepts ETH, but neither emits any event. Incoming funds are invisible to off-chain monitoring tools.

Impact: No audit trail for deposits into the multi-sig contract.

Fix: Both functions now emit a Deposit event. Added zero-amount check to deposit().

Description: emergencyWithdraw transfers tokens but does not update totalWithdrawn[token], breaking withdrawal metrics. Normal proposal-based transfers do update this metric.

Impact: totalWithdrawn understates actual withdrawals after emergency use.

Fix: Added totalWithdrawn[token] += amount; before the transfer.

Description: proposeRemoveSigner checks signerCount > MIN_SIGNERS but does not check whether removal would leave signerCount < requiredSignatures. With 3 signers and threshold 3, removing one leaves 2 signers but threshold stays 3 — making all future proposals unexecutable.

Impact: Governance deadlock if signer removal drops count below threshold.

Fix: Covered by MSH2 — execution-time re-validation now checks signerCount - 1 ≥ requiredSignatures before removing a signer.

Description: _removePendingProposal does a linear scan of the pendingProposals array (up to MAX_PENDING_PROPOSALS = 50). Gas cost grows linearly with the number of pending proposals.

Impact: Minor gas inefficiency; bounded by MAX_PENDING_PROPOSALS (50).

Status: Acknowledged — the array is bounded at 50 entries, keeping gas costs within acceptable limits. A mapping-based index would add complexity for minimal gain.

Description: distributeFees calculates each recipient’s share as (total * bps) / 10_000. Integer division truncates, and the per-recipient rounding errors accumulate. When the sum of all cuts is less than the total, the difference remains stuck in the contract with no mechanism to recover it.

Impact: Rounding dust (up to recipients.length - 1 wei per distribution) permanently locks in the contract, compounding over many distributions.

Fix: The last recipient now receives total - distributed (the remainder) instead of a truncated BPS calculation, eliminating dust accumulation.

Description: accumulateFees had no check for amount == 0. A caller could repeatedly invoke it with zero, emitting misleading FeesAccumulated events and inflating event counts without any actual fee transfer.

Impact: Misleading accounting events and wasted gas. Off-chain indexers counting events would report inflated fee activity.

Fix: Added if (amount == 0) revert InvalidAmount(); at the top of accumulateFees.

Description: emergencyWithdraw transfers tokens out but does not reduce accumulatedFeesByToken or _accrued. After withdrawal, these mappings claim more fees exist than the contract holds, causing future distributeFees calls to attempt transfers exceeding the actual balance.

Impact: Stale accounting after emergency withdrawal breaks fee distribution and creates phantom balances.

Fix: emergencyWithdraw now reduces both accumulatedFeesByToken and _accrued by the withdrawn amount (capped at tracked totals).

Description: The contract has two independent fee collection paths: collectNFTFee→withdrawFees (per-collection fees withdrawn by a recipient) and accumulateFees→distributeFees (accumulated fees split among BPS-weighted recipients). These paths use separate accounting mappings and do not interact, which could confuse integrators.

Impact: Potential confusion; no direct exploit since the paths track separate balances independently.

Status: Acknowledged — the dual paths serve distinct use cases (per-collection vs aggregated distribution) and are intentionally separate.

Description: The receive() function accepts ETH without emitting an event. Direct ETH transfers are invisible to off-chain indexers that rely on event logs.

Impact: ETH deposits via direct transfer are not tracked off-chain, making accounting and reconciliation difficult.

Fix: receive() now emits EthReceived(msg.sender, msg.value).

Description: setTreasury updates the treasury address without emitting an event. Treasury address changes are high-impact governance actions that should be auditable on-chain.

Impact: Treasury migrations are invisible to off-chain monitoring; malicious changes go undetected.

Fix: setTreasury now emits TreasuryUpdated(oldTreasury, newTreasury).

Description: distributeFees contained a balance pre-check comparing IERC20(token).balanceOf(address(this)) against the distribution total. This check is redundant since safeTransfer already reverts on insufficient balance, and the pre-check creates a false sense of security for non-standard tokens.

Impact: No functional impact; the misleading check adds gas and could mask the real failure point.

Fix: Removed the redundant balance pre-check. The safeTransfer call provides the authoritative revert on insufficient balance.

Description: The accumulatedFees state variable (a single uint256) is never written to internally — the contract uses accumulatedFeesByToken (a mapping) for actual accounting. The variable exists solely to satisfy the INFTFeeManager interface requirement.

Impact: The variable always returns 0, which could confuse callers expecting aggregate fee data.

Status: Acknowledged — removing the variable would break the INFTFeeManager interface. It is retained with a comment documenting the interface constraint.

Description: setNftVault accepted address(0) as a valid vault address, which would disable all NFT operations since every function checks nftVault != address(0).

Impact: An admin could accidentally brick all NFT deposit/withdrawal operations by setting the vault to the zero address.

Fix: Added if (vault == address(0)) revert ZeroAddress(); to setNftVault.

Description: The constructor accepted zero addresses for both _treasury and _nftVault parameters. Since treasury is immutable, a zero address would permanently break all treasury interactions with no way to fix it.

Impact: Deployment with zero addresses creates a permanently broken contract that must be redeployed.

Fix: Added if (_treasury == address(0)) revert ZeroAddress(); and if (_nftVault == address(0)) revert ZeroAddress(); to the constructor.

Description: batchDepositNFT and batchWithdrawNFT had no upper bound on array length. A caller could pass arrays large enough to exceed the block gas limit, causing the transaction to revert.

Impact: Denial-of-service risk via gas exhaustion on oversized batches.

Fix: Added MAX_BATCH_SIZE = 50 constant and if (nfts.length > MAX_BATCH_SIZE) revert BatchTooLarge(); to both batch functions.

Description: The receive() function used revert("No direct ETH") which costs more gas than a custom error and is inconsistent with the rest of the contract which uses custom errors throughout (e.g., UnexpectedETH).

Impact: Wastes gas on every accidental ETH send. The string-based revert is also inconsistent with the contract’s error pattern, making ABI parsing harder for off-chain tools.

Fix: Replaced revert("No direct ETH") with revert UnexpectedETH().

Description: batchConsume iterates over routers.length with no upper bound. A treasury caller could pass arrays large enough to exhaust the block gas limit.

Impact: Gas exhaustion DoS on batch consume operations.

Fix: Added MAX_BATCH_SIZE = 50 constant and if (routers.length > MAX_BATCH_SIZE) revert BatchTooLarge();.

Description: Every new token accrued for a router is pushed to routerTokens[router]. Even after consumption zeroes the balance, the token entry remains. Over time this array grows unbounded, and totalAccrued() must iterate the entire array including stale zero-balance entries.

Impact: Gas cost of totalAccrued() increases monotonically. Eventually the view function may exceed gas limits for routers that have traded many tokens.

Status: Acknowledged — the array is append-only by design and totalAccrued() already filters zero balances. Adding cleanup logic would increase accrue() gas cost.

Description: When an app is inactive/unpaid, _consumeAccrued still updated a.lastSweep to block.timestamp. If the app was later reactivated, the sweep timer reset from the last consume call rather than from reactivation, potentially skipping an overdue payout period.

Impact: Reactivated apps lose their accrued sweep eligibility; isDue() returns false immediately after reactivation even if the app was due before deactivation.

Fix: _consumeAccrued no longer updates lastSweep for inactive apps — only active apps get their sweep timer updated.

Description: minAccrualAmount was initialized to 1e6 at the storage declaration, then immediately overwritten to 0 in the constructor. The storage-level default was dead code and misleading.

Impact: No functional impact — the constructor value wins. But the contradictory initialization confuses auditors and future developers.

Fix: Removed the storage-level initialization (= 1e6), keeping only the constructor assignment as the single source of truth.

Description: When a router reactivated its app, lastSweep retained its value from before deactivation. If the app was inactive for a long time, isDue() would immediately return true even though no new fees had accrued during the inactive period, triggering premature consume cycles.

Impact: Premature sweep trigger on reactivation, creating unnecessary gas-wasting consume calls that return (0,0).

Fix: reactivateMyApp now resets lastSweep = uint64(block.timestamp).

Description: getBatchAccrued had no max batch size. While it’s a view function, extremely large arrays could still fail due to gas limits on eth_call.

Impact: Minor — view function only, but inconsistent with the bounded batchConsume.

Fix: Added the same MAX_BATCH_SIZE check for consistency.

Description: emergencyNFTTransfer transferred an NFT out of the vault but did not remove the tokenId from _holdings[nft]. After the transfer, getNFTsByContract(nft) still reported the token as held, and getNFTBalance for ERC-721 tokens returned 1 even though the vault no longer owned it.

Impact: Stale holdings data after emergency transfer. Any code relying on getNFTsByContract or getNFTBalance sees phantom tokens. Subsequent withdrawTo calls for the token would revert since the vault doesn’t own it.

Fix: emergencyNFTTransfer now removes the tokenId from _holdings[nft] after the transfer (for ERC-1155, only when balance reaches 0).

Description: The to parameter was not validated against address(0). For ERC-721, safeTransferFrom to address(0) reverts (OpenZeppelin checks), but for ERC-1155 the behavior depends on the implementation — some may allow it, burning the token permanently.

Impact: Accidental token burn if admin passes zero address for an ERC-1155 token.

Fix: Added if (to == address(0)) revert InvalidAddress();.

Description: An owner can set allowedToken[nft] = false while the vault still holds tokens from that NFT contract. Since withdrawTo checks allowedToken, those tokens become locked — only emergencyNFTTransfer (which requires locked = true) can recover them.

Impact: NFTs stranded in the vault if allowlist is updated carelessly. Emergency recovery is possible but requires locking the entire vault.

Status: Acknowledged — this is a policy decision. The emergency transfer path provides recovery. The risk is documented.

Description: Neither batchDepositFrom nor batchWithdrawTo had an upper bound on array length. Each iteration performs supportsInterface external calls (2 per item), which are expensive. Large arrays can exceed the block gas limit.

Impact: Gas exhaustion DoS on batch operations.

Fix: Added MAX_BATCH_SIZE = 50 constant and check to both batch functions.

Description: Each iteration in the batch loop calls supportsInterface twice (ERC-721 and ERC-1155 checks) via external calls. If the same NFT contract appears multiple times in the batch, the same interface checks are repeated, wasting gas.

Impact: Gas inefficiency when batching multiple tokens from the same contract. Minor since the onlyTreasury caller controls the input.

Status: Acknowledged — caching would add complexity for a gas optimization that benefits only repeated-contract batches.

Description: For ERC-721 tokens, getNFTBalance checks _holdings[nft].contains(tokenId) instead of calling IERC721(nft).ownerOf(tokenId). If _holdings gets out of sync (e.g., after emergencyNFTTransfer), this returns stale data.

Impact: Returns incorrect balance if holdings are stale. Resolved by NVH1 fix which ensures emergency transfers update _holdings.

Fix: Resolved by NVH1 — emergencyNFTTransfer now keeps _holdings in sync, preventing the desync that caused stale data.

Description: donateByCause loops through ALL organizations twice — once in _countVerifiedOrgsForCause and once in the main distribution loop. There was no upper bound on organizationCount. As organizations accumulate, gas cost grows linearly and eventually exceeds the block gas limit.

Impact: Permanent DoS — once enough organizations exist, donateByCause becomes uncallable.

Fix: Added MAX_ORGANIZATIONS = 500 constant enforced in addOrganization, bounding the iteration cost.

Description: fundsRaised is a single uint256 that sums donations across all tokens (USDC with 6 decimals, WETH with 18 decimals, native ETH). The total is meaningless when mixing different decimal scales.

Impact: Misleading accounting metric. Off-chain tools reading fundsRaised cannot derive meaningful totals.

Status: Acknowledged — the metric is informational and not used in any on-chain logic. A per-token tracking mapping would add storage cost for a cosmetic improvement.

Description: donateETH sends ETH via org.wallet.call{value: msg.value}(""). If the wallet is a contract with a malicious receive() function, it could revert to grief donors or consume excessive gas.

Impact: A malicious or buggy wallet contract can block ETH donations to that organization. The reentrancy guard mitigates re-entrancy, but griefing remains possible.

Status: Acknowledged — the wallet is set by a trusted admin role. Adding EOA-only checks would prevent legitimate multi-sig wallets from receiving donations.

Description: donate called IERC20(tokenAddress).safeTransferFrom(...) without validating that tokenAddress is non-zero or a legitimate contract. Passing address(0) results in unclear low-level revert messages.

Impact: Poor UX — unclear revert messages for invalid token addresses.

Fix: Added if (tokenAddress == address(0)) revert InvalidTokenAddress(); to all three donation functions.

Description: organizationCount++ was wrapped in unchecked. The gas savings are immaterial relative to the function’s multiple storage writes and string operations.

Impact: Negligible gas savings with reduced safety clarity.

Fix: Removed the unchecked wrapper.

Description: verifyOrganization checked for duplicate verification but not duplicate unverification. Calling verifyOrganization(id, false) on an already-unverified org emitted redundant events.

Impact: Redundant events cluttering off-chain logs.

Fix: Added symmetry check: if (!verified && !org.verified) revert OrganizationNotVerified();

Description: The pro-rata payout (shares * pool.collateral) / total truncates on each redemption. As totalWinningShares and pool.collateral decrease with each redeemer, rounding compounds. The last redeemer receives less than their fair share, and dust remains stranded in the pool.

Impact: Collateral dust permanently locked after all winners redeem. Winners collectively receive less than the full collateral.

Fix: When shares ≥ totalWinningShares (last redeemer), the payout is set to all remaining pool.collateral, eliminating dust.

Description: seedLiquidity checked that the market exists (mv.creator != address(0)) but did not verify mv.status == MarketStatus.Active. A pool could be seeded for a Resolved, Voided, or Disputed market, locking collateral with no way to trade.

Impact: Collateral locked in a pool for a non-active market. Provider must wait for resolution to call removeLiquidity.

Fix: Added if (mv.status != MarketStatus.Active) revert PoolNotActive(); after the existence check.

Description: The constructor accepted _predictionMarket without validating it’s a non-zero address. Since predictionMarket is immutable, a zero address permanently breaks all functions that call predictionMarket.getMarket().

Impact: Deployment with zero address creates a permanently broken contract.

Fix: Added if (_predictionMarket == address(0)) revert ZeroAddress();.

Description: sell computed payout via LMSR math and then did pool.collateral -= payout without checking that payout does not exceed the available collateral. An underflow would panic-revert with an unclear error.

Impact: Unclear panic error instead of a descriptive revert when LMSR payout exceeds collateral.

Fix: Added if (payout > pool.collateral) revert InsufficientLiquidity(); before the subtraction.

Description: After a market is Resolved, the provider could call removeLiquidity and drain ALL remaining collateral, including collateral owed to winning share holders who hadn’t redeemed yet.

Impact: Winners who haven’t redeemed yet lose their payout. The provider gets collateral that should go to winning share holders.

Fix: Added check: if (winningSharesCounted[marketId] && totalWinningShares[marketId] > 0) revert WinningSharesPending();

Description: redeem accepted both MarketStatus.Resolved and MarketStatus.ResolutionPending. During ResolutionPending, the winning outcome may not be finalized — it could change if a dispute is filed.

Impact: Premature redemption on potentially incorrect outcome. If the outcome changes via dispute, the redeemer keeps tokens they shouldn’t have.

Fix: Changed to only accept MarketStatus.Resolved.

Description: When the pool is not active, getOutcomePrices returns uniform prices as MAX_PRICE / n. For non-divisible outcome counts (e.g., n=3), prices don’t sum to MAX_PRICE.

Impact: Cosmetic — prices don’t sum to MAX_PRICE for non-divisible outcome counts.

Fix: Added rounding remainder to the last outcome, matching the pattern used in _lmsrPrices.

Description: ecrecover returns address(0) for malformed signatures. In the M-of-N dispute council verification path, if any disputeCouncilMembers[j] were address(0), an attacker could forge “valid” signatures. The legacy single-council path had the same risk if disputeCouncil were ever set to address(0) (partially mitigated by the check at the top, but not in the recovery comparison itself).

Impact: Signature bypass — anyone could resolve disputes with forged signatures if any council member address is zero.

Fix: Added if (recovered == address(0)) continue; after each ecrecover call in both M-of-N and legacy paths.

Description: The M-of-N dispute council verification iterated through signatures and incremented validCount each time a match was found, but did not track which council members had already been counted. A single council member’s signature submitted multiple times in the disputeCouncilSigs array would increment validCount each time, allowing a 1-of-N attack to pass an M-of-N quorum.

Impact: Complete bypass of multi-sig dispute council quorum — a single council member could unilaterally resolve disputes.

Fix: Added a usedMask bitmap that tracks which council member indices have been matched, preventing duplicate counting.

Description: require(ok, "ETH transfer failed") is inconsistent with the rest of the contract which uses custom errors. String reverts waste gas on deployment (stored in bytecode) and at revert time (ABI-encoded).

Impact: Gas waste and inconsistent error handling pattern.

Fix: Replaced with if (!ok) revert EthTransferFailed();.

Description: Both bond distribution paths (disputer refund and feeRecipient transfer) used require(ok, "ETH transfer failed") instead of custom errors.

Impact: Same as PMH1 — gas waste and inconsistency.

Fix: Both paths now use if (!ok) revert EthTransferFailed();.

Description: No zero-address check on setAMM. Setting AMM to address(0) silently disables AMM functionality, inconsistent with setFeeRecipient and setDisputeCouncil which both validate non-zero.

Impact: Silent AMM disablement; markets created after would have no market maker.

Fix: Added if (_amm == address(0)) revert ZeroAddress();.

Description: The members array was not validated for zero addresses or duplicates. A zero-address member combined with PMC1 would allow anyone to forge valid signatures. Duplicate members reduce effective quorum (e.g., 3-of-5 with 2 duplicates becomes effectively 3-of-4).

Impact: Weakened dispute council security through zero-address or duplicate member acceptance.

Fix: Added validation loop: each member is checked for address(0) and uniqueness against all preceding members.

Description: params.token is stored without validation. If address(0), markets created without initial liquidity would have a broken settlement token, and any later AMM seeding or trading attempts would fail with confusing errors.

Impact: Markets created with invalid token address; downstream failures with poor error messages.

Fix: Added if (params.token == address(0)) revert ZeroAddress();.

Description: No minimum bound on setDisputeWindow. Setting to 0 makes finalizeResolution callable in the same block as resolution, effectively eliminating the dispute protection period.

Impact: Dispute protection bypassed if owner sets window to 0.

Status: Acknowledged — admin-only function; considered a design decision for operational flexibility.

Description: No minimum enforcement on creation fee or dispute bond. Setting either to 0 removes anti-spam protection (creation fee) or skin-in-the-game incentive (dispute bond).

Impact: Spam market creation or frivolous disputes without economic cost.

Status: Acknowledged — admin-only functions; zero values may be intentional during testing or promotional periods.

Description: If m.disputer is a contract that rejects ETH (no receive or fallback), the bond refund .call{value: bondToReturn} fails, causing resolveDispute to revert. This permanently blocks dispute resolution for the market.

Impact: Market permanently stuck in Disputed state if disputer cannot receive ETH.

Fix: Added fallback: if disputer transfer fails, bond is sent to feeRecipient instead. If both fail, reverts with EthTransferFailed.

Description: The bare receive() external payable {} allowed anyone to send ETH to the contract with no tracking. This ETH gets mixed with creation fees and dispute bonds, inflating withdrawFees payouts. The contract only needs to receive ETH through its payable functions (createMarket, disputeResolution).

Impact: Untracked ETH deposits inflate fee withdrawals.

Fix: Removed receive() — contract only accepts ETH through its payable functions.

Description: The limit parameter is user-controlled with no cap. A very large limit could cause out-of-gas on view calls.

Impact: View call failures for large categories.

Fix: Added if (limit > MAX_BATCH_SIZE) limit = MAX_BATCH_SIZE; with MAX_BATCH_SIZE = 100.

Description: Returns the entire creatorMarkets[creator] array with no pagination. A prolific creator could cause view call failures.

Impact: View call failures for prolific creators.

Fix: Added offset/limit pagination matching getMarketsByCategory pattern, with MAX_BATCH_SIZE cap.

Description: These lifecycle functions did not check pause state. If the contract is paused (emergency), resolution and disputes could still proceed, undermining the pause mechanism.

Impact: Emergency pause does not fully halt protocol operations.

Fix: Added whenNotPaused modifier to all three functions.

Description: The EIP-712 domain type string contained spurious spaces after commas ("uint256 chainId, address verifyingContract, bytes32 salt"), deviating from the canonical EIP-712 specification. Standard libraries produce the hash without spaces, causing signature verification failures for standard-compliant signers.

Impact: Frontends using canonical EIP-712 spacing would produce mismatched domain hashes, breaking all signature verification.

Fix: Removed spurious spaces: "uint256 chainId,address verifyingContract,bytes32 salt".

Description: uint32(uint256(price)) silently truncated Chainlink int256 answers (8–18 decimals) to 32 bits. No checks for price ≤ 0 or price ≥ MAX_PRICE existed before the cast.

Impact: Match finalized at incorrect/garbage price; irreversible fund loss.

Fix: Added if (price ≤ 0) revert BadFinalPrice() and if (uint256(price) ≥ MAX_PRICE) revert BadFinalPrice() before the uint32 cast.

Description: The base virtual claim() computed payoff and transferred directly without routing through _settlePosition(), the only path that deducts grader fees. Also contained an unreachable safeTransferFrom branch for negative payoffs.

Impact: Users could bypass grader fees entirely; dead code implied losers needed to cooperate for winners to claim.

Fix: Rerouted claim() and batchClaimPositions() through _settlePosition(). Removed dead negative-payoff branch.

Description: _executeFinalizeMatchNFTs set matchFinalized[matchId] = true but never set matchFinalPrice. P2P positions on the same matchId became unclaimable (_computePayoff returned 0 for longs when matchFinalPrice == 0).

Impact: All P2P longs on NFT-finalized matches lost their entire position value.

Fix: Added matchFinalPrice assignment based on outcome: SideA → MAX_PRICE - 1, SideB → 1, None → MAX_PRICE / 2.

Description: Every call set matchFinalized[matchId] = true unconditionally, then subsequent batch calls reverted on if (matchFinalized[matchId]) revert MatchIsFinalized(). Only the first batch ever ran; remaining NFTs were permanently locked.

Impact: Permanent NFT lockup for any match requiring multiple finalization batches.

Fix: First call sets matchFinalized and matchFinalPrice. Subsequent calls verify matchWinningSide[matchId] == outcome to prevent tampering, and skip the finalization step.

Description: _markNonceUsed(left.maker, left.nonce) was called inside the loop for each right order match. After the first right order filled, subsequent iterations operated on stale position snapshots with the nonce already marked.

Impact: Double-counting of left order fills; position accounting corruption.

Fix: Moved _markNonceUsed(left.maker, left.nonce) to _executeMatchOrders() after the loop, called once when totalLeftFilled > 0.

Description: The “last grader gets remainder” logic depended on claim order, not grader identity. Race conditions between graders determined who received the rounding dust bonus.

Impact: Non-deterministic fee allocation; unfair rounding advantage.

Fix: All graders now receive the same floor-divided share (pool / numReporters). Rounding dust stays in the pool.

Description: Pool storage p = pools[poolId] was dereferenced before the bounds check. For non-existent pools, p.numOutcomes was 0, so outcome ≥ p.numOutcomes fired InvalidOutcome instead of the correct InvalidPoolId.

Impact: Misleading error for invalid pool IDs; confusing UX and integration bugs.

Fix: Moved poolId bounds check before storage access in both functions.

Description: The check was if (finalPrice > MAX_PRICE), not ≥ MAX_PRICE. A finalPrice of 0 passed validation, giving longs a payoff of 0 and shorts their full position. Combined with C-02f, exploitable via oracle price truncation.

Impact: Forced zero payoff for all longs; complete loss of position value.

Fix: Changed to if (finalPrice == 0 || finalPrice ≥ MAX_PRICE) revert BadFinalPrice().

Description: _computePayoff always returns ≥ 0 (uses absolute magnitude). The else if (payoff < 0) branch calling safeTransferFrom was unreachable dead code that implied losers needed to cooperate for winners to claim.

Impact: Dead code creating false security assumptions about settlement flow.

Fix: Removed dead branch. Added comment explaining payoff is always non-negative.

Description: Fixed salt adds no entropy beyond chainId + verifyingContract.

Impact: Cosmetic — no direct exploit. The salt is part of the committed EIP-712 domain and changing it would break existing signatures.

Fix: Acknowledged — retained for backward compatibility.

Description: The modifier silently set emergencyPaused = false as a side effect when any user called a guarded function after emergencyEndTime. State changes in modifiers are surprising, and this created a race window where the owner couldn’t extend an emergency.

Impact: Emergency could be silently cleared by any user; owner loses ability to extend pause window.

Fix: Modifier now reverts for both active and expired-but-uncleared emergencies. Added explicit clearEmergency() owner-only function with EmergencyCleared event.

Description: assert() consumes all remaining gas on failure. effectiveBalance had the same issue.

Impact: Full gas consumption on revert; poor UX and wasted gas.

Fix: Replaced with revert InvalidPriceDivideArgs() and revert InvalidEffectiveBalancePrice().

Description: trade(), matchOrders(), cancelAll(), etc. have complete implementations in the base abstract contract, duplicating the _execute* internal variants. The SBET.sol overrides call _execute*, making the base implementations dead code.

Impact: Maintenance risk — dead code may drift from actual behavior.

Fix: Acknowledged — the virtual/override pattern is the intended inheritance design. Base functions serve as the interface contract.

Description: Normal settlement only cleared userOutcomeStake[msg.sender][p.outcome] but not userStake[msg.sender], causing getUserActivePools to return stale data.

Impact: Stale pool membership data; incorrect frontend state.

Fix: Added p.userStake[msg.sender] = 0 in both claim paths.

Description: tradeSize = t.longAmount + t.shortAmount was in an unchecked block with a manual overflow check after. The pattern was fragile and the comment was misleading.

Impact: Overflow risk in a critical path; unclear intent.

Fix: Replaced with checked arithmetic. Solidity 0.8.x handles overflow natively.

Description: if (matchRecoveryDeadline[matchId] == 0) { /* no-op */ } — empty block with only a comment.

Impact: Dead code; misleading intent.

Fix: Removed.

Description: _applyTradeAll routes to _applyETHBalances when o.token == address(0), but _adjustETH reverts on negative deltas. The ETH path is broken for any trade where the short side owes.

Impact: Dead code — upstream checks ensure the ETH path is never reached.

Fix: Acknowledged — guarded by upstream validation that validates token != address(0) and treasury.isAllowedToken(token).

Description: MAX_PRICE and MAX_POSITION_MAGNITUDE independently defined in both files. Library internal constants can’t be accessed cross-contract, so true deduplication is impossible.

Impact: Maintenance risk — constants may drift out of sync.

Fix: Added prominent sync comments in both files with IMPORTANT annotation.

Description: (totalStaked * userStake) / outcomeTotal risks overflow on large pool values. The main contract correctly uses Math.mulDiv.

Impact: Incorrect query results for large pools.

Fix: Replaced with Math.mulDiv(totalStaked, userStake, outcomeTotal) for 512-bit intermediate math.

Description: No validation on currentPrice input. If currentPrice > MAX_PRICE, the short path underflows (MAX_PRICE - uint256(currentPrice)). No early return for zero positions.

Impact: Potential underflow in view function; incorrect position valuation.

Fix: Added if (currentPrice == 0 || currentPrice ≥ MAX_PRICE_U32) revert InvalidPriceRange(currentPrice) and if (position == 0) return 0.