ROWA Protocol Security Review Report

The function _derivePositionValues calculates a portfolio's total net asset value (NAV). The portfolio values allocated positions using their full reported NAV:

values_[i] = Math.mulDiv(totalBalance_, price_, 10 ** decimals_);

However, adapters such as Midas charge instant redemption fees (+50 bps), Curve/Uniswap charge a swap fee, etc. when converting vault shares back into underlying assets. These costs are not reflected in portfolio valuation, causing the vault to overstate its total assets and share price. As a result, users can deposit and withdraw against an inflated NAV even when no liquidation occurs during their transaction.

Impact

Users redeeming from available idle liquidity do not bear their proportional share of the redemption costs embedded in allocated positions. Instead, those costs are realized only when allocated assets are eventually withdrawn, shifting losses to remaining shareholders.

Example

Assume:

50% of vault assets are idle.

50% are allocated to Midas.

Midas charges a 60 bps instant redemption fee.

The portfolio values assets as:

Total Assets = Idle + Allocated NAV
 
      = 500 + 500
      = 1000 = 1000

However, the allocated position can only realize:

500 × (1 - 0.006) = 497

making the actual realizable value:

500 + 497 = 997

Despite this, shares continue to be minted and redeemed against a reported NAV of 1000, causing the embedded redemption cost to be borne by users who remain in the vault when the allocated position is eventually redeemed.

Consider valuing allocated assets based on their net realizable value rather than their gross NAV.

The StrategyLogic contract is deployed per vault and handles the functionality for the strategist to manage the vault. It exposes a function triggerRebalance for the keeper or strategist to directly trigger a rebalance. The function checks whether the caller is the keeper or the strategist and then forwards the call to PortfolioCoordinator.executeRebalance. However, this function performs more access control checks using the onlyAuthorizedRebalancer modifier:

modifier onlyAuthorizedRebalancer(address strategy) {
_checkRebalanceAuthorization(strategy);
_;
}
function _checkRebalanceAuthorization(address strategy) internal view {
if (strategy == address(0)) revert Error.ZeroAddress();
if (roleRegistry.hasProtocolRole(Roles.KEEPER_ROLE, msg.sender)) {
return;
}
if (
roleRegistry.hasProtocolRole(Roles.OPS_ROLE, msg.sender) ||
roleRegistry.hasProtocolRole(Roles.PROTOCOL_ADMIN_ROLE, msg.sender) ||
roleRegistry.hasProtocolRole(Roles.SUPER_ADMIN_ROLE, msg.sender)
) {
return;
}
revert Error.NotAllowed("Coordinator: caller not authorized");
}

This means that the caller requires the one of the KEEPER_ROLE, OPS_ROLE, PROTOCOL_ADMIN_ROLE or SUPER_ADMIN_ROLE or the function will revert. The caller of this function is the StrategyLogic contract, but that was deployed by the factory and never gets one of these roles assigned. As a result, the entire triggerRebalance flow will always revert and only the keeper or admin roles can trigger rebalances by calling executeRebalance directly. This revert was never caught in the tests, because it uses a MockPortfolioCoordinator that does not check access control for executeRebalance (e.g. in StrategyLogicUpgradeable.e2e.t.sol and others).

function triggerRebalance(
bytes calldata hint
) external onlyWhenActive whenNotPaused requireRouter {
bool isKeeper = roleRegistry.hasProtocolRole(Roles.KEEPER_ROLE, msg.sender);
bool isStrategist = (msg.sender == IVaultStrategyDeployer(vault).strategyDeployer());
if (!isKeeper && !isStrategist)
revert Error.NotAllowed("Strategy: requires KEEPER or strategist");
portfolioCoordinator.executeRebalance(address(this), keccak256("IMMEDIATE"), hint);
emit RebalanceExecuted(vault, 0, 0);
}

Allow a registered StrategyLogic to call executeRebalance for a specified vault, including a check that this StrategyLogic belongs to that vault.

withdraw() and redeem() calculate the exiting user's shares or asset payout before any liquidation occurs. If the vault lacks enough idle base asset, _withdraw() liquidates deployed positions afterward, but the exit amount is not recalculated and no realized liquidation impact is charged to the exiting shares.

function withdraw(...)
{
...
uint256 shares_ = _convertToShares(grossAssets_, Math.Rounding.Ceil);
_trackWithdrawal(owner, assets, shares_);
_withdraw(caller_, receiver, owner, assets, shares_);
...
}
function redeem(...)
{
...
uint256 grossAssets_ = _convertToAssets(shares, Math.Rounding.Floor);
uint256 fee_ = (withdrawFeeBps_ == 0 ||
caller_ == protocolTreasury ||
owner == protocolTreasury)
? 0
: (grossAssets_ * withdrawFeeBps_) / (10_000 + withdrawFeeBps_);
uint256 netAssets_ = grossAssets_ - fee_;
_trackWithdrawal(owner, netAssets_, shares);
_withdraw(caller_, receiver, owner, netAssets_, shares);
...
}

Inside _withdraw(), the vault liquidates only when the idle balance is insufficient. After liquidation, it only verifies that enough base asset exists for the already-fixed payout, then proceeds with the withdrawal.

if (vaultBalance < assets_) {
uint256 shortfall = assets_ - vaultBalance;
uint256 raised = IStrategyLogic(strategyLogic).liquidateForWithdrawal(
shortfall,
""
);
uint256 vaultBalanceAfter = IERC20(baseAsset).balanceOf(address(this));
if (vaultBalanceAfter < assets_) {
revert Error.InsufficientLiquidityForWithdrawal(assets_, vaultBalanceAfter);
}
}
super._withdraw(caller_, receiver_, owner_, assets_, shares_);

The liquidation path uses bounded swaps, but any realized spread, swap fee, or price impact is not assigned to the withdrawing account. The exiting user receives the pre-liquidation NAV amount, while the realized liquidation cost remains in the vault and reduces NAV for the remaining shareholders.

Recalculate the exit amount after withdrawal-induced liquidation, or otherwise adjust the withdrawal settlement to account for realized liquidation impact.

Charge realized liquidation impact to the exiting shares through an exit-impact fee or equivalent swing-pricing mechanism.

Ensure the withdrawal flow does not finalize assets and shares from pre-liquidation NAV when liquidation is required to satisfy the withdrawal.

The PortfolioCoordinatorUpgradeable.executeWithTriggerAction() function is used to execute a rebalance using the trigger-action pattern. A trigger entry with CRITICAL priority is allowed to bypass both the strategy-wide cooldown and the in-flight overlap gate by preempting any in- flight asynchronous execution.

function executeWithTriggerAction(
address strategy,
bytes calldata hint
)
external
override
whenNotPaused
nonReentrant
onlyAuthorizedRebalancer(strategy)
returns (bytes32 executionId)
{
for (uint256 i; i < entries.length; ) {
...

// Evaluate trigger
(bool triggered, /*reason*/, bytes memory triggerData) = ITriggerEvaluator(evaluator).evaluate(
strategy
);

...
}

...

if (winner.priority == ActionTypes.TriggerPriority.CRITICAL) {
uint64 lastCrit = _lastEntryExecution[strategy][_entryKey(winner)];
if (lastCrit != 0 && block.timestamp < lastCrit + CRITICAL_MIN_INTERVAL) {

revert Error.CooldownActive(
strategy,
uint64(lastCrit + CRITICAL_MIN_INTERVAL - block.timestamp)
);
}
if (_inFlightExecution[strategy] != bytes32(0)) {
_preemptInFlight(strategy);
}
}

...

if (!ranAsync) {
...

executionId = IActionHandler(handler).execute(
strategy,
winner.actionParams,
winnerTriggerData,
hint,
entryExecutionType
);

...
}

The issue arises when there is an in-flight asynchronous execution and part of the strategy's funds has already been transferred to an auxiliary source (auxSource) as part of the swap process. In this case, some trigger evaluators may derive incorrect position values, leading to an incorrect trigger decision. For example, DriftTriggerEvaluator fires when an individual asset's driftBps exceeds thresholdBps. During evaluate(), the asset values are obtained by calling positionValuation.derivePositionValues(), which only considers assets held by the vault and staking contracts, but does not account for assets temporarily held by the auxiliary source. Assume that an asset's calculated drift exceeds the configured threshold because part of its balance is currently held in the auxiliary source. executeWithTriggerAction() will consider the trigger valid and subsequently call _preemptInFlight(), which pulls the funds back from the auxiliary source to the vault. However, after the funds are returned, the asset's actual drift may no longer exceed the threshold and the trigger should not have fired. Despite this, the corresponding action is still executed because evaluate() is performed before _preemptInFlight() is called.

As a result, the protocol may execute an incorrect rebalance based on stale position values whenever an asynchronous execution is in flight. In the worst case, if the selected action handler is PauseActionHandler, the strategy may be unnecessarily paused and its funds temporarily frozen. Similarly, the in-flight asynchronous execution can have impact on the trigger evaluator that depends on the timestamp such as TimeTriggerEvaluator, DepegLadderTriggerEvaluator, MovingAverageTriggerEvaluator. Or for the evaluator such as DrawdownTriggerEvaluator, the high water mark should be updated before the async execution is started.

Consider taking the aux source's balance into account when triggering in the trigger entry's evaluate() function. For the timestamp, high water mark related, consider introducing a function onPreExecution() which allows the evaluator to update the timestamp related state, high water mark state before the execution is triggered.

The CoWSwapModule._lopConsumed() function is used to determine whether a LOP order has already been invalidated on 1inch.

function _lopConsumed(uint256 makerTraits) internal view returns (bool) {
uint256 nonce = LopOrderPolicyLib.nonceOf(makerTraits);
uint256 word = LOP_ROUTER.bitInvalidatorForOrder(address(this), nonce >> 8);
return (word >> (nonce & 0xff)) & 1 == 1;
}

The function calls bitInvalidatorForOrder() with slot = nonce >> 8. However, inspecting the implementation of bitInvalidatorForOrder() in the 1inch LOP router (0x111111125421cA6dc452d289314280a0f8842A65) shows that the provided value is shifted by 8 bits once again.

function bitInvalidatorForOrder(address maker, uint256 slot) external view returns(uint256 /* result */) {
return _bitInvalidator[maker].checkSlot(slot);
}
function checkSlot(Data storage self, uint256 nonce) internal view returns(uint256) {
uint256 invalidatorSlot = nonce >> 8;
return self._raw[invalidatorSlot];
}

As shown above, checkSlot() computes the actual invalidator slot by performing nonce >> 8. Therefore, passing nonce >> 8 to bitInvalidatorForOrder() causes the value to be shifted twice, resulting in an effective nonce >> 16 lookup instead of the expected nonce >> 8. As a consequence, _lopConsumed() queries an incorrect invalidator slot and may incorrectly report that an order has not been invalidated. Since _lopConsumed() is used by reconcileLop(),

this can prevent cancelled or filled orders from being properly reconciled. As a result, the funds accounted for in lopOutstanding may become unsweepable until manual intervention (cancel).

Consider passing the unshifted nonce value to bitInvalidatorForOrder() and allowing the 1inch implementation to derive the correct invalidator slot internally.

- uint256 word = LOP_ROUTER.bitInvalidatorForOrder(address(this), nonce >> 8);
+ uint256 word = LOP_ROUTER.bitInvalidatorForOrder(address(this), nonce);

This ensures the invalidator slot is shifted exactly once (nonce >> 8), matching the 1inch implementation.

PortfolioValuator._getStakedBalance() is called once for each allocation asset and sums getPosition().currentValue across all active staking options registered for that asset. Non- custodial LP adapters derive this value from the vault's raw LP token balance:

// CurveStableLPAdapter.getPosition
uint256 lpBalance = IERC20(LP_TOKEN).balanceOf(vault);
uint256 vp = POOL.get_virtual_price();
uint256 value = Math.mulDiv(Math.mulDiv(lpBalance, vp, WAD), UNDERLYING_UNIT, WAD);

Consider a vault that allocates both USDC and USDT, with two CurveStableLPAdapter deployments using the same USDC/USDT pool:

• Adapter A has UNDERLYING = USDC and is registered under USDC.

• Adapter B has UNDERLYING = USDT and is registered under USDT.

• Both adapters use the same LP_TOKEN.

As a result:

• _getStakedBalance(strategy, USDC) ™ Adapter A ™ LP_TOKEN.balanceOf(vault) × vp, denominated in USDC.

• _getStakedBalance(strategy, USDT) ™ Adapter B ™ the same LP token balance, denominated in USDT.

Therefore, the same LP position is included in NAV twice, effectively inflating the value of the position. This causes totalAssets() to be overstated, allowing redeemers to withdraw more than their fair share and potentially drain value from the remaining holders.

Several existing checks do not prevent this: AssetRegistry._assertStakingOptionUnique() only checks _assets[asset].stakingOptions. Since the two adapters are registered under different assets, they reside in separate arrays and are not considered duplicates.

• _getStakedBalance() iterates over the registry's staking options rather than the strategy's

• entry.stakingOptions, so both adapters are valued regardless of the strategy's configured options.

• ConfigValidator does not check for duplicate receipt tokens across allocation entries.

Note that the same root cause can occur in another form when a vault-held receipt token is also an allocation asset. For example, a vault may allocate WETH with a Lido staking option whose receipt token is wstETH, while wstETH is separately allocated. In both cases, the same underlying balance can be counted twice in NAV. Similarly to CurveCryptoLPAdapter, MetaMorphoSupplyAdapter.

Consider de-duplicating staking positions by receipt token in the valuator. Before adding a position's value, track the getReceiptToken() of each active staking option and skip any option whose receipt token has already been counted. This approach covers both cases, including staking options registered after deployment, without requiring an additional registry-wide index.

The CurveStableLPAdapter.getPosition() function uses POOL.get_virtual_price() to determine the value of the Curve LP tokens held by the vault:

function getPosition(
address strategy,
address asset
) external view override returns (StakingPosition memory position) {
position.asset = UNDERLYING;
position.canWithdraw = true;
if (asset != UNDERLYING) return position;
address vault = IStrategyLogic(strategy).vault();
uint256 lpBalance = IERC20(LP_TOKEN).balanceOf(vault);
if (lpBalance == 0) return position;
uint256 vp = POOL.get_virtual_price();
// lpBalance ™ USD (1e18) ™ underlying token units.
uint256 value = Math.mulDiv(Math.mulDiv(lpBalance, vp, WAD), UNDERLYING_UNIT, WAD);
position.stakedAmount = value;
position.currentValue = value;
}

However, this exposes the valuation to read-only reentrancy, a well-known issue affecting Curve pools that support native ETH withdrawals. During remove_liquidity(), Curve burns the LP tokens before transferring the underlying assets to the recipient. This temporarily reduces the LP token total supply while the pool balances have not yet been updated, causing get_virtual_price() to temporarily return an inflated value:

CurveToken(lp_token).burnFrom(msg.sender, _amount)
for i in range(N_COINS):
...
if i == 0:
raw_call(msg.sender, b"", value=value)

If the withdrawn asset is native ETH, the external call made during the withdrawal allows the recipient to execute arbitrary logic before the Curve withdrawal completes. An attacker can use this callback to interact with another protocol that relies on get_virtual_price() for valuation. For example, assume a RowaVault uses CurveStableLPAdapter to stake stETH through the Curve stETH/ETH pool at 0xDC24316b9AE028F1497c275EB9192a3Ea0f67022.

An attacker could:

1. Deposit a significant amount of assets into the RowaVault and receive vault shares.

2. Provide a large amount of liquidity to the Curve pool.

3. Initiate a large liquidity withdrawal from Curve.

4. During Curve's native ETH transfer callback, reenter the RowaVault.

5. At this point, the Curve LP tokens have already been burned, temporarily inflating get_virtual_price().

6. The RowaVault therefore observes an artificially inflated value for its Curve LP position.

7. The attacker withdraws or redeems their vault shares at the inflated valuation, receiving more assets than they should and potentially extracting a profit.

The core issue is that the vault relies on a Curve price oracle that can be temporarily manipulated during a reentrant callback. Any vault operation that can be reentered during this window and relies on the LP token valuation may therefore be vulnerable to an inflated NAV calculation.

Reference: https://www.chainsecurity.com/blog/curve-lp-oracle-manipulation-post-mortem

Consider calling the pool's withdraw_admin_fees function to trigger the reentrancy lock. Most protocols followed that protection pattern.

The executeWithTriggerAction() function evaluates all enabled trigger entries, selects a winner based on priority, and dispatches the winner's action. The winner is selected using the following loop:

while (true) {
int256 bestPriority = -1;
bool found;
for (uint256 i; i < entries.length; ) {
if (eligible[i] && !skipped[i]) {
int256 p = int256(uint256(entries[i].priority));
if (p > bestPriority) {
bestPriority = p;
winnerIdx = i;
found = true;
}
}
unchecked { ++i; }
}
if (!found) break;
...
skipped[winnerIdx] = true;
anySkipped = true;
}

The comparison uses p > bestPriority, so when two entries have the same priority, the entry with the lower index wins.

CRITICAL is the highest priority in the enum, so a triggered critical entry always becomes the winner. When a critical entry wins while an asynchronous program is in flight, the coordinator cancels that program and then re-evaluates the winner:

if (_inFlightExecution[strategy] != bytes32(0)) {
_preemptInFlight(strategy);
(bool stillTriggered, bytes memory freshTriggerData) = _evaluateEntry(
strategy,
rebalanceRegistry.getEvaluator(winner.triggerType),
winnerKey
);
if (!stillTriggered) {
revert Error.TriggerStaleAfterPreemption(strategy, winnerIdx);
}
winnerTriggerData = freshTriggerData;
}

This re-check exists because position-derived evaluators rely on derivePositionValues(), which excludes balances held by the vault's CoW module. While a program is in flight, an asset may therefore appear under-allocated, causing a drift or exposure trigger to fire even though the corresponding condition does not actually exist.

Cancelling the program returns those balances to the vault, so the trigger is evaluated again and rejected if it no longer fires.

However, rejecting the stale trigger causes the entire transaction to revert. Unlike the selection loop above, there is no fallback to the next eligible entry when the selected winner becomes invalid after preemption.

As a result, consider a vault with two critical entries where the lower-indexed entry fires only because of the in-flight accounting discrepancy. That entry wins the contest every time, causes the program to be cancelled, fails the re-check, and reverts the transaction. The second critical entry is therefore never reached.

Consequently, a genuine emergency condition guarded by the second critical trigger, such as an emergency exit or pause, cannot be executed for as long as the program remains in flight.

Lower-priority entries are not affected because a non-critical winner reverts with ExecutionInFlight while a program is in flight, regardless of whether the trigger remains valid after preemption.

if (!stillTriggered) {
revert Error.TriggerStaleAfterPreemption(strategy, winnerIdx);
}

The root cause is that triggers are evaluated before the in-flight program is cancelled, meaning the evaluation can be based on an incorrect view of the portfolio. The current ordering exists because the overlap decision "depends on the winner's PRIORITY", which is unknown until the triggers have been evaluated.

Consider splitting executeWithTriggerAction() into two entry points so that the caller specifies the priority class up front, removing this ordering dependency:

• executeCriticalTrigger() should cancel any in-flight program before evaluating triggers. Since the cancellation has already returned the CoW balances to the vault, all evaluators will operate on the correct portfolio state, preventing a phantom trigger from becoming eligible. Only critical entries should be considered. The strategy-wide cooldown and per-entry cooldown should continue to be bypassed, as they are today. If no critical entry fires, the function should revert. The revert will roll back the cancellation, preserving a legitimate in- flight program just as the current re-check does.

• executeRoutineTrigger() should consider only non-critical entries and revert with ExecutionInFlight when a program is in flight, matching the current behaviour for these entries.

This removes the stale re-check entirely and eliminates the resulting starvation because the winner is selected only after the relevant entries have been evaluated against the correct portfolio state.

Keepers would then need to choose which entry point to call. The existing simulate() view can be used to determine which path is appropriate, or the keeper can attempt the critical path first and fall back to the routine path if no critical trigger is active.

After executeWithTriggerAction() selects a winning trigger entry, it dispatches the corresponding action handler:

if (!ranAsync) {
// Synchronous path — behavior unchanged from before EX-3.4d.
// Dispatch to acti
 they compute their own deltas)
executionId = IActionHandler(handler).execute(
strategy,
winner.actionParams,
winnerTriggerData,
hint,
entryExecutionType
);

The call is not wrapped in try/catch. Therefore, if the handler reverts, the entire transaction reverts. The winner is selected deterministically based on the current state, with ties broken by selecting the entry with the lowest index. As a result, if a handler reverts for a condition that does not resolve on its own, the same entry will win on every subsequent call and revert again. Any entry with a lower priority, or with the same priority but a higher index, is never reached. AdjustLeverageActionHandler is one handler that can revert in this way. It repays debt using the vault's balance of the debt asset:

address vault = IStrategyLogic(strategy).vault();
uint256 cash = IERC20(ILendingAdapter(lendingAdapter).debtAsset()).balanceOf(vault);
uint256 amount = repayNeeded > cash ? cash : repayNeeded;
if (amount == 0) {

revert Error.NotAllowed("AdjustLeverage: no cash to deleverage");
}

If investors withdraw the vault's debt-asset balance, cash becomes zero and the handler reverts. Notably, the same handler treats its three other early-exit conditions as soft no-ops through _skip(), which emits an event and returns normally:

if (hfNow == type(uint256).max) {
return _skip(strategy, executionId, bytes32("NO_DEBT"), hfNow);
}
if (hfNow >= targetHealthFactor) {
return _skip(strategy, executionId, bytes32("HF_AT_TARGET"), hfNow);
}
if (hfNow == 0) {
// Repaying vault cash into a position the venue will liquidate anyway only
// donates the cash to the liquidator —
 refuse, loudly observable, and let the
// strategy's OTHER protections (CRITICAL de-risk/exit entries) keep running.
return _skip(strategy, executionId, bytes32("INSOLVENT"), hfNow);
}

The comment on the INSOLVENT branch explicitly states that a soft skip is used to allow the strategy's other protections to continue running. The same reasoning applies to the zero-cash condition, but that condition currently causes a hard revert instead. The combination of a hard revert and deterministic winner selection therefore prevents other protections from executing.

Consider a strategy with two trigger entries:

• Entry 0: HEALTH_FACTOR_TRIGGER with the ADJUST_LEVERAGE action at HIGH priority. The priority enum describes HIGH as "Stop loss, collateral management".

• Entry 1: CASH_FLOW_TRIGGER with the INCREASE_CASH action. IncreaseCashActionHandler sells proportionally across the vault's positions to raise base currency, which is also the debt asset.

The sequence is:

1. Investors withdraw, draining the vault's debt-asset balance.

2. The health factor falls below the configured threshold, causing Entry 0 to trigger.

3. The outflow also crosses the cash-flow threshold, causing Entry 1 to trigger.

4. Entry 0 wins the priority contest.

5. AdjustLeverageActionHandler finds that cash is zero and reverts.

6. The entire transaction reverts, so INCREASE_CASH never executes and no positions are sold to raise the required base currency.

7. The next keeper call observes the same state and repeats the failure.

As a result, the vault cannot raise the cash required by ADJUST_LEVERAGE because the only trigger capable of raising that cash is permanently starved by the higher-priority reverting trigger. The health factor can continue deteriorating until the lending position is liquidated, which is precisely the outcome the health-factor trigger is intended to prevent. Recovery requires manual intervention, such as the strategist disabling or reordering the trigger entries, a new deposit providing the required base currency, or a keeper calling executeRebalance() directly to bypass the trigger path. None of these recovery mechanisms occur automatically.

Consider dispatching the action handler inside a try/catch. If the handler reverts, mark the corresponding entry as temporarily ineligible by setting a skipUntil timestamp. Subsequent trigger evaluations should treat the entry as skipped until this timestamp expires, allowing other eligible triggers to execute. Consider clearing the skipUntil value for all entries whenever a trigger or rebalance succeeds, since the portfolio state may have changed and the condition that caused the previous failure may no longer apply.

updateAllocations() can be called by the strategist or an admin and replaces the strategy's allocation list. The function validates each allocation entry independently, but it does not ensure that allocation assets are unique and does not check whether removed assets still have a live vault balance.

function updateAllocations(
StrategyConfig.AllocationEntry[] calldata newAllocations
) external onlyWhenActive requireRouter {
_checkStrategistOrAdmin();
if (newAllocations.length == 0) revert Error.NotAllowed("Strategy: no allocations");
...
}

After validation, the existing allocations are deleted and replaced with the strategist-supplied list.

delete _allocations;
for (uint256 i; i < length; ) {
_allocations.push(newAllocations[i]);
unchecked {
++i;
}
}

The portfolio valuator later derives the NAV asset list directly from _allocations.

function getAssets() external view returns (address[] memory) {
uint256 length = _allocations.length;
address[] memory assets_ = new address[](length);

for (uint256 i = 0; i < length; i++) {
assets_[i] = _allocations[i].asset;
}
return assets_;
}

Each returned asset entry is then valued independently and added to the total NAV.

for (uint256 i; i < length_; ) {
address asset_ = assets_[i];
uint256 walletBalance_ = IERC20(asset_).balanceOf(vault_);
uint256 stakedBalance_ = _getStakedBalance(asset_, registry_);
uint256 totalBalance_ = walletBalance_ + stakedBalance_;
...
values_[i] = Math.mulDiv(totalBalance_, price_, 10 ** decimals_);
total_ += values_[i];
}

A strategist can therefore submit an allocation update that changes NAV without first reconciling the allocation list against actual vault holdings. Removing an asset that the vault still holds excludes that asset from NAV. Adding the same asset more than once can cause the same balance to be counted multiple times. This can distort ERC4626 share pricing around deposits or withdrawals by accounts controlled by, or coordinated with, the strategist.

This gives the strategist a direct primitive to steal value from a vault:

1. Decrease the NAV by removing an asset to make the NAV 50%.

2. Deposit into the vault and obtain shares.

3. Increase the NAV by adding the asset back to make the NAV 100% again.

4. Withdraw all shares and get double the initial deposit's value back.

Require allocation assets to be unique in both initial configuration and updateAllocations().

Reject allocation updates that remove an asset while the vault still holds a non-zero balance for that asset.

Ensure the NAV calculation cannot double-count the same asset even if duplicate entries are present.

In the ROWA vault, the _decimalsOffset function is currently hardcoded to always return 12. This has the adverse effect that the vault's share decimals will always be 12 greater than the base asset, as decimals() calculates this dynamically from the base asset:

function decimals() public view virtual override(IERC20Metadata, ERC20Upgradeable) returns (uint8) {
ERC4626Storage storage $ = _getERC4626Storage();
return $._underlyingDecimals + _decimalsOffset();
}

So for an 18-decimal token like DAI, WETH, etc. The shares will be minted at a decimal of 30. The view function decimals() will also return 30. This is much too large to safely integrate with most on-chain DeFi protocols and off-chain applications. A lot of DeFi protocols rely on tokens having <= 18 decimals and wonˇt work with anything larger. Normally _decimalsOffset is hardcoded for ERC4626 vaults where the base asset is known at deployment time, so it can be set correctly (usually 12 for USDC, 0 for WETH/DAI, etc.). For ROWA it would have to be dynamic based on the configured base asset. Furthermore, the inflation vector is already mitigated with the ERC4626 virtual assets/shares in _convertToShares/Assets and it can be further mitigated by seeding the vault with dead shares if desired.

function _decimalsOffset() internal pure override returns (uint8) {
return 12;
}

The function _decimalsOffset can be used to make the vault' shares always 18 decimals an d it should be dynamically calculated from the base asset's decimals:

base.decimals() < 18 ? 18 - base.decimals() : 0;

resolveDexAdapterselects the first active adapter w hose type isDEXwith without verifying that the adapter supports the asset pair being traded.

// ActionLib.sol:108-118
for (uint256 i; i < allocation.adapters.length; ) {
IAssetRegistry.AdapterInfo memory info = assetReg.getAdapterInfo(allocation.adapters[i]);
if (info.adapterType == IAssetRegistry.AdapterType.DEX && info.active) {
return abi.encode(allocation.adapters[i]);
}
unchecked { ++i; }
}

As a result, when multiple DEX adapters are configured, the function may choose an incompatible adapter even though a later adapter in the list supports the trade. This causes rebalances to revert. The function iterates through allocation.adapters and immediately returns the first active DEX adapter it encounters:

for (uint256 i; i < allocation.adapters.length; ) {
  IAssetRegistry.AdapterInfo memory info =
    assetReg.getAdapterInfo(allocation.adapters[i]);
  if (info.adapterType == IAssetRegistry.AdapterType.DEX && info.active) {
   
return abi.encode(allocation.adapters[i]);
  }
  uncheck unchecked { ++i; }
}

As a result, adapter selection depends solely on ordering, not actual pair support.

Scenario

Assume the allocation is configured with:

adapters = [curveAdapter, uniswapAdapter]

and the supported routes are:

• Curve: BASE ™ token1

• Curve: BASE ™ token2

• Uniswap: BASE ™ token3

When the protocol needs to swap between BASE and token3, resolveDexAdapter selects curveAdapter because it is the first active DEX in the list. token3 pair, causing the swap to revert. Although uniswapAdapter supports the pair, it is never checked because the function returns immediately after finding the first DEX adapter.

If multiple DEX's are supported, consider adding the following check inside the loop.

if (info.adapterType == IAssetRegistry.AdapterType.DEX && info.active) {
++    if (ISpotTradeAdapter(allocation.adapters[i]).isPairSupported(allocation.asset, baseCurrency)) {
return abi.encode(allocation.adapters[i]);
++    }
}

verifyAndCachedecodesreport.expiresAtfrom the Chainlink Data Streams report but never validates or uses it. Instead, cached prices always receive a new expiry based on the protocol'sdefaultCacheTTL.

As a result:

• Expired reports can be accepted.Reports withreport.expiresAt < block.timestampare still verified, cached, and treated as fresh data.

• Cache lifetime can exceed Chainlink's validity window.A report that is about to expire can remain usable for up to the full protocol TTL (ex. 300 seconds), even after Chainlink considers it stale.

This allows oracle data to remain valid in the protocol beyond the oracle network's intended expiry period. Impact All pricing cons umers rely onge getPrice(), including NAV calculations, withdrawals, rebalancing, and fee accounting. If stale or expired reports are cached, these operations may use outdated prices. Additionally, because there is no check that a new report is newer than the currently cached one, a keeper can overwrite a fresh price with an older report, effectively rolling back the cached value.

// reject if already expired by Chainlink's own clock
++ if (report.expiresAt < block.timestamp) revert Error.InvalidDataStreamsReport();
// cache only as long as BOTH constraints allow
verifiedPrices[asset][baseCurrency] = VerifiedPrice({
...
--    expiresAt: currentTimestamp + cachedTTL
++    expiresAt: uint64(Math.min(currentTimestamp + cachedTTL, report.expiresAt))
});

VolatilityTriggerEvaluator reports annualized volatility ~1e9× too small, so for any realistic price evaluation the volatilityBps rounds to 0 and the trigger never fires. The issue is in _calculateStdDev. Returns are stored in 1e18 scale, but each squared difference is divided by PRECISION before taking the square root:

squaredDiff = (squaredDiff * squaredDiff) / PRECISION;
uint256 variance = sumSquaredDiff / (n - 1);
return _sqrt(variance);

This leavesvariancescaled by1e18. When_sqrt()is applied, the result is only1e9-scaledinstead of1e18-scaled. The rest of the calculation still treatsstddevas if it were1e18-scaled, making the computed volatility approximately1e9× too small.

uint256 annualizedVol = (stddev * SQRT_365_SCALED) / SQRT_SCALE;
uint256 volatilityBps = annualizedVol / 1e14;

KeepsquaredDiffat1e36 scaleby removing the division byPRECISION. This allows the square root to naturally return a1e18-scaledstandard deviation, matching the scale expected by the rest of the calculation.

- squaredDiff = (squaredDiff * squaredDiff) / PRECISION; // 1e18 scale ™ sqrt returns 1e9 (incorrect)
+
 squaredDiff = squaredDiff * squaredDiff;               // 1e36 scale ™ s sqrt returns 1e18 (correct)

Proof-of-Concept: Consider taking the aux source's balance into account when triggering in the trigger entry's evaluate() function. For the timestamp, high water mark related, consider introducing a function onPreExecution() which allows the evaluator to update the timestamp related state, high water mark state before the execution is triggered.

function test_PoC_EvenAbsurdSwingsAreUnderReported() public {
uint256[] memory prices = new uint256[](WINDOW_DAYS + 1);
for (uint256 i = 0; i < prices.length; i++) {
prices[i] = (i % 2 == 0) ? 1e23 : 1e16; // 10,000,000x swing every day
}
priceOracle.setHistoricalPrices(asset, baseCurrency, prices);
(bool triggered, bytes32 reason, bytes memory data) = evaluator.evaluate(
address(mockStrategy)
);
(uint256 reportedVolBps, ) = abi.decode(data, (uint256, uint16));
console2.log("Reported vol for 10,000,000x daily swing (bps):", reportedVolBps);
console2.log("Threshold (bps):", uint256(THRESHOLD_BPS));
assertEq(reason, evaluator.REASON_WITHIN_VOLATILITY());
assertGt(reportedVolBps, 0); // ~971: only absurd inputs produce anything non-zero
assertLt(reportedVolBps, THRESHOLD_BPS, "even a 10,000,000x swing stays under 20%");
assertFalse(triggered);
}

Some trigger evaluators maintain internal state that is updated through onPostExecution(). However, the coordinator only calls onPostExecution() for evaluators whose evaluate() function returned true. As a result, evaluators that return false are never notified that a rebalance has completed. Whether this is correct depends on the type of state the evaluator maintains.

This leads to two different issues:

• TimeTriggerEvaluator stores the timestamp of its last trigger. Since this state is only updated when the Time trigger itself fires, it measures the time since the last Time-triggered rebalance instead of the last portfolio rebalance. This can cause redundant rebalances.

• MovingAverageTriggerEvaluator stores crossover detection state. If the Moving Average trigger fires but loses the priority contest to another trigger, onPostExecution() still updates its baseline even though the MA action never executed. This consumes the crossover, causing it to be missed on subsequent evaluations. Additionally, single-direction configurations can leave the baseline stale when an unconfigured crossover occurs.

Each evaluator could maintain only the state it actually requires. TimeTriggerEvaluator should derive its timing from the coordinator's lastRebalanceTimestamp() instead of maintaining its own timestamp.

MovingAverageTriggerEvaluator should remove its dependency on a persisted baseline and derive crossover events directly from historical price data, avoiding state that can become consumed or stale.

When a strategy uses a COMPOSITE trigger that includes one or more stateful child triggers (e.g., TIME_TRIGGER, MOVING_AVERAGE_TRIGGER, or DRAWDOWN_TRIGGER), the child trigger's onPostExecution() hook is never invoked after a successful rebalance. As a result, the child trigger's internal state is not reset, causing TIME_TRIGGER to fire repeatedly and preventing MOVING_AVERAGE_TRIGGER and DRAWDOWN_TRIGGER from updating their crossover or high-water-mark state correctly. The issue stems from CompositeTriggerEvaluator not implementing ITriggerPostExecution and therefore not forwarding the onPostExecution() callback to the child triggers that participated in the evaluation.

CompositeTriggerEvaluator contract should implement ITriggerPostExecution and forward the hook to each referenced child that implements it:

function onPostExecution(address strategy) external override {
if (msg.sender != COORDINATOR) revert Error.CallerNotCoordinatorOrHandler(msg.sender);
bytes32[] storage types = _configs[strategy].triggerTypes;
for (uint i; i < types.length; ++i) {
address ev = triggerRegistry.getEvaluator(types[i]);
if (ev == address(0)) continue;
try ITriggerPostExecution(ev).onPostExecution(strategy) {} catch {}
}
}

Both triggers calculate an asset' allocation asassetValue / totalNAV, wheretotalNAVcomes fromderivePositionValues(). However,derivePositionValues()only sums the assets passed into it and does not include the idle base-currency buffer. This makes the denominator smaller than the actual portfolio NAV, causing reported allocations to be inflated. Drift trigger: targetBpsis based on the full portfolio NAV (assets + cash). The rebalance handler usesderivePortfolioValues, which includes the idle base-currency buffer when calculating current weights. However,DriftTrigger EvaluatorcalculatescurrentBpswithou without the buffer, causing the trigger and executor to use different NAV calculations. Because of this mismatch, the trigger can hide real drift because idle cash makes an asset appear closer to its target allocation than it actually is.

Example:

Target allocation:

• ETH = 40%

• BTC = 40%

• USDC = 20%

Current portfolio:

• ETH = $40

• BTC = $40

• USDC = $20

The true portfolio NAV is $100, so the actual allocation is:

• ETH = 40 / 100 = 40%

• BTC = 40 / 100 = 40%

• USDC = 20 / 100 = 20%

The portfolio is exactly at target.

However,DriftTriggerEvaluatorexcludes the USDC buffer and calculates against only the positions:

• ETH = 40 / 80 = 50%

• BTC = 40 / 80 = 50%

The trigger sees ETH and BTC as over-allocated, even though the portfolio is correctly balanced when the USDC buffer is included.

NAV Exposure trigger: there are two ways the denominator shrinks:

1. Unwatched assets are excluded.

Portfolio:

A = $25, B = $25, C = $25, D = $25

The true portfolio NAV is $100. If the trigger only watches[A [A, B], the denominator becomes $50, so A is reported as:

25 / 50 = 50%

instead of its true allocation:

25 / 100 = 25%

A single watched asset will always appear as 100% allocation because it becomes the entire denominator.

2. Idle cash is excluded.

A = $33, B = $33, Cash = $34

The true portfolio NAV is $100, but the trigger only sees $66 of assets. This causes A to be reported as:

33 / 66 = 50%

instead of its true allocation:

33 / 100 = 33%

Update both triggers to use the full portfolio NAV (including all assets and the idle cash buffer) as the denominator, using PortfolioValuator.getPortfolioValue(strategy)or a or an equivalent buffer- inclusive NAV calculation, while keeping the watched asset list only for selecting which assets to evaluate.

In the targeted mode of DeRiskActionHandler, only the assets flagged in the payload are sold down to cap * (10000 - reductionBps) / 10000 of the position-only NAV. For each flagged asset, the sell amount s_i satisfies:

(v_i - s_i) / (D - S) = rc / 10000

This formula assumes that all flagged assets share the same denominator and that the sale proceeds are removed from the position-value basis, causing the denominator to become D - S. However, this assumption does not hold when the base currency is itself an allocation asset. In that case, selling a non-base asset increases the strategy's exposure to the base currency rather than removing value from the position set. Consequently, if the evaluator also monitors the base currency, it may become flagged after the rebalance, causing the trigger to fire again.

Consider a example universe {ETH, BTC, USDC (base)} with a cap of 30% and reductionBps = 1000, resulting in rc = 27%.

Initial portfolio:

• ETH: $500 (50%)

• BTC: $250 (25%)

• USDC: $250 (25%)

• Total position value D = $1,000

Only ETH is flagged for de-risking.

Using the current formula, the handler computes that approximately $315 of ETH should be sold, assuming the total position value shrinks from $1,000 to $685, leaving ETH at 27% of the remaining NAV. However, because the sale proceeds are denominated in the base currency and the base currency is itself an allocation asset, the proceeds remain within the portfolio instead of leaving the position-value basis.

As a result, ETH is over-sold while the base currency's allocation increases significantly and exceeds the 30% cap. In the next evaluation cycle, USDC is flagged for de-risking (56.5% > 30%), causing the trigger to fire again.

Consider excluding the base currency from the risk-capping path.

The drawdown trigger is intended to detectinvestment losses, but it measures drawdown u singtotal portfolio NAV (AUM)instead ofNAV-pe NAV-per-share.

function _getCurrentNAV(address strategy) internal view returns (uint256) {
...
(, uint256 totalValue) = positionValuation.derivePositionValues(strategy, assets);
return totalValue;
}

The high-water mark is stored using this total value, and drawdown is calculated as:

drawdownBps = ((highWaterMark - currentNAV) * 10_000) / highWaterMark;

Total NAV changes whenever assets enter or leave the vault, so it doesnotrepresent investment performance.

For example:

• Withdrawalsreduce total NAV even if portfolio performance is unchanged, causing the trigger to report a false drawdown.

• Depositsincrease total NAV and raise the high-water mark. A subsequent portfolio loss may not exceed this inflated HWM, delaying or preventing the trigger from firing.

• Fee collections(redeeming accrued fee shares) also reduce total NAV and can incorrectly appear as investment losses.

As a result, the trigger measurescapital flowsrather than actual portfolio performance.

Impact

The drawdown trigger can:

• False positive:Trigger an unnecessary rebalance after withdrawals or fee collections despite no investment loss.
• False negative:Mi Miss or delay a genuine drawdown after deposits because the HWM was increased by new capital rather than portfolio appreciation.

Track the high-water mark using NAV-per-share(pri (price per share) instead of total NAV.

Both PancakeV3Adapter and UniswapUniversalAdapter select the swap pool based solely on the highest raw liquidity() value. However, raw liquidity is not a reliable measure of execution quality across fee tiers (or between Uniswap V3 and V4), since actual output depends on both fees and price impact. As a result, the adapters can route swaps through a pool that returns less output than another available pool. The trade still succeeds because the only protection is the protocol's oracle slippage limit, allowing the value loss to occur silently. Impact: This causes a recurring loss of value on affected swaps and rebalances. Although each swap is bounded by the configured slippage limit, the losses accumulate over time and directly reduce vault NAV. ((Example: (On the PancakeSwap USDC/WETH market, the adapter selects the 0.01% fee tier because it has the highest raw liquidity (1.854e16 vs 1.735e16 for the 0.05% tier). However, on a $250,000 swap, the rejected 0.05% pool returns approximately 77 79 bps more WETH despite having lower reported liquidity.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.30;
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {console} from "forge-std/console.sol";
import {ForkTestBase} from "./ForkTestBase.sol";
import {StrategyConfig} from "../../vault/StrategyConfig.sol";
import {Quote} from "../../types/ExecutionTypes.sol";

import {Roles} from "../../access/Roles.sol";
import {IUniversalRouter} from "../../interfaces/external/uniswap-v4/IUniversalRouter.sol";
import {IPermit2} from "../../interfaces/external/uniswap-v4/IPermit2.sol";
import {IUniswapV3Factory} from "../../interfaces/external/uniswap-v3/IUniswapV3Factory.sol";
import {IPancakeV3Pool} from "../../interfaces/external/pancake-v3/IPancakeV3Pool.sol";
contract ForkPancakeAdapterBestExecBugTest is ForkTestBase {
uint24[4] internal FEES = [uint24(100), uint24(500), uint24(2_500), uint24(10_000)];
function setUp() public override {
vm.createSelectFork(vm.envOr("MAINNET_RPC_URL", string("https://ethereum-rpc.publicnode.com")));
super.setUp();
// Impersonate an authorized ExecutionRouter so the adapter's onlyRouter passes and it
// pulls/pushes against this contract (BaseAdapter P0-14 token flow).
vm.prank(superAdmin);
roleRegistry.grantRole(Roles.FACTORY_ROLE, address(this));
protocolRegistry.authorizeRouter(address(this));
}
function _b2_5_deployAndRegisterAdapters() internal override {
super._b2_5_deployAndRegisterAdapters();
_deployPancakeAdapter();
}
function _buildStrategyConfig() internal view override returns (StrategyConfig.Strategy memory) {
return _buildPancakeStrategyConfig();
}
function _b5_seedVault() internal override {}
// PPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPP
    // The PoC
    // PPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPP
 
    function test_poc_bug_priceBlindPoolSelection_leaksValue() public {
        uint256[2] memory sizes = [uint256(250_000e6), uint256(500_000e6)];
 
        bool demonstrated;
        for (uint256 s = 0; s < sizes.length && !demonstrated; s++) {
            uint256 tradeUsdc = sizes[s];
 
            // (1) The metric the adapter actually ranks on: highest raw in-range liquidity.
            (uint24 maxLiqFee,) = _maxLiquidityTier();

// (2) Confirm the adapter auto-selects exactly that max-raw-L tier.
Quote memory q = pancakeAdapter.getQuote(USDC, WETH, tradeUsdc);
assertEq(
uint256(_pathFee(q.routeData)), uint256(maxLiqFee), "adapter must select the max raw-liquidity tier"
);
// (3) Realized output through the adapter (routes the max-L tier).
uint256 adapterOut;
{
uint256 snap = vm.snapshotState();
deal(USDC, address(this), tradeUsdc);
IERC20(USDC).approve(address(pancakeAdapter), tradeUsdc);
adapterOut = pancakeAdapter.swap(USDC, WETH, tradeUsdc, 0, "");
vm.revertToState(snap);
}
// (4) Realized output through the best tier, measured against the REAL router.
(uint256 bestOut, uint24 bestFee) = _bestRealizedTier(tradeUsdc);
console.log("--- trade size (USDC) ---", tradeUsdc / 1e6);
console.log("adapter picked fee tier :", maxLiqFee);
console.log("adapter realized WETH   :", adapterOut);
console.log("best tier fee           :", bestFee);
console.log("best realized WETH      :", bestOut);
// The bug only surfaces once the trade is large enough that fee-tier + liquidity
// distribution diverge from raw L. Skip to the next size if it hasn't yet.
if (bestFee == maxLiqFee || bestOut <= adapterOut) continue;
// BUG CONFIRMED: a tier the adapter REJECTED (lower raw L) delivers strictly more.
uint256 lossBps = ((bestOut - adapterOut) * 10_000) / bestOut;
console.log(">> SILENT BEST-EXEC LOSS (bps):", lossBps);
// The loss sits INSIDE the vault's 5% oracle floor, so nothing reverts — it is a
            // silent leak on every rebalance, not a self-correcting failure.
            assertLt(lossBps, 500, "loss is within the oracle floor -> no revert, silent leak");
            assertGt(lossBps, 0, "adapter under-filled vs a rejected tier");
            demonstrated = true;
        }
 
        assertTrue(demonstrated, "adapter matched best tier at all tested sizes (bump trade size / re-pin block)");
    }
 
    // PPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPP
    // Helpers
    // PPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPP

/// @dev The fee tier with the highest raw in-range liquidity —
 what the adapter will pick.
function _maxLiquidityTier() internal view returns (uint24 maxLiqFee, uint128 maxLiq) {
for (uint256 i = 0; i < 4; i++) {
address pool = IUniswapV3Factory(PCS_V3_FACTORY).getPool(USDC, WETH, FEES[i]);
if (pool == address(0)) continue;
if (IPancakeV3Pool(pool).slot0() == 0) continue;
uint128 liq = IPancakeV3Pool(pool).liquidity();
if (liq > maxLiq) {
maxLiq = liq;
maxLiqFee = FEES[i];
}
}
}
/// @dev The tier that ACTUALLY delivers the most WETH for `amtIn`, by real swaps on each tier.
function _bestRealizedTier(uint256 amtIn) internal returns (uint256 bestOut, uint24 bestFee) {
for (uint256 i = 0; i < 4; i++) {
address pool = IUniswapV3Factory(PCS_V3_FACTORY).getPool(USDC, WETH, FEES[i]);
if (pool == address(0)) continue;
uint256 snap = vm.snapshotState();
uint256 out = _rawSwap(FEES[i], amtIn);
vm.revertToState(snap);
if (out > bestOut) {
bestOut = out;
bestFee = FEES[i];
}
}
}
/// @dev A raw Pancake V3 exact-in swap through a SPECIFIC fee tier, driving the REAL
///      UniversalRouter with the classic 5-field layout (recipient = this contract).
function _rawSwap(uint24 fee, uint256 amtIn) internal returns (uint256 out) {
deal(USDC, address(this), amtIn);
IERC20(USDC).approve(PCS_PERMIT2, amtIn);
IPermit2(PCS_PERMIT2).approve(USDC, PCS_UNIVERSAL_ROUTER, uint160(amtIn), 
uint48(block.timestamp + 1));
bytes memory path = abi.encodePacked(USDC, fee, WETH);
bytes memory commands = abi.encodePacked(uint8(0x00)); // V3_SWAP_EXACT_IN
bytes[] memory inputs = new bytes[](1);
inputs[0] = abi.encode(address(this), amtIn, uint256(0), path, true);
uint256 before = IERC20(WETH).balanceOf(address(this));
IUniversalRouter(PCS_UNIVERSAL_ROUTER).execute(commands, inputs, block.timestamp + 1);

out = IERC20(WETH).balanceOf(address(this)) - before;
}
/// @dev Extract the fee (bytes[20:23]) from a single-hop packed path token|fee|token (43 bytes).
function _pathFee(bytes memory path) internal pure returns (uint24 fee) {
require(path.length == 43, "unexpected path length");
// solhint-disable-next-line no-inline-assembly
assembly {
fee := shr(232, mload(add(path, 52)))
}
}
}

Select pools based on expected output rather than raw liquidity. The approach could be to query the DEX quoter for each candidate pool and choose the route with the highest quoted output. The same fix should be applied to both adapters, including the Uniswap adapter's V3/V4 routing logic.

The CoWSwapModule.registerLopOrder() function is used to register a 1inch Limit Order Protocol (LOP) maker order so that the module can ERC-1271-sign it.

function registerLopOrder(
bytes32 executionId,
uint256 deltaIndex,
IOrderMixin.Order calldata order,
uint256 minTakingAmount
) external onlyExecutionStrategy returns (bytes32 orderHash) {
...

if (!(ASSET_REGISTRY.isAssetEnabled(makerAsset) && _inUniverse(makerAsset)))
revert Error.NotAllowed("sell token");

if (!(ASSET_REGISTRY.isAssetEnabled(takerAsset) && _inUniverse(takerAsset)))
revert Error.NotAllowed("buy token");

...
}

The function requires both makerAsset and takerAsset to satisfy _inUniverse(), meaning they must be included in the strategy's allocation assets.

function _inUniverse(address asset) internal view returns (bool) {
address[] memory assets = IStrategyLogic(strategy).getAssets();
for (uint256 i; i < assets.length; ++i) {
if (assets[i] == asset) return true;
}
return false;
}

However, checking only the strategy's allocation assets is insufficient. The function should also allow the strategy's baseCurrency, as it may legitimately appear in rebalance orders. During the rebalance flow, CoWRebalancePlanLib.planOrders() generates order intents from the rebalance deltas. In both PASS 2 and PASS 3, the generated orders may use baseCurrency as either the sellToken or the buyToken.

function planOrders(
RebalanceDelta[] memory deltas,
PricingCtx memory ctx
) internal pure returns (OrderIntent[] memory intents) {
...

// PASS 2 — residual net-sells settle asset™base.
for (uint256 s; s < nSell; ++s) {
if (sells[s].value == 0) continue;
OrderIntent memory oi = _intent(
sells[s].asset,
ctx.baseCurrency,

sells[s].value, 
            ctx
        );
        if (oi.sellAmount > 0 && oi.floor > 0) buf[count++] = oi;
    }
 
    // PASS 3 — residual net-deploys settle base™as asset (funded from vault-held base).
for (uint256 b; b < nBuy; ++b) {
if (buys[b].value == 0) continue;
OrderIntent memory oi = _intent(
ctx.baseCurrency,
buys[b].asset,
buys[b].value,
ctx
);
if (oi.sellAmount > 0 && oi.floor > 0) buf[count++] = oi;
}

...
}

As a result, if baseCurrency is not included in the strategy's asset universe, otherwise valid rebalance orders will fail the _inUniverse() check and cannot be registered, preventing the rebalance from being completed. The same issue exists in OrderPolicyLib.validate().

Consider treating the strategy's baseCurrency as a valid asset in addition to the strategy's allocation assets when validating makerAsset and takerAsset. The same change should be applied to OrderPolicyLib.validate() to ensure rebalance orders involving baseCurrency are handled correctly.

While a 1inch LOP rebalance order is waiting to be filled, the vault overstates its NAV, allowing users to redeem more assets than their shares are actually worth. The resulting loss is borne by the remaining vault holders once the order settles. This happens because the vault continues valuing the assets committed to the LOP order at the current market price, even though they can only be sold at the order's minimum execution price. The valuation logic is intended to discount assets that have already been committed to an order:

try IAuxBalanceSource(auxSource_).outstandingBudget(asset_) returns (uint256 c_) {
committed_ = c_;
} catch {
committed_ = balance_;
}
if (committed_ > 0) {
value_ -= discount;
}

This works for CoW orders because their committed amount is stored in outstandingBudget. However, LOP orders track committed assets in a separate lopOutstanding mapping, which is never read during valuation.

As a result, outstandingBudget(asset_) returns 0 for LOP orders, committed_ remains zero, the discount is skipped, and the committed assets continue to be valued at the current market price. This temporarily inflates NAV until the order is filled.

As the consequence, A user can redeem while an LOP order is in flight and receive an inflated payout. Once the order settles at its lower execution price, the difference is paid by the remaining shareholders.

Consider introducing a new function commitedBudget() which includes both the outstandingBudget and lopOutstanding and use that function when compute the committed balance on the auxSource_.

Currently, ROWAVaultUpgradable.totalAssets() uses getPortfolioValue() from PortfolioValuatorUpgradeable to determine the value of the assets held by the strategy in the base currency.

function getPortfolioValue(
address strategy
) external view override returns (uint256 totalValue) {
...

if (auxSource_ != address(0)) {
totalValue += _auxSourceValue(
auxSource_,
assets,
baseCurrency_,
_auxSlippageCap(strategy) 1000)
);
}

...
}
function _auxSourceValue(...) internal view returns (uint256 total_) {
...

if (committed_ > 0) {
uint256 committedValue_ = Math.mulDiv(committed_, price_, 10 ** decimals_);
value_ -= Math.mulDiv(
committedValue_,
slippageCapBps_,
BasisPointsLib.MAX_BPS

);
}

...
}

Notably, _auxSourceValue() applies a discount to all assets held in the auxSource, except for the base currency. From the perspective of totalAssets() alone, this behavior is reasonable because totalAssets() represents the amount of base currency that could be obtained if all assets were liquidated immediately.

However, totalAssets() is also used to determine the exchange rate between shares and assets during deposits and withdrawals. As a result, the discounted valuation of assets in the auxSource can be exploited by users to extract value from the vault.

For example, while an asynchronous execution is in progress, a user can deposit assets while the auxSource assets are being valued at a discount. This causes the user to receive more shares than they would have received based on the assets' actual value. Once the asynchronous execution completes, either through a successful fill or cancellation, the value of totalAssets() increases as the discount is removed. The user can then withdraw the shares obtained from the earlier deposit at the higher share price, capturing the resulting increase in value.

Consider using different valuation mechanisms for deposits and withdrawals: Deposits: value all assets held in the auxSource at their full value without applying the slippage discount.

Withdrawals: apply the slippage discount to all assets held in the auxSource, including the base currency. This prevents users from acquiring excess shares based on a temporarily discounted auxSource valuation while still ensuring that withdrawals conservatively account for the potential liquidation cost.

The OneInchLOPAdapter.swap() function is used to fill a 1inch Limit Order Protocol maker order encoded in the routeData parameter. The function can only be called by the router (ExecutionRouterUpgradeable).

function swap(
address sellAsset,
address buyAsset,
uint256 sellAmount,
uint256 minBuyAmount,
bytes calldata routeData
) external override onlyRouter whenNotPaused nonReentrant returns (uint256 buyAmount) {
IOrderMixin.Order memory order = _decodeOrder(routeData);
...
}

As shown above, the order is decoded from routeData. Tracing the call flow, there is only one location in ExecutionRouterUpgradeable that invokes swap(), namely _executeOrderInternal().

function _executeOrderInternal(
bytes32 orderId,
address adapter,
uint16 maxSlippageBps
) internal returns (ExecutionResult memory result) {
...

if (isSwap) {
ISpotTradeAdapter(effectiveAdapter).swap(
order.sellAsset,

order.buyAsset,
remainingAmount,
effectiveMinBuyAmount,
order.executionData
);
}
...
}

There are two code paths that reach _executeOrderInternal(): executeOrder() and swapImmediate().

For executeOrder(), the specified order must already exist, which only happens after it has been registered through submitOrder(). However, submitOrder() can only be called by a strategy, and we could not identify any code path in StrategyLogic that invokes this function. Consequently, executeOrder() cannot be used to reach OneInchLOPAdapter.swap().

For swapImmediate(), there are two call sites:

• LiquidationLib.liquidate()

• BaseExecutionStrategy._executeDeltas() ™ _ _executeSell() / _executeBuy()

In both cases, hints.adapterSpecificData, which is ultimately forwarded as routeData to OneInchLOPAdapter.swap(), is always initialized as an empty byte array.

hints = ExecutionHints({
maxSlippageBps: maxSlippageBps,
urgency: Urgency.IMMEDIATE,
preferredAdapters: preferredAdapters,
excludedAdapters: new address[](0),
allowPartialFill: false,
minFillPercent: 10_000,
adapterSpecificData: ""
});

As a result, even if swapImmediate() reaches OneInchLOPAdapter.swap(), the function attempts to decode an order from an empty routeData value and cannot execute successfully. In conclusion, we could not identify any valid code path that supplies the required order data to OneInchLOPAdapter.swap(). As a result, 1inch LOP orders are unable to be executed through the adapter.

Consider triggering the submitOrder when registering new orders using 1inch.

The CurveGaugeModule.harvest() function mints all accrued CRV rewards and transfers them directly to the vault:

function harvest() external nonReentrant returns (uint256 minted_) {
uint256 before = crv.balanceOf(address(this));
try minter.mint(address(gauge)) {} catch {}
minted_ = crv.balanceOf(address(this)) - before;
if (minted_ > 0) {
crv.safeTransfer(vault, minted_);
}
try gauge.claim_rewards(address(this), vault) {} catch {}
emit Harvested(minted_, vault);
}

However, a problem arises when CRV is not included as an asset in the vault's allocations. In this case, the harvested CRV is transferred to the vault but is not accounted for when calculating the vault's NAV. As a result, the vault's NAV will be understated by the amount of unallocated CRV rewards.

More importantly, the CRV can become unrecoverable through any designed flow;. Even if the strategist or admin later becomes aware of the issue and attempts to add CRV to the vault's allocations, this is not possible because validateAndReplace requires the allocation's underlying asset to remain unchanged after the modification.

One might argue that the strategist should be aware that the Curve gauge generates CRV rewards and include CRV in the allocations before deploying the vault. However, this assumption does not hold for dynamic strategies.

For example, suppose the vault's initial intention does not include a Curve gauge as a staking adapter because the expected CRV rewards are too low to justify staking the underlying assets. At some point in the future, however, CRV emissions may increase sufficiently for the strategist to decide to allocate assets to the gauge to improve the vault's performance. Since CRV was not included in the original allocations, the subsequently harvested CRV would not be reflected in the vault's NAV and could become permanently inaccessible. Note that the issue does not only apply to CRV rewards; it also applies to any additional rewards provided by the gauge.

Consider swapping the reward tokens to the base asset then transferring it to the vault.

The ActionLib.derivePortfolioValues() function is used to calculate the total portfolio value, including any unallocated base currency held by the vault:

function derivePortfolioValues(
IPortfolioValuator valuator,
address strategy,
address[] memory assets,
address baseCurrency
) internal view returns (uint256[] memory values, uint256 totalValue) {
(values, totalValue) = valuator.derivePositionValues(strategy, assets);
// Check if base currency is in the allocation set
bool baseCurrencyInAllocations;
for (uint256 i; i < assets.length; ) {
if (assets[i] == baseCurrency) {
baseCurrencyInAllocations = true;
break;
}
unchecked {
++i;
}
}
// Add unallocated base currency balance to total
if (!baseCurrencyInAllocations) {
address vault_ = IStrategyLogic(strategy).vault();
uint256 baseBal = IERC20(baseCurrency).balanceOf(vault_);
if (baseBal > 0) {
uint8 baseDecimals = IERC20Metadata(baseCurrency).decimals();
totalValue += Math.mulDiv(baseBal, 1e18, 10 ** baseDecimals);

}
}
}

The function first derives the value of all allocated positions through derivePositionValues(). If the base currency is not included in the allocation set, it then adds the base currency balance held directly by the vault to the total portfolio value.

However, the function only accounts for the unstaked base currency balance held by the vault and does not account for any base currency that has been staked elsewhere.

As a result, whenever the vault's base currency has been staked, the staked amount is omitted from the portfolio valuation. This causes the portfolio value to be understated and may cause the incorrect behaviour in the action handler (eg: PartialRebalanceActionHandler).

Consider including the stake balance into the value calculation in the ActionLib.derivePortfolioValues() function.

MidasMintAdapter.mint()pu pulls the base currency amount from the router and forwards the same value to Midas:

_pullTokens(BASE_CURRENCY, amount);
DEPOSIT_VAULT.depositInstant(
BASE_CURRENCY,
amount,
minReceiveAmount,
referrerId
);

However, Midas expectsamountto be expressed in18-decimal format, while the adapter passes the amount in the token'snative decimals(e.g. USDC = 6 decimals).

For example, a deposit of 20 USDC is pulled as20e6and passed directly to Midas, but Midas expects20e18. Midas therefore converts the value back to native decimals and attempts to pull 0 USDC.

As a result, deposits will revert.

Midas expects the deposit amount in 18 decimals, so scale the base-currency amount up before the call (e.g. amount * 10**(18 - BASE_DECIMALS)) and pass that to depositInstant.

PortfolioValuator._getStakedBalance() calls each staking adapter with a hard gas limit and treats any failure as fatal:

try
IStakingAdapter(options_[j].adapter).getPosition{ gas: ADAPTER_GAS_LIMIT }(
strategy_,
asset_
)
returns (IStakingAdapter.StakingPosition memory position_) {
...
} catch {
revert Error.StakingValuationFailed(options_[j].adapter, asset_);
}

ADAPTER_GAS_LIMIT is set to 100,000 gas. If getPosition() exceeds this limit, the call runs out of gas, the failure is caught, and the entire portfolio valuation reverts.

This can occur with AaveV3BorrowAdapter.getPosition(), which may require more than 100,000 gas when executed with cold storage accesses. The existing test_fork_aave_netEquity_underGasCap does not detect this because _supplyAndBorrow() is executed in the same transaction before the gas measurement. This warms the Aave pool, oracle, Chainlink feeds, and other storage slots under EIP-2929, causing the test to measure only ~32k gas instead of the ~153k gas required for a cold call in a real user transaction.

Consider increase the gas limit or use a per-adapter gas limit based on cold-state measurements. Otherwise we can remove the gas limit for the getPosition().

Proof of Concept: The PoC below demonstrates the difference between cold and warm execution:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.30;
import { IERC20 } from "@openzeppelin/contracts/interfaces/IERC20.sol";
import { console } from "forge-std/console.sol";
import { ForkAaveBorrow } from "./ForkAaveBorrow.fork.t.sol";
import { IStakingAdapter } from "../../interfaces/adapters/IStakingAdapter.sol";
import { IConstituentBound } from "../../interfaces/IConstituentBound.sol";
import { StrategyConfig } from "../../vault/StrategyConfig.sol";
interface IAaveOracleLike {
function getAssetPrice(address asset) external view returns (uint256);
function BASE_CURRENCY_UNIT() external view returns (uint256);
}
contract ForkAaveBorrowAuditPoC is ForkAaveBorrow {
address internal constant AAVE_ORACLE = 0x54586bE62E3c3580375aE3723C145253060Ca0C2;
uint256 internal constant ADAPTER_GAS_LIMIT = 100_000;

function setUp() public override {
super.setUp();
_supplyAndBorrow();
}
function test_fork_auditPoC_M2_getPositionGas_coldVsWarm() public {
bytes memory call = abi.encodeCall(
IStakingAdapter.getPosition,
(address(abStrategy), USDC)
);
// ---- COLD: the first thing this transaction touches ----
uint256 g0 = gasleft();
(bool okCold, ) = address(adapter).staticcall(call);
uint256 usedCold = g0 - gasleft();
assertTrue(okCold, "cold read must at least succeed unbounded");
// ---- WARM: everything above is now in the access list ----
uint256 g1 = gasleft();
(bool okWarm, ) = address(adapter).staticcall(call);
uint256 usedWarm = g1 - gasleft();
assertTrue(okWarm, "warm read");

console.log("getPosition gas COLD :", usedCold);
console.log("getPosition gas WARM :", usedWarm);
console.log("ADAPTER_GAS_LIMIT    :", ADAPTER_GAS_LIMIT);
// Method self-validation: if warmth were not reset between setUp and the test body,
// these two would be equal and the cold figure would be meaningless.
assertGt(usedCold, usedWarm, "warmth IS reset per tx: the cold number is real");
if (usedCold > ADAPTER_GAS_LIMIT) {
console.log(">>> COLD READ EXCEEDS THE FRAME. Overrun:", usedCold - 
ADAPTER_GAS_LIMIT);
} else {
console.log(">>> cold headroom under the frame:", ADAPTER_GAS_LIMIT - usedCold);
}
}
}

Output

getPosition gas COLD : 156945
getPosition gas WARM : 32782

AllocateToYieldActionHandler allows the caller to stake through any adapter. The adapter address is read directly from actionParams, and StakingExecutionLib._validate() only requires the adapter to be whitelisted for the underlying asset, active, and support STAKE. It does not require the adapter to be included in allocation.adapters or to be the first entry:

// StakingExecutionLib.sol:227-235
if (!assetRegistry.isAdapterWhitelisted(underlying, stakingAdapter)) revert ...
if (!assetRegistry.isAdapterActive(stakingAdapter)) revert ...
if (!IBaseAdapter(stakingAdapter).supportsOperation(op)) revert ...

However, HarvestYieldActionHandler resolves exactly one adapter per asset. The internal _resolveStakingAdapter() function returns the first active STAKING entry in allocation.adapters; otherwise, it falls back to AssetRegistry.getAdapterByType():

// HarvestYieldActionHandler.sol:258-278
for (uint256 j; j < allocation.adapters.length; ) {
...
if (info.adapterType == IAssetRegistry.AdapterType.STAKING && info.active) {
return candidate;   // first match wins
}
...
}
return assetReg.getAdapterByType(allocation.asset, IAssetRegistry.AdapterType.STAKING);

Consequently, positions staked through any adapter other than the resolved adapter are never harvested through this action. Multi-venue staking appears to be the intended model, making this single-adapter resolution an outlier:

• PortfolioValuator._getStakedBalance() iterates over all active staking options, so NAV already accounts for multiple venues while harvesting only claims from one.

• AssetRegistry.getAdaptersForAssetByType() already returns an array of adapters, but the handler does not use it.

The impact is currently latent rather than realised. CurveGaugeStakingAdapter is the only adapter whose claimRewards() currently performs any action; Lido, MetaMorpho, CurveStableLP, and CurveCryptoLP all return 0, while both borrowing adapters revert. Additionally, CurveGaugeModule.harvest() is permissionless, so CRV can still reach the vault through a direct call outside the handler.

Nevertheless, the issue is structural: the staking path, configuration schema, and valuator all support multiple venues, while the harvest path is hard-wired to a single adapter. As soon as a second rewarding adapter is introduced without a similar fallback, its rewards can become stranded.

Consider harvesting from all staking venues for the asset rather than only the first resolved adapter. AssetRegistry already exposes the required plural accessor, and the existing try/catch in _harvestPosition() provides isolation so that a failure in one adapter does not prevent harvesting from the others:

address[] memory stakingAdapters = assetReg.getAdaptersForAssetByType(
asset,
IAssetRegistry.AdapterType.STAKING
);
for (uint256 k; k < stakingAdapters.length; ++k) {
uint256 claimed = _harvestPosition(
strategy,
asset,
stakingAdapters[k],
assetReg
);
...
}

CurveGaugeModule.harvest() is responsible for claiming CRV and any additional rewards from the Curve Gauge and forwarding them to the vault:

function harvest() external nonReentrant returns (uint256 minted_) {
uint256 before = crv.balanceOf(address(this));
try minter.mint(address(gauge)) {} catch {
// Gauge not (yet) registered for CRV emissions — extras below still flow.
}
minted_ = crv.balanceOf(address(this)) - before;
if (minted_ > 0) {
crv.safeTransfer(vault, minted_);
}
try gauge.claim_rewards(address(this), vault) {} catch {
// Gauge has no extra-rewards surface — CRV path above is unaffected.
}
emit Harvested(minted_, vault);
}

However, the Gauge's claim_rewards() function is permissionless, allowing anyone to trigger a reward claim on behalf of the module. When _receiver is address(0), the Gauge's internal _checkpoint_rewards() function resolves the reward recipient to the _user address if no default rewards_receiver is configured:

def _checkpoint_rewards(_user: address, _total_supply: uint256, _claim: bool, _receiver: address):
user_balance: uint256 = 0
receiver: address = _receiver
if _user != ZERO_ADDRESS:

user_balance = self.balanceOf[_user]
if _claim and _receiver == ZERO_ADDRESS:
receiver = self.rewards_receiver[_user]
if receiver == ZERO_ADDRESS:
receiver = _user

Since the module address is used as _user, an attacker can front-run harvest() and directly call:

gauge.claim_rewards(address(CurveGaugeModule), address(0));

If no rewards_receiver is configured for the module, the extra rewards are transferred to the module itself rather than the vault. Because harvest() subsequently calls claim_rewards() again, the rewards may no longer be available for the intended transfer and can become stranded in the module.

Consider setting the module's default reward receiver to the vault during initialization:

@external
def set_rewards_receiver(_receiver: address):
"""
@notice Set the default reward receiver for the caller.
@dev When set to ZERO_ADDRESS, rewards are sent to the caller
@param _receiver Receiver address for any rewards claimed via `claim_rewards`
"""
self.rewards_receiver[msg.sender] = _receiver

This ensures that even if an attacker triggers claim_rewards() directly, the additional rewards are sent to the vault rather than remaining in the module.

StrategistLendingGateway allows the vault's strategy deployer to execute lending operations on behalf of their vault through an active lending adapter:

function execute(
address router,
LendingCall[] calldata calls
) external returns (uint256[] memory moved) {
if (router == address(0)) revert Error.ZeroAddress();
if (calls.length == 0) revert Error.NotAllowed("Gateway: empty batch");
address vault = IRouterVaultView(router).vault();
if (msg.sender != IVaultWiringView(vault).strategyDeployer()) {
revert Error.NotAllowed("Gateway: caller is not the vault strategist");
}
address strategy = IVaultWiringView(vault).strategyLogic();
if (strategy == address(0)) {
revert Error.NotAllowed("Gateway: vault not wired");
}
moved = new uint256[](calls.length);
for (uint256 i = 0; i < calls.length; ++i) {
LendingCall calldata c = calls[i];
uint256 floor = _effectiveFloor(c, strategy);
moved[i] = ILendingExecutionRouter(router).lendingOperation(
c.adapter,
c.op,
c.amount,

c.minMoved,
abi.encode(floor)
);
emit StrategistLendingExecuted(
vault,
msg.sender,
c.adapter,
c.op,
c.amount,
moved[i],
floor
);
}
}

However, the function does not restrict which asset the strategist can borrow. As a result, a malicious strategist can borrow an asset that is neither included in the vault's allocations nor used as its base currency. Because the borrowed asset is not accounted for in the vault's NAV, the resulting debt can reduce the vault's NAV and create an opportunity for the strategist to profit:

1. Borrow a non-allocation asset, reducing the vault's NAV.

2. Deposit assets into the vault at the artificially reduced share price, receiving more shares than they should.

3. Repay the borrowed debt, restoring the vault's NAV.

4. Redeem the shares at the restored NAV and capture the resulting profit.

The design documentation states: Register the adapter (LENDING) and add the staking option on the DEBT asset (the net-equity NAV mark). DEBT must be a USD-pegged stable — the mark converts Aave's USD- denominated account data at par; USD~USDC par is an accepted residual (fail-LOW under a real depeg). Therefore, the debt asset is expected to be the vault's base currency. Enforcing this restriction would ensure that borrowed debt is always represented in the vault's NAV and prevent the strategist from manipulating the share price by borrowing an unaccounted asset.

function execute(
address router,
LendingCall[] calldata calls
) external returns (uint256[] memory moved) {
if (router == address(0)) revert Error.ZeroAddress();
if (calls.length == 0) revert Error.NotAllowed("Gateway: empty batch");
address vault = IRouterVaultView(router).vault();
if (msg.sender != IVaultWiringView(vault).strategyDeployer()) {
revert Error.NotAllowed("Gateway: caller is not the vault strategist");
}
address strategy = IVaultWiringView(vault).strategyLogic();
if (strategy == address(0)) {
revert Error.NotAllowed("Gateway: vault not wired");
}

++  address baseCurrency = strategy.asset();
moved = new uint256[](calls.length);
for (uint256 i = 0; i < calls.length; ++i) {
LendingCall calldata c = calls[i];
uint256 floor = _effectiveFloor(c, strategy);

++      require(ILendingAdapter(c.adapter).debtAsset() == baseCurrency);
moved[i] = ILendingExecutionRouter(router).lendingOperation(
c.adapter,
c.op,
c.amount,
c.minMoved,
abi.encode(floor)
);
emit StrategistLendingExecuted(
vault,
msg.sender,
c.adapter,
c.op,
c.amount,
moved[i],
floor
);
}
}

LiquidationLib hard-codes an empty payload for every sale it attempts:

ExecutionHints memory hints = ExecutionHints({
...
adapterSpecificData: ""
});

However, AaveV3SupplyAdapter.redeem() requires redemption parameters and rejects an empty payload:

if (data.length == 0) revert Error.InvalidRedeemData();

The call is wrapped in try ... catch {} (LiquidationLib.sol:136-147), so the revert is swallowed and the corresponding position is silently skipped during liquidation.

try
p.router.swapImmediate(
p.swappable[i],
p.baseCurrency,
assetAmount,
minOutNative,
hints
)
returns (uint256) {
unchecked {
++positionsSold;
}
} catch {}

This creates a mismatch with the vault's withdrawal-liquidity calculation. withdrawalLiquidity() considers any allocation leg with a primary adapter as sellable. Since aToken positions have a primary adapter, their value is included as available liquidity for withdrawals.

Consequently, the vault may report sufficient liquidity for a withdrawal that requires selling aToken positions, while the liquidation process is unable to redeem those positions and silently skips them. The required cash is therefore not raised, causing the withdrawal to fail and potentially leaving users' funds temporarily locked.

Furthermore a related issue exists in BaseExecutionStrategy._buildDeltaHints(), although it has a lower impact. The function hard-codes the minimum amount to zero:

if (isBuy && IBaseAdapter(preferredAdapter).supportsOperation(OperationType.MINT)) {
deltaHints.adapterSpecificData = abi.encode(uint256(0), bytes32(0));
} else if (!isBuy && IBaseAdapter(preferredAdapter).supportsOperation(OperationType.REDEEM)) {
deltaHints.adapterSpecificData = abi.encode(uint256(0));
}

For the MINT path, this does not cause a revert. abi.decode(data, (uint256)) successfully decodes the first 32 bytes of the 64-byte payload, ignoring the trailing word. As a result, minAtokensOut is decoded as 0, making the adapter's minimum-output check ineffective:

if (received < minAtokensOut) {
revert Error.NotAllowed("AaveV3Supply: mint below min");
}

Moreover, the oracle-derived effectiveMinBuyAmount never reaches the MINT/REDEEM adapters. _executeOrderInternal() passes this value only to the SWAP branch, while MINT/ REDEEM operations use order.executionData.

Therefore, the configured minimum buy amount is not properly enforced for MINT/REDEEM operations, potentially allowing the transaction to execute with an insufficient output amount. The same issues apply to Midas mTokens.

Consider encoding the scaled, oracle-raised minimum instead of uint256(0), so the MINT/ REDEEM legs carry the same floor the SWAP branch already enforces.

The function _accrueFeeShares() is intended to prevent fee accrual while the vault is not active. At lines 820 824, it explicitly exits early when the vault is paused or stopped:

function _accrueFeeShares() internal {
...

// Skip accrual if vault is not active (paused/stopped)
if (vaultState != VaultState.Active) {
lastAccrual = uint64(block.timestamp);
return;
}

...
}

However, the pause(), unpause(), and stop() functions do not adjust lastAccrual or settle accrued state during state transitions. This can lead to incorrect fee accrual across inactive periods.

Consider the following scenario:

1. At time T1, the vault is paused. Since _accrueFeeShares() is not called during pausing, lastAccrual remains d T1.

2. At time T2, the vault is unpaused. No activity occurs between T1 and T2, so lastAccrual is still d T1.

3. When the next user action occurs (deposit, withdraw, etc.), _accrueFeeShares() is triggered and accrues fees from lastAccrual up to the current timestamp.

This results in fees being accrued over the interval [T1, T2], even though the vault was inactive during that entire period. Consequently, the treasury receives fee shares for a time window in which no active vault operations were permitted.

Invoke _accrueFeeShares() explicitly during state transitions (pause, unpause, stop) to ensure fee accounting is properly synchronized.

--     function pause(string calldata reason_) public {
++     function pause(string calldata reason_, bool accrueFee) public {
++            if (accrueFee) _accrueFeeShares()
...
}

--     function unpause() public {
++     function unpause(bool accrueFee) public {
++            if (accrueFee) _accrueFeeShares()
...
}

--     function stop() public {
++     function stop(bool accrueFee) public {
++            if (accrueFee) _accrueFeeShares()
...
}

Introducing the accrueFee flag also provides operational flexibility, preventing a scenario where a failure inside _accrueFeeShares() (e.g., a revert condition) could block critical lifecycle actions such as pausing or stopping the vault.

In the liquidate() function, inside the per-asset loop, targetSell18 gets distributed pro-rata across each swappable asset, then converted from value into a concrete token amount assetAmount.

uint256 assetAmount = Math.mulDiv(balance, targetSell18, totalSwappable18);

targetSell18 / totalSwappable18 is the fraction of the entire portfolio that needs to be liquidated, while balance is the vault's asset token balance.

uint256 balance = IERC20(p.swappable[i]).balanceOf(p.vault);

However, minOutNative is used as a minimum check for the swap of assetAmount, but it is not scaled by the vault balance. Instead, it is calculated using values18[i], which is scaled by walletBalance + stakedBalance.

// LiquidationLib.sol
(uint256[] memory values18, uint256 totalSwappable18) = p.valuator.derivePositionValues(
p.strategy,
p.swappable
);
...
uint256 expectedProceedsValue18 = Math.mulDiv(values18[i], assetAmount, balance);
uint256 minOutNative = Math.mulDiv(
(expectedProceedsValue18 * (10_000 - p.maxSlippageBps)) / 10_000,
10 ** p.baseDecimals,
1e18
);

The position value array values18 comes from PortfolioValuator.derivePositionValues(), which values the total balance (walletBalance + stakedBalance):

// PortfolioValuatorUpgradeable.sol
uint256 walletBalance_ = IERC20(asset_).balanceOf(vault_);
uint256 stakedBalance_ = _getStakedBalance(asset_, registry_);
uint256 totalBalance_  = walletBalance_ + stakedBalance_;     // L221
...
values_[i] = Math.mulDiv(totalBalance_, price_, 10 ** decimals_); // L268

Because values18[i] represents the value of (wallet + staked), while balance represents only the wallet balance, the implied per-unit value, values18[i] / balance, is overstated by a factor of (wallet + staked) / wallet. As a result, minOutNative is inflated by the same factor, making it be greater than liquidated asset value, and causing the swap in liquidate() to revert. ( Consequently, any investor whose withdrawal requires the liquidation of a swappable asset with both wallet and staked balances cannot complete that withdrawal.

When a withdrawal fee is enabled, withdraw() and redeem() calculate a fee amount, but the internal withdrawal flow only ensures that the vault has enough base asset for the net user payout. The fee is transferred to the treasury after _withdraw() completes.