feat(EXSC-408): S11 — access-control interfaces + reference adapter#1981
feat(EXSC-408): S11 — access-control interfaces + reference adapter#1981gvladika wants to merge 35 commits into
Conversation
…system constraints) Encode the Diamond/Vault-Wrapper product boundary in the agent harness: - 002-architecture.md (always-apply): add [CONV:ARCH-VAULTWRAPPER] so the boundary fires unconditionally every session, correcting the rule's Diamond-only framing. - 108-vault-wrapper.md (new, scoped): operational constraints that activate on src/VaultWrapper/**, test/solidity/VaultWrapper/**, and the vault-wrapper deploy scripts. The deploy-script glob makes the rule fire at the decision point where the load-bearing script/deploy/facets/ location matters. Product boundary lives in always-apply 002; operational detail in the scoped rule (no duplication). Volatile state stays in Linear. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ultWrapper/ Reflect the S8 decision (#1936) to move the vault-wrapper deploy script out of the Diamond-era script/deploy/facets/ lookup dir into its own folder. Flip the [CONV:VW-DEPLOY-DIR] guidance: scripts are standalone, run by explicit path, and deliberately decoupled from the Diamond deploy tooling — not kept in facets/. Update the activation glob from facets/ to script/deploy/vaultWrapper/**. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add LiFiVaultWrapper, the Solady ERC-4626 beacon implementation behind the factory: write-once initialize, virtual-share inflation mitigation, reentrancy guards, and deposit/withdraw routing to the underlying yield source via an IYieldAdapter delegatecall (the read path stays a plain view call). - Extend IYieldAdapter with deposit/withdraw (delegatecall-only, stateless) and totalAssets(underlying, holder); implement them in ERC4626Adapter. - Capture the deploying factory write-once for the later global circuit breaker. - Replace the temporary MockVaultWrapper: the factory, beacon, and timelock tests now exercise the real implementation; only the underlying is mocked. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…FiVaultWrapper.sol:initialize) CodeRabbit: initialize had no event for off-chain indexing of an instance's config. Emit Initialized(asset, underlying, adapter, vaultWrapperAdmin, factory, integratorShareBps) and cover it with a vm.expectEmit test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…pper.sol:feeRate) CodeRabbit: feeRate/feeEnabled indexed the [4] arrays by _feeType with only Solidity's implicit out-of-bounds panic. Add an explicit InvalidFeeType(uint8) revert making the valid range (0-3) explicit, with a revert test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Each instance now reports a distinct ERC20 name/symbol composed from the
underlying asset symbol ("LI.FI Earn USDC" / "lfUSDC") instead of a shared
generic string, so wallets and explorers can tell instances apart. Reads the
symbol via Solady MetadataReaderLib (handles string/bytes32/missing) and falls
back to "VW" when the asset exposes no symbol.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Wire the upcoming-feature call sites now so follow-up tickets only fill in bodies. Adds a _beforeOperation() guard (pause -> access -> fee accrual) at the top of deposit/mint/withdraw/redeem, overrides the four preview functions to route through _entryFee/_exitFee, and skims/routes fees in the deposit/withdraw hooks via _routeFee. All stubs are no-ops (fees return 0, guards pass), so behaviour is unchanged and the suite stays green; bodies land in the fee (S2), access (S4), and pause (S5) tickets. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the Solady ERC4626 base with OpenZeppelin's ERC4626Upgradeable (v4.9.2, matching the vendored openzeppelin-contracts). Source-verified survey of major vault protocols showed no production use of Solady's ERC4626 and no documented standalone audit of it, whereas OZ's base has broad precedent (MetaMorpho, Gearbox, Ethena) — and Kiln DeFi, the closest comparable (integrator-facing beacon-proxy vaults with delegatecall connectors + per-vault fees), is built on ERC4626Upgradeable. Changes: - Add openzeppelin-contracts-upgradeable submodule (v4.9.2) + remappings; switch the bare @openzeppelin/ remapping to explicit @openzeppelin/contracts/ and @openzeppelin/contracts-upgradeable/ prefixes to avoid the auto-detect clash. - asset/decimals/name/symbol now live in OZ proxy storage via __ERC4626_init / __ERC20_init (per-instance name/symbol still derived from the asset symbol). - initialize uses OZ's initializer guard; adapter routing moves into _deposit / _withdraw overrides (OZ has no after/before hooks); reentrancy via ReentrancyGuardUpgradeable. - Unchanged: delegatecall IYieldAdapter, no-op fee/pause/access seams, fee-adjusted preview overrides, factory and ILiFiVaultWrapper interface. Inflation protection now relies on OZ's virtual-share offset (default +1). All 92 vault-wrapper tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…:278) CR (minor): a delegatecalled adapter runs in this wrapper's storage context, so "stateless, cannot touch storage" is misleading — statelessness is an audit/governance invariant, not enforced by this contract. Reworded accordingly.
…pper.sol:29) CR (major): the proxy initializer is guarded but the implementation itself could be initialized directly. Add a constructor calling _disableInitializers() (OZ best practice, matches Kiln). Rework the unit-test harness to deploy the impl behind an UpgradeableBeacon + BeaconProxy so initialize is still exercised end-to-end through a proxy.
CR (major): a single-shot initializer should not rely solely on upstream factory validation. Reject zero asset/underlying/adapter/admin, integratorShareBps over 10000, and an adapter that resolves the underlying to an asset other than the one passed (defense-in-depth against bricking/mispricing). Add testRevert_ coverage for each path. Redundant in the factory flow but cheap insurance on a fund-custody contract.
CR (major): the prior override pulled from the yield source before super._withdraw spent the allowance and burned shares. Inline OZ's sequence so checks/effects (spend allowance, burn) precede the adapter interaction and the asset transfer, satisfying Checks-Effects-Interactions instead of relying solely on nonReentrant.
# Conflicts: # script/deploy/vaultWrapper/DeployLiFiVaultWrapperFactory.s.sol # src/VaultWrapper/adapters/ERC4626Adapter.sol # src/VaultWrapper/mocks/MockVaultWrapper.sol
Move all VaultWrapper code (wrapper, adapter, deploy script, tests) onto OpenZeppelin v5.6.1 while the Diamond stays on the vendored OZ v4.9.2. - Vendor a second OZ core submodule (lib/openzeppelin-contracts-v5 @ v5.6.1) because v5's upgradeable package imports the non-upgradeable core as a peer. - Bump lib/openzeppelin-contracts-upgradeable v4.9.2 -> v5.6.1. - Route VaultWrapper src/test/script and the upgradeable lib to v5 core via path-scoped remappings; the global @openzeppelin/contracts/ stays v4.9.2. - LiFiVaultWrapper: ReentrancyGuardUpgradeable -> core ReentrancyGuard (v5 is proxy-safe, no __init), IERC20Upgradeable/SafeERC20Upgradeable -> IERC20/ SafeERC20; extract _initErc4626Metadata to avoid v5 stack-too-deep. - Deploy script: v5 UpgradeableBeacon(impl, initialOwner) sets the timelock as owner at construction. - Tests: v5 custom errors (InvalidInitialization, ReentrancyGuardReentrantCall, OwnableUnauthorizedAccount, BeaconInvalidImplementation, TimelockUnexpectedOperationState) and DEFAULT_ADMIN_ROLE. 97 VaultWrapper tests pass; full repo build green (Diamond unaffected). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
LiFiVaultWrapper and the deploy script are unreleased; revert the @Custom:version churn (2.0.0 -> 1.0.0, 1.3.0 -> 1.2.0) so this PR introduces no version change against its base. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add [CONV:VW-OZ-VERSION] (dual-OZ-version split + path-scoped remappings; the high-value, silent-failure constraint) and note v5's two-arg UpgradeableBeacon constructor in [CONV:VW-BEACON]. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace Solady LibClone beacon-proxy deployment with OZ BeaconProxy + Create2 so the VaultWrapper proxy layer is single-vendor OZ. A shared _proxyInitCode() keeps deploy and predictAddress hashing identical init code; salt reuse now reverts with OZ Errors.FailedDeployment. Solady remains only for MetadataReaderLib (no OZ equivalent). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
S11. IVaultAccessControl (isAllowed/isDenied) and ISanctionsOracle (Chainalysis-compatible isSanctioned) interfaces, plus a reference external access adapter combining an owner-managed allowlist/denylist with an optional sanctions oracle. Integrator template; in-wrapper Merkle/mapping backends remain S4. Named IVaultAccessControl (not IAccessControl) to avoid colliding with OZ v5's role-based IAccessControl vendored in this subsystem. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adversarial review gaps: cover batch setters' onlyOwner guard, the toggle-off (false) direction for allow/deny, per-element batch event emission, empty-array batch no-op, and the allowlist-disable reopen path. Add blank lines after vm.expectRevert per [CONV:BLANKLINES]. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Two hardening fixes flagged by review of the S1 core: 1. Adapter return values were ignored and ERC4626Adapter echoed its input, so a fee-charging or short-paying yield source could silently dilute minted shares or let a withdrawal pull from the wrapper's buffer. The adapter now returns the actual asset balance delta, _routeThroughAdapter forwards it, and _deposit/_withdraw revert with AdapterDepositShortfall / AdapterWithdrawShortfall when the yield source moves less than requested. 2. Add a reserved __gap to the upgradeable beacon implementation so future versions can append wrapper state without shifting derived-module storage (append-only, no-reorder upgrade invariant documented inline). Tests: a configurable LossyVault proves both shortfall guards fire; the 58/99 existing suites stay green (standard vaults move exact amounts). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address the review point that the balance-delta deposit measurement does not catch share-side dilution (a vault that consumes the full asset but credits fewer shares). Rather than switch to convertToAssets(shareDelta) — which rounds below the requested amount on any non-1:1 vault and would revert healthy deposits — keep the rounding-safe delta guard (which catches short-accepting sources) and document that internal-deposit-fee / fee-on-transfer sources are unsupported and need a dedicated adapter. Makes the previously-silent 1:1 assumption explicit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…into feature/exsc-408-s11-access-interfaces-reference-adapter
…1.0.0 Review follow-ups on the S1 PR: - ERC4626Adapter: replace Solady SafeTransferLib.safeApproveWithRetry with OZ SafeERC20.forceApprove so the wrapper subsystem stays single-vendor OZ. - Use named arguments on all IERC4626 calls (deposit/withdraw/convertToAssets, balanceOf). - Reset in-development @Custom:version tags to 1.0.0 (adapter, IYieldAdapter, deploy script). - 108-vault-wrapper rule: drop the stale CWIA parenthetical and the historical "do not reintroduce Solady LibClone" note. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…o (LiFiVaultWrapperFactory.sol:174,272)
CodeRabbit (major): the split validation allowed integratorShareBps == 100%,
leaving LI.FI 0% — violating rule 108-vault-wrapper line 72 ("the integrator/LI.FI
split is validated < 100% only"). Both the setDefaultSplit and deploy paths used
`> BPS_DENOMINATOR`; tightened to `>=` and aligned the NatSpec. Added boundary
tests asserting exactly 100% reverts and 99.99% is accepted.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…into feature/exsc-408-s11-access-interfaces-reference-adapter
Code-review follow-ups on the S1 PR: - initialize: rejected only > 100% while the factory (and rule 108) enforce < 100%; aligned the wrapper's defense-in-depth check to >= 10_000 and assert exactly 100% reverts. - _withdraw: stop reimplementing OZ's withdraw sequence; override only the _transferOut seam so OZ keeps the allowance spend, share burn, and Withdraw event. Behaviour is unchanged (redeem assets+fee, revert on shortfall before paying the receiver, skim fee, transfer assets) and it removes the asymmetry with _deposit plus the risk of drifting from OZ on upgrade. - cache asset() in a local across the two reads in the withdraw path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- initialize no longer takes _asset: it derives the asset from _underlying via the adapter's resolveAsset, so the stored asset cannot disagree with what the adapter reports. Drops the redundant param and the now-impossible AssetMismatch error/check; the factory still resolves the asset for its WrapperDeployed event. - vaultWrapperAdmin is now transferable via a two-step (propose/accept) flow, mirroring Ownable2Step, so the per-vault controller can be rotated without a redeploy and a mistyped address cannot strand the role. Reserved storage gap reduced 50 -> 49 to absorb the new pendingVaultWrapperAdmin slot. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…into feature/exsc-408-s11-access-interfaces-reference-adapter
…LiFiVaultWrapper.sol:147) CodeRabbit (major): removing _asset dropped the implicit zero-check the old AssetMismatch validation provided. The reference adapter reverts on a zero asset, but the wrapper should not trust an arbitrary governance-approved adapter. Revert ZeroAddress when resolveAsset returns address(0); test via MockZeroAdapter. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…into feature/exsc-408-s11-access-interfaces-reference-adapter
…trancyGuard OZ v5.6.1 ships no ReentrancyGuardUpgradeable and ReentrancyGuardTransient requires EIP-1153 on every target chain. Document that the plain guard is proxy-safe here (the check treats the proxy's zero-initialized slot as NOT_ENTERED) so the recurring review flag has a standing answer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…into feature/exsc-408-s11-access-interfaces-reference-adapter
The earlier note claimed the guard occupies sequential slot 0; that is wrong. OZ v5's ReentrancyGuard (v5.5.0+) stores its status in a fixed ERC-7201 namespaced slot via StorageSlot and is @Custom:stateless, so it occupies no sequential slot (the wrapper's own `underlying` is slot 0) and is collision-free behind a beacon proxy — which is why OZ retired the Upgradeable variant. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…into feature/exsc-408-s11-access-interfaces-reference-adapter
🤖 GitHub Action: Security Alerts Review 🔍🟢 Dismissed Security Alerts with Comments 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: ✅ No unresolved security alerts! 🎉 |
3 participants
Which Linear task belongs to this PR?
EXSC-408 — S11: Access interfaces + reference adapter
Stacked PR. Base is
feature/exsc-409-s1-core-erc-4626-immutable-args-cwia(#1971, S1). Review the incremental diff against the base; do not merge before S1 lands. First link in the stack S1 → S11 → S5 → S3.Why did I implement it this way?
S11 is foundational and dependency-free: it ships only the external access-control surface (interfaces + a reference adapter) that S4's in-wrapper backends will import. It does not touch the wrapper —
_checkAccessstays a no-op seam until S4.IVaultAccessControl(isAllowed/isDenied), notIAccessControl. The ticket names itIAccessControl, but this subsystem now vendors OZ v5, whose@openzeppelin/contracts/access/IAccessControl.sol(role-based:hasRole/grantRole) would hard-clash on the identifier the moment any S4 file imports both. Renamed to avoid that; the methods (isAllowed/isDenied) are unrelated to OZ's role model anyway. Two independent predicates so one adapter serves allowlist, denylist, or both modes.ISanctionsOraclematches Chainalysis verbatim.isSanctioned(address) → boolis the exactSanctionsListsignature, so a live Chainalysis oracle drops in as the screening source with no shim.ReferenceAccessControlis a template, not a LI.FI-operated contract. Each integrator deploys/forks its own.TransferrableOwnership-gated allowlist + denylist + optional sanctions oracle. Policy: denied = denylisted OR sanctioned; allowed =!denied && (!allowlistEnabled || allowlisted). Single + batch setters; the oracle is called directly (no try/catch) becauseISanctionsOraclecontractually must not revert — a flaky oracle is the integrator's adapter choice, documented in NatSpec. Holds no funds.Checklist before requesting a review
/pr-ready(local CodeRabbit) on this branch and resolved (or explicitly documented) all findings — see.agents/commands/pr-ready.mdChecklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)