elfomoFi Vault Accounting Layer Security Review Report

In _applyWithdrawalAccounting(), when the (capped) withdrawal amount equals currentTotalAssetsInRef, both currentTotalAssetsInRef and grossVirtualSupplyX18 are reset to zero:

function _applyWithdrawalAccounting(uint256 refAmount) internal {
    if (refAmount == 0 || currentTotalAssetsInRef == 0) {
        return;
    }
 
    uint256 assetsOut = refAmount > currentTotalAssetsInRef
        ? currentTotalAssetsInRef
        : refAmount;
 
    if (assetsOut == currentTotalAssetsInRef) {
        currentTotalAssetsInRef = 0;
        grossVirtualSupplyX18 = 0; // @audit wiped even if LP tokens remain
        return;
    }
    // ...
}

This logic basically assumes that if all tracked assets are drained, then all LP shares must have been redeemed too. But that's not always true. In practice, currentTotalAssetsInRef is just the last recorded value before the epoch update, and it can be different from the totalAssetsInRef passed into settleEpoch().

As a result, grossVirtualSupplyX18 can be reset to zero while LP tokens are still in circulation, leading to incorrect gross rate calculations.

For example, with zero fees, the gross rate should always equal the net rate.

1. Initial state:

• totalSupply = 1000
• currentTotalAssetsInRef = 1000
• currentNetRate = 1e18
• grossVirtualSupplyX18 = 1000e18

2. User requests withdrawal of 900 LP tokens at rate 1.0

• refAmount = 900

3. A loss epoch is settled (no withdrawals processed)

• currentTotalAssetsInRef = 900

4. Strategy recovers to 1000 assets. Next epoch settles the withdrawal with fillAmount = 900, passing totalAssetsInRef = 100 (i.e., 1000 - 900):

• _applyWithdrawalAccounting(900)

    → assetsOut = min(900, 900) = 900
  
    → equals currentTotalAssetsInRef 
  
    → triggers full reset
    
    → grossVirtualSupplyX18 = 0
  

• fillWithdrawRequest burns 900 LP tokens

    → totalSupply = 100
  

At this point, 100 LP tokens still exist, but grossVirtualSupplyX18 = 0.

5. A new deposit of 100 arrives

_applyDepositAccounting() detects grossVirtualSupplyX18 = 0, falls back to the net rate (1.0), and only accounts for the new deposit:

• totalSupply = 200
• currentTotalAssetsInRef = 200
• grossVirtualSupplyX18 = 100e18

As the result:

• netRate = 200 * 1e18 / 200 = 1.0e18
• grossRate = 200 * 1e36 / 100e18 = 2.0e18

This creates a 2x divergence in a zero-fee system, where gross and net rates should be identical.

Consider reverting the withdrawal transaction when refAmount > currentTotalAssetsInRef as the withdrawn amount shouldn't exceed the total asset.

A withdrawal request stores a fixed refAmount at request time, based on the current vault rate. Later, fillWithdrawRequest() only checks that the fill stays within the stored min/max bounds and does not reprice the request using the current rate. The request itself has no expiry, and the user cannot cancel it directly.

function requestWithdraw(
    uint256 vaultId,
    uint256 lpAmount,
    uint256 targetRefAmount,
    uint256 minRefAmount,
    uint256 expiry,
    bytes calldata signature
) external nonReentrant returns (uint256 requestId) {
    Vault storage vault = _getVault(vaultId);
    require(vault.state != VaultState.Paused, InvalidVaultState());
    require(lpAmount > 0, InvalidAmount());
 
    bytes32 digest = _consumeWithdrawSignature(
        msg.sender, vaultId, lpAmount, targetRefAmount, minRefAmount, expiry, signature
    );
 
    uint256 rate = IVault(vault.vaultAddress).getRate();
    require(rate > 0, InvalidRate());
    uint256 refByRate = Math.mulDiv(lpAmount, rate, RATE_PRECISION);
    uint256 refAmount = Math.min(targetRefAmount, refByRate);
    require(refAmount >= minRefAmount && refAmount > 0, InvalidAmount());
 
    IERC20(vault.lpTokenAddress).safeTransferFrom(msg.sender, address(this), lpAmount);
 
    requestId = nextRequestId++;
    requests[requestId] = WithdrawRequest({
        vaultId: vaultId,
        user: msg.sender,
        lpAmount: lpAmount,
        refAmount: refAmount,
        minRefAmount: minRefAmount,
        status: RequestStatus.Pending
    });
 
    emit WithdrawRequested(
        requestId, vaultId, msg.sender, lpAmount, targetRefAmount, refAmount, minRefAmount, digest
    );
}

If the vault rate rises while the request remains pending, the user can still be filled only at the older request-time cap.

Add an on-chain expiry or timeout to withdrawal requests and allow stale unfilled requests to be canceled through the request lifecycle. The fill path should also avoid relying indefinitely on a request-time capped amount when the current vault rate has moved.

The createVault() function sets the lpTokenAddress for a vault. This address is used for minting and burning shares within the VaultsManager contract during vault operations.

However, the system does not validate whether the lpTokenAddress provided in createVault() matches the actual vault.lpTokenAddress() variable. If these addresses mismatch, settlements and share pricing will fail because the vault will use an incorrect supply to calculate rates.

Additionally, there is no uniqueness check for lpTokenAddress in the createVault() function. If two vaults are created with the same lpTokenAddress, the share calculations for both vaults will be broken.

function createVault(
    ...
) external onlyOwner {
    require(vaultId != 0 && vaultId != type(uint256).max, InvalidAmount());
    require(vaults[vaultId].vaultAddress == address(0), VaultAlreadyExists(vaultId));
    require(
        vaultAddress != address(0)
            && tradingVaultAddress != address(0)
            && curatorAddress != address(0)
            && lpTokenAddress != address(0),
        InvalidAddress()
    );
    require(vaultIdByAddress[vaultAddress] == 0, VaultAddressAlreadyUsed(vaultAddress));
    require(maxLpDepositCap > 0, InvalidAmount());
    require(uint256(protocolFeeMilliBps) + uint256(curatorFeeMilliBps) <= MILLI_BPS_DENOMINATOR,
        InvalidFee());
    require(
        IERC20Metadata(lpTokenAddress).decimals() == IERC20Metadata(address(refToken)).decimals(),
        InvalidLpTokenDecimals()
    );
 
    uint256 rate = IVault(vaultAddress).getRate();
    require(rate > 0, InvalidRate());
 
    vaults[vaultId] = Vault({
        vaultAddress: vaultAddress,
        tradingVaultAddress: tradingVaultAddress,
        curatorId: curatorId,
        protocolFeeMilliBps: protocolFeeMilliBps,
        curatorFeeMilliBps: curatorFeeMilliBps,
        curatorAddress: curatorAddress,
        lpTokenAddress: lpTokenAddress,
        state: VaultState.Paused,
        isEnabledForSwaps: isEnabledForSwaps,
        highWaterMarkRate: rate,
        lastUpdatedRate: rate,
        lastUpdatedPreFeeRate: rate,
        lastUpdatedTimestamp: block.timestamp,
        maxLpDepositCap: maxLpDepositCap
    });
    vaultIdByAddress[vaultAddress] = vaultId;
 
    emit VaultCreated(vaultId, vaultAddress, lpTokenAddress, curatorId);
}

Add validation checks for LP token address consistency and uniqueness in createVault().

The current implementation of VaultsManager.deposit() assumes that the full amount specified by the user is received by the contract. This does not hold for fee-on-transfer tokens, which deduct a fee during transfers. Specifically:

• In deposit(), LP tokens are minted based on the input refAmount, even though the actual amount received may be lower. This mismatch can break accounting assumptions and lead to inconsistencies in the vault's state, especially if a fee-on-transfer token is used, whether intentionally or by mistake.

uint256 lpByRate = Math.mulDiv(refAmount, RATE_PRECISION, rate);
// If the backend caps LP output below the current onchain quote, the excess ref amount stays in the
// vault and benefits existing LPs instead of becoming protocol revenue.
lpAmount = Math.min(targetLpAmount, lpByRate);
require(lpAmount >= minLpAmount && lpAmount > 0, InvalidAmount());
 
uint256 lpSupply = IVaultShares(lpTokenAddress).totalSupply();
// Fee share minting can push total supply above the cap; the cap only constrains user deposit minting.
require(lpSupply + lpAmount <= vault.maxLpDepositCap, ExceedsLpDepositCap());
 
refToken.safeTransferFrom(msg.sender, vaultAddress, refAmount);

To mitigate this issue, consider implementing a balance check before and after the transfer to determine the actual amount transferred.