Re-Audit: Fix Verification

Field	Value
Protocol	MarkIt — Binary Prediction Market on Base
Auditor	Claude Code (Anthropic)
Date	2026-02-23
Commit	f718e58fdc718aaf2a53ba79a033912270a1d0a7
Compiler	Solidity 0.8.24 (via_ir = true, evm_version = paris)
Scope	Audit fix verification + new attack surface from architectural changes
Baseline	Prior audits 2026-02-21 (LP Fee Accumulator) and 2026-02-23 (Full Protocol)
Test Suite	257 tests, 0 failures, 0 skipped (5 suites)
Status	PASS WITH NOTES

---

1. Fix Verification Summary

Finding	Severity	Status	Verified	New Issues	Test Coverage
M-1: Withdrawal queue pays fulfillment-time NAV	Medium	FIXED	VERIFIED	None	test_audit_navSnapshotLocksInPrice, test_audit_navSnapshotStaleProof, test_audit_partialFulfillmentOnInsolvency, test_audit_partialFulfillmentFloatExactlyZero
M-2: emergencyRemoveMarket orphans capital	Medium	FIXED	VERIFIED	None	test_audit_recoverMarketLifecycle, test_audit_recoverMarketRevertsIfActive, test_audit_recoverMarketOnlyOwner, test_audit_recoverMarketRevertsAfterCollected
L-1: Fee underflow with extreme parameters	Low	FIXED	VERIFIED	None	test_audit_feeCapBoundary10000Succeeds, test_audit_feeCapBoundary10001Reverts
L-2: currentNAV() iterates all markets (gas)	Low	FIXED	VERIFIED	None	test_audit_totalDeployedTracksLifecycle
L-5: Dynamic LP fee has no hard cap	Low	FIXED	VERIFIED	None	Existing fee fuzz tests cover boundary
I-1: No two-step ownership transfer	Info	FIXED	VERIFIED	None	test_audit_ownable2StepRequiresAcceptance, test_audit_acceptFactoryOwnership
I-2: setEngine accepts address(0)	Info	FIXED	VERIFIED	None	test_audit_setEngineZeroReverts
I-3: sweepLosing accepts 0 shares	Info	FIXED	VERIFIED	None	test_audit_sweepLosingZeroReverts

All 8 fixes are present, correct, and tested. No new vulnerabilities introduced.

---

2. Fix Verification Details

M-1: NAV Snapshot at Request Time — VERIFIED

Fix location: LpVault.sol:263-266 (requestWithdrawal), LpVault.sol:319 (processQueue)

Implementation:

// requestWithdrawal() — line 264
uint256 navPerShare = totalShares > 0
    ? (currentNAV().toWad() * WAD) / totalShares
    : WAD;

The navPerShareAtRequest is computed BEFORE shares[msg.sender] and totalShares are decremented (line 269-270). This correctly captures the share price at request time.

At fulfillment time (line 319):

uint256 usdcOwed = req.shares.wadMul(req.navPerShareAtRequest).fromWad();

This uses the stored snapshot, not the current share price. Correct.

M-2: recoverMarket — VERIFIED

Fix location: LpVault.sol:529-550

The recoverMarket() function:

1. Reads amount from removedDeployments[market] (no admin-supplied value) — line 531

2. Guards against phantom NAV: checks if engine is Withdrawable with 0 vault LP shares — lines 537-539

3. Restores active tracking and deletes removedDeployments[market] — line 547

Correct. The phantom NAV guard prevents the most dangerous attack vector.

L-1: Fee Cap Validation — VERIFIED

Fix location: MarketEngine.sol:212

require(p.baseFeeBps + p.feeK + p.protocolFeeBps <= 10_000, "Fees exceed 100%");

Prevents construction with parameters that could cause underflow in _calculateFee. Correct.

L-2: O(1) NAV via totalDeployed — VERIFIED

Fix location: LpVault.sol:570-572

function currentNAV() public view returns (uint256) {
    return float + totalDeployed;
}

totalDeployed is maintained via increments/decrements in createAndFund (line 382), createAndFundBatch (line 410), _collect (line 487), emergencyRemoveMarket (line 518), and recoverMarket (line 546). All paths covered. Correct.

L-5: Dynamic LP Fee Hard Cap — VERIFIED

Fix location: MarketEngine.sol:418

if (effectiveFeeBps > baseFeeBps + feeK) effectiveFeeBps = baseFeeBps + feeK;

Caps the LP fee to the theoretical maximum. Prevents unbounded fee growth at extreme imbalance ratios. Correct.

I-1: Ownable2Step — VERIFIED

Both MarketEngine (line 16) and LpVault (line 49) and MarketFactory (line 12) now inherit Ownable2Step. The acceptFactoryOwnership() function (line 554) enables 2-step factory ownership transfer through the vault. Correct.

I-2: setEngine Rejects Zero — VERIFIED

Fix location: OutcomeToken.sol:41

if (_engine == address(0)) revert ZeroEngineAddress();

Correct.

I-3: sweepLosing Rejects Zero — VERIFIED

Fix location: MarketEngine.sol:673

if (shares == 0) revert ZeroAmount();

Correct.

---

3. M-1 Deep Dive: NAV Snapshot Architecture

3.1 Ordering Analysis

In requestWithdrawal():

1. Line 264: navPerShare computed using current totalShares and currentNAV()

2. Line 269: shares[msg.sender] -= sharesToRedeem

3. Line 270: totalShares -= sharesToRedeem

The snapshot is taken BEFORE share decrement. This matches sharePrice() (line 576-578) which uses the same formula: (currentNAV().toWad() * WAD) / totalShares. Ordering is correct.

3.2 totalShares Decrement Timing

Shares are decremented at request time, not fulfillment. This prevents ghost share dilution.

Path analysis — functions that read totalShares between request and fulfillment:

• sharePrice() (line 576): Returns (NAV.toWad() * WAD) / totalShares. After request, totalShares is smaller, but NAV is unchanged (no USDC left the vault yet). This means sharePrice for remaining LPs increases slightly — correct, as the queued shares are economically burned.

• deposit() (line 228-229): New depositor gets (usdcAmount.toWad() * totalShares) / nav.toWad(). With lower totalShares and same NAV, they get fewer shares per USDC — correct, as each remaining share represents more value.

• currentNAV() (line 570): Returns float + totalDeployed. Not affected by totalShares. Correct.

• userValue() (line 583-584): Returns (currentNAV() * shares[user]) / totalShares. After request, user's shares decreased and totalShares decreased proportionally. Correct.

No path produces incorrect results from the early decrement.

3.3 Partial Fulfillment Analysis

In _processQueue() (lines 317-337):

uint256 usdcOwed = req.shares.wadMul(req.navPerShareAtRequest).fromWad();
uint256 usdcPaid = usdcOwed > float ? float : usdcOwed;
req.fulfilled = true;
float -= usdcPaid;

(a) queueHead advances: Line 335 queueHead++ executes after fulfillment. Correct.

(b) float hits 0 not negative: If usdcOwed > float, then usdcPaid = float, and float -= float = 0. No underflow possible. If usdcOwed <= float, subtraction is safe. Correct.

(c) entry cannot be re-processed: req.fulfilled = true on line 326. The loop skips fulfilled entries on line 305-309. Even if processQueue() is called again, queueHead has advanced past this entry. Correct.

(d) next entry processes normally: After partial fulfillment, float may be 0. The next entry checks _allMarketsCollected() first — if that passes and float is 0, it gets usdcPaid = 0 and is marked fulfilled. This is the correct "partial at zero" behavior. The LP accepted snapshot risk. Correct.

3.4 Ghost Share Fix Verification

After requestWithdrawal(5000e18) by LP1 who had 10000e18 shares:

• totalShares drops from 10000e18 to 5000e18

• shares[lp1] drops from 10000e18 to 5000e18

• float unchanged, totalDeployed unchanged, so NAV unchanged

sharePrice() = NAV.toWad() * WAD / 5000e18 — exactly 2x the original price per remaining share.

LP2 depositing gets usdcAmount.toWad() * 5000e18 / NAV.toWad() — at the new price. No dilution from ghost shares. Verified by test_audit_depositDuringPendingWithdrawal.

3.5 Multiple Sequential Requests

If LP1 requests withdrawal, then LP2 requests withdrawal:

• LP2's navPerShareAtRequest is computed with post-LP1-decrement totalShares

• Since NAV is unchanged but totalShares is lower, LP2's snapshot price is higher

• This is correct: LP2's remaining shares are worth more per share since LP1's shares are economically burned

Verified by test_audit_navSnapshotLocksInPrice (lines 1517-1522) which confirms navPerShare2 >= navPerShare1.

3.6 removedDeployments Staleness Check

recoverMarket() deletes removedDeployments[market] on line 547. After recovery, the market is back in activeMarkets and isActiveMarket. If emergency-removed again, a fresh value is written to removedDeployments. If recovered again, it's deleted again. No stale value persists across cycles.

The phantom NAV guard (lines 537-539) prevents recovery after the underlying capital has already been withdrawn. Correct.

---

4. Invariant Status

Invariant 1: sum(lpShares[all users]) == totalLpShares (MarketEngine)

HOLDS. depositLp() increments both lpShares[msg.sender] and totalLpShares by the same amount. withdrawLp() decrements both by the same amount. No other function modifies either. Verified by all LP accounting tests and fuzz test test_fuzz_lpDepositShareAccounting.

Invariant 2: feePerShare monotonically non-decreasing

HOLDS. feePerShare is only modified in placeBet() (line 530): feePerShare += lpFee.toWad().wadDiv(totalLpShares). The addition of a non-negative quantity (fee is always >= 0, and totalLpShares > 0 when reached) ensures monotonic increase. No code path decrements it.

Invariant 3: No LP receives fees before their deposit

HOLDS. The Synthetix _accrueRewards() pattern (line 327-334) snapshots feePerShare at deposit time via userFeePerSharePaid[user] = feePerShare. Pending fees are computed as lpShares[user].wadMul(feePerShare - userFeePerSharePaid[user]). A new LP with userFeePerSharePaid[user] == feePerShare earns zero pre-existing fees. Verified by test_feeSniping_lateDepositorGetsZeroPreExistingFees.

Invariant 4: USDC.balanceOf(engine) >= winningLiabilities

HOLDS. The solvency check in placeBet() (lines 582-584) runs after every bet:

if (_maxLiability > _engineUsdc) revert SolvencyViolation(_maxLiability, _engineUsdc);

Winner redemptions decrease liability proportionally to USDC transferred out. LP withdrawals compute lpPool = balance - winningLiabUsdc and never touch reserved winner funds. Verified by 16 fuzz tests including test_fuzz_solvencyInvariant and test_fuzz_dynamicSkewCapSolvency.

Invariant 5 [NEW]: totalShares (vault) == sum of all shares[] mappings

HOLDS. deposit() increments both shares[msg.sender] and totalShares. requestWithdrawal() decrements both. No other function modifies either. Queued shares are economically burned at request time — they do NOT count toward totalShares. Verified by test_audit_depositDuringPendingWithdrawal and the _sumUserShares() helper in vault tests.

Invariant 6 [NEW]: totalDeployed == sum of deployments[market] for all activeMarkets

HOLDS. Every createAndFund and createAndFundBatch call increments both totalDeployed and deployments[market]. _collect() decrements both. emergencyRemoveMarket() decrements both (moves to removedDeployments). recoverMarket() increments both. The _assertInvariants() helper in LpVault.t.sol (line 144-148) verifies totalDeployed == _sumDeployments() throughout tests. Verified by test_audit_totalDeployedTracksLifecycle and test_invariant_navEqualsFloatPlusDeployments.

---

5. New Attack Surface Assessment

5.1 Front-running requestWithdrawal() to manipulate navPerShareAtRequest

SAFE.

navPerShareAtRequest is computed from currentNAV() = float + totalDeployed. An attacker cannot change totalDeployed (owner-only market operations). They could deposit USDC to increase float, which increases NAV and thus navPerShareAtRequest — but this also increases totalShares proportionally (deposit mints shares at current price), so navPerShareAtRequest is unchanged.

A large bet in an active market does not change float or totalDeployed — bets go directly to the MarketEngine, not the vault. The vault values deployed capital at cost until collect(), so bet activity cannot inflate NAV.

Conclusion: No manipulation vector exists.

5.2 Griefing partial fulfillment by draining float

SAFE (with documented caveat).

An attacker could drain float by front-running processQueue() with... nothing. Float can only decrease via createAndFund (owner-only) or withdrawal fulfillment itself. Non-owner users cannot reduce float.

The only scenario where float is near-zero at fulfillment time is when the owner deployed most capital. In that case, partial fulfillment pays whatever is available. Each LP gets paid (possibly partially) and the queue advances. This is by design — LPs accept snapshot risk.

Caveat: If the owner intentionally deploys all float just before processQueue, they could cause all pending withdrawals to receive $0. This is a centralization risk inherent in the owner-operated vault design, not a new vulnerability from the fixes.

5.3 recoverMarket() on already-collected market

SAFE.

The phantom NAV guard at LpVault.sol:537-539 explicitly checks:

if (mState == STATE_WITHDRAWABLE && IMarketEngine(market).lpShares(address(this)) == 0) {
    revert MarketAlreadyCollected(market);
}

This prevents recovering a market whose capital has already been extracted. Verified by test_audit_recoverMarketRevertsAfterCollected.

Edge case considered: What if the market is not yet Withdrawable but vault has 0 LP shares? This shouldn't happen — the vault LP deposits are only removed by withdrawLp() which requires Withdrawable state. The guard correctly covers the only dangerous scenario.

5.4 totalShares decrement with zero remaining shares

SAFE.

If all LPs request withdrawal, totalShares drops to 0. At this point:

• sharePrice() returns WAD (1e18) — the if (totalShares == 0) guard on line 577

• deposit() uses the if (totalShares == 0) branch for 1:1 minting — line 222-224

• currentNAV() still returns float + totalDeployed — independent of shares

• processQueue() uses stored navPerShareAtRequest, not current sharePrice — line 319

No division by zero. No incorrect calculation. Correct.

5.5 Two-step ownership intermediate state

SAFE.

Between transferOwnership(newOwner) and acceptOwnership():

• The current owner retains full control (owner() still returns old owner)

• pendingOwner() is set but has no privileges

• All onlyOwner functions still work for the current owner

• The pending owner cannot do anything until they call acceptOwnership()

For the vault's factory ownership (via emergencyTransferFactoryOwnership):

• The vault calls factory.transferOwnership(newOwner)

• The factory is in pending state — the vault is still the factory owner

• The vault can still create markets until the new owner calls factory.acceptOwnership()

No dangerous intermediate state exists.

---

6. Unresolved Findings — Status

L-3: withdrawalQueue array grows unboundedly — STILL ACCEPTABLE

The withdrawalQueue array only grows (push, never pop). queueHead advances to skip processed entries, but storage is never reclaimed.

Analysis: This is storage bloat, not a gas DoS. processQueue() iterates from queueHead and is bounded by MAX_QUEUE_ITERATIONS = 50. User-facing functions (deposit, requestWithdrawal) don't iterate the queue. The only gas cost is the withdrawalQueue.push() in requestWithdrawal(), which is O(1).

Severity unchanged. Still just a storage concern. No functional impact.

L-4: Markets created outside vault are disconnected — STILL ACCEPTABLE

If the factory owner creates a market outside the vault (directly via factory), the vault has no record of it. This is documented and by design — the vault is not intended to track externally-created markets.

No new code changes make bypass easier. The factory is onlyOwner for createMarket, and the vault holds factory ownership. Only the vault owner (via emergencyTransferFactoryOwnership) could temporarily reclaim factory ownership to create external markets.

Severity unchanged.

I-4: depositTimestamp reset on every deposit — STILL ACCEPTABLE

depositTimestamp[msg.sender] = block.timestamp in deposit() (line 235) resets on every deposit. A user who deposited 1 day ago and deposits 1 wei today gets a fresh holding period.

This is by design — prevents circumventing the holding period with pre-existing deposits. The UX trade-off (full reset vs. weighted average) is acknowledged and acceptable for Phase 1.

Severity unchanged.

I-5: Test coverage gap (multi-deposit + interleaved bets + partial withdraw) — PARTIALLY ADDRESSED

The original finding flagged a missing test for: LP1 deposits, bets happen, LP1 deposits again, more bets, LP1 partially withdraws.

New coverage: The M-1 tests add significant interleaved operation coverage:

• test_audit_navSnapshotLocksInPrice — two LPs request at different times

• test_audit_partialFulfillmentOnInsolvency — three markets, two LPs, partial pay

• test_audit_depositDuringPendingWithdrawal — deposit after withdrawal request

Additionally, test_multiDeposit_interleavedBets_partialWithdrawal in SecurityAudit.t.sol (existing) directly covers the flagged scenario for MarketEngine-level LP operations.

For the vault specifically, the exact multi-deposit + interleaved bets + partial vault withdrawal sequence is still not explicitly tested as a single end-to-end scenario. However, the individual components are well-covered.

Severity reduced to informational; no functional risk identified.

---

7. New Findings

No new Critical, High, or Medium findings.

INFO-1: Sequential requestWithdrawal() price increase

Severity: Informational Location: LpVault.sol:264-266

When multiple LPs request withdrawal in the same transaction context (or sequential blocks), each subsequent request sees a slightly higher navPerShareAtRequest because totalShares decreases while NAV stays constant.

In test_audit_navSnapshotLocksInPrice (line 1522): assertGe(navPerShare2, navPerShare1).

This is mathematically correct — each remaining share IS worth more after some shares are burned. However, it means later withdrawers get a slightly better price than earlier ones for the same NAV. The difference is proportional to the fraction of shares being withdrawn and is small in practice.

Impact: Negligible. The price increase accurately reflects the remaining shares' value. No exploit path exists — an attacker cannot time their request to manipulate the ordering since requests are processed in FIFO order regardless.

Recommendation: Document this behavior for LPs. No code change needed.

INFO-2: Partial fulfillment loss is permanent

Severity: Informational Location: LpVault.sol:323-326

When partial fulfillment occurs (usdcPaid < usdcOwed), the LP permanently loses the difference. The entry is marked fulfilled = true and cannot be re-processed.

This is by design — the LP accepted the snapshot price risk. However, the shortfall may not be communicated clearly to the LP.

Recommendation: Emit the shortfall amount in the WithdrawalFulfilled event (already emits both usdcOwed and usdcPaid, so the shortfall is derivable). Frontend should display partial fulfillment status clearly.

---

8. Test Coverage Assessment

New Tests Added (in LpVault.t.sol)

Test	Finding Covered
test_audit_navSnapshotLocksInPrice	M-1: NAV snapshot
test_audit_navSnapshotStaleProof	M-1: Stale-proof snapshot
test_audit_partialFulfillmentOnInsolvency	M-1: Partial fulfillment
test_audit_partialFulfillmentFloatExactlyZero	M-1: Float drain to zero
test_audit_floatDrainAndRecovery	M-1: Queue blocking + recovery
test_audit_recoverMarketLifecycle	M-2: Full recovery cycle
test_audit_recoverMarketRevertsIfActive	M-2: Active guard
test_audit_recoverMarketOnlyOwner	M-2: Access control
test_audit_recoverMarketRevertsAfterCollected	M-2: Phantom NAV guard
test_audit_totalDeployedTracksLifecycle	L-2: Counter integrity
test_audit_ownable2StepRequiresAcceptance	I-1: 2-step ownership
test_audit_acceptFactoryOwnership	I-1: Factory 2-step
test_audit_depositDuringPendingWithdrawal	Ghost share fix

Tests in SecurityAudit.t.sol

Test	Finding Covered
test_audit_feeCapBoundary10000Succeeds	L-1: Fee cap boundary (success)
test_audit_feeCapBoundary10001Reverts	L-1: Fee cap boundary (revert)
test_audit_sweepLosingZeroReverts	I-3: Zero sweep guard
test_audit_setEngineZeroReverts	I-2: Zero engine guard

Final Test Counts

Suite	Tests	Pass	Fail
MarketEngine.t.sol	63	63	0
MarketFactory.t.sol	7	7	0
CreateMarket.t.sol	12	12	0
SecurityAudit.t.sol	94	94	0
LpVault.t.sol	81	81	0
Total	257	257	0

Remaining Gaps

1. Vault-level multi-deposit + interleaved bets + partial withdrawal — individual components tested but not as single E2E flow

2. Fuzz testing of navPerShareAtRequest — the snapshot math is tested with concrete values but could benefit from fuzz coverage with random deposit/withdrawal/resolution sequences

3. Concurrent withdrawal requests from same user — not tested (user requests withdrawal, deposits again, requests again)

None of these gaps represent high-risk scenarios.

---

9. Overall Assessment

PASS WITH NOTES

All 8 audit fixes are correctly implemented and tested. The M-1 fix (NAV snapshot) is architecturally sound with correct ordering of state mutations, proper partial fulfillment handling, and no ghost share dilution. The M-2 fix (recoverMarket) includes a phantom NAV guard that prevents the most dangerous attack vector. No new vulnerabilities were introduced by any fix.

The two informational notes (sequential price increase, permanent partial fulfillment loss) are inherent to the snapshot-based withdrawal design and require no code changes — only documentation.

---

10. Conclusion

The MarkIt protocol's fix implementation is thorough and well-tested. The M-1 NAV snapshot architecture correctly addresses the most critical finding from the prior audit without introducing new attack surfaces. All 6 core invariants hold. The vault's centralization risk (owner-controlled market operations) remains the primary trust assumption, which is acknowledged and appropriate for Phase 1. The protocol is ready for continued testnet operation with the fixes in place.