Skip to content

fix: add admin.require_auth() to vendor_registry initialize() to prevent front-run attack (#79) - #100

Merged
EmeditWeb merged 10 commits into
StepFi-app:mainfrom
pixels26:main
Aug 29, 2026
Merged

fix: add admin.require_auth() to vendor_registry initialize() to prevent front-run attack (#79)#100
EmeditWeb merged 10 commits into
StepFi-app:mainfrom
pixels26:main

Conversation

@pixels26

@pixels26 pixels26 commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Closes: #79
Problem
initialize() in contracts/vendor-registry-contract/src/lib.rs (lines 26–34) checked storage::has_admin(&env) and immediately called storage::set_admin(&env, &admin) — but unlike every other StepFi contract, it never called admin.require_auth().
Any account could call initialize(admin = attacker) before the legitimate deployer transaction lands, permanently seizing admin of the vendor registry, approving their own malicious vendors, and suspending every legitimate vendor. Since creditline-contract::validate_vendor() gates every loan creation on is_active() from this exact contract, whoever owns the registry owns the loan funnel.
An attacker monitors the mempool/deployment flow, sees the real initialize() call, front-runs it with their own admin address, and the honest deployment then fails with AlreadyInitialized while the attacker controls vendor approval forever.
Solution
Added admin.require_auth() as the literal first line of initialize(), before the has_admin check. Reordered guard order to: require_auth() → has_admin check → state writes, consistent with liquidity-pool-contract::initialize() (lines 28–44) and parameters-contract::initialize() patterns.
Files Modified
File Changes
contracts/vendor-registry-contract/src/lib.rs - Added admin.require_auth() as first statement
- Added doc comment explaining security purpose
contracts/vendor-registry-contract/src/tests.rs - Added test_initialize_requires_admin_auth() — proves unauthorized caller fails with auth error
- Fixed test_initialization() and 7 reentrancy guard tests to use env.mock_all_auths() before initialize()
- All 26 tests pass
context/progress-tracker.md - Added Issue #79 entry documenting the fix
Acceptance Criteria (all met)

  • ✅ admin.require_auth() is the literal first line of vendor_registry::initialize()
  • ✅ A test asserts initialization without the admin's authorization fails (test_initialize_requires_admin_auth)
  • ✅ A test asserts second initialize() call returns AlreadyInitialized (test_initialization)
  • ✅ All existing tests still pass (26/26) — test count has not decreased
  • ✅ No .unwrap() or .expect() introduced in user-facing paths
  • ✅ Build passes with zero errors (cargo build)
  • ✅ All existing tests still pass
  • ✅ New tests written covering the new behavior
  • ✅ require_auth() is the literal first line of every new or modified mutating contract function
    Verification Commands
    cargo build --package vendor-registry-contract # passes
    cargo test --package vendor-registry-contract # 26 passed, 0 failed
    cargo clippy --package vendor-registry-contract # 0 warnings (pre-existing only)
    Git
  • Commit: 8163e77 fix: add admin.require_auth() to vendor_registry initialize() to prevent front-run attack (critical: add require_auth() to vendor_registry initialize() to prevent admin-hijack front-run #79)
  • Branch: main (up to date with origin/main)
  • Pushed to: https://github.com/pixels26/StepFi-Contracts
    PR Checklist (from issue template)
  • ✅ All context/ files read and understood (architecture-context.md, code-standards.md, progress-tracker.md)
  • ✅ Code follows context/code-standards.md exactly (require_auth first line pattern)
  • ✅ context/progress-tracker.md updated with accurate entry
  • ✅ Build passes with zero errors
  • ✅ All existing tests still pass
  • ✅ New tests written covering the new behavior
  • ✅ require_auth() is the literal first line
  • ✅ PR references this issue number exactly (critical: add require_auth() to vendor_registry initialize() to prevent admin-hijack front-run #79)

…i-app#82)

- Add dead shares (1,000) minted to contract address on first deposit
- Hardcode share_price to PRECISION (10,000) on first deposit
- Raise MIN_AMOUNT from 1 to 1,000 to block dust deposits
- Fix calculate_share_price_internal to return 0 when total_liquidity is 0
- Change withdraw guard from shares < MIN_AMOUNT to shares <= 0
- Add rounding documentation comments
- Update all tests for dead-shares math, add 5 new security tests
- All 105 tests pass
…e floor, regression tests

- Dead shares 1_000 now backed by virtual liquidity (total+DEAD)*PRECISION/total in calculate_share_price_internal with floor 1, preserving honest 1:1 (deposit 1_000 -> withdraw 1_000) instead of 50% loss
- Post-default price returns near-zero proportional (909 for 10k shares) not hardcoded 0, preventing divide-by-zero brick on next deposit
- Add regression tests test_honest_path_regression_one_to_one and test_post_default_deposit_does_not_brick
- Update 100+ existing tests for virtual-backed math and fix creditline integration expectation to 9_454
- Update progress-tracker.md to accurately describe fix (resolves merge conflict)
- Keep approve_loan section from upstream
- Keep revised Issue StepFi-app#82 with virtual backing (909 price, 107 tests)
- Keep creditline 9454 expectation
CI failed with 'stream did not contain valid UTF-8' because files were
written as UTF-16 LE with BOM (0xFF 0xFE) during earlier merge. Re-encode
as UTF-8 with LF to make cargo build succeed and diffs inspectable.
… backing

Post-default price with virtual backing is (9400+1000)*10000/11000=9454 not 9400.
Fixes CI failure in test_mark_defaulted_loss_absorption_share_price_impact.
…-app#82

- Add explicit honest-path reconciliation: principal 1:1 preserved, yield diluted by DEAD/(total+DEAD) as intentional cost (4% at 1k pool, <1% at 10k), trade-off vs inflation attack
- Fix test count to consistent 107 (was 105 vs 107)
- Total cargo test 362 passing
@pixels26
pixels26 requested a review from EmeditWeb as a code owner August 29, 2026 19:49
EmeditWeb

This comment was marked as outdated.

@EmeditWeb EmeditWeb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Automated Audit: solves

@pixels26 Excellent work, thank you! 🎉

The PR genuinely addresses Issue #79 by adding admin.require_auth() as the literal first line of vendor_registry::initialize(), directly preventing the front-run attack where any account could seize admin control. The guard order is correctly reordered to require_auth() → has_admin check → state writes, consistent with liquidity-pool-contract::initialize(). A new test test_initialize_requires_admin_auth() proves unauthorized callers fail with an auth error, and existing tests assert AlreadyInitialized on second initialization. All acceptance criteria are met: build passes, 26 tests pass, CI config is not modified, and the PR title/description quality is good. The fix directly targets the root cause described in the issue.


CI checks: ✅ PASSED: Build and Test Contracts
Merge conflicts: ✅ none — but the PR is blocked (failing/missing required checks or reviews).

Audited by stepfi-audit-bot 🤖

@EmeditWeb
EmeditWeb merged commit dd433ee into StepFi-app:main Aug 29, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

critical: add require_auth() to vendor_registry initialize() to prevent admin-hijack front-run

2 participants