Manifest Finance Protocol

Proof of concept: PoC

The _checkSender on AuthHook.sol is correctly trying to retrieve the user by msgSender(). However this practice must be done only if the periphery interacting with PoolManager is trusted.

Otherwise, the msgSender() can be manipulated to return any address.

Use a trusted pool of periphery contracts that will be calling PoolManager as stated in Uniswap v4 docs. Otherwise, do not allow the execution.

Manifest Team: Resolved.

Proof of concept: PoC

There is no KYC check in the _update function of StakedUSHBase.sol. If a user’s KYC is revoked, they cannot call cooldownAssets since it will revert.

However, they can still call transfer to move their tokens to another KYC’d account, which can then call cooldownAssets.

Remove in _checkRestrictions the checkSanctioned and checkBanned, and use checkUser instead. Otherwise, acknowledge this behavior as an acceptable risk and ensure it is documented.

Manifest Team: Acknowledged.

Proof of concept: PoC

sUSH holders may be banned or sanctioned. When either status applies, users are blocked from depositing/withdrawing/transferring sUSH.

Separately, the sUSH admin can redistribute a restricted user’s balance (either burn to rewards or mint to another user) via redistributeLockedAmount, which is documented as:

The address to burn the entire balance which is restricted (banned or sanctioned)

However, redistributeLockedAmount only checks isBanned for from and to, and does not check the sanctioned status.

As a result, sanctioned users are treated as unrestricted in this path, causing the function to revert (since isFromRestricted is false) and effectively preventing admins from redistributing sanctioned users’ sUSH, and leaving them stuck in the users' wallets.

Include isSanctioned check for both from and to users in redistributeLockedAmount.

Manifest Team: Resolved.

The USHPriceOracle contract will be used to update USHToken prices, which are expected to have 8 decimals based on comments in the contract.

However, the MIN_PRICE (1e4) and MAX_PRICE (1e12) constants are defined based on 6-decimal price values.

If the minimum and maximum price constants are correct and the updater submits prices using 6-decimal precision, a 100x discrepancy will occur since external integrators expect 8-decimal precision.

Conversely, if the updater submits prices using 8-decimal precision, external integrators will receive the correct values, but the effective minimum and maximum bounds will be significantly lower than intended:

the minimum price will be $0.0001 instead of $0.01, and the maximum price will be $10,000 instead of $1,000,000.

Resolve the discrepancy according to the intended decimal precision. If 8 decimals are expected as indicated by the comments, update the MIN_PRICE and MAX_PRICE constants to 1e6 and 1e14 respectively.

Manifest Team: Resolved.

The AuthHook does not have beforeInitialize permission.

If the hook deployment and the initialization of the Manifest-controlled permissioned UniV4 pool are not performed in the same transaction, any user can initialize the pool and set an arbitrary initial sqrtPriceX96, since the parameters required to calculate the poolKey can be known beforehand.

Similarly, the hook has beforeDonate permission set to false. Since the protocol does not want non-KYC’d users to interact with the permissioned UniV4 pool, this action should also be restricted.

Set both the beforeInitialize and beforeDonate permissions to true. beforeInitialize should allow only protocol admins to initialize the pool.

beforeDonate should either allow only KYC’d users, similar to other hooks, or disallow donations entirely, depending on the protocol’s preferences.

Manifest Team: Resolved.

The version in the USHToken contract is "1k", while the domain separator is calculated using the hardcoded version value "1".

As a result, permit signatures that use the correct version cannot be validated because the signature digest is different.

Similarly, the StakedUSHTokenBase contract inherits ERC20Permit. However, token symbol is passed in the constructor instead of token name.

Note that permit signatures can still be valid if users sign based on the public DOMAIN_SEPARATOR, even though the versions and names do not match. However, it will be invalid if users sign it based on the correct version.

Ensure that the constant version value matches the version used in the domain separator.

Manifest Team: Resolved.

If cooldownDuration is set to zero, all users can unstake immediately. However, if it is only reduced (not zero), users who started cooldown earlier remain subject to the longer duration, while new users benefit from the shorter one.

This creates an unfair disadvantage for unaware users who don't recall cooldownAssets after the parameter change.

Store both the block.timestamp and the cooldownDuration active at that time. When checking for unstake eligibility, allow users to unstake at the earlier of the two values (stored block.timestamp + stored cooldownDuration or stored block.timestamp + new cooldownDuration).

This ensures fairness and consistency when cooldown parameters change.

Manifest Team: Resolved.

When a user initiates a cooldown via cooldownAssets or cooldownShares, The contract records a future cooldownEnd timestamp and increases the underlyingAmount held in the cooldown mapping.

However, the current design does not allow users to undo or adjust these requests. Once a cooldown is started:

The expiry time can only be extended further by making additional cooldown calls. If a user changes their mind (they no longer want to unstake), there is no way to cancel or scale down the request. The system forces them to complete the full cooldown process.

Introduce functionality that lets users reverse or reduce cooldown commitments.

Manifest Team: Acknowledged.

Both cooldownAssets and cooldownShares send the assets to the silo as the receiver:

// StakedUSH.sol
_withdraw(msg.sender, address(silo), msg.sender, assets, shares);

In the base implementation, the auth layer enforces KYC on the receiver inside the shared withdraw path via onlyAuthApproved(receiver) > authManager.checkUser(receiver).

Since the Silo is a system contract that isn’t KYC’d, the call reverts until it has been granted KYC through the grantKYC function, causing a temporary DoS when starting the cooldown.

Ensure that the Silo contract is KYC’d by invoking the grantKYC function, either within the deployment script or manually immediately after deployment.

Manifest Team: Acknowledged.

The getKycStatus function checks only the KYC status of an account and does not consider whether the account is banned.

In contrast, the batchGetKycStatus function first checks the banned status of the account and returns false regardless of the KYC status if the account is banned.

As a result, the KYC status of the same account may appear differently depending on which function is used.

Consider updating one of the functions based on the intended behavior so that both functions behave consistently.

Manifest Team: Resolved.

The transferFrom function in the USHToken contract checks the from and to addresses to ensure they are authorized. However, it does not check msg.sender.

As a result, a banned or sanctioned user can still interact with the protocol and transfer other users’ funds if they have an allowance.

The same issue occurs in _update function of StakedUSH token as well.

Check msg.sender as well and block their interaction with the protocol.

Manifest Team: Resolved.

The StakedUSHBase contract has an initializeVault function that initially deposits 10,000 USHTokens to prevent donation attacks. This function can only be called by the owner when the totalSupply is 0.

However, there is no guarantee that this function will be the first deposit unless the deployment and initializeVault call are performed atomically.

A malicious user could front-run initializeVault and deposit only the MIN_SHARES amount of tokens. As a result, initializeVault would be blocked afterward because the totalSupply would no longer be zero.

Although this scenario could occur, it does not result in the same consequences as a traditional donation attack, as the contract enforces _checkMinShares and prevents regular users from minting zero shares.

Ensure that the deployment and initializeVault call are performed atomically. Note that this requires the deployer address to be the _owner specified in the constructor.

Alternatively, a check can be added to the _deposit function to revert if totalSupply is zero. This guarantees that initializeVault is executed as the first deposit, since regular deposits cannot occur until the owner mints the initial shares.

Manifest Team: Resolved.

requiresMultiSigApproval() only checks whether the change exceeds the max percentage:

return _exceedsMaxChange(currentPrice, newPrice);

However, when the updater calls updatePrice, it can also revert on the time-based interval:

if (block.timestamp < s.lastUpdateTimestamp + s.minUpdateInterval) revert UpdateTooFrequent();

Therefore, requiresMultiSigApproval may return false (suggesting updater is fine) while an updater transaction would still revert due to the minimum interval check.

Consider including the interval check in requiresMultiSigApproval.

Manifest Team: Resolved.

The permit function in SolmateERC20Upgradeable uses ecrecover to validate signatures without implementing checks for signature malleability. It has no validation that s is in the lower half of the curve's order.

While the nonce mechanism (nonces[owner]++) prevents signature replay attacks, the lack of proper ECDSA checks remains a deviation from best practices.

It's recommended to require the s value to be in the lower half order.

Manifest Team: Resolved.

The StakedUSHBase.sol contract uses transferInRewards to distribute rewards linearly over an 8-hour vesting period.

This allows stakers to deposit before transferInRewards is called and withdraw after the vesting ends, effectively diluting rewards intended for long-term stakers.

For this dilution to be effective:

• cooldownDuration must be set to 0.
• cooldownDuration must not be increased during the 8-hour vesting window.

Distribute rewards in smaller, more frequent intervals, making dilution gains negligible. Allow the vesting period to be configurable when calling transferInRewards, enabling adjustments based on reward size.

Manifest Team: Acknowledged.

The AuthManager contract correctly implements checks to prevent redundant state changes in its single-user functions like banUser, removeBan, grantKyc, and revokeKyc.

But, the corresponding batch functions (batchBan, batchRemoveBan, batchGrantKyc, batchRevokeKyc) lack these checks.

They unconditionally write to storage for every user in the provided array, even if a user's status is already the intended state. This inconsistency leads to unnecessary SSTORE operations in a loop.

Consider implementing the same check to skip the redundant state changes.

Manifest Team: Acknowledged.

USHToken.sol inherits from SolmateERC20Upgradeable, whose ERC20 implementation allows direct transfers to address(0) without decreasing totalSupply.

Meanwhile, USHToken.sol exposes a circulatingSupply function that returns totalSupply. This means that tokens transferred to address(0) are still counted, leading to an incorrect circulating supply calculation.

Prevent transfers to address(0) or update circulatingSupply to exclude balances at address(0) from its calculation.

Manifest Team: Resolved.

The setAuthManager function in the USHToken contract updates the authManager address, which is used to determine authorized users.

Although it is an owner-only function, sanctioned and banned checks can be bypassed during an update if the new authManager contract does not yet reflect the sanction and ban status maintained by the previous manager.

If an update is to occur, the new contract should persist the state of the previous contract before being set via the setAuthManager call.

Ensure that the new authManager contract preserves the state of the previous one before the update.

Manifest Team: Acknowledged.

The setAuthManager function must be called twice to set the manager. The first call designates the pending manager and starts the timelock, while the second call, made after the timelock period, finalizes the manager.

However, if the second call is made before the timelock ends, it resets the pending manager and restarts the timelock, regardless of whether the provided address is the same as the current pending manager or a different one.

While this is an onlyOwner function, consider adding a check in the else block so that _setAuthManagerReqTimestamp is updated only if the provided address is different from _pendingAuthManager.

Manifest Team: Acknowledged.

In AuthHook.sol, the internal _checkSender function validates users by checking whether they are banned, sanctioned, or have KYC. This validation is enforced at _beforeSwap, _beforeRemoveLiquidity, and _beforeAddLiquidity.

However, the external view function hasValidAuth only checks for hasKyc and isBanned, not the sanctioned status. As a result, hasValidAuth can return true for a sanctioned user, even though _checkSender would revert when the same user interacts with the pool.

This creates an inconsistency between the view function and the actual logic.

Update the hasValidAuth view function to also verify the sanctioned status of the user, ensuring consistency with the _checkSender logic.

Manifest Team: Resolved.

In USHPriceOracle.sol, two constants MIN_PRICE and MAX_PRICE define absolute lower and upper bounds for the price.

The contract also enforces additional constraints, such as minimum/maximum update intervals and minimum/maximum percentage change per update.

Unlike these other constraints, which can be bypassed by the multiSigAddress, the MIN_PRICE and MAX_PRICE bounds cannot be overridden.

This design may lead to unwanted situations where the protocol cannot update the oracle price to reflect real market conditions, even if the value falls outside of the hard-coded thresholds.

If the primary security concern is preventing a compromised priceUpdater from setting arbitrary values, the interval and percentage checks already provide sufficient safeguards.

In contrast, the hard-coded min/max values could unnecessarily block the protocol from setting a valid price in the future.

Consider making the MIN_PRICE and MAX_PRICE thresholds adjustable, or allow them to be bypassed by the multiSigAddress. This ensures that the protocol can adapt to changing market conditions.

Manifest Team: Resolved.

When cooldown is enabled, users must call cooldownAssets / cooldownShares to begin the unlock timer, and later call unstake to withdraw once the timer expires.

Both cooldown entry points overwrite the user’s single cooldown timer:

// in cooldownAssets / cooldownShares
cooldowns[msg.sender].cooldownEnd = uint104(block.timestamp) + cooldownDuration;
cooldowns[msg.sender].underlyingAmount + uint152(assets);

This resets cooldownEnd to “now + duration” on every new request and aggregates the amounts. If a user starts a cooldown (e.g., for 100 tokens) and later starts another cooldown (another 100), the second call extends the wait for the entire balance (200) to the new, later cooldownEnd.

Consider refactoring the cooldown behavior to allow multiple cooldowns, which requires decent refactoring or document the current behavior clearly for users.

Manifest Team: Acknowledged.

There is no limit on what value expectedIndex can take. It only checks that it is greater than the old index.

However, the priceUpdater could set it to type(uint256).max, making it impossible to call the updatePrice function again.

Ensure that expectedIndex is greater than the old index, but only by 1.

Manifest Team: Resolved.

The _exceedsMaxChange function checks whether newPrice has moved beyond maxDailyChangePercentage compared to currentPrice.

It returns true if the limit is exceeded, otherwise false. However, the return value is only used when the caller is priceUpdater, making it useless in the case of multiSigAddress.

Move _exceedsMaxChange inside the if block that checks whether the caller is priceUpdater

Manifest Team: Resolved.

The contract includes checks to prevent redundant state changes in some setter functions like setPriceUpdater, setMultiSigAddress, and setEmergencyPause by reverting with "AlreadyInThisState".

This is a good practice as it avoids unnecessary storage writes and event emissions.

However, setMaxDailyChangePercentage and setMinUpdateInterval functions do not have the same checks.

Consider adding the check to setMaxDailyChangePercentage and setMinUpdateInterval to ensure the new value is different from the existing one.

Manifest Team: Resolved.

The approve function in the USHToken contract include the whenNotPaused modifier. While it makes sense to use whenNotPaused for actual transfer functions, applying it to approve overly restricts users.

This is particularly important when users want to revoke their approvals during a paused period. In the event of a contract pause, users may reasonably want to revoke their approvals as a precautionary measure. Restricting this action during a paused state results in poor user experience.

The OpenZeppelin extension of ERC20Pausable only pauses the _update function, which affects transfers and minting/burning, but does not affect approvals (Reference).

Note that the same issue exists in the permit function as well.

Consider removing the whenNotPaused modifier from approve to allow users to revoke their approvals even during a paused state. Alternatively, ensure this behavior is explicitly documented to inform users.

Manifest Team: Resolved.

The variable maxDailyChangePercentage in USHPriceOracle enforces the maximum price change per update, not per day.

The current name implies that the restriction applies over a 24-hour period, which is misleading.

Rename the variable to reflect its true behavior.

Manifest Team: Resolved.

In StakedUSH.sol, the functions cooldownAssets and cooldownShares use the onlyAuthApproved(msg.sender) modifier to check if the caller is approved. However, both functions later call _withdraw, which again validates caller, owner, and receiver.

This results in redundant authentication checks, since the same validation is effectively performed twice.

Remove the onlyAuthApproved(msg.sender) modifier from cooldownAssets and cooldownShares to avoid redundant checks.

Manifest Team: Resolved.

In StakedUSHBase.sol, the _update function uses an if-else statement to handle zero-address transfers.

This structure is redundant because super._update is always executed, and the only difference is whether restrictions are checked. The branching reduces readability without changing functionality.

Refactor the function to use a single if statement, checking restrictions only when both from and to are not the zero address. This simplifies the flow and avoids unnecessary branching.

Manifest Team: Resolved.

In the wallets batch updaters for example, the code emit BatchAuthorizedWalletsAdded(length) / BatchAuthorizedWalletsRemoved(length) using the input array length, even if some entries are duplicates or already in the desired state.

That means the event can claim “N added/removed” when fewer wallets actually changed.

Only count and emit per-wallet events when a flip actually occurs. The same issue exists in the batch-ban and batch-kyc functions.

Manifest Team: Acknowledged.

/**
* @title IChainalysisOracle
* @dev Interface for the official Chainalysis Oracle at
0x40c57923924b5c5c5455c48d93317139addac8fb
*/

The comment above appears in lines 20–23 of the SanctionsList contract, but it should belong to the IChainalysisOracle interface.

Remove the comment from the SanctionsList contract and add it to the IChainalysisOracle interface.

Manifest Team: Acknowledged.

The updateChainalysisOracle function updates the oracle address but does not set chainalysisEnabled to true if it was previously false.

When chainalysisEnabled is false, setting a new oracle requires two calls: updateChainalysisOracle and setChainalysisEnabled.

Consider setting chainalysisEnabled to true within updateChainalysisOracle when updating the oracle.

Manifest Team: Acknowledged.

The BURNER_ROLE in the USHToken contract can burn any amount from any user without restrictions. This behavior should be clearly documented for users.

Note that the sole purpose of this finding is to inform users.

No fix is required, as this is a design choice of the protocol. It is recommended that this behavior be documented, and the finding can be acknowledged.

Manifest Team: Acknowledged.

The StakedUSH contract overrides the renounceRole function and does not allow this operation.

However, this is not the case in the USHToken contract, and any role including the admin role can be renounced.

Additionally, the contract does not check for a zero address when setting the defaultAdmin in the initialize function.

Consider overriding renounceRole for at least the defaultAdmin role, and adding a zero address check in the initialize function

Manifest Team: Resolved.

The protocol implements a namespaced storage layout for its upgradeable contracts. Each contract has a storage location derived from the keccak hash of its name, such as keccak256("storaged.ush.token") or keccak256("v1.storage.sanctions.list.manifest.finance").

While this approach prevents storage collisions between contracts, it is considered best practice to use the storage location formula from ERC-7201 for namespaced storage layouts, which involves double hashing and masking with ~0xff.

Consider using the ERC-7201 formula to determine storage locations as a best practice.

Manifest Team: Resolved.

In AuthManager.sol, there is a storage variable maxBatchSize that defines the maximum number of updates allowed in a single batch.

This variable is supposed to control batch limits across all batch functions. However, the implementation directly uses the constant MAX_BATCH_SIZE instead of using the storage variable.

Make use of the maxBatchSize storage variable in all relevant batch functions instead of the MAX_BATCH_SIZE constant, ensuring smooth upgradability.

Manifest Team: Resolved.

Unused errors in IUSHToken.sol interface:

• TooFrequentBurn
• TooExcessiveBurn
• InsufficientBalance
• Sanctioned

Unused events in IUSHToken.sol interface:

• Sanction
• MintFailure
• BurnFailure

Unused errors in AuthHook.sol contract

• OnlyAuthedUser
• UserBanned
• InvalidAuthManager
• TimelockNotExpired
• PendingManagerMismatch

Remove unused events/errors

Manifest Team: Resolved.

The decimals function in USHToken is redundant because the number of decimals is already set in storage by inheriting from SolmateERC20Upgradeable, which already implements the decimals function that returns the stored value.

Remove the decimals function from USHToken.

Manifest Team: Resolved.

The comment on the circulationSupply function in the USHToken contract states, "Returns total supply less reserves," but this is misleading because the function always returns the total supply.

Update the comment.

Manifest Team: Resolved.

In USHPriceOracle.sol, the initialize function emits the event PriceUpdated(uint256 indexed newPrice, uint256 oldPrice, address indexed updater) event.

However, the updater field is set to address(this) instead of the actual caller (msg.sender) or the designated owner/updater address.

Emitting address(this) as the updater does not provide meaningful information, since the contract itself cannot directly act as the updater.

Update the event emission to use either msg.sender or the explicitly provided updater/owner address to ensure the event accurately reflects the responsible for initializing the price.

Manifest Team: Acknowledged.

According to the documentation of the protocol, USAToken will serve as the governance token.

Additionally, it states that USHToken holders can vote to initiate the redemption process for real-world assets.

However, both of these tokens are regular ERC20 tokens without the ERC20Votes implementation.

While it is possible to use standard ERC20 tokens for governance voting through off-chain mechanisms, the ERC20Votes implementation provides additional features such as delegation, checkpoints, and historical voting power data.

It is important to note that incorporating this feature would add complexity to the protocol, and the decision should ultimately depend on protocol-specific requirements.

Consider using ERC20Votes if on-chain voting data and delegation are important for the protocol.

Otherwise, ensure that the off-chain voting mechanism includes snapshots or other safeguards to prevent manipulations such as buy-vote-sell immediately.

Manifest Team: Acknowledged.

With EIP-7702, any KYC'd user can set their account code to allow non-KYC users to interact with the protocol, effectively bypassing the KYC check.

Normally, non–KYC'd users can hold and transfer USHToken, but they cannot interact with the permissioned Uniswap pool or perform swaps.

However, this restriction can be bypassed with EIP-7702. Consider the following scenario:

• Alice is not KYC'd
• Bob is KYC'd
• Bob sets his account code using EIP-7702 to execute a swap on the Uniswap pool (Bob’s account

calls UniversalRouter).

• Alice calls Bob’s account and executes the swap through it.
• Since msgSender in UniversalRouter is Bob’s account, the KYC checks are bypassed.

In this way, a single KYC'd user can enable all other non-KYC users to interact with the protocol.

One option is to also check the KYC status of tx.origin. However, this could introduce new restrictions and may lead to unexpected behaviors.

Another option is to monitor users off-chain and ban those who behave in this way.

Manifest Team: Acknowledged. We will track this off-chain.

The _beforeDonate hook is designed to allow manifestAccounts to donate while preventing any other accounts from doing so.

The hook first checks whether the sender is one of the allowed accounts via _checkAllowed, and then tries to get the msgSender from it.

However, none of the hardcoded addresses, including the UniversalRouter and PositionManager, support the donate functionality. Reference.

The only way to donate is by directly calling the PoolManager or using custom routers. As a result, _checkAllowed always reverts during _beforeDonate.

If donations are expected to be allowed for manifestAccounts, either a custom router must be implemented and verified in _checkAllowed, or the manifestAccounts should directly call the PoolManager.

In the latter case, the sender should not be checked via _checkAllowed but should instead be validated directly using checkManifestAccount.

Manifest Team: The issue was resolved in commit 7cff6c8.

The AuthHook enforces a fixed set of hardcoded addresses as trusted senders. However, this restricts the protocol if expansion to other chains is required or if Uniswap introduces new routers.

Consider storing the trusted senders in a mapping, and implementing owner-only setter functions for flexibility.

Manifest Team: The issue was resolved in commit 7cff6c8.

The min and max prices in the USHPriceOracle are based on 6-decimal values, and the priceUpdater or multisig updates the prices using the same 6-decimal format.

However, the comments in the USHPriceOracle and USHPriceOracleStorage contracts, as well as in the IUSHPriceOracle interface, still indicate that prices use 8 decimals, which is misleading for external integrators.

Update all comments to ensure that integrators receive the correct price information.

If the prices are intended to use 8 decimals, like Chainlink, the previous issue remains, and the min/max prices need to be updated accordingly.

Manifest Team: The issue was resolved in commit d1a4e3e.

As a fix for the previous L-09 issue, the s value is checked to ensure it is in the lower half of the curve order. However, this check is performed directly on the bytes32 value without converting it to uint256.

As a result, a lexicographic comparison is used instead of a numeric one, which could lead to incorrect behavior if the byte order is misinterpreted.

Cast the s value to uint256 before comparing it, as done in the OpenZeppelin library

if (uint256(s) > 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) { revert
}

Manifest Team: The issue was resolved in commit c70a1d0.

Transfer and Approval events in SolmateERC20Upgradeable declare the amount parameter as indexed. While syntactically valid, this deviates from the ERC-20 standard which defines amount as non-indexed.

Many off-chain indexers, wallets, and analytics tools assume amount is encoded in the data section, not in a topic; this will break integrations and monitoring relying on the standard ABI encoding.

Change Transfer and Approval events to the ERC-20 standard form (Reference).

Manifest Team: Resolved.

The version mismatch in the USHToken contract from the previous L-01 issue has been fixed; however, the name mismatch in StakedUSHTokenBase still persists.

The ERC20 name is set to "Staked USH" while ERC20Permit is initialized with the name "sUSH".

EIP-2612 requires the EIP-712 domain name to match the ERC20 name; a mismatch causes off-chain signatures to be computed over a different domain than the contract enforces, making permit signatures fail or be non-standard.

Initialize ERC20Permit with the same name string used by ERC20. For example: ERC20("Staked USH", "sUSH"); ERC20Permit("Staked USH");

Manifest Team: The issue was resolved in commit a14393e.

Deposits are disallowed until the admin performs initializeVault() because _deposit requires totalSupply() = 0.

However, standard ERC4626 views like maxDeposit/previewDeposit do not reflect this and may suggest deposits are possible, causing integrators to attempt deposits that always revert.

Override maxDeposit and/or previewDeposit to return 0 or otherwise signal unavailability when totalSupply() = 0.

Alternatively, this can be acknowledged if the protocol intends to initialize the vault atomically or immediately after deployment.

Manifest Team: Resolved. We handled this in the deployment script.

checkUser function in IAuthManager interface is declared without the view modifier, despite documentation explicitly requiring it remain view-only.

Mark IAuthManager.checkUser as external view in the interface.

Manifest Team: The issue was resolved in commit e914b89.

KYC'd users will interact with UniversalRouter to perform swaps, while manifestAccounts will interact with PositionManager to add or remove liquidity.

However, UniversalRouter allows minting positions via PositionManager but does not support liquidity adjustments or position burns. (Reference1, Reference2).

// should only call modifyLiquidities() to mint
_checkV4PositionManagerCall(inputs);
(success, output) = address(V4_POSITION_MANAGER).call{value: address(this).balance}(inputs);

During this call, UniversalRouter is the msg.sender of the PositionManager contract. Since PositionManager is one of the allowed senders in the AuthHook, _getSender(sender) returns the UniversalRouter address, and the action reverts because that address is not a manifestAccount.

However, adding this router to the manifestAccounts would bypass the entire check and allow anyone, including non-KYC’d users, to mint positions through the router.

The current behavior of disallowing position minting through UniversalRouter is more aligned with the intended restriction logic; however, manifestAccounts should be aware of this limitation.

Be aware of this behavior and ensure that manifestAccounts always interact with PositionManager directly, rather than through UniversalRouter, for any liquidity-related actions.

Additionally, ensure that UniversalRouter is never added to the list of manifestAccounts, as this would bypass the intended restrictions.

Manifest Team: Acknowledged. UniversalRouter should never be granted ManifestAccount permissions.

Only KYC'd users can swap tokens in the permissioned Uniswap pool. While the SETTLE_ALL and TAKE_ALL actions use the msgSender address, the SETTLE and TAKE actions accept arbitrary addresses.

These addresses can be provided to the UniversalRouter by the caller and then retrieved using the _mapPayer and _mapRecipient internal functions on the router side. Reference.

A KYC'd user can perform swaps on behalf of non-KYC'd users. While this is similar to a KYC'd user executing a swap themselves and then transferring the tokens to a non-KYC'd user, in this case, the funds are exchanged directly between Uniswap and the non-KYC'd user.

Be aware of this behavior. If this is not acceptable, unlike allowing a non-KYC'd user to hold and/or transfer tokens, this action may also need to be restricted.

However, the recipient address is not part of the SwapParams, so this cannot be restricted in the hook. It is a feature of the router. As a result, if this action must be restricted, it needs to be enforced at the token transfer level.

Manifest Team: Acknowledged. We will track this off-chain.

_exceedsMaxChange may revert with a panic error if the currentPrice is 0. Previously, this was not possible because MIN_PRICE was a constant.

However, with the updates, MIN_PRICE is now configurable and can be set to 0. While the owner is trusted, a compromised owner account could set this value to 0, which would break future price updates.

Enforce that newMinPrice is greater than 0 in the set_MIN_PRICE function.

Manifest Team: The issue was resolved in commit 865b7f9.

The owner can set MIN and MAX constraints to inconsistent values (e.g., MIN_PRICE > MAX_PRICE or MIN_UPDATE_INTERVAL > MAX_UPDATE_INTERVAL).

This makes all subsequent updates fail bound checks, effectively freezing oracle updates until the owner fixes the configuration.

Add validation in setters to ensure invariants hold, e.g., require(newMin < current MAX) and require(newMax > current MIN).

Manifest Team: The issue was resolved in commit 7aa8f89.

The updatePrice function checks whether the expected index is greater than the current update counter to prevent old updates. There are two checks for this:

if (expectedIndex < s.updateCounter + 1) revert IndexMustBeGreater();
if (expectedIndex = s.updateCounter + 1) revert IndexMustGrowAsSequence();

However, the first check is redundant, as the second check is more restrictive and requires the index to be exactly the next one.

if (expectedIndex < s.updateCounter + 1) revert IndexMustBeGreater() check can be removed.

Manifest Team: The issue was resolved in commit 728234b.

The NatSpec comment for the checkBanned function, in both the AuthManager contract and the IAuthManager interface, reads:

“Reverts with InvalidAddress if the account is the zero address, or UserNotPermitted if the user is banned.”

However, the function returns when the address is 0 and does not revert.

Update the comment.

Manifest Team: The issue was resolved in commit 8f62397.

The comment on line 22 of the USHPriceOracleStorage contract and line 80 of the IUSHPriceOracle interface still indicates that the change is daily:

“Maximum allowed daily price change percentage.”

However, with the updates, this no longer refers to a daily price change but to the price change between each update.

Update comments.

Manifest Team: The issue was resolved in commit 17e8705.

In AuthHook.sol, the _beforeInitialize callback enforces a different access control pattern than the other callbacks.

Specifically, it calls authManager.checkManifestAccount(sender) directly, without first validating the sender through _checkAllowed and then checkManifestAccount(_getSender(sender)).

The team’s intention is to allow pool initialization directly via the Uniswap PoolManager.initialize function when using a manifest account.

However, if the PositionManager contract itself is ever added to the manifest accounts, this design unintentionally permits any user to initialize a pool with this hook, effectively bypassing intended restrictions.

For uniformity and to prevent unauthorized pool initialization, apply the same validation logic in _beforeInitialize as used in other callbacks.

Manifest Team: The issue was resolved in commit 7cff6c8.

In AuthHook.sol, the setAuthManager function allows the owner to set a new pendingAuthManager, update it if called with a different address, or finalize the update once the timelock period has elapsed.

However, there is currently no mechanism to cancel an existing pending authorization request.

Once a pendingAuthManager is set, the only way to clear it is by replacing it with another address and waiting through the timelock.

This could limit flexibility if the owner wants to abort an update without initiating a replacement.

Consider adding a method that allows the owner to cancel the current pending authorization request, resetting both _pendingAuthManager and _setAuthManagerReqTimestamp to their default values.

This would provide a clean way to abort an in-progress update safely.

Manifest Team: The issue was resolved in commit a7c0a0a.

Owner-only setters that adjust the global bounds (MIN/MAX for price, interval, and change percentage) do not emit events.

Silent changes reduce transparency and can surprise off-chain systems. These setters should emit events as best practice.

Emit dedicated events for each adjustable parameter change, including old and new values.

Manifest Team: The issue was resolved in commit e29f256.

The Quoter address is hardcoded as an allowed sender, and authorization is checked via _getSender(sender). However, the Uniswap Quoter does not implement the msgSender() function.

As a result, the try-catch will fail and default to treating the sender (i.e., the Quoter contract) as the user.

This leads to two scenarios. It is impossible to quote swaps without granting KYC to the Quoter contract.

However, if the Quoter is KYC’d, then everyone can interact with the protocol through the Quoter.

All official Quoter addresses on supported chains should be KYC’d in order to perform quotes. Also, be aware that there is no way to prevent non-KYC’d users from performing quotes without implementing your own Quoters.

Manifest Team: Resolved. We handled this in the deployment script.