Skip to content

docs(natspec): document authorizer-trust assumption in call-sites - #323

Open
thedavidmeister wants to merge 1 commit into
mainfrom
2026-06-21-issue-315-authorizer-natspec
Open

docs(natspec): document authorizer-trust assumption in call-sites#323
thedavidmeister wants to merge 1 commit into
mainfrom
2026-06-21-issue-315-authorizer-natspec

Conversation

@thedavidmeister

Copy link
Copy Markdown
Collaborator

Closes #315

Adds @dev notes to four functions in OffchainAssetReceiptVault that
call s.authorizer.authorize(...) (a non-view call on an owner-swappable
external contract):

  • setAuthorizer — documents that every downstream authorize call-site
    assumes the owner installs a non-hostile authorizer; a malicious
    authorizer can brick the vault or perform external mutations outside
    nonReentrant-guarded entrypoints.
  • authorizeReceiptTransfer3 — all receipt accounting and freeze
    checks complete before the authorize call; a re-entrant authorizer
    cannot extract value from guarded entrypoints (deposit/withdraw/
    confiscate*).
  • certify — authorize is called AFTER writing certifiedUntil (CEI
    pattern); a re-entrant authorizer re-reads the already-updated value
    with no state to corrupt.
  • _update — authorize is called BEFORE super._update; reentrancy
    safety rests on the ERC20 underflow guard and allowance requirements
    preventing overspend on re-entry.

NatSpec-only change; no bytecode impact.

Co-Authored-By: Claude noreply@anthropic.com

Adds @dev notes to setAuthorizer, authorizeReceiptTransfer3, certify,
and _update explaining the non-view authorizer call, the CEI/reentrancy
argument at each site, and the remaining governance-trust assumption that
the owner installs a non-hostile authorizer. Addresses the defense-in-depth
residuals from the reentrancy audit documented in #315.

Co-Authored-By: Claude <noreply@anthropic.com>
@thedavidmeister thedavidmeister self-assigned this Jun 21, 2026
@coderabbitai

coderabbitai Bot commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@thedavidmeister, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 42 minutes and 4 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1d0d486d-8584-4ff1-aacb-6786d39c2c37

📥 Commits

Reviewing files that changed from the base of the PR and between 8f68b12 and e2260f0.

📒 Files selected for processing (1)
  • src/concrete/vault/OffchainAssetReceiptVault.sol
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 2026-06-21-issue-315-authorizer-natspec

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@thedavidmeister

Copy link
Copy Markdown
Collaborator Author

Rejecting for an incomplete enumeration: this @dev on setAuthorizer presents the complete set of non-view authorize(...) call-sites, but lists 5 of 7 — it omits confiscateShares and confiscateReceipt, the two value-moving confiscation paths (both carry nonReentrant, which the reentrancy note here references without listing them as call-sites).

The rework is this same PR with the full 7-site enumeration (authorizeReceiptTransfer3, _afterDeposit, _afterWithdraw, certify, _update, confiscateShares, confiscateReceipt) and the confiscate functions' nonReentrant status noted. The other claims verified against source and are worth keeping as written: certify's CEI ordering (certifiedUntil written before the authorize call) and _update's authorize-before-super._update analysis.

@thedavidmeister thedavidmeister added the human:needs-work Human reviewer: needs rework label Jul 6, 2026
@thedavidmeister thedavidmeister added ai:needs-work AI vetter: needs rework (code issue) and removed human:needs-work Human reviewer: needs rework labels Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai:needs-work AI vetter: needs rework (code issue)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reentrancy audit residuals: ETH-refund outside guard + non-view authorizer trust (defence-in-depth)

1 participant