Fix admin permission scoping#63
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, push a new commit or reopen this pull request to trigger a review.
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughImplements target-scoped function-admin grants (admin, target, selector), rewires protected callsites to pass contract address into admin lookups, corrects selector bindings, adds events and recovery paths, expands regression tests, and synchronizes ADR/docs/ops/slither records for P0-ADMIN-001. ChangesP0-ADMIN-001 selector scoping and permission model
Sequence DiagramsequenceDiagram
participant User
participant ProtectedContract
participant StreamAdmins
User->>ProtectedContract: call protected(selector)
ProtectedContract->>StreamAdmins: retrieveFunctionAdmin(user, address(this), selector)
StreamAdmins-->>ProtectedContract: true/false
alt false
ProtectedContract->>StreamAdmins: retrieveGlobalAdmin(user)
StreamAdmins-->>ProtectedContract: true/false
end
ProtectedContract-->>User: execute or revert
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@claude please review this PR for admin permission security, especially target-scoped function-admin semantics, selector mismatch fixes, owner/root role-management recovery, zero-address handling, event coverage, docs/test traceability, and whether the stated scope boundary around signer lifecycle / pause controls is honest and safe. |
|
@coderabbitai review Please review the latest head for admin authorization correctness, target-scoped selector grants, selector mismatch regressions, zero-address/event coverage, and docs/test traceability. Please also flag any concern from the |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get the PR diff and key source files
gh pr view 63 --repo 6529-Collections/6529Stream --json files,additions,deletions,title,body | jq '.files[].path'Length of output: 877 🏁 Script executed: #!/bin/bash
# List all Solidity and test files changed in this PR
gh pr diff 63 --repo 6529-Collections/6529Stream 2>/dev/null | head -3000Length of output: 126379 🏁 Script executed: #!/bin/bash
# Find all relevant Solidity files
fd -e sol | head -60Length of output: 2118 🏁 Script executed: #!/bin/bash
# Verify all FunctionAdminRequired usages pass address(this) as target
rg -n "retrieveFunctionAdmin" smart-contracts/ --type sol
# Verify no old 2-arg retrieveFunctionAdmin calls remain
rg -n "retrieveFunctionAdmin(msg.sender, [^a]" smart-contracts/ --type sol || echo "None found (expected)"
# Check AuctionContract's full modifier/admin check pattern
rg -n "FunctionAdminRequired\|isAdminRequired\|retrieveFunctionAdmin\|retrieveGlobalAdmin" smart-contracts/AuctionContract.sol
# Check IStreamAdmins interface matches StreamAdmins implementation
cat smart-contracts/IStreamAdmins.sol
# Verify the 3-arg retrieveFunctionAdmin everywhere
rg -n "retrieveFunctionAdmin" smart-contracts/ test/Length of output: 3586 🏁 Script executed: #!/bin/bash
# Check if any FunctionAdminRequired modifier still uses old 2-arg form
rg -n "retrieveFunctionAdmin\(msg\.sender,\s*_selector\)" smart-contracts/ || echo "No old 2-arg calls remain"
# Check selector mismatch fixes are clean
rg -n "changeMetadataView\.selector\|setMerkleRoot\.selector" smart-contracts/ || echo "No old selector mismatches remain in FunctionAdminRequired"
# Count FunctionAdminRequired usages per file
rg -c "FunctionAdminRequired" smart-contracts/
# Check event definitions vs emissions
rg -n "emit GlobalAdminUpdated\|emit FunctionAdminUpdated" smart-contracts/StreamAdmins.sol
# Check _setFunctionAdmin is only called from authorized context
rg -n "_setFunctionAdmin" smart-contracts/StreamAdmins.solLength of output: 1444
✅ Admin Authorization Correctness — CleanAll 9
✅ Selector Mismatch Fixes — CleanAll three documented mismatches are resolved:
The ✅ Zero-Address / Event Coverage — Clean
|
|
@coderabbitai review Latest head |
|
@claude please review latest head |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ops/AUTONOMOUS_RUN.md`:
- Line 38: Update the "Last updated" header timestamp value (the table row
starting with the cell '| Last updated |') to reflect the latest decision-log
entry; replace the existing `2026-06-10 11:52 UTC` timestamp with the newer
timestamp matching the most recent entry (e.g., `2026-06-10 12:13 UTC`) so the
run-state metadata is current.
In `@smart-contracts/StreamMinter.sol`:
- Around line 79-85: In the mint function add explicit, early validation: verify
that _tokenData.length, _saltfun_o.length, and _numberOfTokens.length each equal
_recipients.length and revert with clear error messages if not; additionally,
for each recipient ensure _numberOfTokens[i] > 0 before doing
`_numberOfTokens[i] - 1` (revert with a descriptive message if zero) and guard
all array accesses to avoid out-of-bounds panics. Apply the same validations to
any subsequent loop or batch-processing block that uses the same arrays (the
other block referenced around the same method) so all indexing is safe and
failures are deterministic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9867d39e-7add-4211-a323-a43941f8c500
📒 Files selected for processing (21)
docs/adr/0004-admin-governance.mddocs/known-blockers.mddocs/status.mdops/AUTONOMOUS_RUN.mdops/ROADMAP.mdops/SLITHER_BASELINE.mdsmart-contracts/AuctionContract.solsmart-contracts/DependencyRegistry.solsmart-contracts/IStreamAdmins.solsmart-contracts/RandomizerNXT.solsmart-contracts/RandomizerRNG.solsmart-contracts/RandomizerVRF.solsmart-contracts/StreamAdmins.solsmart-contracts/StreamCore.solsmart-contracts/StreamCuratorsPool.solsmart-contracts/StreamDrops.solsmart-contracts/StreamMinter.soltest/README.mdtest/StreamAdminSelectors.t.soltest/StreamAdmins.t.soltest/StreamCoreAdminCharacterization.t.sol
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get commits on the PR to identify what changed since the previous review
gh pr view 63 --repo 6529-Collections/6529Stream --json commits | jq '.commits[-5:] | .[] | {oid: .oid, message: .messageHeadline}'Length of output: 671 🏁 Script executed: #!/bin/bash
# Get the diff of the latest commit acb1e6d specifically
gh api repos/6529-Collections/6529Stream/commits/acb1e6d --jq '.files[] | {filename: .filename, additions: .additions, deletions: .deletions}' 2>/dev/null || echo "Trying alternate approach"
gh api "repos/6529-Collections/6529Stream/commits/acb1e6d" 2>/dev/null | jq '.files[] | {filename: .filename, patch: .patch}' | head -200Length of output: 11109 🏁 Script executed: #!/bin/bash
# Check the StreamAdmins test file for asymmetry coverage
rg -n "tdhSigner\|signer.*registrar\|registrar.*signer\|asymmetr\|global.*admin\|CanManageRoles\|ImplicitOperational\|no.op\|noop\|vm\.prank" test/ --type sol | head -60Length of output: 510 🏁 Script executed: #!/bin/bash
# Check Slither baseline doc for triage notes
fd "SLITHER" -e md | xargs cat 2>/dev/null || fd -i "slither" -e md | xargs cat 2>/dev/nullLength of output: 16435 🏁 Script executed: #!/bin/bash
# Verify no-op prank removal
rg -n "vm\.prank(address(this))" test/ --type sol | head -20
rg -n "prank" test/StreamAdmins.t.sol | head -30Length of output: 260
✅ Finding 1 — tdhSigner asymmetry: addressed
The constructor comment in ✅ Finding 2 — Slither Low delta: triaged
Net +3 Low findings are fully accounted for. No new production-contract Lows are open. ✅ Observation — No-op
|
|
@coderabbitai review Latest head |
|
@claude please review latest head |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get what files changed in the latest commit
gh api "repos/6529-Collections/6529Stream/commits/acac51aa7d1745ed7f677bd9f6a620ec68c4224a" 2>/dev/null | jq '.files[] | {filename: .filename, additions: .additions, deletions: .deletions, patch: .patch}'Length of output: 8635 🏁 Script executed: #!/bin/bash
# Find and examine the StreamFixture helper to check if collection phases are set up
fd "StreamFixture" -e sol | xargs cat 2>/dev/null || rg -n "deployStream\|collectionPhases\|publicStartTime\|setCollectionPhases" test/ --type sol -A3 | head -80Length of output: 2760
✅
|
| Guard | Position | Covers |
|---|---|---|
| Array length equality (all 4 arrays) | Pre-loop | Mismatched input arrays |
_recipients.length > 0 |
Pre-loop | Empty batch |
_numberOfTokens[y] > 0 |
Inside loop | Per-slot zero-quantity |
The zero-quantity guard fires inside the loop, after the phase check. StreamFixture.deployStream calls setCollectionPhases(1, block.timestamp, block.timestamp + 30 days), so publicStartTime > 0 and block.timestamp >= publicStartTime both hold — the phase check passes and execution correctly reaches the zero-quantity guard. ✅
✅ Exact revert tests — Correct
_assertMintReverts uses keccak256(revertData).assertEq(keccak256(abi.encodeWithSignature("Error(string)", expectedReason)), ...) — a true exact-match on ABI-encoded revert data, not just string comparison. All three scenarios (length mismatch, empty batch, zero quantity) map cleanly to the expected revert string. Both success and revertData are consumed by assertions, so no unused-return Slither delta is expected. ✅
✅ Run-state and counts — Consistent
- Timestamp:
11:52 UTC→12:34 UTC✅ - Test counts: 130 → 133 (3 new
StreamMinterValidationtests) ✅ - Focused pattern updated to include
StreamMinterValidation✅ - Decision log entry appended ✅
✅ Slither — Unchanged at 648
No new production-contract guards introduce new detector hits; the new test file's consumed return values avoid an unused-return delta. ✅
No new concerns. The batch input validation surface is now complete, tested with exact revert assertions, and consistent with the documented 648-finding Slither baseline. Approving.
🐇 ✨
[approve]
|
Merge decision for latest head
|
1 participant
Summary
Closes #34. Refs #33.
This PR implements the P0-ADMIN-001 admin permission fix:
StreamAdminsfunction-admin grants from(account, selector)to(account, target, selector)FunctionAdminRequiredcall site to check permissions againstaddress(this)StreamCore.setCollectionData,StreamCore.updateCollectionInfo, andStreamCuratorsPool.setMultipleMerkleRootsowner()an implicit operational admin on protected protocol contractsfalseStreamMinter.mintbatch guards for length mismatch, empty batch, and zero quantity after review identified panic-prone indexingScope boundary: signer lifecycle operations, pause domains, production deployment ceremony, and richer collection-admin mutation roles remain tracked follow-up work. This PR makes the current protected-function surface selector-accurate and target-scoped.
Tests / Validation
forge test --match-contract "Stream(MinterValidation|Admins|AdminSelectors|CoreAdminCharacterization)Test" -vvv- 22 passing testsmake check- 133 passing testspowershell -NoProfile -ExecutionPolicy Bypass -File scripts\check.ps1- 133 passing testsforge fmt --check smart-contracts\StreamMinter.sol test\StreamMinterValidation.t.solplus prior touched Solidity/test filesgit diff --check.\.venv-tools\Scripts\slither.exe . --config-file slither.config.json --foundry-compile-all --json <temp-file>arbitrary-send-eth: 0StreamAdminshigh/medium findings: 0StreamAdminsmissing-zero-check: 0test/StreamMinterValidation.t.solunused-return: 0Review Notes
The diff includes
forge fmtnormalization in touched legacy Solidity files, so some files show formatting churn around the small semantic authorization changes.Summary by CodeRabbit
Bug Fixes
Tests
Documentation