Kiln Phase 3: Fix Reviews

Description

The new implementation of the SectorAccountingEngine._dispatchRedeem will revert or early return (depending on the strict parameter) if the vehicle from which the SAE is redeeming has been unauthorized.

This change creates two problems:

1. The new behavior is going against one important assumption and invariant previously held by the protocol: users should always be able to redeem from a sub-vehicle even if such sub-vehicle has been unauthorized.
2. The QueueStrategyEngine.unallocate function is not aligned to the new behavior of the SectorAccountingEngine._dispatchRedeem function creating a side effect issue that would prevent the protocol/users to be able to redeem assets when SectorAccountingEngine.requestWithdrawable is executed.

The QueueStrategyEngine could select a vehicle that is not authorized but will skip (or revert) when the _dispatchRedeem loop iteration is executed. The result is that the total shares unallocated will be lower than what they could have been if other vehicles would have been selected by the QueueStrategyEngine.

Let's assume that we have this queue in the QueueStrategyEngine

• vehicle_1 not authorized
• vehicle_2 authorized

We want to deallocate 100 assets and both vehicles can sustain that request from an allocation POV.

The QueueStrategyEngine will consume the 100 assets requested by using vehicle_1, but the _dispatchRedeem will silently fail (when invoked by requestWithdrawable) because the vehicle_1 is not authorized.

Recommendation

Kiln should explain why the SectorAccountingEngine dispatch redeem behavior and logic have been changed and refactor the code to correctly handle the issue described above.

Spearbit

Acknowledged. This issue will be checked in the next audit.

Description

The _screenUserMove function is executed by the Conduit contract when transfer and transferFrom are executed. Those functions are executed when Conduit's shares are moved from an account to another and the Conduit needs to validate if the action can be allowed depending on the Conduit and AccountList configuration/rules.

function _screenUserMove(address from, address to) internal view {        ConduitStore.Storage storage $ = ConduitStore.getStorage();        // FeeManager fee dispatch bypasses the gate: accrued cShare fees must be dispatchable regardless of        // transferEnabled/AccountList. Exempt as a SENDER only; screening then rests on keeping fee recipients clean.        if (from == address($.feeManager)) {            return;        }        if (!_ready()) {            // Pre-enable: only the deployer may move shares.            if (msg.sender != $.deployer) {                revert ConduitErrors.TransferNotAllowed(from, to);            }        } else if (!_transferAllowed($, from, to)) {            revert ConduitErrors.TransferNotAllowed(from, to);        }    }

The current logic is making a "special" case when the from address is the $.feeManager and is basically whitelisting both the $.feeManager but also the fee recipient (see how FeeManager.dispatchERC20 works).

For a "normal" user the _screenUserMove function would have reverted if

• the Conduit was not ready
• the $.transferEnabled is equal to false
• the _accountList.canTransfer(from, to) returns false

The AccountList.canTransfer returns false when

• from or to is sanctioned
• from or to is in the blacklist
• $.mode is AllowlistMode.STRICT and from and to are not both in the whitelist

Once the to user has received the shares, the same permissions are not applied when it will create a REDEEM request (to receive the corresponding assets) if the receiver of the assets is the same as msg.sender. In that case the Conduit is much less restrictive because AccountList.canRedeem is executed, which reverts only if the user is sanctioned.

The current behavior could allow, for example, a blocked or not whitelisted (when the mode is strict) Fee Receiver to receive and redeem shares even if the account is indeed in a blacklist or not in a whitelist.

Kiln should apply the same rules applied to the "normal" users also to the Fee Recipients of the Fee Manager.

Recommendation

Kiln should

1. Not allow the Fee Manager to transfer Conduit's shares when the Conduit is not ready
2. Skip the $.transferEnabled if the from is the Fee Manager but apply anyway the AccountList.canTransfer checks (when configured)
3. The Fee Manager and the Fee Recipients should be properly configured inside the Access Manager to be allowed (or not) to perform the transfer and receive them.

Spearbit

Acknowledged. This issue will be checked in the next audit. Kiln's follow-up fix in PR 459 (commit ef8e1d0e, respecting !ready() and screening the destination) appears to target this finding and should be verified.

Description

As already explained in the "SectorAccountingEngine._dispatchRedeem should not early return/revert when the vehicle is not authorized anymore" Finding, it's possible that during the requestWithdrawable execution, the QueueStrategyEngine contract selects a vehicle that will be silently skipped by the _dispatchRedeem execution (when strict=false).

When _manualShortfall == 0 in requestWithdrawable it means that the QueueStrategyEngine was able to fulfill the whole withdrawal request with the vehicles available in the queue system.

But if the _dispatchRedeem silently fails because the vehicle is not authorized anymore, the premise held by _manualShortfall won't be true anymore. The requestWithdrawable has not been able to redeem the full requested amount while the RequestWithdrawableShortfall has not been triggered to signal it.

Recommendation

Kiln should consider refactoring the logic to trigger RequestWithdrawableShortfall if the underlying _dispatchRedeem was unable to request the redeem for the whole expected amount.

Spearbit

Acknowledged. This issue will be checked in the next audit.

Description

Both the allocate and unallocate functions in QueueStrategyEngine have a new logic that must account for previous iterations "commitment" that have been fulfilled by the same vehicle toward a deposit or redeem.

The logic is not using these commitment values against the upper bound deposit target and the lower bound redeem target.

unallocate

uint256 _vehicleSharesHoldings = _accountingEngine.vehicleSettledShares(_currentQueueEntry.vehicle);
            // 3. we check if the vehicle is not already under the target (with threshold tolerance), otherwise we skip it
            uint256 _effectiveTarget = _currentQueueEntry.target.upperBound();            if (_vehicleSharesHoldings <= _effectiveTarget) {                continue;            }

The _vehicleSharesHoldings variable holds the amount of shares of a vehicle that the MV owns. Let's say that the queue is configured like this (for the same vehicle)

• remainingAssets = 100
• 1 share = 1 asset (to make things easy for the example)
• entry 0: target = 80, threshold = 10
• entry 1: target = 70, threshold = 10
• _accountingEngine.maxRedeemable returns a value like ~INF (we don't care)

_vehicleSharesHoldings is equal to 100 shares

First iteration

_vehicleSharesHoldings = 100_effectiveTarget = 80 + 10 = 90100 <= 90 → FALSE → OK_deltaSharesToTarget = 100 - 80 = 20_deltaSharesToTarget = 20 - 0 (_priorShares) = 20_sharesToUnallocate → 20

The first iteration has "committed" to deallocate 20 shares (20 assets).

So theoretically the _dispatchRedeem will deallocate, after the first loop, 20 shares and the vehicle would return _vehicleSharesHoldings = 80.

Now let's see the second iteration of unallocate

_vehicleSharesHoldings = 100_effectiveTarget = 70 + 10 = 80100 <= 80 → FALSE → OK_deltaSharesToTarget = 100 - 70 = 30_deltaSharesToTarget = 30 - 20 (_priorShares) = 10_sharesToUnallocate → 10

The second iteration has unallocated 10 more shares, but in reality if we had "simulated" correctly the deallocations of the shares from the first iteration the check if (_vehicleSharesHoldings <= _effectiveTarget) { would have returned TRUE because the _vehicleSharesHoldings is not 100 but 100 - _priorShares = 80.

The result is that the unallocate has 10 unallocated more shares than the _currentQueueEntry.target should have permitted because we are not including _effectiveTarget in the if (_vehicleSharesHoldings <= _effectiveTarget) check

allocate

The same problem that I described for the unallocate function also happens for the allocate one.

uint256 _vehicleSharesHoldings = 0;            if (!_effectiveTarget.isUnlimited()) {                _vehicleSharesHoldings = _accountingEngine.vehicleEstimatedShares(_currentQueueEntry.vehicle);
                // 6. Skip if at target (within threshold tolerance)                if (_effectiveTarget.isAtTarget(_vehicleSharesHoldings)) {                    continue;                }            }

Let's say that we are in this scenario:

• share:asset ratio is 1
• _vehicleSharesHoldings = 70
• maxDepositable = INF (we don't care)
• _config.cap = INF (we can allocate as much as we want from the VehicleManager POV)
• entry 0: target = 80, threshold = 0
• entry 1: target = 100, threshold = 20

First iteration

_priorAssets = 10_effectiveTarget = _currentQueueEntry.target_vehicleSharesHoldings = 70_effectiveTarget.isAtTarget(70) = FALSE 	holdings >= t.value - t.threshold -> 70 >= 80-0 -> 70 >= 80 -> FALSE_deltaSharesToTarget = _effectiveTarget.remainingCapacity(70) -> 80-70 = 10 shares_deltaAssetsToTarget = 10 assets - 0 (_priorAssets) = 10 assets

So "theoretically" after the first iteration of the _dispatchDeposit we have allocated 10 assets and the new _vehicleSharesHoldings = 80

Now let's see the second iteration

_priorAssets = 0_effectiveTarget = _currentQueueEntry.target_vehicleSharesHoldings = 70_effectiveTarget.isAtTarget(70) = FALSE 	holdings >= t.value - t.threshold -> 70 >= 100-20 -> 70 >= 80 -> FALSE_deltaSharesToTarget = _effectiveTarget.remainingCapacity(70) -> 100-70 = 30 shares_deltaAssetsToTarget = 30 assets - 10 (_priorAssets) = 20 assets

The second iteration has allocated 20 assets.

This would not have happened if _effectiveTarget.isAtTarget(_vehicleSharesHoldings)) would have included the prior committed asset in the check by evaluating _effectiveTarget.isAtTarget(_vehicleSharesHoldings + _priorAssets_CONVERTED_IN_SHARES)).

Recommendation

Kiln must account for _priorAssets and _priorShares inside the sanity checks performed against the targets used by the allocate and unallocate functions to avoid overdepositing or overredeeming compared to what's expected by the configured target.

Spearbit

Acknowledged. This issue will be checked in the next audit.

Description

• AccountList.sol?lines=369,371: the scenario where address(_oracle) == address(0) should be impossible in AccountList._isSanctioned after the validation of if (!$.sanctionsEnabled) {. toggleSanctions, setSanctionsOracle and initialize reverts when $.sanctionsEnabled && $.sanctionsOracle == address(0). Consider removing it after proper review.
• OwnerRegistry.sol?lines=315,322: if the Conduit's query has been wrapped and finalized, the Conduit will execute OwnerRegistry.unwrap that will burn the NFT and reset the records relative to the query stored in the mappings $.queries and $.tokenToHashId. Kiln should document that the OwnerRegistry.getOwner function will return address(0) even if the query had an owner and the owner had changed from the original one stored in the query records.
• VehicleManager.sol?lines=283,283: in retrieveQueryRedeemQueueAssets use $.asset instead of fetching it from the $.accountingEngine
• SectorAccountingEngine.sol?lines=626,626: consider inlining the code of _syncMoveVehicleActivation in the move function given that it's only used there
• All the new factories that use the IQueryRegistry in their SpawnParams struct are missing the natspec documentation for the registry attribute
• Consider renaming the IQueryRegistry registry attribute in the SpawnParams struct of all the factories to queryRegistry. We already have the AssetRegistry and using just the variable name registry can be confusing.
• Consider validating the params.registry in the _paramsChecks function of all the factories to be consistent with the existing behavior
• QueryRedeemQueue.sol?lines=359,359: the QueryRedeemQueue.redeem function is using the onlyMultiVehicleOrManager modifier even if it's only called by the MultiVehicleFacet. Replace the modifier with onlyMultiVehicle
• QueryRedeemQueue.sol?lines=395,395: The QueryRedeemQueue.retrieve function is using the onlyMultiVehicleOrManager even if the function is called only by the VehicleManager. Replace it with a new modifier that allows only the VM to be the msg.sender.

Recommendation

Kiln should consider implementing the above suggestions.

Spearbit

Acknowledged. This issue will be checked in the next audit.

Description

At the very end of both the _dispatchDeposit and _dispatchRedeem flows we have this sanity check

// 7. Create and dispatch the deposit query        if (_effectiveAmount > 0) {            depositQuery = _createQuery(Mode.DEPOSIT, $.asset, vehicle, _effectiveAmount, minOutput, data);            SubQuery memory _subQuery =                $.subQueryEngine.createSubQuery(vehicle, depositQuery, settledDestination, rejectedDestination);            dispatchState = $.subQueryEngine.progressQuery(_subQuery, depositQuery);            emit Dispatched(vehicle, settledDestination, rejectedDestination, depositQuery, dispatchState, operationId);        }        // 8. Strict callers (operator `dispatch()`) cannot tolerate a silent no-op: either the        //    block above was skipped (`_effectiveAmount == 0`) or `progressQuery` itself        //    produced an EMPTY transition. Either way the strict caller's intent (slippage        //    protection via `minOutput`, vehicle-specific `data`) would be discarded. The        //    non-strict path leaves staged assets parked in the vehicle sector for the asset        //    manager to recover.>>>        if (strict && dispatchState == State.EMPTY) {>>>            revert EmptyStrictDispatch(vehicle, Mode.DEPOSIT, amount, _sectorBalance);>>>        }

The natspec mentions "progressQuery itself produced an EMPTY transition" but this scenario is impossible. The $.subQueryEngine.createSubQuery execution has only two options:

1. The internal checks revert
2. the vehicle.create is executed. If it's executed (without reverting) the query state cannot be EMPTY because otherwise the vehicle would have reverted or the QueryRegistry would have reverted (EMPTY→EMPTY is not a valid transition).

Given the above statement, the only reason for which we can revert with EmptyStrictDispatch is because strict == true && _effectiveAmount == 0. dispatchState is indeed equal to State.EMPTY but only because we have (in this scenario) triggered the if (_effectiveAmount > 0) { branch.

It's incorrect and misleading to say that dispatchState could be empty because we have entered that branch and the subquery engine has generated an "empty" query/state. That should not be possible.

Recommendation

Kiln should consider the following:

1. update the natspec in both the dispatch functions to correct the wrong statement
2. consider refactoring the logic of the sanity check to be explicit and make the code more clear and easier to understand and read

if (_effectiveAmount > 0) {    // current logic} else if ( strict ) {	// _effectiveAmount == 0 && strict == true    revert EmptyStrictDispatch(vehicle, Mode.DEPOSIT, amount, _sectorBalance);} else {	// _effectiveAmount == 0 && strict == false	return (depositQuery, State.EMPTY);}

Spearbit

Acknowledged. This issue will be checked in the next audit.

Code Overview

This report is a fix-review pass over the changes introduced in PR #459 (commit ca898a1). The review examined the contracts to verify that the fixes applied in this commit are correct and complete.

The review surfaced findings that remain open. These have not been resolved in this phase and are documented here so the team can analyze and remediate them, and so they can be carried forward as watch-points in future security reviews.