Quillon Vaults

Share minting uses assets * totalSupply / totalAssets(), rounding down, with no virtual offset. When the vault is empty, the exchange rate is defined entirely by whatever the first depositor does.

An attacker deposits 1 wei and receives one share. They then transfer a large amount of the asset directly to the vault without calling deposit. totalAssets() now includes the donation while totalSupply is still 1, so a single share is worth the entire balance.

Any later deposit smaller than the donation computes assets * 1 / (donation + 1), which rounds down to zero shares. The deposit is accepted, the depositor receives nothing, and the assets accrue to the attacker’s single share.

Total loss of any deposit made while the attacker controls the inflated share. Losses are bounded only by the size of victim deposits, and the pattern can be repeated whenever the vault returns to an empty state.

Introduce a virtual share and asset offset in the conversion functions (for example a decimals offset as in OpenZeppelin’s ERC4626 implementation), or mint dead shares to an unrecoverable address at initialisation. In addition, revert any deposit that would mint zero shares and any mint that would require zero assets.

Fixed with a decimals offset of 6 and a zero-share revert on deposit and mint. Verified in the remediation commit; INV-03 added to the invariant suite.

estimatedTotalAssets() values the LP position by reading the pool’s current reserves and computing the position’s share of each side at the spot ratio. The vault includes this estimate in totalAssets(), which drives both minting and redemption.

Reserves can be moved arbitrarily within one transaction using borrowed capital. Because the valuation reads reserves at the moment of the call, an attacker can skew the ratio, interact with the vault at the distorted price, and restore the pool before the transaction ends.

The LP token itself is not priced naively (the strategy decomposes it into underlying amounts), but both amounts are derived from the same manipulable reserves, so the decomposition does not help.

Value extraction from depositors in a single transaction, repeatable while the strategy is active. Profitability depends on pool depth and the strategy’s share of vault assets; at the reviewed configuration the strategy held about 60% of assets.

Value positions with a manipulation-resistant reference price: a time-weighted average price over a window long enough that sustaining a distortion is uneconomic, or an external oracle with deviation checks. Additionally compare spot to the reference and revert deposits and withdrawals when the deviation exceeds a bound.

Valuation now uses a 30-minute TWAP with a 2% spot deviation guard applied to deposits and withdrawals. INV-02 holds under fuzzing with flash-loan sized reserve changes.

setFeeRecipient(address) is declared external without the onlyGovernance modifier applied to every other setter in the contract. The function writes the address that receives performance and management fees on each harvest.

The omission appears to have been introduced when the function was moved from the vault into StrategyManager: the vault version was restricted, the moved version was not. No test covered calling the setter from an unprivileged account.

Loss of all protocol fees from the moment of the change until it is reverted. Depositor principal is not directly affected, but the timelock delay means the protocol cannot react quickly.

Restrict the function to the timelocked governance role, emit an event on change, and add a negative test for every privileged function.

Modifier added; FeeRecipientUpdated emitted; negative tests added for all governance setters.

claim(requestId) checks that the request is ready, transfers the assets to the owner, and only then sets requests[requestId].claimed = true. The function has no reentrancy guard.

For a standard ERC-20 the transfer does not call back into the recipient, so the ordering is harmless. If the vault is deployed with an asset that invokes hooks on transfer (ERC-777 style or tokens with receive callbacks), the recipient regains control before the claimed flag is written and can call claim again for the same request.

Multiple payouts for one request, draining assets reserved for other queued withdrawals. Standard ERC-20 deployments are not affected, which keeps likelihood low, but the vault is designed to support arbitrary assets.

Follow checks-effects-interactions: mark the request claimed before transferring, and add a reentrancy guard to claim and claimMany.

State update moved before the transfer; nonReentrant added to claim and claimMany. INV-04 holds under fuzzing with a hook-enabled test token.

EIP-4626 requires that rounding always favours the vault: previewWithdraw and previewMint must round up, previewDeposit and previewRedeem must round down. The implementation rounds down in previewWithdraw, so a withdrawal of a given asset amount burns fewer shares than it should.

The difference is at most one share unit per call, but it is systematic and in one direction. An account that repeatedly withdraws small amounts extracts slightly more value than its shares represent.

Gradual value leakage from remaining depositors. Negligible per call at current gas prices, but it violates the standard integrators rely on.

Use Math.mulDiv with Rounding.Ceil for share calculations on withdraw and mint paths, and add invariant tests for all four conversion paths.

Rounding direction corrected; INV-01 added for all four conversion paths.

The vault implementation behind the UUPS proxy does not call _disableInitializers() in its constructor. The proxy was initialised correctly at deployment, but the implementation contract itself still has uninitialised storage and a callable initialize.

An attacker can call initialize on the implementation address and become its owner and upgrader. In UUPS, the upgrade logic lives in the implementation, so the attacker can then call upgradeToAndCall on the implementation itself and execute arbitrary code in its context.

Proxy storage is unaffected, but corrupting or destroying the implementation can make the proxy unusable until governance deploys and upgrades to a new implementation. Since EIP-6780 limits SELFDESTRUCT, the practical impact today is lower, which is why the finding is rated medium.

Call _disableInitializers() in the implementation constructor and include an initializer test in the deployment script.

Constructor added; deployment script asserts the implementation reports initialised.

VaultBase declares state variables and is inherited by QuillonVault, which declares its own variables afterwards. VaultBase reserves no storage gap and does not use namespaced storage.

If a future upgrade adds a variable to VaultBase, every variable in QuillonVault shifts by one slot and reads values that belonged to a different variable. Storage-layout tooling would catch this only if it is run on every upgrade.

No current impact. Raises the risk of storage collisions in future upgrades, which could corrupt accounting irrecoverably.

Reserve a __gap array in each upgradeable base, or migrate to ERC-7201 namespaced storage before the next upgrade, and run a storage-layout diff in CI.

Acknowledged. The team will migrate to ERC-7201 namespaced storage in the next major version and has added a storage-layout check to CI.

updateDelay(uint256) accepts any value, including zero. The function is itself timelocked, so a reduction takes one full delay to apply, but once applied every later change, including implementation upgrades, executes immediately.

The timelock is the only control that gives depositors time to exit before a harmful upgrade. Allowing it to reach zero removes that guarantee for the remainder of the protocol’s life.

Removes users’ ability to react to governance actions. Combined with key compromise, enables an immediate drain.

Enforce a minimum delay in the contract (we recommended 48 hours) and alert on any queued delay change.

A 24-hour floor was added. The team considers 24 hours sufficient for its user base; our recommendation of 48 hours stands and the residual risk is documented.

Several setters change risk parameters (strategy debt ratios, deviation bounds, keeper address) without emitting events. Off-chain monitoring has to diff storage to notice changes.

No direct impact. Reduces the effectiveness of monitoring and incident response.

Emit an event with old and new values for every privileged state change.

Events added for all setters.

Contracts declare pragma solidity ^0.8.20 and import libraries that are no longer used. The tested compiler version may differ from the deployed one.

No direct impact. Compiler drift between testing and deployment.

Pin the compiler version and remove unused imports.

Pinned to 0.8.24; unused imports removed.