6.1 Large withdrawals spanning multiple strategies revert when Makina strategy hits machine liquidity limit
When a user withdraws from the Concrete vault and the vault's idle balance is insufficient, the vault iterates through strategies in deallocationOrder and calls onWithdraw(...) to pull assets:
function _executeWithdraw(...) internal virtual returns (uint256) {
// ...
for (uint256 i; i < strategiesCounter; ++i) {
// ...
uint256 withdrawableAmountFromStrategy = IStrategyTemplate(deallocationOrder[i]).maxWithdraw();
uint256 withdrawAmount =
(withdrawableAmountFromStrategy >= desiredAssets) ? desiredAssets : withdrawableAmountFromStrategy;
if (withdrawAmount > 0) {
uint256 actualWithdrawn = IStrategyTemplate(deallocationOrder[i]).onWithdraw(withdrawAmount);
// ...
}
}
// ...
}
The vault calculates withdrawAmount = min(desiredAssets, strategy.maxWithdraw()), meaning it requests exactly maxWithdraw() whenever the user needs more than what the strategy reports as available.
The strategy's maxWithdraw() correctly accounts for machine liquidity:
function maxWithdraw() external view returns (uint256) {
uint256 maxWithdrawableAssets = IMachine(MAKINA_MACHINE).maxWithdraw();
return Math.min(maxWithdrawableAssets, _getStrategyOwnedAssets());
}
However, the strategy's onWithdraw(...) function adds +1 to the calculated shares to ensure rounding doesn't cause the user to receive less than requested:
function onWithdraw(uint256 _amountToWithdraw) external ... returns (uint256 amountWithdrawn) {
// @audit-issue The +1 padding can cause sharesToRedeem to exceed machine's liquidity.
uint256 sharesToRedeem = IMachine(MAKINA_MACHINE).convertToShares(_amountToWithdraw) + 1;
sharesToRedeem = Math.min(sharesToRedeem, _getStrategyOwnedShares());
amountWithdrawn = IMachine(MAKINA_MACHINE).redeem(sharesToRedeem, ROYCO_VAULT, _amountToWithdraw);
// ...
}
The Math.min(...) inside onWithdraw() only caps shares at the strategy's owned balance - it does not re-validate against the machine's available liquidity after the +1 is added. The machine's maxWithdraw() function returns the accounting token balance held by the machine contract. When funds are deployed to Calibers (yield-generating strategies), this balance is reduced while the strategy's share value (based on total AUM) remains unchanged. This creates a scenario where machine.maxWithdraw() is less than the strategy's total position value.
In this case, strategy.maxWithdraw() returns machine.maxWithdraw() (the limiting factor), and the vault calls onWithdraw(...) with exactly that amount. The strategy then calculates sharesToRedeem = convertToShares(machine.maxWithdraw()) + 1, causing convertToAssets(sharesToRedeem) to exceed machine.maxWithdraw() by approximately one share's worth. The machine's redeem(...) then reverts with ExceededMaxWithdraw.
Since the vault's _executeWithdraw(...) has no try-catch around the onWithdraw(...) call, the entire withdrawal transaction reverts. The vault does not fall back to the next strategy in deallocationOrder. This means that when a user requests a large withdrawal that requires pulling funds from multiple strategies, if the Makina strategy is in the deallocation sequence and hits this boundary condition, the entire withdrawal fails - even if subsequent strategies had sufficient liquidity to cover the remaining amount.
This is a common scenario when most vault funds are allocated to strategies and the underlying Makina machine has deployed funds to yield-generating Calibers, reducing its liquid balance while maintaining the same total AUM.
Users can work around this by requesting slightly less than maxWithdraw(), and administrators can resolve it by calling deallocateFunds(...) directly (which does not have the +1 padding). However, these workarounds are not obvious and users will experience unexpected failed transactions.
Recommendation
Consider capping sharesToRedeem based on machine liquidity in addition to owned shares. Alternatively, document this behavior clearly so integrators and users are aware of the limitation.
Resolution
Fixed
6.2 The onWithdraw(...) function redeems one share when called with zero amount
The onWithdraw(...) function is called by the Concrete vault to withdraw assets from the strategy when the vault's idle balance is insufficient to fulfill a user's withdrawal request. The function calculates the number of shares to redeem by converting the requested asset amount to shares and adding 1 to handle rounding:
function onWithdraw(uint256 _amountToWithdraw) external ... returns (uint256 amountWithdrawn) {
// @audit-issue When `_amountToWithdraw` is zero, this evaluates to 0 + 1 = 1.
uint256 sharesToRedeem = IMachine(MAKINA_MACHINE).convertToShares(_amountToWithdraw) + 1;
sharesToRedeem = Math.min(sharesToRedeem, _getStrategyOwnedShares());
amountWithdrawn = IMachine(MAKINA_MACHINE).redeem(sharesToRedeem, ROYCO_VAULT, _amountToWithdraw);
// ...
}
When _amountToWithdraw is 0, the convertToShares(...) function returns 0, resulting in sharesToRedeem = 1. This causes one share to be redeemed from the Makina machine, returning its equivalent asset value to the vault despite no assets being requested. The test suite explicitly covers this scenario in test_onWithdraw_withZeroAmount(), acknowledging that "some shares may be redeemed" with a zero input.
Currently, this edge case is not triggered in practice because the Concrete vault only calls onWithdraw(...) when withdrawAmount > 0. However, if this strategy is reused in a different context or integrated with other systems that do not perform this check, it could lead to unintended share redemptions.
Recommendation
Consider adding an early return when _amountToWithdraw is zero, or documenting this behavior clearly in the function's NatSpec to prevent misuse in future integrations.
Resolution
Acknowledged
6.3 The rescueToken(...) function does not emit an event as required by the IStrategyTemplate interface
The IStrategyTemplate interface defines the rescueToken(...) function as a mechanism for authorized admins to recover tokens that may have been accidentally sent to the strategy. The interface specification includes a MUST-level requirement stating that the function "MUST emit appropriate events for the rescue operation."
function rescueToken(address token, uint256 amount) external;
// - MUST emit appropriate events for the rescue operation.
However, the rescueToken(...) function in the RoycoVaultMakinaStrategy contract does not emit any event upon successfully rescuing tokens:
function rescueToken(
address _token,
uint256 _amount
) external override(IStrategyTemplate) restricted {
require(
_token != MACHINE_SHARE_TOKEN,
INVALID_TOKEN_TO_RESCUE()
);
_amount = _amount == 0
? IERC20(_token).balanceOf(address(this))
: _amount;
// @audit-issue No event is emitted after the rescue.
IERC20(_token).safeTransfer(msg.sender, _amount);
}
Recommendation
Consider defining and emitting a dedicated event (e.g., TokenRescued(address token, uint256 amount, address recipient)) within the rescueToken(...) function to comply with the IStrategyTemplate interface specification and improve off-chain observability of admin operations.
Resolution
Fixed
6.4 The rescueToken(...) function only checks against MACHINE_SHARE_TOKEN instead of asset() as specified by the IStrategyTemplate interface
The IStrategyTemplate interface defines the rescueToken(...) function as a mechanism for authorized admins to recover tokens that may have been accidentally sent to the strategy. The interface specification includes MUST-level requirements for which token should be blocked from rescue:
function rescueToken(address token, uint256 amount) external;
// - MUST only allow rescue of tokens that are not the strategy's
// primary asset (asset()).
// - MUST revert if attempting to rescue the strategy's primary
// asset token.
However, the rescueToken(...) function in the RoycoVaultMakinaStrategy contract checks against MACHINE_SHARE_TOKEN (the Makina Machine's share token) rather than ASSET (the base/accounting token returned by asset()):
function rescueToken(
address _token,
uint256 _amount
) external override(IStrategyTemplate) restricted {
// @audit Checks only against MACHINE_SHARE_TOKEN, not ASSET.
require(
_token != MACHINE_SHARE_TOKEN,
INVALID_TOKEN_TO_RESCUE()
);
_amount = _amount == 0
? IERC20(_token).balanceOf(address(this))
: _amount;
IERC20(_token).safeTransfer(msg.sender, _amount);
}
Recommendation
Consider aligning the check with the IStrategyTemplate specification by also blocking ASSET from rescue
Resolution
Acknowledged
6.5 maxWithdraw() does not account for paused state or Machine recovery mode
The maxWithdraw() function reports available liquidity without checking whether the strategy is paused or whether the underlying Makina Machine is in recovery mode. Both of these conditions cause onWithdraw(...) to revert, but maxWithdraw() does not reflect them:
function maxWithdraw() external view override(IStrategyTemplate) returns (uint256) {
// @audit No check for paused() or Machine recovery mode.
uint256 maxWithdrawableAssets = IMachine(MAKINA_MACHINE).maxWithdraw();
return Math.min(maxWithdrawableAssets, _getStrategyOwnedAssets());
}
The onWithdraw(...) function has a whenNotPaused modifier, and internally calls Machine.redeem(...) which has a notRecoveryMode modifier:
// Strategy
function onWithdraw(uint256 _amountToWithdraw)
external override(IStrategyTemplate) whenNotPaused onlyRoycoVault
returns (uint256 amountWithdrawn)
{
// ...
amountWithdrawn = IMachine(MAKINA_MACHINE).redeem(...);
}
// Machine
function redeem(uint256 shares, address receiver, uint256 minAssets)
external override nonReentrant notRecoveryMode returns (uint256)
{
// ...
}
When either condition is active, maxWithdraw() returns a positive value but onWithdraw(...) reverts. Because the Concrete vault's _executeWithdraw(...) loop has no try-catch around the onWithdraw(...) call, the revert propagates and fails the entire withdrawal transaction. The vault cannot fall back to the next strategy in deallocationOrder, blocking all user withdrawals that would need to pull from this strategy, even if subsequent strategies had sufficient liquidity.
Recommendation
Consider returning 0 from maxWithdraw() when the strategy is paused or when the Machine is in recovery mode.
Resolution
Fixed in both maxWithdraw(...) and maxAllocation(...).
6.6 Undocumented calldata structure for allocation and deallocation parameters
The allocateFunds(...) and deallocateFunds(...) functions expect a 64-byte bytes calldata parameter that is decoded using inline assembly. However, the expected structure of this calldata (two uint256 values packed sequentially) is not documented in the function's NatSpec or elsewhere in the contract. This forces callers to inspect the assembly implementation to understand how to properly encode parameters. This is an example of internal implementation details leaking to external callers, which is considered poor API design.
Recommendation
Consider adding NatSpec documentation to both functions describing the expected calldata format, specifying that the first 32 bytes should contain the amount (or shares) and the second 32 bytes should contain the minimum output value.
Resolution
Fixed. Added NatSpec comments.
