fix: beneficiary validation, clawback access-control audit, lock_assets reentrancy docs, asset index tests (#406, #405, #398, #397)

[codexhange]

Description

Implements the four Stellar Wave issues (#406, #405, #398, #397) — one commit per issue. Two are code fixes; two turned out to report already-mitigated behaviour, and per this repo's own SECURITY_FIXES.md convention (#357/#358/#363) those are delivered as a documented verification plus the regression tests that keep the property pinned, rather than as invented code.

Related Issues

See closing references below (#406, #405, #398, #397).

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• Test improvements
• Documentation update

Changes

• [security] vesting-wallet: clawback doesn't require admin auth #406 — vesting-wallet: "clawback doesn't require admin auth" (696a603)
  • The reported function does not exist: clawback appears nowhere in soroban/contracts/vesting-wallet/. Nothing in the contract performs a token-level clawback, so there is no unauthenticated clawback to fix.
  • No new clawback was added on purpose — it would be a brand-new admin capability the codebase has never had, i.e. an expansion of attack surface in the opposite direction of the report, with undefined semantics (who receives the amount, interaction with released_amount, behaviour after revocation).
  • Instead: (1) a SECURITY_FIXES.md audit section recording the finding and tabulating the authorization on every value-moving/authority-changing entry point (initialize, release, revoke, emergency_withdraw, transfer_beneficiary, transfer_admin) — all correctly gated; (2) a new regression test test_admin_only_entry_points_require_admin_auth that authorizes no admin and asserts each admin-gated entry point is rejected with state untouched, then that the real admin still succeeds. Any future entry point missing require_auth() fails this test.
• [bug] vesting-wallet: release doesn't validate beneficiary address #405 — vesting-wallet: release doesn't validate the beneficiary address (48a08d2)
  • New VestingError::InvalidInput = 8; initialize now rejects the zero beneficiary exactly as the issue specifies, since a zero address can never sign release and would strand the whole vested amount.
  • Applied the same guard to transfer_beneficiary — the same defect with the same "no recovery mechanism" impact, and the only other path that can set a beneficiary. A valid address still transfers as before.
  • Tests: initialize rejects the zero beneficiary (and mints nothing); transfer_beneficiary rejects the zero address, leaves the old beneficiary intact, and still accepts a real one.
• [security] farming-pool: lock_assets allows reentrancy via token transfer #398 — farming-pool: lock_assets allows reentrancy via token transfer (7c78df5)
  • Verified the code is already correct: all validations run first and the position (including the extended unlock_ledger and recomputed credit_rate) is persisted before token::transfer, i.e. strict checks-effects-interactions; a failed transfer traps and reverts everything. Below that, Soroban's ContractReentryMode defaults to Prohibited, so a reentrant token is rejected by the host before any of our code runs.
  • The issue asked to "verify this is sufficient and add documentation", so: a # Reentrancy posture (#398) section on lock_assets documenting both layers, the transfer semantics ([security] farming-pool: lock_assets doesn't validate token transfer success #363), and the two tests that prove it, plus a SECURITY_FIXES.md section. No behavior change, and the two existing reentrancy tests were not modified.
• [enhancement] factory: add pool_asset_index for efficient asset lookups #397 — factory: add pool_asset_index for efficient asset lookups (963f498)
  • The on-chain index the issue asks for already exists: DataKey::AssetPools(Address) -> Vec<u32> plus the constant-time DataKey::AssetPoolCount(Address) companion, both written by the shared create_pool_inner (so create_pool and create_pools_batch are covered), and get_pools_by_asset_range reads the index first, falling back to the bounded registry scan only for records predating the index.
  • What was missing was proof. Added four tests: the index is written by create_pool and returns only the requested asset's pools (total still reports the whole registry); pool_count_by_asset agrees with the indexed lookup; create_pools_batch indexes identically to single creation; and a paginated walk resumes without dropping or duplicating indexed pools (the [bug] factory get_pools_by_asset returns next_start_id that may skip matching pools #327 resume invariant on the index path).
  • Also corrected the get_pools_by_asset_range docstring, which still told integrators to index pool_crtd events for "zero-gas instant lookups" and omitted the on-chain index that now exists.
  • Note: the factory crate is #![no_std], so the test module declares extern crate std; (same as farming-pool's tests) for the std::vec::Vec assertions.

Testing

• New tests: 1 (vesting auth regression) + 2 (beneficiary validation) + 4 (factory asset index)
• Existing suites untouched otherwise — reentrancy tests, revoke tests, factory paging tests all unmodified
• Follows the repo's existing test layout (src/test.rs per contract, security-fixed doc convention in SECURITY_FIXES.md)

Note: verification was done by code review only (no cargo builds in this environment, per contribution constraints); CI will run cargo test, clippy, and fmt.

Closes #406
Closes #405
Closes #398
Closes #397

[drips-wave]

@codexhange Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

[netlify]

✅ Deploy Preview for sdcontracts ready!

Name	Link
🔨 Latest commit	beea7cd
🔍 Latest deploy log	https://app.netlify.com/projects/sdcontracts/deploys/6ab95ed232937d00082098b6
😎 Deploy Preview	https://deploy-preview-421--sdcontracts.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.