Skip to content

fix(guild): role/membership gates on unsecured entry points + per-function permission linter (#19) - #63

Merged
merlik787-droi merged 3 commits into
Riddlrealm:mainfrom
XxHugheadxX:fix/19-guild-permissions-audit
Jul 29, 2026
Merged

fix(guild): role/membership gates on unsecured entry points + per-function permission linter (#19)#63
merlik787-droi merged 3 commits into
Riddlrealm:mainfrom
XxHugheadxX:fix/19-guild-permissions-audit

Conversation

@XxHugheadxX

Copy link
Copy Markdown
Contributor

Closes #19

Audit result

All 13 pub entry points in guild already call require_auth(); the gaps were
one layer deeper — the role/membership authorization check. Full per-method
matrix in docs/audits/admin-permissions-matrix.md.

What

  • New assert_member helper, extracted from the inline membership checks in
    vote / vote_withdrawal so every membership gate shares one implementation
    and one panic message.
  • execute_withdrawal and deposit now gate on membership (assert_member).
  • disband now uses the canonical assert_active gate.
  • scripts/check-missing-permissions.sh now runs a per-function AST checker
    (tools/check-permissions, using syn) instead of the per-file grep, which
    passed an entire contract on a single require_auth match anywhere.
  • Rewrote the placeholder admin-permissions-matrix.md with the real guild audit.
  • Removed a dead Map import (unblocks clippy -D warnings).

Design decision

execute_withdrawal gates on membership, not officer: the authority already
lives in the vote quorum; execution is ministerial, and requiring an officer
would strand guilds whose officer is absent.
(member_can_execute_approved_withdrawal) locan
still execute an approved withdrawal.

Honest severity (not overstated)

  • execute_withdrawal — a genuine access-control gap, but low severity: funds go
    to proposal.officer and a vote quorum is already required. Now members-only.
  • deposit — not an exploit; an intentional,
    (non-members can no longer deposit), matchid the
    membership gates already on voting.
  • disband — was already once-only via acheck; this switches it to the canonicala
    "Guild disbanded" message as every other mutator). Consistency/hardening,
    not a vulnerability fix.

Linter scope

The checker enforces the contracts in its ENd,
the subject of this issue) and returns zero `ne-off
workspace-wide run surfaced ~140 pre-existing candidate functions across ~45
other contracts; triaging those is separate follow-up work. (Clippy warn-levels
for the script are tracked in #46.)

Tests

3 new #[should_panic] (per acceptance crite
existing guild suite green (6/6 total).

Verification (local)

  • cargo clippy -p guild -- -D warnings — cl
  • cargo test -p guild — 6/6 passing
  • bash scripts/check-missing-permissions.sh
  • cargo build --target wasm32-unknown-unknown --release -p guild — ok

@XxHugheadxX

Copy link
Copy Markdown
Contributor Author

Note: CI failures here are unrelated to this change

The CI / build and clippy failures on this PR are a pre-existing,
repo-wide
issue, not caused by this change. Every open PR is currently red
with the same error, and fmt (which doesn't compile) passes.

Root cause: ed25519-dalek 3.0.0 was recently published, and its changed
CryptoRng/rand_core bounds fail to compile soroban-env-host's testutils:

error[E0277]: the trait bound ChaCha20Rng: ed25519_dalek::rand_core::CryptoRng
is not satisfied
error: could not compile soroban-env-host (lib) due to 1 previous error

Because Cargo.lock is git-ignored, every fresh CI resolve pulls the broken
3.0.0 (Adding ed25519-dalek v2.2.0 (available: v3.0.0) in the logs).

This change is verified locally with ed25519-dalek pinned to 2.2.0:

  • cargo clippy -p guild -- -D warnings — clean
  • cargo test -p guild — 6/6 passing
  • bash scripts/check-missing-permissions.shOK (zero MISSING_AUTH_CHECK rows)
  • cargo build --target wasm32-unknown-unknown --release -p guild — ok

Suggested infra fix (repo-wide, separate from this PR): either commit
Cargo.lock, or pin ed25519-dalek = "=2.2.0". Happy to open a separate PR for
that if useful — it would unblock every open PR, which are all red for this same
reason.

@merlik787-droi
merlik787-droi merged commit 2c64a54 into Riddlrealm:main Jul 29, 2026
3 checks 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.

Auditing missing require_auth in guild admin entry points

2 participants