Contract Updates

In the unwrap function the transferFrom invocation transfers the amount specified by the user, however this is a shares amount rather than a token amount.

Use the assets value in the transferFrom invocation instead.

The overridden _update function in the RebasingToken contract has been updated to include a conversion to shares for the amount being transferred. However the vault treats the amount being specified for the transfer/mint/burn action as a share amount.

Therefore in the _clearDeposit and withdraw functions when the _mint and _burn functions are called with values that have already been converted to shares, these values are doubly converted to shares thus perturbing the accounting system.

Consider removing the conversion to shares in the _update function, otherwise refactor the vault to treat the amounts passed into the transfer/mint/burn to be raw asset amounts rather than share amounts.

The BracketWrappedVault contract inherits from PausableUpgradeable without implementing functions to expose the pause/unpause functionality.

As a result the wrap and unwrap functions cannot be paused.

Implement pause and unpaused permissioned external functions.

In the BracketWrappedVault contract the constructor is used to initialize the contract with the correct vault, decimals, name and symbol.

However the BracketWrappedVault contract is a UUPS implementation contract, therefore this assignment should occur in the initialize function to instantiate the storage slots of the proxy contract being used with this implementation contract. And the constructor of the implementation contract should use disableInitializers to disable the initialization of the UUPS implementation contract.

Move the constructor logic into an initialize function and use disableInitializers in the constructor of the BracketWrappedVault contract.

The BracketWrappedVault constructor fails to call the __Ownable_init function of the OwnableUpgradeable base contract. As a result no owner is assigned for the protocol and upgrades using the UUPS functionality cannot take place.

In the new initialize function for the BracketWrappedVault contract, invoke the __Ownable_init function and assign the appropriate owner.

The activeBalanceOf has been changed to use the vanityAssetsPerShare function which maintains it’s original behavior of fluctuating the current reported balance of users by the vanityNav, however the activeSupply function still uses the assetsPerShare function which is now reliant on the reported nav from the previous epoch.

As a result the reported values from the summation of user balances and the totalSupply result will not agree when the vanityNav does not match the reported nav from the previous epoch.

Update the activeSupply function to use the vanityAssetsPerShare function instead of the assetsPerShare function if it is desired that the supply and balances of users float with the vanityNav.

In the BracketWrappedVault the convertToShares and convertToAssets functions are used with the assetsPerShare rate to convert to and from shares.

However the assetsPerShare rate is not what will be used to determine how many shares ought to be transferred, instead the vanityAssetsPerShare rate is used in the _update function.

This will result in an unexpected amount of shares being transferred when the vanityNav does not match the nav of the last epoch.

Use the vanityAssetsPerShare instead of the assetsPerShare in the wrap and unwrap functions when converting between shares and assets.

In the _update function a conversion to shares by the vanityAssetsPerShare rate has been introduced. This may be fine for new vaults which are being introduced, however for existing vaults this will immediately change the interpretation and valuation of entries in the balances mapping.

Do not perform this upgrade for vaults which already exist

In the _getDiff function the _currentNav is always used in the denominator of the basis points diff calculation. As a result, if the nav ever goes to zero for any vault and is then updated to a nonzero value, the update will panic revert in the _getDiff function due to attempted division by 0.

Consider early returning a default value of 0 from the the _getDiff function to avoid division by zero in this case.

The maxBpsIncrease value cannot be assigned higher than 10_000 basis points, meaning that any nav increase cannot reflect a doubling in the price of a bracket vault share.

This may inadvertently over-limit the appropriate amount of share value increase that can occur in a given epoch, since it could be reasonable for a fund to double it’s nav in a week long epoch in certain outlier circumstances.

Consider if the 10_000 basis point cap should hold for the increase validation, or if it should be higher.

In the wrap and unwrap functions, amounts of zero are allowed which may lead to unexpected behavior. To limit the unexpected paths that can be taken these values should be validated against.

In the wrap function, even non-zero but very small amount values can round to a zero share amount, so in the wrap function the share value should be validated to be nonzero.

In the wrap function, validate that the share value is not zero, and in the unwrap function validate that the amount value is not zero.

The approval amounts in the rebasingToken contract are tracked and spent based on an asset basis rather than a share basis which leads to a mismatch between the shares which can be transferred relative to the approval amount over time.

For example, if User A approves User B to spend 10 amount at T = 1 when the assets per share is 1, then User B can spend 10 shares from User A.

However if at T = 2 the assets per share is 2, User B can now only spend 5 shares from User A.

This may simply be the expected behavior, since the value of the assets has gone up and so the approval is desired to be in the final amount rather than the shares amount.

However this creates notable behavior when rounding is taken into account, where small approval amounts can be spent without the transferring of actual assets. For example, if the asset per share rate is 1.1e18, a user is approved to spend 10e18 tokens from User A, and they transfer 1 wei of assets from User A the approval will decrease by 1 wei without transferring any assets from User A since the resulting transferred shares value rounds to zero.

Overall these behaviors are innocuous but worth noting and confirming if they are the expected behavior.

In the RebasingToken contract the convertToShares and convertToAssets functions which accept only a shares amount have been introduced and are not used anywhere in the codebase. This indicates that they are for usage by integrating protocols or users interacting with the vault.

In this case, it should be noted that these functions may be misleading, since they use the assetsPerShare value which is based on the per share rate of the last epoch, which is not what is reflected in the user balances by way of the vanityAssetsPerShare rate being used in the activeBalanceOf function.

Be aware of this potentially misleading behavior of the convertToShares and convertToAssets functions.

In the canDeposit and canWithdraw functions there is no case that handles the CLOSED ActionState nor is there a default return value for the function if none of the explicit cases are matched.

Consider adding a default return false statement to silence the compiler warning and implicitly cover the CLOSED ActionState case.

In the NavUpdater contract there is an active state tracking for each vault, however this mirrors the active state tracking for each vault already done in the KYCWhitelist contract.

Consider if this state can be deduplicated to simply rely on the KYCWhitelist tracking of the active state of a vault.

In the constructor for the BracketWrappedVault the initializer invocations are missing for the PausableUpgradeable, and UUPSUpgradeable contracts.

These initializer functions do nothing, however it is a best practice to call these initializer functions in the event that a future version of these base contracts in a future upgrade would have some logic.

Consider calling the initializer functions for the PausableUpgradeable, and UUPSUpgradeable contracts in the new BracketWrappedVault initializer function.

Due to the round down conversion in the convertToAssets function there may be cases where a dust amount of shares are left held by the BracketWrappedVault contract after all users have withdrawn.

Simply be aware of this behavior.

The VaultRouter has been introduced to allow users to easily wrap WETH and deposit into the vault with. However this contract allows users to bypass the KYC restriction because the BracketVault performs validation on the msg.sender in the deposit function, which would be the VaultRouter contract, which must be whitelisted to function.

Either perform the whitelist validation also in the VaultRouter contract or consider removing the VaultRouter contract entirely.

The BracketVault has a decimals amount of whatever the underlying token has and uses convertToShares to determine the amount of vault tokens to mint.

However the default value for the assetsPerShare is 1e18 when the epoch is 0, this leads to extreme precision loss and ultimately loss of funds for lower decimal vaults.

Report the default assetsPerShare as 10 ** _decimals. Furthermore ensure that the updates for the nav for these vaults is in line with the decimals of the vault to avoid loss of funds and precision loss.

Previously, the transferFrom and transfer functions accepted value parameters that represented share values. However now they have been updated to accept value parameters that represent asset values.

Originally, the approvals consumed in transferFrom would be in shares, however now the approvals consumed in transferFrom are based on the provided value parameter in assets.

So the net effect on approvals of this upgrade for existing vaults is that the approval values made by users will be changed from share values to asset values. Depending on the NAV of the vault this will either increase or decrease the amount transferrable by approvals for users.

Be aware of this change and ensure it will not cause any significant unexpected changes for users. Users should be aware of this change as well.

The balanceOf function has been updated to only rely on the activeBalance and ignore the inactiveBalance.

This is however contradictory to the implementation of the totalSupply function which reports the sum of the activeSupply and inactiveSupply as the totalSupply.

As a result the sum of the individual user balances does not match the aggregate totalSupply reported by the BracketVault.

If it is desired to only factor in the active shares, then update the totalSupply function to only track the activeSupply.

The KYCWhitelist contract is a UUPSUpgradeable implementation contract, however storage variables have been added before the isUsedNonce mapping which will change the storage values that it is mapped to.

This will reset all of the usedNonce reporting and allow replay of these signatures.

Only append new storage variables after the isUsedNonce mapping.

The KYCWhitelist now has a blacklist mode, whereby only blacklisted addresses are considered not whitelisted for deposits and withdrawals.

However this blacklist is easily bypassed by a blacklisted user by transferring their deposit tokens or bracket vault tokens to a non-blacklisted address they control and performing the desired actions with the vault.

Be aware of this loophole and consider performing transfer checks if the KYC whitelist is in blacklist mode.

In the BracketVault contract the totalPendingShares variable is tracked to represent the total sum of all pending shares which technically belong to users but have not yet been minted to their underlying balances mapping entry.

The use case for this variable is to ensure that the totalSupply reported is the same as the sum of the user balances. However the tracking of the totalPendingShares does not correctly allow this.

The totalPendingShares variable is adjusted for withdrawals, however this has nothing to do with the pending shares that should be minted to users via deposits and thus perturbs the totalSupply accounting. For example, the totalSupply is based on the totalMintedShares()) + totalNonMintedShares() result. totalNonMintedShares simply returns totalPendingShares for the BracketVault contract.

Whenever a withdrawal is initiated, it is both burned from the totalMintedShares and added to the totalPendingShares. This means there is no net change in the reported totalSupply, which does not agree with the underlying user balances tracking.

Furthermore, the totalPendingShares deduction by the withdrawalShares in the _processDepositsWithdrawals function is also invalid as this has no impact on the pending deposit shares of users.

Remove the deduction of withdrawalShares from the totalPendingShares in the _processDepositsWithdrawals function.

And remove the addition of shares to the totalPendingShares in the withdraw function.

With the advent of EIP 7702 accounts can now create code at their own address. This means that any whitelisted user can open up the whitelisted functionality to be used by non-whitelisted users.

Consider the following scenario:

• Assume protocol has a whitelist mapping, where only users who are whitelisted can call function A
• Bob is whitelisted for his address 0xFF
• Bob uses 7702 to set his 0xFF account code to:

contract {

     address victimSystem;

     function callThis(...) external {
           victimSystem.whitelistedFunction(...);
     }

}

• Now anyone can call the whitelisted function through calling callThis on 0xFF

Be sure to monitor whitelisted accounts to ensure that they do not add functionality that opens up the whitelist to non-whitelisted accounts. Otherwise consider requiring that msg.sender == tx.origin for the whitelist.

The assetsPerShare function has been updated to use the NAV value of the last epoch rather than the vanityNav of the current epoch.

As a result, for existing vaults this will cause a stepwise jump in the balance reported to users and the reported totalSupply.

This may cause unexpected issues for integrations which may rely on this data and may be unexpected for users.

Be sure to document this change for integrators and users who may be impacted.

The lock frequency was previously hardcoded at 7 days, however now it is configurable by the admin lite role and upon initialization.

The vault could however benefit from some validations on the frequency to ensure that it is neither configured to long or too short.

For example, the BracketVault contract will stop functioning after type(uint16).max epochs, therefore an epoch time of 10 minutes is too low.

Similarly, there is no validation that the configuration of the withdrawDelay is greater than some minimum and less than some maximum.

Consider if validations should be introduced which ensure the lockFrequency and withdrawDelay is bounded between a maximum and minimum range on initialization and updates.

With the new NavUpdater contract there is no way to update the vanityNav with the updateVanityNav function as this function is not invoked in the NavUpdater contract.

Be aware of this unreachable function.

In the NavUpdater contract the onlyActiveVault modifier is unused.

Consider implementing it’s use or removing it.

The NavUpdater has an open initialize function which can be called by anyone and used to grant the caller the default admin role of the contract.

If the NavUpdater has already received the NAV_UPDATER_ROLE for any vault instances a malicious actor could initialize it and unexpectedly start vaults or issue nav updates for existing vaults.

Be sure to initialize the NavUpdater contract before it receives any permissions. Furthermore, ideally the NavUpdater contract is initialized in the same transaction as it’s deployment, or at least in the same deployment script.

In the RebasingToken balanceOf function, the inactiveBalanceOf contributor has been removed, which will cause a stepwise change in the balances of users and Smart Contracts which have pending deposits.

This may be unexpected and could potentially cause issues in integrating systems.

Be sure to warn any integrators and users of this impending stepwise change.

After the balances and totalSupply mismatch is resolved from M-02, Balances Do Not Match Total Supply, there remains a rounding error which invalidates the sum of user balances == totalSupply invariant.

This is due to rounding which occurs upon converting between users shares and asset amounts on transfers and deposit/withdrawals and on the calculation of user balances and totalSupply.

Extensive fuzz testing has shown that the inaccuracy can only occur in the direction of sumOfUserBalances ≤ totalSupply, and is at maximum off on the order of thousands of wei.

Be aware of this rounding difference and invalidation of the sum of user balances == totalSupply invariant.

In the BracketWrappedVault contract the IERC20 interface is explicitly imported but not used.

Consider removing the IERC20 interface import.

With the advent of EIP 7702 accounts can now create code at their own address. This means that any whitelisted user can open up the whitelisted functionality to be used by non-whitelisted users.

Consider the following scenario:

• Assume protocol has a whitelist mapping, where only users who are whitelisted can call function A
• Bob is whitelisted for his address 0xFF
• Bob uses 7702 to set his 0xFF account code to:

contract {

     address victimSystem;

     function callThis(...) external {
           victimSystem.whitelistedFunction(...);
     }

}

• Now anyone can call the whitelisted function through calling callThis on 0xFF

Be sure to monitor whitelisted accounts to ensure that they do not add functionality that opens up the whitelist to non-whitelisted accounts. Otherwise consider requiring that msg.sender == tx.origin for the whitelist.

The KYCWhitelist canTransfer function has been added to the _update function and is now validated upon every invocation of _update. This means that the canTransfer validation applies for mints and burns when the from or to address is zero.

This may be unexpected given the naming of the predicate function, canTransfer, however seems appropriate given that this is how the blacklist is enforced.

Consider if it is expected that the canTransfer validation is performed on mints and burns.

In the fixTotalNonMintedShares function the sumWithdrawals value only increments until the maxEpoch which is assigned as currentEpoch + withdrawDelay.

However if the withdrawDelay is updated it may be possible that withdrawals exist past the defined maxEpoch and may be missed by the correction.

Be sure that the maxEpoch is in line with the latest withdrawal for each vault when the fixTotalNonMintedShares function is invoked.

The fixTotalNonMintedShares function can be called again when the pending non minted shares return to zero. And for vaults which are never “fixed” this function can be called unintentionally.

This function is gated by the admin lite role so it is trusted. Simply be aware of this quirk.