Add community security audit report & contract safety improvements

[saiboyizhan]

Summary

Independent community security audit of BAP578.sol with verified code fixes. As a builder shipping a prediction market on BSC, I audited the BAP-578 reference implementation to assess integration safety and contribute back to the ecosystem.

Audit Report: SECURITY-AUDIT.md — 16 findings (2 Critical, 4 High, 3 Medium, 4 Low, 3 Informational)

Code Fixes (13 issues resolved in contracts/BAP578.sol):

Fix	Description
C-01/C-02	emergencyWithdraw() restricted to unallocated funds via totalAgentBalances tracker
H-03	Added require(msg.value == 0) in free mint path to prevent ETH lock
M-01	Added tokensOfOwnerPaginated() for safe bounded queries
M-02	Added FreeMintsPerUserUpdated event to setFreeMintsPerUser()
M-03	Added string length validation for all metadata fields
L-02	Added fallback() with descriptive revert
L-04	Added delete agentStates/agentMetadata cleanup after burn
I-02	Added nonReentrant to fundAgent() for defense-in-depth
New	Added public burn() — _burn() was previously unreachable dead code
New	Added require(msg.value > 0) to fundAgent()
New	Added require(amount > 0) to withdrawFromAgent()
New	Fixed isFreeMint fragile pattern (increment tokenId before use)
New	Added uint256[39] __gap for upgrade-safe storage

Design-level recommendations (UUPS timelock, pull-based treasury, snapshot-based free mints) are documented in the audit report as suggestions for the team to evaluate.

Test Results

All 33 tests passing (30 original + 3 new tests for emergency withdraw protection, owner withdrawal, and burn functionality).

Files Changed

• SECURITY-AUDIT.md — Full audit report
• contracts/BAP578.sol — Bug fixes
• test/BAP578.test.js — Updated and new tests

[christelbuchanan]

Thanks @saiboyizhan we are reviewing this now. Great improvements proposed.

[christelbuchanan]

@saiboyizhan ,

This is a great PR thank you. Directionally, the audit is well-scoped and the top two criticals are real, high-impact issues for any contract that lets users deposit funds (fundAgent() flow). The SECURITY-AUDIT.md as a standalone artifact is valuable and sets a strong dev-first rubric.

We have some initial suggestions here:

1. Please confirm totalAgentBalances invariants

• Show that every balance-affecting path updates it (deposit/withdraw/burn edge cases)
• Add a test that fuzzes: multiple agents, multiple deposits/withdraws, then assert address(this).balance >= totalAgentBalances always holds.

2. Burn semantics

• Confirm burn() permissions (owner/approved)
• Confirm behavior when agent has balance (must revert or auto-withdraw)
• Add tests: cannot burn with balance > 0; state deleted after burn.

3. Free mint ETH-lock fix

• Add a test that calling free mint with msg.value > 0 reverts with the intended message (this was a High in the audit).

4. Pagination function

• Add tests for tokensOfOwnerPaginated() boundaries (offset >= balance, limit=0, last page).

5. Upgradeable storage gap

• Confirm contract is actually intended to be upgradeable (UUPS is mentioned in the audit).
• Ensure the __gap sits at the end of storage layout and doesn’t collide with existing slots.

[saiboyizhan]

Thanks for the detailed review! I'll add the test cases for all 5 items and push an update.

[saiboyizhan]

Hi @christelbuchanan, just a friendly follow-up — I've pushed the test cases covering all 5 areas you requested (commit 3224c6b), and CI is fully green (11/11 checks passing).

Let me know if there's anything else needed for this to move forward. Happy to make further adjustments if needed!

[christelbuchanan]

Thanks so much @saiboyizhan
Hey @snowmanxm pls check

[greptile-apps]

Greptile Summary

This PR is a community security audit contribution that ships a detailed audit report (SECURITY-AUDIT.md) alongside 13 concrete code fixes to BAP578.sol. The most impactful fixes — scoping emergencyWithdraw to only unallocated ETH via a totalAgentBalances tracker, adding require(msg.value == 0) on the free mint path, providing a public burn() function, cleaning up storage after burn, and adding string-length validation — are all correctly implemented and well-tested with 30+ new test cases.

Key observations from the review:

• emergencyWithdraw emits no event: Every other ETH-moving function (fundAgent, withdrawFromAgent) emits an event, but emergencyWithdraw is silent. Given this function's history as the primary audit finding (C-01/C-02), the lack of an event makes on-chain monitoring blind to its execution.
• Arithmetic underflow can permanently brick emergencyWithdraw: The subtraction address(this).balance - totalAgentBalances on line 380 has no explicit guard. If initializeV2 seeds a value 1 wei too high (operator error under a race condition), the function reverts with an arithmetic panic on every call and cannot be corrected without a V3 upgrade — since reinitializer(2) is a one-time call.
• burn() is the only state-mutating function without nonReentrant: All other agent state-modifying functions (fundAgent, withdrawFromAgent, createAgent) are guarded. While not immediately exploitable, it is inconsistent with the contract's stated defense-in-depth philosophy.
• The __gap[39] storage gap is correctly positioned and correctly consumes one slot for the new totalAgentBalances variable (from what would be a gap of 40 if no new variables were added). BAP578V2Mock is a pre-existing test contract and the upgrade tests are valid.
• The previously flagged issues around initializeV2 access control, off-chain seeding risks, tokensOfOwnerPaginated overflow safety, and the CEI ordering in burn() are either correctly handled in the current HEAD or have already been discussed in prior review threads.

Confidence Score: 3/5

• The core security fixes are sound, but two unresolved issues in emergencyWithdraw (missing event, permanent liveness lock from over-counted seed) and an inconsistent nonReentrant omission on burn() should be addressed before merging.
• The 13 audit fixes are correctly implemented and well-tested. The totalAgentBalances tracker properly scopes emergency withdrawal, the free-mint ETH lock is plugged, and burn/storage cleanup are correct. However, emergencyWithdraw — the most sensitive function in the contract — still has two new issues introduced by this PR: it emits no event (reducing auditability) and its unchecked subtraction can be permanently bricked by an incorrect initializeV2 seed, a scenario the PR's own documentation acknowledges is a race-condition risk. The missing nonReentrant on burn() is a lower-priority inconsistency. These issues do not rise to the level of the original C-01/C-02 findings, but they warrant fixes before deployment.
• contracts/BAP578.sol — specifically the emergencyWithdraw function (lines 379–384) and the burn function (lines 274–277).

Important Files Changed

Filename	Overview
contracts/BAP578.sol	13 targeted security fixes applied (C-01/C-02 emergencyWithdraw scoping, H-03 ETH lock, M-01 pagination, M-02 event, M-03 string validation, L-02 fallback, L-04 burn cleanup, I-02 nonReentrant, plus new burn(), fundAgent/withdrawFromAgent guards, and __gap). Remaining concerns: emergencyWithdraw emits no event; arithmetic underflow on line 380 permanently disables the function if initializeV2 seeds an over-counted value; burn() is the only state-mutating function without nonReentrant.
test/BAP578.test.js	Solid new test coverage for totalAgentBalances invariant, burn semantics, free-mint ETH lock fix, pagination edge cases, and upgrade storage gap. Tests correctly use ethers v5 API (consistent with the rest of the suite). One misleading comment on line 672 ("Use a paid mint") describes three free mints, but test logic itself is correct. BAP578V2Mock dependency exists as a pre-existing file at contracts/mocks/BAP578V2Mock.sol.
SECURITY-AUDIT.md	Well-structured community audit report documenting 16 findings across 5 severity levels. The critical findings (C-01/C-02) and high findings are clearly described with impact analysis and concrete code recommendations. Design-level recommendations (UUPS timelock, pull-based treasury) are appropriately flagged as suggestions. No issues with the document itself.

Sequence Diagram

sequenceDiagram
    participant User
    participant BAP578
    participant Treasury
    participant Owner

    Note over BAP578: V1→V2 Upgrade Path
    Owner->>BAP578: upgradeToAndCall(newImpl, initializeV2(seed))
    BAP578-->>Owner: totalAgentBalances = seed

    Note over BAP578: Free Mint (H-03 fix)
    User->>BAP578: createAgent(to, ...) [msg.value == 0 enforced]
    BAP578-->>User: tokenId minted, isFreeMint[tokenId] = true

    Note over BAP578: Paid Mint
    User->>BAP578: createAgent(to, ...) [msg.value == MINT_FEE]
    BAP578->>Treasury: transfer MINT_FEE
    BAP578-->>User: tokenId minted

    Note over BAP578: Fund / Withdraw Agent
    User->>BAP578: fundAgent(tokenId) [ETH]
    BAP578-->>BAP578: agentStates[id].balance += value\ntotalAgentBalances += value

    User->>BAP578: withdrawFromAgent(tokenId, amount)
    BAP578-->>BAP578: balance -= amount\ntotalAgentBalances -= amount
    BAP578-->>User: transfer amount

    Note over BAP578: Emergency Withdraw (C-01/C-02 fix)
    Owner->>BAP578: emergencyWithdraw()
    BAP578-->>BAP578: unallocated = balance - totalAgentBalances
    BAP578-->>Owner: transfer unallocated only

    Note over BAP578: Burn (new public function)
    User->>BAP578: burn(tokenId) [balance must be 0]
    BAP578-->>BAP578: delete agentStates/agentMetadata/isFreeMint
    BAP578-->>User: emit AgentBurned

Loading

_{Last reviewed commit: "fix: add nonReentran..."}

[saiboyizhan]

Hi @christelbuchanan @snowmanxm — friendly follow-up!

Greptile flagged a valid issue on initializeV2 missing onlyOwner, which I'll push a fix for shortly (along with two minor cleanups: event ordering in burn() and isFreeMint cleanup).

Once that's pushed, all review feedback from both of you + Greptile should be fully addressed. Would love to get this across the finish line — let me know if there's anything else needed!

[greptile-apps]

emergencyWithdraw emits no event

emergencyWithdraw is a high-impact administrative action — it moves unallocated ETH out of the contract — yet it emits no event. Given the sensitivity of this function (it is the re-scoped successor of what the audit identified as a rug-pull vector), the absence of an event makes it impossible for off-chain monitoring tools, security dashboards, or users to detect when it is called, how much was withdrawn, or whether the invariant address(this).balance >= totalAgentBalances was preserved after the call.

All other ETH-moving functions in this contract (fundAgent, withdrawFromAgent) emit events. emergencyWithdraw should too:

Suggested change

	function emergencyWithdraw() external onlyOwner nonReentrant {
	uint256 unallocated = address(this).balance - totalAgentBalances;
	require(unallocated > 0, "No unallocated balance");
	(bool success, ) = payable(owner()).call{ value: unallocated }("");
	require(success, "Emergency withdraw failed");
	}
	function emergencyWithdraw() external onlyOwner nonReentrant {
	uint256 unallocated = address(this).balance - totalAgentBalances;
	require(unallocated > 0, "No unallocated balance");
	(bool success, ) = payable(owner()).call{ value: unallocated }("");
	require(success, "Emergency withdraw failed");
	emit EmergencyWithdraw(owner(), unallocated);
	}

You'll also need to declare the event near the other events:

event EmergencyWithdraw(address indexed recipient, uint256 amount);

[snowmanxm]

@saiboyizhan , check this

[greptile-apps]

emergencyWithdraw permanently bricked if initializeV2 seed is over-counted

Line 380 computes address(this).balance - totalAgentBalances. In Solidity 0.8+ this is an implicit checked subtraction — if totalAgentBalances > address(this).balance, the entire function reverts with an arithmetic panic.

This matters because initializeV2 seeds totalAgentBalances from an off-chain-computed parameter. If the operator supplies a value that is even 1 wei too high (e.g., due to a miscalculation or rounding error), the invariant address(this).balance >= totalAgentBalances is immediately violated. From that point on, every call to emergencyWithdraw will panic-revert — permanently — because:

1. Subsequent fundAgent calls increment both address(this).balance and totalAgentBalances by the same amount, preserving the deficit.
2. Subsequent withdrawFromAgent calls decrement both by the same amount, also preserving the deficit.
3. reinitializer(2) can only be called once, so there is no way to correct the seed value without a V3 upgrade.

The consequence is that any unallocated ETH that accumulates in the contract (e.g., from future accidental sends, or from tokens sent directly to the contract despite receive() reverting via selfdestruct) would be permanently unrecoverable via emergencyWithdraw.

An explicit guard before the subtraction makes the failure mode visible and non-permanent within a single call:

Suggested change

	uint256 unallocated = address(this).balance - totalAgentBalances;
	require(address(this).balance >= totalAgentBalances, "Accounting invariant violated");
	uint256 unallocated = address(this).balance - totalAgentBalances;

This converts the silent arithmetic panic into a readable revert message, which is also easier to diagnose in transaction traces.

[snowmanxm]

@saiboyizhan , check this

[greptile-apps]

burn() lacks nonReentrant guard

All other state-changing functions that modify agentStates or ETH accounting (fundAgent, withdrawFromAgent, createAgent) are protected with nonReentrant. The burn() function modifies agentStates (via delete in _burn()), agentMetadata, and isFreeMint, but has no reentrancy protection.

While _burn() makes no external ETH calls and is therefore not currently reentrancy-vulnerable, adding nonReentrant is low-cost and consistent with the defense-in-depth philosophy already applied elsewhere in the contract (e.g., fundAgent was hardened with nonReentrant as I-02 in the audit, explicitly for this reason). Future upgrades that add callbacks or hooks inside _burn() would silently lack the guard.

Suggested change

	function burn(uint256 tokenId) external onlyTokenOwner(tokenId) {
	_burn(tokenId);
	emit AgentBurned(tokenId, msg.sender);
	}
	function burn(uint256 tokenId) external onlyTokenOwner(tokenId) nonReentrant {
	_burn(tokenId);
	emit AgentBurned(tokenId, msg.sender);
	}

[snowmanxm]

@saiboyizhan , check this