Polystream Vault

Description

The codebase implements two safeguards against quick changes in the total managed funds.The first one is done in the Accountant on the updateExchangeRate()

if (highWaterMark > 0) {    uint256 deviation = newExchangeRate > highWaterMark ?         newExchangeRate - highWaterMark :         highWaterMark - newExchangeRate;    uint256 deviationPercentage = deviation.mulDiv(1e18, highWaterMark, Math.Rounding.Ceil);        if (deviationPercentage > maxDeviation) {        _pause();        emit EmergencyPaused(deviationPercentage, newExchangeRate);        return; // Exit early without updating state    }}

If the exchange rate (totalAssets/totalShares) changes too significantly, the Accountant will pause and break.

The second safeguard is in the HubVault updateUnderlyingBalance function itself. It will trigger if the asset balance changes too significantly:

uint256 percentageChange = _calculatePercentageChange(lastPricePerShare, newExchangeRate);
if (percentageChange > maxPercentageChange) {        _pause();return;}

This will never be reached, as the accountant's pause will return before updateUnderlyingBalance() is ever called.

Recommendation

We recommend also pausing the vault in this case

Description

Every time a user creates a withdrawal their nextRequestId gets incremented.

// Get next request ID for userrequestId = nextRequestId[msg.sender];nextRequestId[msg.sender]++;

So a user (or a bot interacting with the protocol frequently) with many interactions will have a significantly high nextRequestId. This will lead to a problem as the program will call the following at the end of finalizing a withdrawal:

// Remove user from pending queue only if they have no other active requestsif (!_hasActiveRequests(user)) {    pendingUsers.remove(user);}

The _hasActiveRequests() function then works as follows:

function _hasActiveRequests(address user) internal view returns (bool hasActive) {        // Check if user has any requests from ID 0 to their nextRequestId        for (uint256 i = 0; i < nextRequestId[user]; i++) {            if (requests[user][i].shares > 0) {                return true;            }        }        return false;    }

, As this will loop 0 -> nextRequestId[user] and nextRequestId[user] will increase with every withdrawal, this will revert at some point and lead to none of the users' withdrawals being finalizable anymore, and thus getting stuck.

Recommendation

We recommend instead counting the number of active withdrawal requests per user. They can be incremented on every new creation and decremented on every cancel/finalization.

Description

HubVault.updateUnderlyingBalance() calculates newExchangeRate using the following formula:

uint256 newExchangeRate =    _totalAssets(newAggregatedBalance).mulDiv(1, totalVaultSupply, Math.Rounding.Floor);

Since the exchange rate is computed per 1 unit, this causes significant precision loss. For example, with 5000 assets and 4000 shares, the true exchange rate is 1.25, but the calculation truncates it to 1.

Because of this truncation, the percentage deviation check becomes inaccurate, and the contract may fail to pause when the exchange rate deviates beyond the allowed threshold.

Recommendation

Compute newExchangeRate using a scaled base such as 10 ** decimals() to preserve precision:

uint256 newExchangeRate =    _totalAssets(newAggregatedBalance).mulDiv(10 ** decimals(), totalVaultSupply, Math.Rounding.Floor);

Description

In order to validate the input, SpectraExecutionSchema iterates through the commands array:

for (uint256 i = 0; i < commands.length; i++) {    uint8 command = uint8(commands[i]);        // Validate command sequence    if (i < commands.length - 1) {        _validateCommandSequence(command, uint8(commands[i + 1]), inputs[i]);    }}

However, the logic only validates the current command against the next command, and never validates the last command. This allows a curator to place an unsafe or malicious final command (e.g., an arbitrary token transfer to their own address), bypassing validation entirely.

Recommendation

Ensure that every command in the list is validated.

Description

The _validateCommandSequence function of the SpectraExecutionSchema is used to verify that the commands being executed through spectra are valid and in the correct order. The commands that should be executable are the following:

uint256 constant TRANSFER_FROM = 0x00;uint256 constant TRANSFER_FROM_WITH_PERMIT = 0x01;uint256 constant TRANSFER = 0x02;uint256 constant DEPOSIT_ASSET_IN_IBT = 0x04;uint256 constant DEPOSIT_ASSET_IN_PT = 0x05;uint256 constant DEPOSIT_IBT_IN_PT = 0x06;uint256 constant REDEEM_IBT_FOR_ASSET = 0x07;uint256 constant REDEEM_PT_FOR_ASSET = 0x08;uint256 constant REDEEM_PT_FOR_IBT = 0x09;uint256 constant CURVE_ADD_LIQUIDITY = 0x0c;uint256 constant WRAP_VAULT_IN_4626_ADAPTER = 0x10;uint256 constant UNWRAP_VAULT_FROM_4626_ADAPTER = 0x11;uint256 constant CURVE_NG_ADD_LIQUIDITY = 0x17;uint256 constant CURVE_ADD_LIQUIDITY_SNG = 0x1B;uint256 constant CURVE_REMOVE_LIQUIDITY_ONE_COIN = 0x0e;uint256 constant CURVE_REMOVE_LIQUIDITY_ONE_COIN_SNG = 0x1D;uint256 constant CURVE_NG_REMOVE_LIQUIDITY_ONE_COIN = 0x19;

However, when the provided commands are validated in _validateCommandSequence, it is actually never verified that they are contained in this list, thus an array of potentially malicious commands can be executed without any restrictions.

Recommendation

We recommend reverting for every command not on the list of trusted commands.

Description

The documentation for updateMinUpdateDelay states, "Must not exceed 7 days." So the function should revert if the provided newMinUpdateDelay > 7days.

/// @notice Update the minimum delay between updates/// @dev Callable by authorized accounts only. Must not exceed 7 days./// @param newMinUpdateDelay The new minimum update delay (in seconds)function updateMinUpdateDelay(uint256 newMinUpdateDelay) external requiresAuth whenNotPaused {            uint256 oldDelay = minUpdateDelay;    minUpdateDelay = newMinUpdateDelay;        emit MinUpdateDelayUpdated(oldDelay, newMinUpdateDelay);}

However, this is actually never checked and any value can be set.

Recommendation

We recommend adding a check for newMinUpdateDelay <= 7 days

Description

The requestWithdraw() function is used to queue withdrawals:

/// @notice Request a withdrawal with escrow and rate lock./// @param shares The amount of shares to withdraw./// @param userMaxLossBps The maximum loss basis points the user accepts (0 = use default)./// @param allowThirdPartyComplete Whether to allow solver buyouts (protocol nodes can always complete)./// @return requestId The ID of the created withdrawal request.function requestWithdraw(    uint256 shares,    uint16 userMaxLossBps,    bool allowThirdPartyComplete

It correctly ensures that the WithdrawEscrow is currently not paused; however, it ignores whether the vault is paused.

Recommendation

We recommend checking if the vault is currently paused at the start of the requestWithdraw() function.

Description

The userMaxLossBps can be set by a user on the creation of a withdrawal request. It states how many BPS of loss the user is willing to incur on their withdrawal (which might get executed by a node).

uint16 userMaxLossBps;          // 0 => use default

However the user can never enforce that he does not want his withdrawal to be executed at all if any losses occur. This is due to the following reason. If the user sets the value to 0, the program will use the default which will be avove zero. So the user is forced to allow each withdrawal to incur at least 1 BPS (0.01%) of loss.

Recommendation

We recommend adding a bool that lets the user indicate whether to use the default for userMaxLossBps.

Description

To avoid charging past time periods with newly updated fee rates, the fee changes provided to Accountant.updateFeeRates() should only take effect after updateExchangeRate() has been executed. Otherwise, the new rates apply retroactively, causing inaccurate fee accounting.

Recommendation

Use one of the following approaches:

• Call updateExchangeRate() at the beginning of updateFeeRates() to ensure all fees are settled up to block.timestamp before applying new rates.
• Alternatively, queue the new fee rates and apply them only after the next updateExchangeRate() call, updating the rates at the end of that function.

Description

When a user requests a withdrawal through WithdrawEscrow, they specify userMaxLossBps — the maximum loss they are willing to accept. If, at withdrawal maturity, the exchange rate implies a loss greater than this value, the user cannot complete the withdrawal and can only cancel it and create a new request.

However, after canceling and re-requesting, the user is exposed to the risk of an even worse exchange rate, potentially resulting in losses far greater than those at the time of the original matured request.

Recommendation

Allow the user, at the moment the withdrawal is matured, to explicitly accept the current loss even if it exceeds their originally specified userMaxLossBps, enabling them to complete the withdrawal without canceling and retrying.

Description

As the price of a withdrawal request increases over time, solvers will likely submit a slightly higher payAmount because they cannot predict the exact block in which their transaction will be executed. However, the solverBuyout() function always charges the user with the full payAmount, even when the fair price minPay is lower.

Recommendation

Charge the user only the fair price minPay, instead of the payAmount provided by the solver.

Description

The spectra schema is intended to ensure that the curator can not call malicious series of instructions. The following snippet is intended to ensure this:

} else if (i == 0) {    // Output is IBT, next command must be IBT-related    if (nextCommand != REDEEM_PT_FOR_ASSET &&         nextCommand != REDEEM_PT_FOR_IBT &&         nextCommand != CURVE_ADD_LIQUIDITY &&         nextCommand != CURVE_NG_ADD_LIQUIDITY &&         nextCommand != CURVE_ADD_LIQUIDITY_SNG &&         nextCommand != UNWRAP_VAULT_FROM_4626_ADAPTER &&         nextCommand != TRANSFER) {        revert SpectraExecutionSchema__InvalidCommandSequence();    }}

However this actually are PT not IBT related instructions. As a result this will allow for a bypass of the intended behavior.

Recommendation

We recommend adapting the accepted commands to the following:

// Output is IBT, next command must consume or handle IBT safelyif (nextCommand != DEPOSIT_IBT_IN_PT &&    nextCommand != REDEEM_IBT_FOR_ASSET &&    nextCommand != CURVE_ADD_LIQUIDITY &&    nextCommand != CURVE_NG_ADD_LIQUIDITY &&    nextCommand != CURVE_ADD_LIQUIDITY_SNG &&    nextCommand != UNWRAP_VAULT_FROM_4626_ADAPTER &&    nextCommand != TRANSFER) {    revert SpectraExecutionSchema__InvalidCommandSequence();}

Description

During Accountant.updateExchangeRate() call, the deviation between newExchangeRate and highWaterMark is calculated. If it exceeds maxDeviation, the contract is paused.

if (highWaterMark > 0) {    uint256 deviation = newExchangeRate > highWaterMark ?         newExchangeRate - highWaterMark :         highWaterMark - newExchangeRate;    uint256 deviationPercentage = deviation.mulDiv(1e18, highWaterMark, Math.Rounding.Ceil);        if (deviationPercentage > maxDeviation) {        _pause();        emit EmergencyPaused(deviationPercentage, newExchangeRate);        return; // Exit early without updating state    }}

If deviation is legitimate, admins will likely attempt to increase maxDeviation and then unpause the contract. However, the system enforces a strict upper bound of 10%:

function updateMaxDeviation(uint256 newMaxDeviation) external requiresAuth whenNotPaused {    require(newMaxDeviation <= 1e17, Errors.Accountant__MaxDeviationTooHigh()); // 10% max        uint256 oldDeviation = maxDeviation;    maxDeviation = newMaxDeviation;        emit MaxDeviationUpdated(oldDeviation, newMaxDeviation);}

If the actual deviation exceeds 10%, admins cannot set maxDeviation above that value, meaning it becomes impossible to unpause the contract. The system becomes permanently stuck until an upgrade is deployed.

Recommendation

Remove the requirement that newMaxDeviation must be ≤ 10%, allowing admins to temporarily raise the threshold in exceptional cases and safely unpause the protocol.

Description

The manage() function allows for a multicall through the HubVault to arbitrary addresses.

/**     * @notice Allows manager to make arbitrary function calls from this contract.     * @dev Callable by MANAGER_ROLE.     */    function manage(address[] calldata targets, bytes[] calldata data, uint256[] calldata values)        external        requiresAuth        whenNotPaused        returns (bytes[] memory results)    {        uint256 targetsLength = targets.length;        results = new bytes[](targetsLength);        for (uint256 i; i < targetsLength; ++i) {            results[i] = targets[i].functionCallWithValue(data[i], values[i]);        }    }

To do this, the caller provides 3 arrays, one for the targets, one for the data, and one for the values. However, it is not ensured that all of those are of the same length, which could lead to unexpected reverts.

Recommendation

We recommend ensuring that targets.length() == data.length == values. length().

Description

In the initializer of HubVault the provided _feeRecipient will be set without any checks.

function initialize(IERC20 _asset, address _owner, string memory _name, string memory _symbol, address _feeRecipient) public initializer {    __Context_init();    __ERC20_init(_name, _symbol);    __ERC4626_init(_asset);    __Auth_init(_owner, Authority(address(0)));    __Pausable_init();    __ReentrancyGuard_init();    maxPercentageChange = 1e16; // 1%    feeRecipient = _feeRecipient;}

However, when looking at the update function, it is intended that feeRecipient can't be set to zero:

/// @notice Update the fee recipient address./// @dev Callable by authorized accounts only./// @param newFeeRecipient The new fee recipient address.function updateFeeRecipient(address newFeeRecipient) external requiresAuth {    require(newFeeRecipient != address(0), Errors.InvalidFeeRecipient());    emit FeeRecipientUpdated(feeRecipient, newFeeRecipient);    feeRecipient = newFeeRecipient;}

Recommendation

We recommend adding a zero address check to the initializer.

Description

The platform fees are calculated as follows:

uint256 platformFeeShares = totalSupply    .mulDiv(platformFee, feeDenominator, Math.Rounding.Floor)    .mulDiv(timeDelta, 365 days, Math.Rounding.Floor);

This actually leads to rounding, which results in lower fees than intended.

Recommendation

We recommend rounding up when calculating the platform fees.

Description

The performance fees are calculated as follows:

// feeShares = totalSupply * (gain / highWaterMark) * (performanceFee / feeDenominator)feeShares = gain.mulDiv(totalSupply, highWaterMark, Math.Rounding.Floor)              .mulDiv(performanceFee, feeDenominator, Math.Rounding.Floor);

However, this leads to less fees being charged than intended.

Recommendation

We recommend rounding up when calculating the performance fees.

Description

The users nextRequestId[user] will increase over time. This will lead to an OOG issue in the getUserRequests() function.

for (uint256 i = 0; i < nextId; i++) {    if (requests[user][i].shares > 0) {        activeCount++;    }}
// Second pass: populate arraysrequestIds = new uint256[](activeCount);requestDetails = new WithdrawRequest[](activeCount);uint256 index = 0;
for (uint256 i = 0; i < nextId; i++) {    if (requests[user][i].shares > 0) {        requestIds[index] = i;        requestDetails[index] = requests[user][i];        index++;    }}

This function will loop over 0 -> nextRequestId[user], which will lead to an OOG revert once nextRequestId[user] has reached a certain value.

Recommendation

We recommend tracking a lastActive id and only iterating over the range lastActive -> nextRequestId[user]. This way, the looping is limited unless a user willingly leaves an old request open (for which there is no benefit).

Description

The schema for the Across protocol is used to verify and check transactions. In the current version, it includes the deposit function:

// Across deposit function - extract only necessary addressesfunction deposit(    address recipient,    address originToken,    uint256, /*amount*/    uint32, /*destinationChainId*/    uint64, /*relayerFeePct*/    uint32 /*quoteTimestamp*/) external view virtual returns (bytes memory restrictedData) {    // Return only the addresses needed for Merkle verification    restrictedData = abi.encodePacked(        recipient,        originToken    );}

However, this function is actually deprecated in the across bridge:

function depositDeprecated_5947912356(    address recipient,    address originToken,    uint256 amount,    uint256 destinationChainId,    int64 relayerFeePct,    uint32 quoteTimestamp,    bytes memory message,    uint256 maxCount) external payable;

Recommendation

We recommend removing the deprecated function.

Description

Both the speedUp() and estimateAmountToReceive() functions implemented in the across schema are currently not implemented in the actual across protocol in the same way.

// Across speedUp function - extract only necessary addresses    function speedUp(        address recipient,        address originToken,        uint256, /*amount*/        uint32, /*destinationChainId*/        uint64, /*relayerFeePct*/        uint32, /*quoteTimestamp*/        bytes32 /*depositId*/    ) external view virtual returns (bytes memory restrictedData) {        // Return only the addresses needed for Merkle verification        restrictedData = abi.encodePacked(            recipient,            originToken        );    }
    // Across estimateAmountToReceive function - extract only necessary addresses    function estimateAmountToReceive(        address originToken,        uint256, /*amount*/        uint32, /*destinationChainId*/        uint64 /*relayerFeePct*/    ) external view virtual returns (bytes memory restrictedData) {        // Return only the addresses needed for Merkle verification        restrictedData = abi.encodePacked(            originToken        );    }

As a result, these will never be callable through the schema.

Recommendation

We recommend removing the functions.

Description

Inside WithdrawEscrow.requestWithdraw() the shares amount is cast to uint96 without ensuring that the provided value fits into the uint96 type. If the value exceeds the max of uint96, it will be silently truncated, leading to incorrect withdrawal amounts.

Recommendation

Before casting, validate that shares is less than or equal to type(uint96).max to prevent truncation.

Description

The WithdrawEscrow implements two functions that finalize a withdrawal request.

/// @notice Complete withdrawal as the current owner (self-service).    /// @param user The original user who created the request.    /// @param requestId The ID of the withdrawal request to complete.    /// @return assetsOut The amount of assets received.    function completeWithdrawSelf(address user, uint256 requestId) external nonReentrant whenNotPaused returns (uint256) {        return _completeWithdraw(user, requestId, msg.sender);    }
    /// @notice Complete withdrawal as authorized node.    /// @param user The user whose withdrawal to complete.    /// @param requestId The ID of the withdrawal request to complete.    /// @return assetsOut The amount of assets received.    function completeWithdraw(address user, uint256 requestId) external requiresAuth nonReentrant whenNotPaused returns (uint256) {        return _completeWithdraw(user, requestId, msg.sender);    }

The separation is intended so that one can be called by the user and the other by the authorized node. However, both result in the same function with the same arguments, and the authorization is checked there. As a result, keeping completeWithdrawSelf and renaming it to completeWithdraw will suffice.

Recommendation

We recommend removing the function.

Description

A withdrawal request has two important timeframes for processing. The withdrawDelay determines how long it takes for a withdrawal to be finalised. The completionWindow determines how long the withdrawal can take.

struct AssetConfig {    uint32 withdrawDelay;        // seconds to maturity    uint32 completionWindow;     // after maturity

Both these values can be updated using an admin function.

/// @notice Update withdrawal parameters.    function setParams(        uint32 _withdrawDelay,        uint32 _completionWindow,        uint16 _defaultMaxLossBps    ) external requiresAuth {        if (_withdrawDelay == 0) revert Errors.Escrow__InvalidWithdrawDelay();        if (_completionWindow == 0) revert Errors.Escrow__InvalidCompletionWindow();        require(_defaultMaxLossBps <= MAX_LOSS_BPS_CAP, Errors.Escrow__TooMuchMaxLoss());
        config.withdrawDelay = _withdrawDelay;        config.completionWindow = _completionWindow;        config.defaultMaxLossBps = _defaultMaxLossBps;
        emit WithdrawParamsUpdated(_withdrawDelay, _completionWindow, _defaultMaxLossBps);    }

However, while the _withdrawDelay is tracked in the user's withdrawal request in the maturity value, the completionWindow at creation is not saved. As a result, the users' withdrawal requests made pre-change will still be affected by the changed completion window.

// Check timing - allow cancel before maturity or after completion windowif (block.timestamp >= req.maturity && block.timestamp <= req.maturity + config.completionWindow) {    revert Errors.Escrow__NotCancellable();}

Recommendation

We recommend tracking the completion timestamp in the struct.

Description

The maxPercentageChange variable ensures the vault pauses if the underlying balance changes significantly. It is ensured that this pause occurs within a 0-10% range.

/// @notice Update the maximum percentage change allowed before the vault is paused./// @param newMaxPercentageChange The new maximum percentage change. Max value is 1e17 (10%).function updateMaxPercentageChange(uint256 newMaxPercentageChange) external requiresAuth {    require(newMaxPercentageChange < MAX_PERCENTAGE_THRESHOLD, Errors.InvalidMaxPercentage());    emit MaxPercentageUpdated(maxPercentageChange, newMaxPercentageChange);    maxPercentageChange = newMaxPercentageChange;}

However, it is never checked whether the percent change is set to 0, which would cause an instant pause on any update (unless the balance doesn't change, which is highly unlikely).

Recommendation

We recommend ensuring that maxPercentageChange > 0.

Description

The InvalidMaxPercentage error is documented as:

/// @notice Thrown when the new max percentage is greater than the current max percentage.error InvalidMaxPercentage();

However, this is not true, as the error actually occurs when the new max percentage is greater than the threshold, not the old one.

Recommendation

We recommend adapting it to:

/// @notice Thrown when the new max percentage is greater than the max percentage threshold.error InvalidMaxPercentage();

Description

The initializer of Accountant emits the following error if the vault is set to 0.

require(_vault != address(0), Errors.InvalidFeeRecipient());

However, this is incorrect as the vault is not the fee receiver.

Recommendation

We recommend emitting the correct error code.

Description

The documentation of getAvailableBalance() states the following:

/// @notice Get the available balance./// @dev Returns the vault's asset balance minus pending redemption assets./// @return The available balance for instant redemptions.function getAvailableBalance() public view returns (uint256) {    return _getAvailableBalance();}

However this is untrue as the returned value does not account for the pending assets.

Recommendation

We recommend adapting the comment to "@dev Returns the vault's asset balance".

Description

The feeDenominator variable is once set to 10,000 in the initializer of Accountant

feeDenominator = 10_000;

The admin can't change it and will stay at this value forever.

Recommendation

We recommend using a constant for the feeDenominator.

Description

if (highWaterMark > 0) {    uint256 deviation = newExchangeRate > highWaterMark ?         newExchangeRate - highWaterMark :         highWaterMark - newExchangeRate;    uint256 deviationPercentage = deviation.mulDiv(1e18, highWaterMark, Math.Rounding.Ceil);
    ...}

highWaterMark is initialized to 1e18 during contract deployment and is never allowed to decrease below this value. Since the variable can never be zero, the conditional statement adds unnecessary branching and can be removed.

Recommendation

Remove the redundant highWaterMark > 0 check.

Description

As the recipient of a Balancer flash loan should always be the Manager contract, the recipient parameter in Manager.flashLoan() is unnecessary.

Recommendation

Remove the recipient parameter and always pass address(this) as the flash-loan recipient:

BalancerFlashLoan.flashLoan(address(this), tokens, amounts, userData);