Description

Function startRestakingValidators() does give the right to withdraw to eigenlayer. This means eigenlayer can't use it to restake (fully understood with help of the project).

Also an additional cli action is required to register with eigenlayer. This is not clear in the documentation.

Recommendation

Investigate what should be done to give the right to withdraw to eigenlayer.

Consider making the required actions clear in the documentation. Also consider adding a comment at function startRestakingValidators() making clear additional actions are required.

Puffer

Added to the documentation.

Description

When user claims the token reward, the code calls getMerkleRoot() to retrieve the merkle root. When pendingMerkleRoot is not set then block.timestamp >= pendingMerkleRootActivationTimestamp is always true and then an empty merkle root is returned. This blocks the user from claiming the reward.

This situation happens when cancelPendingMerkleRoot() is called: both pendingMerkleRoot and pendingMerkleRootActivationTimestamp are reset to 0.

After the pendingMerkleRoot is set, the user needs to wait for 7 days to make block.timestamp >= pendingMerkleRootActivationTimestamp return true. Repeatedly doing cancelPendingMerkleRoot() can result in a complete denial of service for claiming (this normally shouldn't happen).

Recommendation

The code has to validate if pendingMerkleRoot is empty as well when retrieving merkleRoot:

function getMerkleRoot() public view returns (bytes32) {-   if (block.timestamp >= pendingMerkleRootActivationTimestamp) {+   if (pendingMerkleRoot != bytes32(0) && block.timestamp >= pendingMerkleRootActivationTimestamp) {         return pendingMerkleRoot;    }    return merkleRoot;}

Description

When setNewMerkleRoot() is called with the same newMerkleRoot, then the pendingMerkleRootActivationTimestamp is shifted forward, which leads to a delay in activation (assuming it is done before the pendingMerkleRootActivationTimestamp).

If accidentally (due to a configuration error), this is done repeatedly, the newMerkleRoot will never be activated.

Recommendation

Consider checking that newMerkleRoot!=pendingMerkleRoot.

Description

Not all init functions are called in the initialize() functions. The risks is limited because these functions are currently empty. However that could change in the future.

Recommendation

Consider also calling the following:

In initialize() of UniFiAVSManager:

• __UUPSUpgradeable_init()
• __Context_init()

In initialize() of InstitutionalVault:

• __Context_init()

Puffer

Fixed in: inst PR 28 and preconf PR 54.

Description

When redrawing the last ETH from NonRestakingWithdrawalCredentials with withdrawETH(), the following calulcation will revert is some additional ETH has been donated to the NonRestakingWithdrawalCredentials contract.

$.nonRestakedValidatorsETH -= uint128(difference);

This could lead to a denial of service of the call to withdrawNonRestakedETH().

This can be fixed with setValidatorsETH(), but in the mean time additional ETH can be donated.

Note: a donation of 1 Wei is sufficient.

Recommendation

Consider not reverting when $.nonRestakedValidatorsETH < uint128(difference).

This can be done with saturatingSub().

Description

The construction of using balanceBefore and balanceAfter is vulnerable with reentrancy attacks. Because in that situation the same balanceAfter could be counted for multiple reentrant calls.

This is unlikely to happen because:

• withdrawNonRestakedETH() has access control via restricted
• noRestakingWithdrawalCredentials points to a known contract

However error is authorisations in combination with customDelegateCall() could change this.

Recommendation

To eliminate the risk, consider adding a nonReentrant modifier.

Puffer

Due to refactoring customDelegateCall() was removed.

Description

Contract UnifiRewardsDistributor uses ReentrancyGuard. However there is also a more gas efficient version avaible, using transient storage.

Recommendation

Consider using ReentrancyGuardTransient.

Description

The assignment uint256 amountToClaim = amounts[i] - claimedSoFar; reverts when amounts[i] < claimedSoFar, without giving an explicit error message.

Recommendation

Consider giving an explicit error message when amounts[i] < claimedSoFar.

One solutions would be to use saturatingSub() in combination with the check for amountToClaim == 0.

Description

In contract UniFiAVSManager, EnumerableSet.UintSet isn't used.

Recommendation

Consider removing this line:

-using EnumerableSet for EnumerableSet.UintSet;

Description

Function _getAvsOperatorStatus() uses abi.encodeWithSelector(), which doesn't do checks so errors are not detected.

Recommendation

Consider using abi.encodeCall():

import { IAVSDirectoryExtended } from "./interfaces/EigenLayer/IAVSDirectoryExtended.sol";` ...-   abi.encodeWithSelector(bytes4(keccak256("avsOperatorStatus(address,address)")), address(this), operator)+   abi.encodeCall(avsOperatorStatus, (address(this), operator));...

Puffer

The reason we use encodeWithSelector is because avsOperatorStatus() is not defined (in the interface) and we don't want to include the entirety of the AVS Directory interface.

Description

According to a comment in AVSDirectoryStorage.sol, avsOperatorStatus() is deprecated:

/// @notice Returns the registration status of each `operator` for a given `avs`./// @dev This storage will be deprecated once M2-based deregistration is removed.mapping(address avs => mapping(address operator => OperatorAVSRegistrationStatus)) public avsOperatorStatus;

Recommendation

Consider changing the code to be compatible with the situation where avsOperatorStatus() can no longer be called.

Puffer

Updated the AVS to implement new methods of the upcoming EigenLayer Slashing upgrade as AVSDirectory is being deprecated.

Changes:

• Removed 2-step operator deregistration because operator deregistrations are now processed by EigenLayer regardless of the callback reverts or not
• Uses AllocationManager instead of AVSDirectory
• Implements OperatorSets required by the new EigenLayer slashing update

Description

Serveral typecasts in the constructor of UniFiAVSManager are not necessary.

Recommendation

Consider changing the code to:

-AVS_DIRECTORY = IAVSDirectory(address(avsDirectoryAddress));+AVS_DIRECTORY = avsDirectoryAddress;-EIGEN_REWARDS_COORDINATOR = IRewardsCoordinator(address(rewardsCoordinatorAddress));+EIGEN_REWARDS_COORDINATOR = rewardsCoordinatorAddress;

Description

Function deregisterValidators() keeps the validator.index, which means registerValidators() can't be done again for the same validator / validatorPubkey, because its requires the validator.index to be 0.

Recommendation

Check if there is a usecase for using the same validator again. If so, change the code to allow this. Otherwise add a comment to make this clear foro future maintainers/readers

Puffer

Registering again is not allowed because that would allow an operator to increase its fees.

Description

The creation of the NonRestakingWithdrawalCredentials contract is done via customDelegateCall() and a factory, from InstitutionalVault and only has to be done once. This is a relatively complicated way.

Additionally the customDelegateCall() is very powerfull and thus increased the risks.

Recommendation

Consider creating an initializeV2() function with reinitializer(2) to do the equivalent of deployNewStakingWithdrawalCredentials(). Make sure to call this in one transaction with the contract upgrade to prevent frontrunning.

Also consider removing customDelegateCall().

Description

InstitutionalVault doesn't fully comply to eip-4626.

Recommendation

Consider doing the following:

• return 0 in the functions maxDeposit() / maxMint() / maxWithdraw() / maxRedeem() when the related functions are paused;
• in functions maxWithdraw() / maxRedeem() take into account the maximum available ETH that can currently be retrieved: e.g. should be maximized to WETH.balanceOf(address(this)) + (address(this).balance and converted to shares for maxRedeem().

Puffer

Acknowledged. We are aware of this. The readme in the institutional repository says:

The vault is not 100% ERC4626 compliant to the interface as it accounts for assets that are not in the Vault, but it is not a problem for the current use case.

Description

In function InstitutionalVault(), __deprecated_withdrawer is assigned a value, however this isn't used.

As this isn't used a value of 0 might be used as well. This could be more gas efficient and also makes clear the value isn't used.

Recommendation

Consider changing the code to:

-__deprecated_withdrawer: address(this)+__deprecated_withdrawer: address(0)

Description

Function completeQueuedWithdrawals() uses token address 0. This is fine if this token address isn't used, which is the case when strategy!= _BEACON_CHAIN_STRATEGY.

The related function queueWithdrawals() does indeed use _BEACON_CHAIN_STRATEGY, however completeQueuedWithdrawals() doesn't check/enforce this is the case.

If the wrong strategy is used, a revert will occur that might be difficult to trace.

Recommendation

Consider checking/enforcing that the only strategy used in completeQueuedWithdrawals() is _BEACON_CHAIN_STRATEGY.

Description

The function completeQueuedWithdrawals() and withdrawNonRestakedETH() are similar but behave differently in updating the accounting.

withdrawNonRestakedETH() decreases nonRestakedValidatorsETH, however completeQueuedWithdrawals() doesn't decrease restakedValidatorsETH.

This can be corrected with setValidatorsETH(), but is would be more consistent to also do this in completeQueuedWithdrawals().

Recommendation

Consider decreasing restakedValidatorsETH in completeQueuedWithdrawals(), when receiveAsTokens[]==true.

Description

Function requestEigenPodConsolidation() doesn't check array lengths, while most other functions do, which is inconsistent.

Recommendation

Consider checking srcPubkeys.length == targetPubkeys.length.

Puffer

Acknowledged. The transaction will revert somewhere down the line.

Description

The function requestEigenPodConsolidation() has the comment:

The remainder is donated to the vault and not refunded to the caller

However EigenPod::requestConsolidation() does refund the excess ETH.

Recommendation

Consider changing the comment.

Puffer

The vault doesn't refund to the caller, although the EigenPod does return to the vault.

Description

In function getWithdrawalCredentials() the following comment is no longer valid:

(this contract address)

Recommendation

Consider removing or changing the comment.

Description

The commit for the review is: https://github.com/PufferFinance/Puffer-Preconf/commit/4346c76216a53ec7fd3059380e97dd7141f981d1

This goes from 24093199:

struct UniFiAVSStorage {   ...   uint64 deregistrationDelay;   EnumerableSet.AddressSet allowlistedRestakingStrategies;}

To: 4346c762:

struct UniFiAVSStorage {    ...    uint64 deregistrationDelay;    mapping(uint8 => uint256) _deprecated_bitmapIndexToChainId;    mapping(uint256 => uint8) _deprecated_chainIdToBitmapIndex;    EnumerableSet.AddressSet allowlistedRestakingStrategies;    mapping(bytes validatorPubkey => ValidatorData validatorData) validators;    mapping(uint256 validatorIndex => bytes validatorPubkey) validatorIndexes;    mapping(address operator => OperatorData operatorData) operators;}

Which would be tricky because two mappings are inserted in between deregistrationDelay and allowlistedRestakingStrategies and thus the storage layout would be shifted.

However the onchain version already has these additional mappings. So when upgrading from that, it would be ok.

struct UniFiAVSStorage {    ...    uint64 deregistrationDelay;    mapping(uint8 => uint256) bitmapIndexToChainId;    mapping(uint256 => uint8) chainIdToBitmapIndex;    EnumerableSet.AddressSet allowlistedRestakingStrategies;}

Recommendation

Consider adding an intermediate step in the commits to allow for traceability.

Spearbit

This isn't easily fixable on github because the PRs are already merged. Set to acknowledged.

Description

The structs OperatorCommitment and OperatorDataExtended are defined in both IUniFiAVSManager and DeprecatedOperatorData. This is confusing for readers of the code.

Recommendation

Consider renaming the version in DeprecatedOperatorData to incidate they are deprecated.

Description

The eigenPod functions requestWithdrawal() and requestConsolidation() can be called via the InstitutionalVault.

However the equivalent functions of NonRestakingWithdrawalCredentials : requestWithdrawal() and requestConsolidation() have to be called directly on the NonRestakingWithdrawalCredentials contract. This is inconsistent.

Recommendation

Consider also adding functions in InstitutionalVault to call the NonRestakingWithdrawalCredentials functions.

Also consider renaming the functions to make clear for which underlying contract they are meant because they have similar names.

Description

Function requestConsolidation() concatenates two dynamic types with bytes.concat(). This allows for collisions.

This could be abused to emit ConsolidationRequested instead of SwitchToCompoundingWithdrawalCredentials.

Note: this is unlikely to occur because requestConsolidation() is authorized.

Note: the comparable contract [EigenPod] does check the length of the Pubkeys.

Recommendation

Consider checking the length of request.srcPubkey and request.targetPubkey.

Puffer

An invalid consolidation request may still emit an event on the smart contract, but they simply wouldn't get accepted by the beacon chain. Technically. You can do a consolidation request from EOA for 2 random pubkeys (of legit length) and the tx would not revert. So from when we are dealing with contracts, you can trigger the events in the function even though nothing would really happen.

Description

NonRestakingWithdrawalCredentials::requestWithdrawal() does emit WithdrawalRequested.

However the comparable function EigenPod::requestWithdrawal() can also do an emit ExitRequested:

// Emit event depending on whether the request is a full exit or a partial withdrawalif (request.amountGwei == 0) emit ExitRequested(pubkeyHash);else emit WithdrawalRequested(pubkeyHash, request.amountGwei);

Recommendation

Consider the usefulness of emitting ExitRequested() and consider adding it.

Puffer

Added indexed as an alternative solution instead of creating a new event.

Description

The functions EigenPod::requestWithdrawal() and EigenPod::requestConsolidation() send back excess ETH.

However the comprable functions NonRestakingWithdrawalCredentials::requestWithdrawal() and NonRestakingWithdrawalCredentials::requestConsolidation() don't do this.

Recommendation

Consider also returning excess ETH.

Puffer

Acknowledged, we are dealing with dust amounts.

Description

The error CommitmentChangeNotReady() is no longer used.

Recommendation

Consider removing the unused error(s).

Description

Several functions still use memory in the interface definition, while the implemenation uses calldata. This is inconsistent.

Additionally setOperatorCommitment() and updateAVSMetadataURI() still use memory in the function parameters.

Recommendation

Consider changing setOperatorCommitment() and updateAVSMetadataURI() to use calldata.

Consider making the interface consistent with the implementation. Also inherit from the interface to make sure they are consistent:

-contract UniFiAVSManager is UniFiAVSManagerStorage, ... {+contract UniFiAVSManager is IUniFiAVSManager, UniFiAVSManagerStorage, ... {

Puffer

Solved in PR 54 and PR 55.

Description

A comment in createVault() says the return parameters are: {vault, access manager}, however the real return parameters are {access manager, vault}.

Recommendation

Consider updating the comment.

Puffer

Acknowledged. The factory is already deployed on the mainnet.

Description

When making customExternalCall, the code check if the action is prohibited.

(bool isAllowed,) = IAccessManager(authority()).canCall(_msgSender(), target, bytes4(data));

However, the restricted user can bypass the restriction via customDelegateCall

Recommendation

Consider add the validation

(bool isAllowed,) = IAccessManager(authority()).canCall(_msgSender(), target, bytes4(data));

to customDelegateCall as well.

Description

Several variables could be cached so save gas, provided there is no stack issue.

Recommendation

Consider caching the following:

• array lengths in for loops
• indexed variables in a for loops, that are used multiple times
• call to functions inside a for loop, that always give the same result, like getMerkleRoot() and getWithdrawalCredentials()

Description

Function registerClaimer can be optimized by taking actions outside the for loop.

Recommendation

Consider changing the code in the following way:

function registerClaimer(address claimer, PubkeyRegistrationParams[] calldata params) external {+   BLS.G1Point[] memory g1Points = new BLS.G1Point[](2);+   BLS.G2Point[] memory g2Points = new BLS.G2Point[](2);+   g1Points[0] = NEGATED_G1_GENERATOR();    for (uint256 i = 0; i < params.length; ++i) {        ...     -       BLS.G1Point[] memory g1Points = new BLS.G1Point[](2);-       g1Points[0] = NEGATED_G1_GENERATOR();        g1Points[1] = ...        ...-       BLS.G2Point[] memory g2Points = new BLS.G2Point[](2);        g2Points[0] = ...        g2Points[1] = ...        ...    }}

Description

In function _startValidators(), the conversion with _convertGweiToWei() could be after the loop to save some gas.

Recommendation

Consider changing the code in the following way:

uint256 totalAmount;for (uint256 i = 0; i < pubKeys.length; i++) {-   totalAmount += _convertGweiToWei(amountsInGwei[i]);+   totalAmount += amountsInGwei[i];}+   totalAmount = _convertGweiToWei(totalAmount);