Manual audit results: community-wave8 stage2 (5 repos, 5 scanner findings + independent audit)

[omarespejel]

Manual Audit Results: Community Wave 8 Stage-2

Responding to issue #33 — independent manual verification of all 5 scanner findings, plus additional vulnerabilities found by deep audit.

Auditor: @omarespejel (manual deep-audit pass)
Scope commit: community-wave8-2026-03-09-stage2
Repos audited: 5 | Scanner findings reviewed: 5 | Additional findings: 8

---

Part 1 — Scanner Finding Verdicts (TP/FP)

Verdict CSV

finding_id,repo,ref,file,class_id,manual_verdict,manual_notes
LESSKNOWNI-001,avnu-labs/avnu-contracts-v2,006e3ddd9ddae28be73336842684f7db55273a2f,src/components/fee.cairo,FEES_RECIPIENT_ZERO_DOS,tp,"initialize() writes fees_recipient without an is_non_zero() guard. collect_fee_bps() short-circuits on zero recipient so fees silently go nowhere, but transfer is still called with zero address recipient under certain token implementations. Real DoS / fee-loss vector confirmed."
LESSKNOWNI-002,lambdaclass/yet-another-swap,f3ee03a3564a37698e9589f564ac63aa59dab283,crates/yas_core/src/contracts/yas_factory.cairo,CRITICAL_ADDRESS_INIT_WITHOUT_NONZERO_GUARD,tp,"constructor accepts owner: ContractAddress with no is_non_zero() assert. Deploying with owner=0 permanently locks all owner-gated functions (enable_fee_amount, set_owner). pool_class_hash has the guard but owner does not."
LESSKNOWNI-003,lambdaclass/yet-another-swap,f3ee03a3564a37698e9589f564ac63aa59dab283,crates/yas_core/src/contracts/yas_pool.cairo,CRITICAL_ADDRESS_INIT_WITHOUT_NONZERO_GUARD,tp,"Pool constructor accepts factory, token_0, token_1 with no is_non_zero() guards. Tokens are validated in the factory's create_pool before deployment, but pools can also be deployed directly (deploy_syscall is permissionless on Starknet). Direct deployment with zero token addresses creates a bricked pool."
LESSKNOWNI-004,lambdaclass/yet-another-swap,f3ee03a3564a37698e9589f564ac63aa59dab283,crates/yas_faucet/src/yas_faucet.cairo,CRITICAL_ADDRESS_INIT_WITHOUT_NONZERO_GUARD,tp,"Faucet constructor writes token_address with no guard. If token_address=0, all faucet_mint and withdraw_all_balance calls silently send to/call address 0. Owner address also has no zero-check."
LESSKNOWNI-005,lambdaclass/yet-another-swap,f3ee03a3564a37698e9589f564ac63aa59dab283,crates/yas_faucet/src/yas_faucet.cairo,IRREVOCABLE_ADMIN,tp,"OpenZeppelin Ownable used; no two-step ownership transfer and no renounce-ownership is exposed. If owner key is lost, faucet funds are permanently locked (withdraw_all_balance only callable by owner). Low severity as intended for testnet, but irrevocable admin pattern is confirmed."

Summary: All 5 scanner findings are TRUE POSITIVES. Precision on this wave: 5/5 = 100%.

---

Part 2 — Independent Deep Audit: Additional Findings

The following vulnerabilities were not caught by the scanner (false negatives).

---

FN-001 — briqNFT/briq-protocol — Disabled bonding-curve state update (price manipulation / free briqs)

• Repo: briqNFT/briq-protocol@5db5f81
• File: src/briq_factory.cairo — fn buy()
• Severity: High
• Class: LOGIC_DISABLED_STATE_UPDATE

Description:
The buy() function computes a bonding-curve price and charges the buyer, but the three lines that update the curve state are commented out:

// briq_factory.last_purchase_time = get_block_timestamp();
// briq_factory.last_stored_t = t + amount * DECIMALS();
// briq_factory.surge_t = surge_t + amount * DECIMALS();
// BriqFactoryTrait::set_briq_factory(world, briq_factory);

This means:

1. The bonding curve t (total supply variable) is never incremented → every purchase uses the same base price forever, ignoring the intended price increase mechanism.
2. The surge component is never updated → flash-loan style repeated buying in one block does not incur the surge penalty.
3. The price is permanently stuck at the initial t / surge_t set in initialize(), making the economic model entirely non-functional.

Developer advice: Remove the comments and restore the state update, or explicitly document this as a known temporary deviation from the whitepaper with a tracking issue. At minimum add an assertion or a comment block explaining the invariant.

---

FN-002 — briqNFT/briq-protocol — migrate_legacy_briqs relies on exact balance match (griefing / DoS)

• File: src/migrate.cairo — fn migrate_legacy_briqs()
• Severity: Medium
• Class: STRICT_EQUALITY_DOS

Description:

assert(legacy_briqs.balanceOfMaterial_(caller.into(), 1) == qty.into(), 'bad nb of briqs');

The check requires the caller's entire balance of material-1 briqs to equal qty. If a user has any partial balance or receives even 1 briq after computing their qty off-chain, the migration reverts. A griefing attacker can send 1 briq to a victim's legacy address immediately before they call migrate_legacy_briqs, permanently blocking that user's migration.

Developer advice: Change the check to >= or allow partial migration (pass qty, check balance >= qty).

---

FN-003 — briqNFT/briq-protocol — assemble() allows admin to mint sets for arbitrary owners with no event

• File: src/set_nft/assembly.cairo — fn assemble()
• Severity: Medium
• Class: PRIVILEGE_ESCALATION / MISSING_EVENT

Description:

if owner != caller {
    world.only_admins(@get_caller_address());
}

An admin can assemble a set on behalf of any owner address. There is a // TEMP for migration comment, but there is no corresponding event emitted for admin-minted sets, and no time-lock or removal path for this privilege. Combined with the TODO: events are currently emitted directly on the contract note, admin-minted NFTs are invisible in the event log.

Developer advice: Emit a dedicated AdminAssembledSet { admin, owner, token_id } event. Add an explicit deadline after which the migration bypass is automatically disabled, or remove it post-migration.

---

FN-004 — briqNFT/briq-protocol — Box unboxing: hardcoded box ID table is not upgradeable

• File: src/box_nft/unboxing.cairo — fn get_box_infos()
• Severity: Low
• Class: HARDCODED_CONFIG / UPGRADABILITY

Description:
Box metadata (briq count, attribute group, attribute ID) is hardcoded in an if/else chain. assert(false, 'invalid box id') is the final fallback. Any new box type requires a full contract redeployment. The incomplete comments (// ducks : attribute_group_id: left blank) suggest the table is already known to be incomplete.

Developer advice: Store box metadata in Dojo world storage, settable by admins. This also enables on-chain auditability of what each box contains.

---

FN-005 — eqlabs/starknet-multisig — Cairo 0 contract: execute_transaction callable by anyone, no signer check

• Repo: eqlabs/starknet-multisig@4bb8255
• File: src/multisig.cairo
• Severity: Medium (design note, not a bug per se, but worth flagging)
• Class: ACCESS_CONTROL_PERMISSIVE

Description:
execute_transaction(nonce) has no _require_signer() check. Any address (including contracts) can trigger execution of a sufficiently-confirmed transaction. While threshold confirmation protects against unauthorized approval, allowing arbitrary callers to pull the execution trigger enables:

• MEV-style front-running of transaction execution order
• Forced execution at an inopportune time (e.g., during an unfavorable market condition for DeFi targets)

Developer advice: Add _require_signer() at the top of execute_transaction if the protocol intent is that only signers can finalize. If open execution is intentional, document it explicitly.

---

FN-006 — eqlabs/starknet-multisig — No zero-address check for signer entries

• File: src/multisig.cairo — _set_signers()
• Severity: Low
• Class: CRITICAL_ADDRESS_INIT_WITHOUT_NONZERO_GUARD

Description:
_set_signers calls _require_unique_signers (duplicate check) but does not check that individual signer addresses are non-zero. Adding address(0) as a signer is semantically invalid: get_caller_address() never returns zero on Starknet, so a zero signer slot can never be used to confirm/submit, but counts toward signers_len, inflating the effective denominator of the threshold check.

Developer advice: Add assert(signer.is_non_zero(), 'zero signer not allowed') inside the signer-writing loop.

---

FN-007 — milancermak/cairo-4626 — Cairo 0 ERC-4626: deposit/withdraw share math uses integer division with no rounding direction protection

• Repo: milancermak/cairo-4626@67a9e76
• File: contracts/erc4626/library.cairo
• Severity: Medium
• Class: ROUNDING_DIRECTION_ATTACK

Description:
The vault's convert_to_shares and convert_to_assets use total_assets / total_supply integer division without consistently rounding in the vault's favor. ERC-4626 spec (EIP-4626) mandates:

• deposit/mint → round down shares (favor vault)
• withdraw/redeem → round up assets (favor vault)

Without explicit rounding direction, an attacker can exploit the rounding error to extract 1 wei more than entitled per transaction, accumulating losses to the vault. Compounded with the "first depositor inflation attack" (depositing 1 wei to set total_supply=1 and then donating assets), the vault is vulnerable to share price manipulation.

Developer advice: Add +1 rounding in the appropriate directions per EIP-4626 spec. Add a minimum initial deposit or virtual shares offset (OpenZeppelin's ERC-4626 uses 10**decimalsOffset virtual shares to mitigate inflation attacks).

---

FN-008 — lambdaclass/yet-another-swap — is_valid_callback_contract only checks non-zero, not whitelist

• File: crates/yas_core/src/contracts/yas_pool.cairo
• Severity: Medium
• Class: INSUFFICIENT_CALLBACK_VALIDATION

Description:

fn is_valid_callback_contract(callback_contract: ContractAddress) -> bool {
    callback_contract.is_non_zero()
}

The callback validation in swap() and mint() only checks that the caller is non-zero. Any contract that implements IYASSwapCallback / IYASMintCallback can be the callback target, including malicious re-entrant contracts. While Starknet's single-call-per-tx model reduces classic reentrancy risk, a malicious callback can still:

• Manipulate oracle-dependent state between the balance snapshot and the balance check
• Call back into swap() on a different pool that shares state

Developer advice: Maintain a factory-registered whitelist of valid callback callers (i.e., only pools/routers deployed by the factory). The factory address is already stored in the pool.

---

Part 3 — Skill Performance Summary

Metric	Value
Scanner findings reviewed	5
True Positives	5
False Positives	0
Precision	100%
Additional findings (FN)	8
Critical FNs	0
High FNs	1 (FN-001 bonding curve disabled)
Medium FNs	4
Low FNs	2
Repos with missed findings	4/5

---

Part 4 — Developer Recommendations Summary

Repo	Priority Action
avnu-contracts-v2	Add assert(fees_recipient.is_non_zero()) in initialize()
yet-another-swap (factory)	Add assert(owner.is_non_zero()) in constructor
yet-another-swap (pool)	Add non-zero guards for token_0/token_1/factory in constructor
yet-another-swap (faucet)	Add non-zero guards for owner and token_address
yet-another-swap (faucet)	Consider two-step ownership transfer
briq-protocol	Restore bonding curve state update in buy() (HIGH)
briq-protocol	Fix strict-equality migration check to >=
briq-protocol	Add admin-assembly event; set migration expiry
briq-protocol	Move box metadata to world storage
starknet-multisig	Add signer check on execute_transaction
starknet-multisig	Add zero-address signer validation
cairo-4626	Fix EIP-4626 rounding directions; add inflation attack protection
yet-another-swap (pool)	Whitelist-validate callback contracts via factory registry