6.1 Pending assets are not reset when moving to a new era, allowing claims on the new pool
The CoveredMetavault utilizes a concept of "claimable eras" to manage the lifecycle of the deposit pool. When the totalClaimableAssets of the vault are fully drained (e.g., due to premium streaming), the claimableEra increments, effectively marking the previous pool as dead.
The _syncEpoch(...) function is responsible for updating a controller's state to match the current vault state. It handles two main transitions:
- Era Sync: If the controller's
claimableEra is behind the global era, it resets the controller's claimableUnits to zero and updates the era; - Epoch Sync: If the controller's
lastSyncedEpoch is behind the current epoch, it processes any pendingAssets by calculating the corresponding unit assignment and adding them to claimableUnits;
The issue is that pendingAssets represents deposits that were requested in a previous epoch and settled into the global pool at that time. If the global claimableEra has incremented since the user's last sync, it implies that the pool—to which those pendingAssets were contributed—has been fully exhausted.
However, the logic in _syncEpoch clears claimableUnits upon an era change but does not clear pendingAssets. The execution flows immediately into the epoch sync block, where it converts these stale pendingAssets (using the historical epochAllocations) into units and adds them to the controller's claimableUnits.
Since the controller's era was just updated to the *new* era, these units are now valid claims against the *new* era's pool. This effectively allows a user to carry over a claim from a drained/dead pool into a fresh pool, potentially diluting new depositors or stealing assets they did not contribute to.
Recommendation
Consider resetting pendingAssets to zero and updating the lastSyncedEpoch when a controller is detected to be in an outdated era. This ensures that claims tied to a fully drained pool are correctly discarded.
Resolution
Fixed in commit b34107. Resolved by pre-minting metavault shares during settlement, simplifying vault accounting and eliminating the need for eras.
6.2 Users may be restricted from withdrawing their full remaining balance if it is below the minimum request amount
The CoveredMetavault enforces a minimum asset amount ( minimumRequestAssets) for asynchronous redemption requests initiated via requestRedeem(...).
This check ensures that the resulting underlying asset value from the requested shares meets the minimum threshold.
The constraint is enforced within requestRedeem(...) before the redemption request is recorded:
function requestRedeem(uint256 shares, address controller, address owner)
external
override
// ...
returns (uint256 requestId)
{
require(shares != 0, ZeroShares());
require(controller == owner, InvalidController(controller));
require(owner == msg.sender, InvalidOwner(owner));
uint256 redeemAssetAmount = _convertToAssets(shares, Math.Rounding.Floor);
_requireMinimumRequestAssets(redeemAssetAmount); // The minimum request assets check is enforced here.
// ...
}
If a user's total remaining share balance corresponds to an underlying asset value that is less than minimumRequestAssets, they will be blocked from initiating a withdrawal using requestRedeem(...) because the check in _requireMinimumRequestAssets(...) will revert.
While the assets are not permanently locked, this mechanism hinders a user's ability to fully exit the position if their final balance is low.
Recommendation
Consider modifying the minimum request asset requirement to allow requests that are either greater than or equal to the configured minimum, OR requests that represent the user's entire remaining balance. This maintains the protection against "dust" operations while guaranteeing that users can always fully exit their position.
Resolution
Fixed in commit b5a250. Removed this restriction for redemptions, allowing users to always redeem their full remaining balance. Minimums are now enforced by the offchain settlement engine that operates the keeper, while users can still settle and claim redemptions permissionlessly via settleMaturedRedemption.
6.3 Floor rounding in premium calculation causes the claimable pool to overpay premiums
The _streamPremium() function calculates and transfers a premium fee based on the total settled assets of the vault. Since the vault's assets are logically partitioned into "backed assets" and "claimable assets", the premium payment must be proportionally deducted from both pools.
When updating the totalClaimableAssets to reflect the deduction, the contract calculates the new balance using the ratio of assets remaining after the premium payment:
// src/CoveredMetavault.sol
// ...
if (claimableAssetsTotal != 0) {
uint256 newClaimableAssetTotal =
claimableAssetsTotal.mulDiv(assetsAfter, assetsBefore, Math.Rounding.Floor); // @audit-issue Rounding down the remaining assets
// ...
_vaultStorage.totalClaimableAssets = newClaimableAssetTotal;
// ...
By using Math.Rounding.Floor to calculate newClaimableAssetTotal, the contract rounds down the remaining assets in the claimable pool. This effectively means the reduction applied to the claimable pool (the premium paid by it) is rounded *up*.
Consequently, the claimable pool bears the burden of the rounding error, paying slightly more premium than the backed assets pool. Over time, this slight overpayment creates a disadvantage for users whose assets are sitting in the claimable pool waiting to be claimed, as their claimable units will correspond to a slightly smaller total asset amount than mathematically precise.
Recommendation
The current implementation places the burden of precision loss on the claimable pool. While switching to Math.Rounding.Ceil would shift this burden to the backed pool, the current design systematically favors the backed assets.
Consider evaluating this trade-off. If the intent is to protect the backed pool, no change is needed, but consider documenting this behavior to incentivize users to finalize claims.
Resolution
Fixed in commit b34107. Resolved by pre-minting shares at settlement so that premiums are applied via the share price only, eliminating the need to scale the claimable pool.
6.4 Premium streaming can be enabled prematurely before the first settlement
The CoveredMetavault uses the lastPremiumTimestamp state variable to track when the premium was last streamed. A value of 0 is intended to serve as a sentinel indicating that the vault has not yet undergone its first settlement, ensuring that no premium is accrued or charged for the period preceding the first epoch. This logic is enforced in the _streamPremium function, which checks that lastPremiumTimestamp != 0 before calculating the premium.
However, the lastPremiumTimestamp variable can be set to a non-zero value before the first settlement in two scenarios:
- During Initialization: The
initialize(...) function explicitly sets lastPremiumTimestamp to block.timestamp if the initial premiumRateBps is non-zero; - Updating Premium Rate: The
setPremiumRateBps(...) function calls _streamPremium() to settle accrued premiums before updating the rate. The _streamPremium() function unconditionally updates lastPremiumTimestamp to the current timestamp at the end of execution, even if the calculation was skipped because the previous timestamp was 0;
In both cases, lastPremiumTimestamp becomes non-zero before the first settle() call. As a result, when the first settlement eventually occurs, the vault will calculate and charge premiums for the duration elapsed since initialization or the rate update. Note that is not expected for the vault to hold funds at this time.
Recommendation
Consider modifying the initialize(...) function to ensure lastPremiumTimestamp remains 0, even if a premium rate is set. Additionally, update the setPremiumRateBps(...) function to check if lastPremiumTimestamp is 0; if so, it can skip the _streamPremium() call and simply update the rate, preserving the sentinel value until the first settlement.
Resolution
Fixed in commit 3fc8d3. Resolved by removing the lastPremiumTimestamp assignment from the initialiser and guarding premium streaming in setPremiumRateBps to preserve the sentinel value until the first settlement.
6.5 The requestRedeem function uses an external call to transfer vault's shares
The CoveredMetavault contract functions has the requestRedeem function to return shares to the vault in exchange for assets. The function explicitly validates that the caller is the owner of the shares or has the appropriate permissions.
However, the contract uses an external call to transfer the shares from the user to the contract itself.
function requestRedeem(uint256 shares, address controller, address owner) external {
// ...
if (msg.sender != owner) {
revert InvalidOwner(owner);
}
// ...
// @audit-issue This requires prior approval and unnecessarily increases gas costs.
IERC20(address(this)).safeTransferFrom(owner, address(this), shares);
// ...
}
By using safeTransferFrom on address(this), the contract triggers the standard ERC20 allowance check. This creates a circular dependency where the user must approve the vault contract to spend the vault's own tokens, even though the user is the one initiating the transaction.
This leads to a degraded User Experience as the user must sign and broadcast two transactions (approve and redeem) instead of one. Additionally, it increases the gas cost due to the external CALL opcode and the storage operations required for the allowance check.
Recommendation
Consider replacing the external safeTransferFrom call with the internal _transfer function inherited from the ERC20Upgradeable contract. This will bypass the redundant allowance check and allow users to redeem their shares in a single transaction.
Resolution
Fixed in commit 915324. Resolved by replacing the external safeTransferFrom with an internal _transfer call when locking vault shares in requestRedeem.
6.6 Unfair unit distribution in _computeUnitAssignment favors early claimers
The _computeUnitAssignment function determines the share of claimable units a controller receives based on their pending assets. The algorithm calculates maxAssignableUnits by determining the minimum number of units that must be reserved for the remaining unassignedAssets.
However, the function implements an "optimistic" rounding mechanism. If the standard pro-rata calculation ( unitsToAssign) is less than maxAssignableUnits and the division results in a remainder, the function explicitly increments unitsToAssign by 1.
// ...
// Start from floor and clamp for extra safety.
unitsToAssign = pendingAssets.mulDiv(epochTotalUnits, epochTotalAssets, Math.Rounding.Floor);
if (unitsToAssign > maxAssignableUnits) unitsToAssign = maxAssignableUnits;
// @audit-issue This logic rounds up the assignment, consuming units meant for later users.
// Grant at most +1 unit if it won't breach the remaining units limit.
if (unitsToAssign < maxAssignableUnits) {
uint256 remainderNumerator = mulmod(pendingAssets, epochTotalUnits, epochTotalAssets);
if (remainderNumerator != 0) {
unitsToAssign += 1;
if (unitsToAssign > maxAssignableUnits) unitsToAssign = maxAssignableUnits;
}
}
}
This logic systematically favors users who sync their state (claim) earlier in the epoch. By rounding up their unit allocation, early claimers extract more value from the epochTotalUnits pool than their exact pro-rata share. Since the total number of units is fixed, this over-allocation reduces the unassignedUnits available for subsequent users. Consequently, users who claim later will inevitably receive fewer units than their fair share to balance the epoch, resulting in an unfair distribution based on claim order.
Recommendation
Consider removing the conditional block that increments unitsToAssign by 1. Relying on standard floor rounding ( Math.Rounding.Floor) for all unit assignments ensures that every user receives a consistent and fair share of the units relative to their assets, regardless of the order in which they sync.
Resolution
Fixed in commit b34107. Replaced _computeUnitAssignment with a simpler, per-epoch, floor-based share allocation that removes order-dependent bias.
6.7 claimableRedeemRequest returns non-zero for already claimed request
claimableRedeemRequest is a view function that returns the number of shares that can be claimed, if the redeem request is already settled. But, this number is valid only until the actual claim is processed. After the shares are already claimed via redeem, the request will not have claimable shares as they were already processed.
function claimableRedeemRequest(uint256 requestId, address controller)
external
view
override
returns (uint256 claimableShares)
{
RedeemRequestStorage storage redeemRequestStorage = _vaultStorage.redeemRequests[requestId];
if (redeemRequestStorage.controller != controller || redeemRequestStorage.timestamp == 0) return 0;
// Audit - this is valid only under the shares are not claimed by the user. Post redeem call, this function should return 0 shares considering the shares were already claimed.
==> return redeemRequestStorage.settled ? redeemRequestStorage.shares : 0;
}
Recommendation
Consider documenting this behavior for integrators and users that can use this feature.
Resolution
Documented the current behaviour in both NatSpec comments and the integrator guides.
6.8 Single-step ownership transfer can lead to permanent loss of ownership
The transferOwnership() function from the Vault contract is used to transfer control of the contract to a new address. The function validates that the newOwner is not the zero address, revokes the owner role from the caller, and grants the owner role to the newOwner address.
However, the function performs this transfer in a single step. It does not verify that the newOwner account is accessible or correct beyond the zero address check.
This means that if the current owner accidentally passes an incorrect address, the ownership of the vault will be permanently lost. Since the current owner revokes their own role immediately during the call, there is no way to recover access.
function transferOwnership(address newOwner) external {
// ...
require(newOwner != address(0), "Invalid address");
_revokeRole(OWNER_ROLE, msg.sender);
// @audit If newOwner is incorrect, ownership is lost forever.
_grantRole(OWNER_ROLE, newOwner);
}
Recommendation
Consider implementing a two-step ownership transfer process. This pattern allows the current owner to propose a newOwner address, but the transfer is not finalized until the newOwner explicitly accepts it. This approach ensures that the newOwner address is valid and under control before the previous owner relinquishes their rights.
Resolution
Fixed in commit 5960af. The contract now implements a two-step ownership transfer process.
6.9 Use Namespaced storage for the vault
CoveredMetavault is an upgradeable contract. In order to minimise the storage layout conflicts, it is recommended to use Namespaced storage instead of using the storage slot of this contract.
Recommendation
Use namespace storage for storage variables.
Resolution
Fixed in commit 08e074. Resolved by using ERC-7201 compliant namespaced storage for vault state.
