BracketFi, Round 1

The existing calculateBurn logic incorrectly calculates how much of the token should be returned to the user for the given amount of brktETH value.

For example:

(1) User deposits 10 WSTETH and receives 10 brktETH. Assume this is the entire brktETH supply.

(2) ETH rate of WSTETH goes from 1 ETH to 1.1 ETH.

(3) User burns all 10 brktETH shares, but does not receive 10 WSTETH. Instead they get (1.1 ether * 11 ether / 1 ether) which is 12.1 WSTETH, more than the 10 WSTETH they put in.

This can be extremely detrimental to protocol as a user may extract more funds than appropriate, as well as prevent a depositor from withdrawing all their shares. Furthermore, this issue will occur every single time burn is called. To accurately calculate how much token should be returned, instead the ETH value of the redeemed brktETH should be divided by the ETH value of 1 token.

Change the calculation to return Math.mulDiv(value, 1 ether, oracle.getRate(token));

The mint function in the BrktETH contract currently deposits tokens into the contract—updating the token’s totalDeposit—before calculating the amount to mint. As a result, the user’s newly deposited tokens are included in the total value during the share calculation, causing the user to receive fewer shares than intended.

For example:

(1) Alice deposits 10 WSTETH and mints 10 brktETH

(2) Bob deposits 10 WSTETH directly after, and mints 5 brktETH (10 brktETH * 10 ETH / 20 ETH), although he should receive the same amount of brktETH as Alice due to 50-50% supply of the pool.

Consequently, this causes loss of assets for the depositor as they receive less shares than necessary, and will occur each time after the first mint.

Modify the mint function so that it calculates the amount of tokens to be minted before depositing them into the contract.

The burn function currently burns the user’s brktETH tokens before calculating the amount of collateral they are entitled to. By doing so, the calculation for colAmount takes place after the user’s share has already been removed from the supply, which may cause them to receive more collateral than they should or end up with nothing at all if they are the only remaining shareholder.

Modify the burn function to calculate the collateral amount before burning the user’s brktETH.

In the updateNav function there is no update to the lastNavUpdate variable, therefore the confirmation period validation will never apply past the initial 1 day period after the vault starts.

Update the lastNavUpdate variable in the updateNav function

The current calculateMint implementation assumes that if totalSupply is zero, there is no existing pool of assets and thus sets brktAmount = value:

function calculateMint(uint256 value) public view returns (uint256 brktAmount) {
    uint256 supply = totalSupply();

    if (supply != 0) {
        brktAmount = Math.mulDiv(supply, value, getTotalValue());
    } else {
        brktAmount = value;
    }
}

However, in a very specific edge case scenario, it could be possible that totalSupply is non-zero but getTotalValue is actually zero, for example, due to a sudden asset devaluation. In that case, a division by zero would occur blocking any new deposits (once the issue where you deposit before calculating the minted brktAmount is corrected).

Update the calculateMint function as shown below:

function calculateMint(uint256 value) public view returns (uint256 brktAmount) {
    uint256 supply = totalSupply();
    uint256 totalValue = getTotalValue();

    if ((supply != 0) && (totalValue != 0)) {
        brktAmount = Math.mulDiv(supply, value, totalValue);
    } else {
        brktAmount = value;
    }
}

If the NAV_UPDATER calls updateNav with a newNav that is lower than the previous one, a user can frontrun the update and trigger a withdrawal. If the delay is only 1 epoch, then the user's shares will be valued at the previous epoch's NAV which is higher, withdrawing more assets and avoiding the loss.

Consider enforcing that the delay is greater than 1 epoch, otherwise clearly document this risk and using a private RPC.

The claimManagerPerformanceFees function may be called once every 90 days and each time the accruedManagerPerformanceFees are reset to zero.

This way a manager does not have to continue to pay down a large negative performance fee if it has accrued and has been being paid off for a long duration of time.

However if a manager submits an updateNav call with a negative performance fee directly before the 90 period is over they can almost immediately avoid the negative performance fee by calling the claimManagerPerformanceFees function again, whether on purpose or by accident.

Consider using a separate interval for clearing negative performance fees which resets every time a new negative performance fee is added.

Within functions whitelistCollateral and blacklistCollateral the index of the token is retrieved to whitelist/blacklist the token respectively. The issue is that the collateralsIndex starts from 1, so the whitelist/blacklist will be set for the wrong token: uint256 index = collateralsIndex[token];

Use function _getIndex instead since it subtracts the returned index by 1.

The BracketVault.withdrawableAssets() should allow users to query the max amount of assets they can withdraw from the vault.

However, this function will incorrectly subtract pending deposit shares when the lastDeposit has occurred in a previous epoch. The assets for a lastDeposit that occurred in a previous epoch should be represented as withdrawable though since they will be minted to the user in the withdraw function.

Modify the withdrawableAssets view function such that the pendingDepositShares are only removed from the withdrawable amount when the epoch of the lastDeposit is the same as the current epoch:

function withdrawableAssets(address account) external view returns (uint256) {
   uint256 lastNav = epoch == 0 ? 1e18 : navs[epoch - 1];
   Deposit memory _deposit = lastDeposit[user];
   if (_deposit.epoch == epoch) return convertToAssets(sharesOf(account) - convertToShares(_deposit.assets, vanityNav), lastNav);
   else return convertToAssets(sharesOf(account), lastNav);
}

The BrktETH contract relies on the getTotalValue function to determine the total value of all collateral denominated in ETH, which is critical for functions like minting and burning. The process involves fetching each collateral’s value and summing the results. However, if any one token’s price retrieval fails, the entire getTotalValue function reverts—causing all dependent operations (e.g., minting, burning) to fail as well. For instance, the EZETH token uses an external oracle that performs certain validations when fetching its price, which could revert if those validations fail. This creates a single point of failure that disrupts the contract’s functionality whenever a single token’s price feed encounters an issue.

Implement a fallback oracle for each token so that if the primary method reverts (e.g., due to external validation errors), the contract can still fetch the token’s price. This prevents the entire getTotalValue function—and consequently critical operations like minting or burning—from halting when a single price feed fails.

In the _processDepositsWithdrawals function the available balance for withdrawal by the users is based on the _getManagerAvailableBalance, which does not set aside the accruedManagerPerformanceFees (if positive), accruedManagerTvlFees, and accruedBrktTvlFees.

If a manager has not claimed these fees in a significant amount of time, but then submits an updateNav where they cannot fully cover all withdrawals for this epoch then the full manager balance or approval amount is used up.

The fee amounts will still be tracked in the accruedBrktTvlFees, accruedManagerTvlFees, and accruedManagerPerformanceFees variables, but the manager will not be able to claim these amounts as the brktEth and brktEth approval has been removed from the manager address.

Consider reducing the available amount by the accruedManagerPerformanceFees (if positive), accruedManagerTvlFees, and accruedBrktTvlFees in the _processDepositsWithdrawals function.

Function swapCollateral lacks validation that tokenIn != tokenOut. In such a case, there could be an early return not to waste gas on calculating the rebalance and transfers.

Early return if tokenIn == tokenOut

The _getWstethRate function calls the stEthPerToken() function in the WSTETH contract, which returns the amount of stETH per wstETH. This value is then treated as if stETH were pegged 1:1 to ETH. However, stETH can and has previously depegged from ETH (Ref), making this assumption unreliable.

Use a reliable price feed, such as Chainlink’s stETH-ETH feed (Link), to accurately determine the ETH value instead of assuming a one-to-one peg.

Over time fees are accrued within the BracketVault: accruedManagerPerformanceFees, accruedManagerTvlFees, and accruedBrktTvlFees.

However, there is no guarantee that there is sufficient brktETH balance for the fees to be claimed because user withdrawals may decrease the manager's balance.

For example:

1. Nav is 1 ether,
2. Alice deposits 10 brktETH and gets 10 shares. She is the sole depositor.
3. Nav increases to 2 ether, with >0 fees accumulated.
4. Alice burns 10 shares to get 20 brktETH, which comes from the manager's balances.
5. Fee claim is attempted but manager does not have sufficient balance for the fee and causes a ERC20InsufficientBalance revert.

Ensure managers are aware to set aside brktETH for fees.

The mint and burn functions in the BrktETH contract rely on the vault’s total collateral value to determine the number of shares or collateral token a user will receive. Because the vault uses tokens that can change in value at any time—for example, through rebases or slashings. This can lead to unexpected outcomes, such as the user ending up with fewer shares minted, or returning a different ratio of collateral when burning tokens.

Implement slippage control that allows users to specify acceptable thresholds.

The following param/error is not used in BracketVault contract:

• feeClaimer state variable
• DepositIsCurrentEpoch error

Remove unused code or consider adding it to the current implementation.

The BracketOracle currently supports 10 collateral tokens. In case all 10 are added and deposits are non zero, the getTotalValue function will need to iterate through every token to fetch the rate, adding around 270,000 gas, with Renzo's Staked ETH rate being the most gas intensive.

As a result, main actions will have an increased gas consumption:

• burn: 325671
• mint: 362535

As the protocol is deployed on the Ethereum mainnet , this gas cost can become a barrier for users to invest in the BrktEth vault.

Consider this scenario when adding supported collaterals.

There are ten collaterals currently supported in BracketOracle, but BrktETH.addCollateral does not check if the collateral being added is currently supported.

As the addCollateral is only called once, it will be wise to make a call to getRate(token) to ensure its supported.

In the future, when new collaterals are added (i.e. apxETH, ETHx), this check will ensure BracketOracle is updated first.

When adding a new collateral, consider executing getRate(token) to verify if the oracle supports it.

During deposit and withdraw, the assets param is not validated, so it can be 0. Although there is no major impact, the function does not revert and events are emitted, potentially causing issues in the UI.

Consider validating for zero asset amount during deposit and withdraw.

The manager address is set during vault initialization. However, if there is an issue with manager or it's compromised, there is no admin function to update this address.

Similarly, withdrawal delay is set at initialization but can't be updated again.

Consider adding admin functions to update manager and withdrawalDelay.

The BracketVault, BracketOracle and BrktETH, contracts are currently marked as unlicensed, as indicated by the SPDX license identifier at the top of the file: SPDX-License-Identifier: UNLICENSED

Using unlicensed contracts can lead to legal uncertainties and conflicts regarding the usage, modification and distribution rights of the code.

It is recommended to choose and apply an appropriate open-source license to the smart contract. Some options are:

1. MIT License: A permissive license that allows for reuse with minimal restrictions.
2. GNU General Public License (GPL): A copyleft license that ensures derivative works are also open-source.
3. Apache License 2.0: A permissive license that provides an express grant of patent rights from contributors to users.

The balanceOf function relies on the sharesOf to determine the balance of a user. The sharesOf function includes the pendingDepositShares which can include funds that the user has not yet been minted from the lastDeposit and may not be able to presently mint in the case where the lastDeposit.epoch == epoch.

As a result the balanceOf and sharesOf function reflects funds that are not available to the user and may lead to confusion for users and integrators.

Be aware of this behavior, if it is expected then be sure to document this clearly for users and integrators.

The updateVanityNav function may be called to set the vanityNav to something other than 1e18 before the vault has been started with the startVault function.

This may lead to unexpected behavior and should not be a supported interaction.

Consider validating that the epoch is greater than 0 in the updateVanityNav function.

The owner of BrktETH is able to blacklist collaterals, preventing any further deposits. However, this does not prevent collateral withdrawals.

If one of the external protocols is compromised, it will create a bank run, as users will try to withdraw the other collaterals not affected. In this case, a pausing mechanism will help to address the situation and implement the appropriate fixes.

Pausing the protocol can also allow owner to prevent user actions during an upgrade or fix.

Consider adding a pausing mechanism, inheriting from OZ PausableUpgradeable and implement the admin functions pause and unpause.

In the initialize function there is an address 0 check against all entries of the tokens array, however the addCollateral function already implements such a check.

Remove the redundant address zero check in the initialize function for the tokens entries.

The manager will use brktETH from vault depositors for certain investment strategies. Therefore, the brktETH tokens will need to be burned to extract the underlying collateral.

The issue relies on the withdrawal mechanism, as users will need to wait 5 days to be able to claim the collateral. This creates a barrier for the manager and affects investments during this waiting period.

Consider adding an exception for managers, either receiving the collateral immediately or reduce the waiting period.

The addCollateral function does not validate a maximum number of collateral tokens that can be added to the array.

As a result the owner may on accident add too many collateral tokens over time which must all be looped over in the getTotalValue function. This may cause operations to require more than the block gas limit or at least be quite expensive to operate.

Consider adding a validation against a maximum number of supported collaterals. Additionally, consider implementing functionality to be able to remove collateral tokens from the collaterals list when they are blacklisted and have no deposits so that this does not become an issue over time.

In the swapCollateral function there is no way for the caller to specify the worst rate they are willing to accept for a swap.

There is no slippage at the bracket level, however the rate reported by some tokens may be manipulated by a malicious actor who wishes to grief the owner who is making the swap through bracket.

For example, the Renzo EZEth price may be manipulated by force sending Ether to the depositQueue and withdrawQueue address.

https://github.com/Renzo-Protocol/contracts-public/blob/master/contracts/RateProvider/BalancerRateProvider.sol#L31

https://github.com/Renzo-Protocol/contracts-public/blob/9be7ca8619adc089dcd533a6796e1750d23061b5/contracts/RestakeManager.sol#L429

Consider adding a maxAmountIn parameter to the swapCollateral function to allow the caller to protect themselves from any potential griefing attacks that may apply to any arbitrary collateral token.

In the removeCollateral function the collateralsIndex entry for the token that is moved from the back of the list to replace the token being moved is not updated. Furthermore the index entry of the collateralsIndex for the token being removed is not cleared.

This will perturb the accounting of the system and validation of collaterals.

Delete the entry for the token being removed from the collateralsIndex mapping.

Furthermore update the collateralsIndex entry for the token at the back of the list that is being moved to overwrite the removed token.

The removeCollateral function should look as follows:

function removeCollateral(address token) external onlyRole(DEFAULT_ADMIN_ROLE) {
      uint256 index = _getIndex(token);
      if (collaterals[index].whitelisted) revert CollateralNotBlacklisted();
      if (collaterals[index].totalDeposit != 0) revert CollateralNotEmpty();

			address moveToken = collaterals[collaterals.length - 1];

      collaterals[index] = moveToken;
      collateralsIndex[moveToken] = index + 1;

      delete collateralsIndex[token];
      collaterals.pop();
}

Throughout the BrktEth contract several invalid index checks are made which errantly assume that a 0 index indicates that the token is not a supported collateral.

In the whitelistCollateral, blacklistCollateral, and removeCollateral functions the result of the _getIndex is compared against 0 to see if the collateral token has been configured. However this is incorrect as the returned index from the _getIndex function is already reduced by 1.

Remove the errant index 0 check in the whitelistCollateral, blacklistCollateral, and removeCollateral functions.

The lastNavUpdate variable is updated to block.timestamp before the conditional check that uses this variable. Consequently, the condition lastNavUpdate + NAV_NO_CONFIRMATION_PERIOD > block.timestamp will always evaluate to true. As a result, only accounts with the NAV_CONFIRMER_ROLE will be able to call the updateNav function.

Update lastNavUpdate only after the condition is validated.

Several upgradeable contracts inherit OpenZeppelin contracts/libraries that require calling their respective __[LibraryName]_init functions inside the contract’s own initialize function.

Although some contracts do make certain initializer calls (e.g. __Ownable_init(msg.sender), __ERC20Permit_init("Bracket Liquid Staked ETH"), __ERC20_init(...), etc.), others do not consistently call all the initializers of their inherited contracts. For example:

• BracketPricing inherits UUPSUpgradeable but never calls __UUPSUpgradeable_init().
• BrktETH inherits UUPSUpgradeable, PausableUpgradeable, etc. and only partially initializes some of them (e.g., __AccessControlDefaultAdminRules_init is called, but there is no corresponding call to __UUPSUpgradeable_init() or __Pausable_init()).

Review each upgradeable contract’s parent classes to identify all required initializers (e.g. __UUPSUpgradeable_init, __Ownable_init, __AccessControl_init, __Pausable_init, etc.) and ensure they are properly invoked in the contract’s initialize function.

In the BracketPricing contract, the function _getPythPrice is declared but not implemented, making any Oracle.Pyth configuration inoperable.

Implement _getPythPrice to properly fetch and decode the Pyth price.

The _getChainlinkPrice function in BracketPricing contract multiplies the price by 1e10 to convert it to 18 decimals (uint256(price) * 1e10). This works only if the Chainlink aggregator returns an 8-decimal price. If an aggregator uses any other number of decimals, the result becomes incorrect.

Retrieve the feed’s actual number of decimals via priceFeed.decimals() and scale the value accordingly. A more robust example:

function _getChainlinkPrice(address oracle) internal view returns (uint256) {
    AggregatorV2V3Interface priceFeed = AggregatorV2V3Interface(oracle);

    uint8 decimals = priceFeed.decimals();
    (
        ,
        int256 price,
        ,
        ,
    ) = priceFeed.latestRoundData();

    if (price <= 0) revert ZeroPrice();

    // Convert the price to 18 decimals
    uint256 scaledPrice = uint256(price);
    if (decimals < 18) {
        scaledPrice *= 10 ** (18 - decimals);
    } else if (decimals > 18) {
        scaledPrice /= 10 ** (decimals - 18);
    }

    return scaledPrice;
}