Puffer: Puffer Institutional

Description

An ETH or WETH donation can block _accrueFees() and thus setValidatorsETH(), which is a known issue.

As we understand from the project one of the reasons for this is to protect incorrect oracle updates, however this check isn't very effective for that.

Recommendation

Consider having a seperate sanity check on the values for restakedValidatorsETH and nonRestakedValidatorsETH to prevent oracle mistakes.

Then in the fee calculation: skip the fee calculation if the reward is outside of the bounds and don't revert.

Puffer

Acknowledged. The breaker is a deliberate halt, not a permanent block. If it trips, the admin clears it by raising the APR cap and reporting, or by removing the donated ETH/WETH via rescueAnything() (independent of fee accrual, works for any donation size), so reporting can never be permanently blocked.

We keep the revert rather than skipping the fee, skipping would advance the baseline past a genuine reward and forfeit that fee for good.

Cantina

Acknowledged.

Description

A 100% fee cap could lead to division by 0.

The code allows:

• maxTotalFeeBps = 10_000
• sum recipient bps = 10_000

With these values denom - numByBps can be zero and the mulDiv() of feeShares reverts.

Additionally a 100% fee is not practicle.

Recommendation

Consider lowering the fee cap and/or check for denom - numByBps being zero.

Description

If initializerV3() isn't run directly after createVault(), then getWithdrawalCredentials() will return 0 and startNonRestakingValidators() uses empty credentials and then funds will not be able to be withdrawn.

Because createVault() is authorized the risk of this is low.

Recommendation

Make sure to always call initializerV3() directly after createVault().

Also consider adding a check in getWithdrawalCredentials() to verify noRestakingWithdrawalCredentials is set.

Description

Several unsafe type conversions are done. These could truncate a value is the value is too large.

This won't be an issue in practice because there is not that many ETH. For comparison: _increaseNetLPFlow() checks for overflows.

Recommendation

Consider using SafeCast.toUint128().

Description

The DelegationManager has a depositScalingFactor which could interfere with the value of scaledShares.

Recommendation

Consider checking depositScalingFactor() has the correct value.

Description

The tokens[] array is of size 1, but the withdrawals[] array could be bigger. In that case completeQueuedWithdrawals() will revert because it tries to index tokens[] with the same values it indexes withdrawals[].

function completeQueuedWithdrawals(...) ... {        uint256 n = withdrawals.length;        for (uint256 i; i < n; ++i) {            _completeQueuedWithdrawal(withdrawals[i], tokens[i], receiveAsTokens[i]);        }    }

Recommendation

Consider making the array of tokens[] the same size as withdrawals[].

Description

Received ETH calculation in completeQueuedWithdrawals() might be inaccurate if slashing has occured.

Recommendation

Consider using the balanceBefore / balanceAfter method, similar to withdrawNonRestakedETH().

Description

Function completeQueuedWithdrawals() might revert if totalAmount is larger than restakedValidatorsETH. This might happen in the oracle update isn't accurate.

Recommendation

Consider using the saturating subtraction like withdrawNonRestakedETH() does.

Description

The timing of setValidatorsETH is important:

• if it is in the same block as a function that increments/decrements restakedValidatorsETH / nonRestakedValidatorsETH, it will overwrite these values, depending on the order;
• staking ETH might be in transit for a while and setValidatorsETH might not take this into account.

This is also important because the fee calculation is not idempotent (e.g. losses don't have fees, so the order is relevent).

Recommendation

Possibly solutions:

• carefully timing the calls;
• include the inflight assets in the setValidatorsETH() values;
• revert in setValidatorsETH if restakedValidatorsETH / nonRestakedValidatorsETH are updated in the last N blocks and recalculate the values and try again.

Puffer

Acknowledged. The mitigation is operational that needs to be coordinated with oracle.

Cantina

Acknowledged.

Description

If totalAssets() ever reaches a low value, the require() might revert because rewards could them be larger than maxRewards.

Additionally the early return doesn't do an emit.

Recommendation

Consider removing the early return, so the standard emit will be used. Only do the require when totalAssets() has a non trivial value. For example in the following way:

function _accrueFees(Storage storage $) internal virtual {    ...-   if ($.lastTotalAssets == 0) {-       $.lastTotalAssets = uint128(newTA);-       $.netLPFlowSinceLastReport = 0;-       $.lastReportTimestamp = uint64(block.timestamp);-       return;-   }    int256 rewards = ...    if (rewards > 0) {        uint256 r = uint256(rewards);        ...+       if ($.lastTotalAssets > 1 ether) // otherwise the require might be too strict             require(r <= maxRewards, RewardsExceedSanityBound());        _mintFeeShares($, r, newTA);    }    $.lastTotalAssets = uint128(newTA);    $.netLPFlowSinceLastReport = 0;    $.lastReportTimestamp = uint64(block.timestamp);    emit FeesAccrued(rewards, newTA);}

Description

Fee recipient changes are applied to the current unreported reward period, even if the rewards were earned before the change. Fees are only minted when setValidatorsETH() is called.

If 10 ETH of rewards accrued while the recipient list was empty, the admin can set 1000 bps before the oracle report. The next report mints fee shares using the new 1000 bps.

As a result, the new recipient receives fees on rewards earned before the fee was configured.

The reverse is also possible: the admin can clear recipients before the report and avoid fees on already-accrued rewards.

Recommendation

Fee configuration changes should only apply to future rewards, not the current unreported period.

Consider the fix: Accrue fees before changing recipients or bps

Puffer

Acknowledged. Fee configuration (setFeeRecipients(), setMaxTotalFeeBps(), setMaxAprBps()) is restricted to the trusted vault admin, who already holds strictly broader control over share supply and assets via adminMint(), adminBurn(), and rescueAnything(). Any value obtainable by timing a fee-config change around an oracle report is a subset of those existing privileges and grants no additional capability.

Cantina

Acknowledged.

Description

Different ways to send ETH are used:

• NonRestakingWithdrawalCredentials::withdrawETH() uses Address.sendValue();
• InstitutionalVault::redeemETH() and InstitutionalVault::withdrawETH() use receiver.call{...}().

This is inconsisent.

Recommendation

Consider using the same method everywhere.

Description

Array lengths in completeQueuedWithdrawals() are not checked:

Recommendation

Consider adding the following checks:

• withdrawals.length == receiveAsTokens.length
• withdrawals[i].strategies.length == 1
• withdrawals[i].scaledShares.length == 1

Description

Several values can be retrieved, except from lastReportTimestamp.

Recommendation

Consider adding a function getLastReportTimestamp() if this is useful.

Description

The value of 10_000 is used several times. If this is ever updated than one location could forgotten.

Recommendation

Consider having a constant for 10_000.

Description

The error NotEnoughETH is not accurate, because the check is done for WETH.

Recommendation

Consider changing the error in the following way:

-NotEnoughETH+NotEnoughWETH

Description

A comment for createVault() is incorrect, because the returned values are in the reverse order.

@return The address of the vault and the address of the access manager

Recommendation

Consider updating the comment and also consider using named return variables.

Description

For loops with array lengths can be optimized because in every loop iteration the length is calculated.

Recommendation

Consider caching the array length.

Puffer

Acknowledged. This is a negligible optimization: storage-array loops already cache the length, and for calldata arrays the per-iteration length read is cheap. We leave the loops as-is, also because caching it in startValidators would add a local that might trips a stack-too-deep in that function. Not worth changing.

Cantina

Acknowledged.