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

[thedavidmeister]

Context

A deep, line-by-line reentrancy audit of the whole vault/receipt/authorizer/oracle surface (done alongside the #309 fix, PR #313) found the system reentrancy-safe: every value-moving flow funnels through _deposit/_withdraw, both nonReentrant, so the shared guard spans the ERC1155 acceptance callback (the #309 surface) and the underlying-asset transfer; withdraw is textbook CEI; burns fire no recipient callback; and the #309 operator-leak is closed everywhere.

No change is required for #309. This issue tracks the residuals — all currently non-exploitable, defence-in-depth or governance-trust items — so they aren't lost.

Residuals

1. ETH-refund sendValue sits OUTSIDE the nonReentrant guard

ReceiptVault.sol:220, 237, 301, 326, 348 — deposit/mint/previewMint/previewDeposit/convertToShares end with Address.sendValue(_msgSender(), address(this).balance) (raw call, all gas) after _deposit has returned and released the guard.

• Benign today: it is the last statement, all state is settled, and a re-entrant deposit/mint is a fresh, independent call that re-reads the oracle (no carried-over price/state to corrupt).
• It is, however, the one attacker-controllable raw-call reentry point outside nonReentrant.
• Suggested hardening (optional): refund via a pull pattern, or move the refund inside the guarded region, or assert address(this).balance == 0 invariants where applicable.

2. Authorizer-reachable paths not individually nonReentrant

OffchainAssetReceiptVault.sol:391 (authorizeReceiptTransfer3), :607 (certify), :622 (share _update/transfer) call s.authorizer.authorize(...), which is non-view (IAuthorizeV1.authorize, interface L63) and the authorizer is owner-swappable (setAuthorizer, L375). These paths are not wrapped in the shared nonReentrant guard.

• Safe today: in each case effects are committed before the authorize call (certify writes certifiedUntil first), or ERC20 accounting (underflow on super._update) prevents overspend, so a re-entrant authorizer extracts no value; and the value-moving entrypoints it could re-enter (deposit/withdraw/confiscate*) are themselves guarded and revert.
• The guarantee ultimately rests on the owner not installing a hostile/buggy authorizer — a governance-trust assumption.
• Suggested: document the authorizer-trust assumption explicitly in NatSpec, and consider whether certify/share-_update/authorizeReceiptTransfer3 warrant the nonReentrant guard as defence-in-depth.

3. Minor

• _setAuthorizer (:364) staticcalls IERC165(newAuthorizer).supportsInterface(...) on an owner-supplied address — onlyOwner, not exploitable beyond the owner's authority.
• The oracle price read (ERC20PriceOracleReceiptVault.sol:141 _nextId → priceOracle.price{value:…}()) runs before the _deposit guard. Benign for reentrancy (fresh re-read per call); the real concern there is oracle correctness/trust, already documented in the contract header.

Notes

• None of these block or require changes for the H01 - ERC1155 Acceptance Callback Enables Persistent Operator Approval Via _msgSender Spoofing #309 fix (PR fix #309: _msgSender spoofing via ERC1155 acceptance callbacks #313).
• Full call-site table available from the audit; can be pasted in on request.

🤖 Generated with Claude Code