GMI Index

Proof of concept: PoC

The request that is currently being executed in RequestHandler.executeRequest() is cleared at the end of the function. This presents a critical problem as users can execute a deposit/withdraw request with a callback to an arbitrary address that they pass by using assetVault.depositWithCallback() or assetVault.redeemWithCallback().

This callback will be executed before the request gets removed, leaving room for exploitation. assetVault.cancelRequest() immediately cancels a request and returns the funds to the user.

1. Create a deposit/withdraw request with a callback to an arbitrary contract we control.
2. The keeper picks up the request and executes it.
3. We call assetVault.cancelRequest() in the afterDepositExecution()/afterWithdrawalExecution()

callback to cancel the request and return the funds to us immediately. 4. We now have the same funds/vault shares as before the request but have also received the funds from the request.

The exploit described above puts all funds in the asset vaults at risk of being stolen.

Call aggregateVault.clearRequest(key) before executing the callback.

Umami Team: The issue was resolved in commit 8227df2.

Proof of concept: PoC

When a user cancels their request with the function cancelRequest, it is verified that the user cancelling the request is indeed the sender who sent the request. Afterwards, the funds contained in the Asset Vault are sent to the user depending on the amount of the request.

However, there is no validation done to ensure that a user who deposited/redeemed into one Asset Vault is not cancelling the created request on the other Vault.

For example, for illustrative purposes, consider the drastic scenario of a user creating a 1 ether deposit into the ETH Vault. The user can then trivially call cancelRequest on the USDC Vault, and be refunded 1e18 USDC. This leads to an enormous loss of funds for the USDC Vault depositors which is extremely easy to perform.

Use the vault attribute of the OCRequest to ensure that cancellation is only performed for the Asset Vault in which the deposit/redeem was intended.

Umami Team: The issue was resolved in commit ea5e0e7.

Chainlink LLO prices are only updated during the opening and closing of rebalancing periods and the RequestHandler.executeRequest function. Yet, LLO prices are used in the logic for users to initiate withdraws/deposits on the AssetVault when previewing the deposit fee with the previewDepositFee function and previewing the withdrawal fee with the previewWithdrawalFee function.

Stale LLO prices, which users cannot update themselves, will cause users's withdraw/deposit initiations to revert as the withdraw/deposit fee estimation code checks that the LLO prices are no more stale than 1 Arbitrum block.

Additionally, protocol operators should update the latest LLO prices before calling the cycle or fulfulRequests functions during rebalance as these functions rely on up-to-date LLO prices.

Do not rely on LLO pricing for the user-initiated deposits and withdrawals. Instead in the previewDepositFee and previewWithdrawalFee functions pass false as the useLlo value.

Umami Team: The issue was resolved in commit 54ada0e.

Proof of concept: PoC

A Denial of Service (DoS) attack can occur on rebalances by forcing the Umami-calculated estimateExecutionFee to be less than GMX’s minExecutionFee.

As only one token is being deposited on a rebalance, Umami calculates the estimateExecutionFee and enters an if statement, returning the smaller value compared to if it were to deposit two tokens.

The issue arises when an attacker forcibly sends 1 wei of the opposite token to GMX. The other gas limit value is then used, which is greater than what Umami used to calculate estimateExecutionFee.

This leads to a revert in the validateExecutionFee function. With this attack, it becomes impossible to perform a rebalance, rendering the core feature of the protocol unusable.

Send excess WETH for the execution fee, as any excess would be refunded anyway. This mitigates the risk of the DoS attack and ensures the rebalance functionality remains functional.

Umami Team: The issue was resolved in commit 8f3f436.

Proof of concept: PoC

In the adjustToBalance function when the shareValue is insufficient to cover the entire underAllocation, the difference array with the entire positive underAllocations is returned. However, the shareValue is insufficient to cover these underAllocation amounts.

As a result, whenever a share amount is minted that is unable to cover the entire underAllocation, the entire underAllocation amounts will be charged to the caller while only remunerating the caller with the insufficient share amount that was specified.

This issue is most clearly demonstrated with a mint of a single wei. The single wei will be insufficient to cover the entire underAllocation, as a result, the caller is errantly required to provide the entire underAllocation amount in GM tokens to mint the specified single wei of GMI.

Similarly, this issue is present with withdrawals, where redeeming a single wei of shares will result in the withdrawer receiving the entire over-allocation amount. This is a fundamental accounting error and will significantly affect the assetVault share values and the GMI valuation over time.

Replace the return statement on line 72 with: Solarray.arrayAddProportion(toBalanceAmount, shareValue, difference, underAllocation, true);

Umami Team: The issue was resolved in commit ea5e0e7.

Proof of concept: PoC

The protocol charges users an amount for gas since the protocol employs an asynchronous model where keepers pick the transactions up and execute them. The protocol checks whether the user has sent enough ETH in msg.value and if so adds their transaction to the uncompleted transaction queue.

The issue here comes in due to how those gas fees are handled. The following checks whether the user has sent enough funds to cover the gas to be expended by the keeper: require(msg.value >= gas, "AggregateVault: !gasRequirement");

The issue with the above check is that it assumes that gas is in terms of ETH instead of in gas units as it is.

Given that the gas stipend for a request is within the 100,000 - 1,000,000 range the transaction's gas cost on the the user's side will be extremely low - less than a billionth of a cent since ETH is in 18 decimals. This will cause the protocol to lose funds in keeper gas fees on every deposit/withdrawal request that gets executed.

Consider converting the gas units into a notional value before checking whether the amount passed by the user is sufficient by multiplying it by tx.gasprice.

Umami Team: The issue was resolved in commit 8227df2.

Proof of concept: PoC

The protocol charges users fees on deposit and withdrawal. Those fees are based on the percentages set by the protocol and on the size in the asset vault's native token of the amount being deposited/withdrawn.

The issue here is due to a share size being passed to the aggregateVault.previewWithdrawalFee function even though the function that calculates the said fee - VaultFees.getWithdrawalFee() assumes it is a native token amount.

As the vault's TVL grows and more yield is gained through its strategies, each share will be worth more. However, this will not be represented when calculating the withdrawal fee as the calculations will think that the amount of shares passed in is the native token amount.

This directly impacts the protocol as the fees it will receive on withdrawal will be substantially lower than expected leading to a loss of fees for the protocol.

To mitigate the issue convert the vault shares into their native asset’s worth before passing them to aggregateVault.previewWithdrawalFee().

Umami Team: The issue was resolved in commit ea5e0e7.

GMX fees get added on top of the base withdraw/deposit fees if they are enabled through shouldUseGmxFee. In the case of a withdrawal, those fees are calculated based on the withdrawal size in GMI.

The issue arises due to the following line, which gets used to turn the withdrawal size into GMI, which then gets turned into corresponding GM token amounts: gmi.sharesToMarketTokens(size * gmi.pps(prices) / 10 ** ERC20(asset).decimals(), prices).

The formula used for the calculation does not convert a USD notional amount into GMI, but quite the opposite.

This will always lead to a much larger fee due to the skewed GM token amounts, thus losing users' funds through excessive fees.

Convert size into a USD notional value and use a formula for converting USD into GMI: size * 1e18 / gmi.pps(prices).

Umami Team: The issue was resolved in commit ea5e0e7.

Proof of concept: PoC

The global state variable vaultGmiAttribution represents the exact amount of GMI attributed to each vault. Throughout the codebase it is repeatedly set using the _commitGmiDeltaProportions function from the LibCycle library.

This function incorrectly uses the vaultGmiAttribution values as percentages, which they are not, instead of absolute values. Because of this, the prevAmounts are extremely large and any _amt being added to will be minuscule in comparison which leads to almost 0% change in the proportions. The larger the amount is, the more the allocations will deviate from the intended values.

A direct, severe issue, appears during a rebalance when internally swapping GMI for native assets for internally settable differences between the 2 vaults. After swapping native tokens (L146-L152) the new allocations need to be saved from each vault, removing GMI from one vault and adding to another (L156-L172).

Since the call to _commitGmiDeltaProportions results in no practical change in the percentage, users are directly losing funds via depreciation of vault shares, since ETH will be swapped in these cases but GMI attribution has not changed.

To illustrate the impact, consider the following scenario:

• Total GMI valuation of $6 million
• Initial vault GMI allocation: 57% (57.0000090250015061%) USDC vault and 43% (42.9999909749984939%) WETH vault an amount approx $300K GMI (5% of GMI amount) is needed be moved from one vault to another

In this case, the current, incorrect implementation shows that the vault allocation, after adding the new amount, is: USDC: 57% (57.0000095000015852%), ETH: 43% (42.9999904999984147%). The new amount impact is erased and $300K worth of ETH is not GMI attributed.

If the correct implementation would be used, the resulting allocations are USDC: 60% (60.0000095000015853%), ETH: 40% (39.9999904999984146%). The error in this case is an absolute 3% value in allocation.

Modify the _commitGmiDeltaProportions to correctly work with and save the values as absolute amounts.

Umami Team: The issue was resolved in commits 90b2627 and e3e3cef.

The storage of AggregateVault has two variables for the two different gas fees that are paid by users when they create a request: executionGasAmount and executionGasAmountCallback. The former is for normal requests and the latter is for requests with callbacks.

The issue arises due to how executionGasAmountCallback is handled when calling the callback address the user provided. executionGasAmountCallback is passed directly as a gas stipend to the callback call even though it is intended to cover the whole call.

This issue causes the protocol, and more specifically the keeper, to provide a much higher gas stipend to the callback, resulting in loss of funds on every transaction. Another potential issue is the depletion of the keeper's ETH balance through large amounts of malicious requests aimed at disrupting deposits and withdrawals of innocent users.

Consider passing executionGasAmountCallback - executionGasAmount as a gas stipend to callback calls.

Umami Team: The issue was resolved in commit 8227df2.

When the rebalance fees are computed with the _getVaultRebalanceFees function, the magnitude of the funds withdrawn during the epoch is added to the lockedBalanceSansUserDelta with the userPositionDelta variable. The resulting lockedBalanceSansUserDelta is ultimately the amount that is fee’d.

However this incorrectly fees the remaining assets in the vault, the issue becomes clear considering the following (unreasonable, yet demonstrative) example:

• 90% of the funds in the vault are withdrawn in a single epoch
• 10% of the funds remain, and the users holding that remaining amount are subject to a fee based upon the entire 100%.
• Those who withdrew are not subject to this fee.
• The remaining users are exposed to an exorbitant fee as a percentage of their holdings.

This specific example is hyperbolic and unlikely to ever arise but is used merely to demonstrate the inaccuracy of the fee logic and the smaller-scale inequality that will occur on every rebalance.

Additionally, the current fee calculations clearly misaccount these withdrawn amounts because they are treated as if they were in the system for the entire epoch. The performanceFeePercent, managementFeePercent, and timelockYieldPercent are all computed based on the percentYear of the past epoch and applied to these withdrawn amounts.

However, the withdrawn amounts by definition cannot have been present in the vault for this entire period, in the worst case they will have been withdrawn from the vault at the beginning of the epoch.

Do not fee the remaining vault amounts based on the withdrawn amounts during the epoch.

Umami Team: The issue was resolved in commit ea5e0e7.

When a rebalance is underway, the GMI shares to be minted are validated by the _validateMintableAmounts function from the GMI contract.

The function incorrectly considers that the maximum allowed USD equivalent value to be deposited into the asset-specific GMX pool is via the backing token with the lowest availability, not the highest. This results in incorrect asset allocations for cases where the equivalent amount value cannot be deposited in the pool via the saturated backing token, but could have been deposited in the other one.

In _validateMintableAmounts, the maximum ETH value in USD (mintableEth) and maximum USDC value in USD (mintableUsdc) that can be deposited into each GMX asset market per backing token are calculated. Out of these two amounts, the largest should be selected as exactly how much can be deposited into the specific GMX pool using only one operation.

The issue is that the maxMintable chooses the smaller, not the larger out of the 2 values.This results in an incorrect maximum allocation amount for that particular asset pool, lower than it can be deposited. Consider a situation where a GMX pool gets long saturated and the protocol does a rebalance towards the short token.

The _validateMintableAmounts function will incorrectly indicate that the maximum you can deposit into that saturated pool is almost nothing since it uses the lowest available amount from the saturated one for validation.

This situation would result in depositing into the fallback pool, which will revert when also saturated. Ultimately, the protocol becomes imbalanced, risking the loss of user funds.

Base the previewMint and _validateMintableAmounts functions maximum mint amount on the asset being used to mint, not always take the greater or the smaller one. This would eliminate any issue that may appear due to over or underestimating the maximum amount.

Umami Team: The issue was resolved in commit 3940624.

In the deposit function, the previewMint function is called with the shares that are to be minted to the caller for their deposited GM amounts.

The previewMint function contains the _validateMintableAmounts validation at the end of the function which accounts for the share value being deposited into the GMX V2 system and reverts if the additional deposit tokens would put the GM market over the deposit cap.

However, during a deposit, these GM tokens have already been minted and there are no additional long or short tokens that will be deposited into the GM market.

Therefore, this validation erroneously accounts for long/short tokens being deposited when they will not be, and as a result, causes unnecessary reverts when these phantom long/short token amounts exceed the deposit cap in GMX V2, ultimately causing DoS attacks on deposits.

Do not perform the _validateMintableAmounts validation when depositing already minted GM tokens into GMI.

Umami Team: The issue was resolved in commit ea5e0e7.

Deposits and withdrawals to/from the asset vaults are a 2 step operation. The user initiates the action, thus creating a pending order and a keeper asynchronously executes that operation.

Although the execution keeper operates relatively fast, between 1-3 blocks since the initial request, there will still exist situations where an operation is initiated exactly before a rebalance is opened. During a rebalance, operations cannot be executed by the keeper, as such the user action will only be executed after the rebalance closes.

An issue is that users expect their deposit/redeem to result in the exact amounts indicated by the previewDeposit and previewRedeem functions at that time, but because of the price being recalculated again at the time the operation is executed, users may experience negative slippage and obtain fewer tokens.

During an epoch, a meaningful difference may not appear due to fast keeper response, but for those transactions that ultimately do become pending during a rebalance, the price difference may be significant enough of a loss. Since these operations flow normally, this situation will occur.

Add a slippage parameter when users deposit/redeem into the vault which will be passed and used by the RequestHandler when invoked by the keepers. Since the vaults are not meant to be ERC4626 compliant, this alteration does not come with a negative impact on the protocol.

Umami Team: The issue was resolved in commit ea5e0e7.

In the rebalanceGmi function, if isOppositeDirection is true, it attempts to swap one token for the other to rebalance before the minting and burning processes. This is achieved by swapping through UniswapV3.

When the swap is executed, the _minOut is set to zero. This means that regardless of how much slippage the swap incurs, the execution will continue. This poses a security risk, as attackers can perform a sandwich attack on this swap on UniswapV3 and steal funds if they have sufficient capital.

The impact of this is that any attempt to rebalance while isOppositeDirection is true will lead to an excess loss of funds due to the lack of slippage protection.

Set a _minOut value so that swaps do not incur more slippage than expected.

Umami Team: The issue was resolved in commit 7aa7161.

Proof of concept: PoC

Before depositing funds into the protocol, the deposit functions perform a check to ensure the vault cap will not be exceeded: require(totalAssets() + assets <= previewVaultCap(), "AssetVault: over vault cap");

However, the totalAssets() function does not consider the funds that are still pending to be sent into the AggregateVault. As a result, User A can deposit funds that reach the cap but will not be included in the TVL.

User B will then make another deposit, and since the current TVL has not been updated yet, their deposit to AssetVault will also go through. Once both requests are settled, the vault cap will be bypassed.

Validate the vault cap upon request execution so that it cannot be easily exceeded by a potentially significant amount.

Umami Team: The issue was resolved in commit ea5e0e7.

Guardian Team: Because the balance of unlodged assets is now added to the vault cap validation in the AssetVault, the more assets that are waiting to be lodged the less likely it is that the deposited assets will fail the validation. Consider omitting asset.balanceOf(address(this)) from the AssetVault cap validation.

Proof of concept: PoC

epochDelta is used to track the amount of deposits/withdrawals during an epoch. It grows positive when more funds are being deposited than being withdrawn and vice versa.

The issue arises due to how the protocol increments and decrements epochDelta. The protocol increments the delta with the amount deposited after the fees get subtracted from it, thus only incrementing with the amount that entered the AggregateVault.

aggregateVault.incrementEpochDelta(underlyingToken, assetsSansFees.toInt256()) However, the same pattern is not followed in the withdrawal logic. Instead of decreasing the whole amount that leaves the vault only size - fees get subtracted.

aggregateVault.incrementEpochDelta(underlyingToken, -(assetsSansFees.toInt256())) This will result in an imbalance where depositing the same amount has a higher impact on increasing epochDelta compared to the mitigating effect of withdrawing, thereby exposing the protocol to fund loss due to reduced fees.

This happens due to the subtraction of positive epochDelta from the current TVL during withdrawal fee calculations.

Decrement epochDelta by assets instead of assetsSansFees .

Umami Team: The issue was resolved in commit 54ada0e.

When a deposit request is made on behalf of another account and fails execution, the native assets are sent to the receiver instead of the sender of the request, who initiated the request and provided the funds.

This may be unexpected behavior as the sender would expect to receive their funds back if the request was not successfully executed.

Consider sending the funds to the sender on a deposit request fail.

Umami Team: The issue was resolved in commit ea5e0e7.

In the adjustToBalance function, when there is an overallocation of GM tokens during a deposit the overallocation is offset by depositing into all other GM tokens at a ratio between the weight of the over-allocated market and the other market. This is desired during deposits as the higher-weighted market should receive a greater amount to move closer to the desired weightings.

However, during a deposit, the opposite occurs. A larger portion of GM tokens are withdrawn from the higher target weight market, ultimately moving the market balances further away from the desired weightings rather than closer.

For example, consider the following scenario: The desired weightings are 0.2, 0.3, 0.25, and 0.25 respectively. And the respective GM token balances are 25, 25, 20, and 30.

The third market is underallocated by 5 GM tokens so markets 0,1, and 3 will have an increased withdrawal amount relative to the proportion of their target weighting to the target weighting of market 2.

Market 0 will only be decreased by 4 GM tokens, while market 1 will be decreased by 6 GM tokens. However the desired weighting of market 1 is higher than that of market 0, therefore this rebalancing moves the GM distribution further away from the desired weighting.

Withdrawals will often move GMI away from the desired weighting of GM tokens resulting in the desired positions not being met. This directly undermines the protocol’s goal of maintaining a balanced index for GMI.

When the adjustToBalance function is being used in a withdrawal context the balanceWeightings should be calculated using a weights[i] / weights[j] ratio rather than the weights[j] / weights[i] ratio so that higher weighted GM markets are reduced less than lower weighted ones.

Umami Team: Acknowledged.

In the previewMint function, the gmValueToMint returned from the GmiUtils.adjustToBalance function is rounded down as the shareValue which is used to determine the GM token amounts required uses round-down division.

Therefore the amount of GM tokens that the aggregateVault will use to mint GMI is often less than the value of the GMI shares that the aggregateVault receives.

This is not an issue when the aggregateVault holds all of the GMI shares, however in the future when other third-party actors may also hold GMI, then all other holders of GMI are diluted by deposits.

Consider rounding the shareValue up in the adjustToBalance function to avoid diluting other holders of GMI for the future where the aggregateVault is not the only holder of GMI.

Umami Team: The issue was resolved in commit 54ada0e.

When depositing GMI, the computed PPS used to calculate the shares received is rounded down. However, in the future, GMI may be supported for other third-party users. In that case, it would be important to round the PPS up upon deposits.

This way, any third-party users would receive less share value than the amount of GM tokens they deposit, rather than more share value compared to the amount of GM tokens they deposit. The precision loss due to rounding the PPS down is trivial but may pose a risk in the future.

Consider rounding the PPS up when computing the amount of shares to mint upon depositing into GMI.

Umami Team: The issue was resolved in commit 54ada0e.

In the VaultFees contract the YEAR constant is defined as being exactly 365 days in seconds, rather than 365.25 days to account for leap years.

Consider changing the YEAR value to 31557600 to account for leap years.

Umami Team: The issue was resolved in commit b919f11.

In the getVaultPPS function, round-down division is used regardless of whether the function is being used to determine the shares a user receives upon a deposit or the assets a user receives on withdrawal.

This behavior rounds in the favor of the user upon deposits, allowing the user to deposit at a lower PPS due to precision loss. Though the precision loss is minor and is unlikely to have an impact it could be leveraged in a more complex attack.

To avoid any potential manipulations as a result of this precision loss, consider rounding up when the getVaultPPS function is being used to determine the amount of shares a user will receive for a deposit and rounding down when the getVaultPPS function is being used to determine the amount of assets a user will receive from a withdrawal.

Umami Team: The issue was resolved in commit 54ada0e

Guardian Team: We recommend rounding against the user in the AssetVault’s pps function during a redeem operation by passing isDeposit = false.

Neither the cycle function nor the fulfilRequests function validate that the system is indeed within a rebalancing period when they are being called.

These functions can only be triggered by a trusted party, however, they should be restricted to only rebalance periods as important validation and caching must occur before these functions are invoked.

Validate that the protocol is currently in a rebalancing period in the cycle and fulfilRequests functions.

Umami Team: The issue was resolved in commit b919f11.

The setPeripheral function does not allow a value to be assigned for the Peripheral.CallbackHandler. Nor is the Peripheral.CallbackHandler is used anywhere in the codebase.

Consider removing the redundant Peripheral.CallbackHandler value from the Peripheral enum, otherwise implement the desired use case for the Peripheral.CallbackHandler value.

Umami Team: The issue was resolved in commit b919f11.

When distributing the overallocated difference, the algorithm divides the jth weight by the ith weight. If the ith weight is 0, a division by 0 revert will occur.

Because adjustToBalance is called in previewMint which is used in function _increaseGMI, a rebalance could fail due to a reverted cycle operation.

Ensure none of the weights are set to 0.

Umami Team: Acknowledged.

The assetVault shares are valued at the cached vaultState.rebalancePPS during the rebalance period, but as soon as the rebalance period is closed, the valuation of the assetVault shares will experience a stepwise jump to the current valuation.

Because of this stepwise jump, the assetVault shares should not be a candidate for collateral in any borrow/lending system, as there is a risk that a malicious user could deposit vault shares as collateral while they are valued at the cached price and ultimately have an insolvent position when the assetVault share price drops.

Borrow/lending platforms introduce a safety threshold between liquidation and solvency to address this, however there is a small risk that the magnitude of the stepwise jump exceeds this safety threshold. Any occurrence of this should be rare as rebalances are not intended to be large.

This is simply a warning to anyone who would integrate with the Umami GMI system and potentially accept the assetVault shares as collateral.

Umami Team: Acknowledged.

When the callback functionality is enabled by the RequestHandler contract, by calling the setCallbackEnabled function, there is no event is emitted.

Emit an event on the call to the setCallbackEnabled function.

Umami Team: The issue was resolved in commit 54ada0e

The following functions are not used in the protocol:

• _vaultGmiProportion
• _exactOutputSwap

Consider removing these functions.

Umami Team: The issue was resolved in commit b919f11.

The gas amount forwarded to the request.callback contract is always the current executionGasAmountCallback in storage: gas: storageViewer.getExecutionGasAmountCallback()

This presents an issue as users are made to pay it on submitting the request: uint256 gas = _gasRequirement(callback != address(0));

In the case of a change of the executionGasAmountCallback amount, users will either get more gas than they paid for or will get less.

Consider keeping the gas amount the user paid for as a value in the request struct and using it instead.

Umami Team: Acknowledged.

In the closeRebalancePeriod function, the performance fees for the previous epoch are collected after setting the vaultState.rebalanceOpen to false. As a result, the cached vaultState.rebalancePPS is not returned when using the getVaultPPS function to determine whether the performance fees should be charged.

This behavior errantly treats the current vaultPPS after the rebalance as if it applied for the entire period before the rebalance occurred.

This results in cases where the performance fee is missed due to the rebalance dipping the vaultPPS below the watermark.

This may occur as a result of swapping fees and/or fees from GMX. Additionally, the performance fee may be charged when instead it should not if the vaultPPS is increased above the watermark after the rebalance, though this case is rarer.

Close the rebalance period after fees are calculated so that the cached vaultPPS is used rather than the PPS resulting from the rebalance.

Umami Team: Acknowledged.

In the rebalanceGmi function the vaultIdx is unnecessarily fetched using the getTokenToAssetVauldIndex function when the vault index is directly available as the index of the for-loop, i.

Use the for-loop index to indicate the vault rather than fetching the vaultIdx with the getTokenToAssetVaultIndex function.

Umami Team: The issue was resolved in commit b919f11.

In the redeem and redeemWithCallback functions, the owner is assigned as the request.sender when the request is created. The request.sender is used to validate the account that may cancel the request in the Umami system.

However, in some cases, the owner may not be the creator of the request. The owner may approve a third-party actor to create requests on their behalf. In this case, it would be appropriate to allow this third-party creator to cancel the request as well, however, they are not able to do so.

Consider changing the request.sender to the msg.sender in the redeem and redeemWithCallback functions. Otherwise, consider allowing both the owner and the request creator to cancel the request.

Umami Team: The issue was resolved in commit b919f11.

Guardian Team: A user with an allowance can cancel the request with function cancelRequest, or have the request execution fail, and end up with the original owner's shares. Clearly document this behavior.

In the _increaseGmi function the assetToMintFrom is assigned to the _asset parameter in every iteration of the for-loop, however, it is unnecessary to declare this assetToMintFrom variable as it will always be the _asset value.

Remove the assetToMintFrom variable declaration and use the _asset parameter value directly.

Umami Team: The issue was resolved in commit b919f11.

Rebalancing the vaults results in a fee being paid to the designated feeRecipient at that time. This recipient, as it is, can be changed at any time by a configurator by calling the setFeeRecipient function from the AggregateVault contract.

Changing the fee at a random point represents a loss for the original fee recipient, as they have been the recipient since the last rebalance up until that point. Consider the situation during a normal operation period and right before opening a rebalance period.

When setting the new fee recipient, set a pending recipient, which is changed only when the rebalancing period is closed and the fee is paid. At that point, the fee is paid to the old recipient and the pending recipient becomes the actual fee recipient.

Umami Team: Acknowledged.

When configuring the vaultFees with the setVaultFees function, there is no requirement that the depositFeeEscrow and withdrawalFeeEscrow are configured to nonzero addresses if the deposit and withdrawal fees are configured to nonzero amounts.

Therefore, these fee amounts can get stuck in the vault contracts rather than being returned to the user or sent to any fee receiver.

Consider adding validation such that a nonzero fee amount cannot be configured without assigning the appropriate fee receiver to a nonzero address.

Umami Team: Acknowledged.

Proof of concept: PoC

GMX swap fees get calculated and subtracted from a user whenever they deposit/withdraw shares from an asset vault. GMX fees in VaultFees.getWithdrawalFee() are calculated with the size's backing in GM tokens based on their respective weights.

The PPS and TVL of a vault are calculated with the liquid reserves of the native token in the aggregate vault, the GMI attributed to that vault, and also the opened external hedging position.

The issue here arises due to VaultFees:137 assuming that the whole withdrawal size is backed in GMI tokens and calculating it accordingly. This makes users get charged a GMX fee on 100% of their withdrawal size instead of only the fraction that needs to be liquidated through GMX, thus making the users lose funds due to the excessive fees.

Consider calculating the GM market token amounts based on a fraction of size that corresponds to the current fraction of GMI reserves / TVL.

Umami Team: This is intended. We charge the fee for the entire withdrawal as if it were coming from GMI.

When calling the functions _pause or _unpause from the PausableVault contract, for each function there will be between 1 and 3 identical events being emitted. This can cause confusion for any 3rd party integrator.

• for the _pause function: minimum 1 Paused event and maximum 2 more from the _pauseDeposit and _pauseWithdrawal function calls
• for the _unpause function: minimum 1 Unpaused event and maximum 2 more from the _unpauseDeposit and _unpauseWithdrawal function calls

For both the _pause and _unpause functions, remove the default emitted event, and for each pausing/unpausing event, either add an argument to identify if it was a deposit or withdrawal that was paused/unpaused, or create 2 separate events e.g. PausedDeposits/PausedWithdrawals.

Umami Team: The issue was resolved in commit 54ada0e.

The nettedThreshold variable can be set by the configurator by calling the function setThresholds from the AggregateVault vault to any value.

Setting it to over the maximum BPS will result in blocking all rebalances that opt to validate netting due to an underflow operation in the NettingMath library at line 95.

Add a limit check for the _newNettedThreshold input in the setThresholds function so that it does not surpass the maximum BPS.

Umami Team: The issue was resolved in commit 54ada0e.

Users can perform a deposit/withdrawal request with a callback to an arbitrary contract upon the completion of the request processing.

The issue arises because users are required to incur costs for the maximum gas amount they can utilize in the callback, even if their callback necessitates only a fraction of that for execution.

However, the unutilized gas in this process is not refunded, ultimately leading users to lose the value of any leftover gas in the callback.

Consider refunding the value of the gas units left at the gas price during the creation of the request.

Umami Team: This is by design as the the keeper also has rebalance cost.

The rebalancing fee is calculated based on the total TVL that was managed at the start of the current epoch. This fee is deducted when closing an epoch and, by calling the closeRebalancePeriod function from the AggregateVault contract.

The fee is incorrectly calculated because, in the closeRebalancePeriod function call, the epoch delta is cleared by a _resetEpochDeltas function call on line 221, before the actual fee collection is done on line 237. This results in the overall fee being calculated on the ending epoch TVL instead of the opening TVL.

A higher fee will be deducted if the epoch has more deposits and a lower fee will be deducted if the epoch has more withdrawals.

The end user is unaware of the fee value until the end of the epoch which, depending on the value, might have determined whether or not the user participated in the protocol during that epoch. Fee predictability is needed for users of the protocol.

Reset the epoch delta after the fees have been collected.

Umami Team: The issue was resolved in commit 8227df2.

When closing the rebalance period, a validation is done so that the current vault holding ratio is within the accepted upper and lower thresholds.

This validation is incorrect because it uses the index allocations that were saved when the rebalancing period was opened, which are stale, instead of using the current index allocations determined at the moment the rebalance is closing, and that are saved in the vaultIndexAllocation variable.

Because of this, the vault ratio is always slightly incorrect since it calculates the vault index exposure as a factor of the current vault holdings (vaultCumulativeHoldings) but compares it to the vault holdings at the time when rebalancing was opened (vaultHoldings) in the function vaultDeltaAdjustment from the NettingMath library.

This slight difference may lead to the vault ratio exceeding the threshold and resulting in an execution revert. Or passing a ratio when it would exceed the threshold normally.

When closing the rebalance period, always use the current index allocation, not the stale one saved when the rebalance period was opened.

Umami Team: We only care about the total GMI in the vaults when validating the netting check.

When a user creates a request to either withdraw or deposit into Umami, they need to supply enough ETH to cover the execution fee.

This is so that the Umami keepers have funds to execute the transaction. Users who made the deposit or withdrawal can also cancel their request if it has not been executed yet.

The issue is that the ETH that the user supplied is not returned to the user. This is inconsistent behavior as the user loses any ETH that they sent for a request that never got executed.

Consider returning the ETH to the user since the keeper never used it.

Umami Team: We chose to keep the gas here.

When the vault fees are set by calling the function setVaultFees from the AggregateVault contract, there are no checks that do not surpass the equivalent of 100%.

A mistake by the configurator or a compromised configurator can set the fees to such a value that they equate the entire vault assets. When a rebalancing happens, the vault assets will be sent to the fee recipient, which can also be set by the configurator via setFeeRecipient.

Add limitations so that fees cannot surpass 100% but should also include a lower, maximum allowed threshold.

Umami Team: Acknowledged.

For the closeRebalancePeriod function to successfully execute, it must pass the check in the checkNettingConstraint function.

If the vault receives too little or too many GM tokens on a mint, it will cause the vaultIndexAllocation to change and push the vaultRatio outside of its bounds.

An attacker can take advantage of this by force-sending some ETH or USDC to the GMX depositVault while Umami is minting. This will cause Umami to receive more GM tokens than expected, which will increase the vaultIndexAllocation and consequently the vaultRatio.

If the vault ratio is already near the upper bound, it would only take a small amount to push the vaultRatio beyond the upper bound.

If the closeRebalancePeriod function cannot successfully execute, the protocol will remain in a rebalance state for longer than intended.

This manipulation would be capital-intensive for an attacker and can be avoided by disabling the netting validation. However, it would be prudent to be aware of this risk when setting targets that would put the vaultRatio near the upper limit.

Umami Team: Acknowledged.

Rebalancing the vaults results in a fee being paid in assets taken from the vaults themselves. Settings of the rebalance fee percentages can be directly changed by calling the setVaultFees function from the AggregateVault contract.

This is an issue, as changing the fee settings during an epoch alters the perceived risk that users associate when interacting with the vaults, high taxes would also make the vaults less appealing and in return would result in a lower utilization.

The epochDelta component as well as fee watermark PPS can be changed at any time by calling the setAssetVaults function from the AggregateVault contract. Again this is an issue as it changes the fee during an epoch.

When calling setVaultFees, have the new fee percentages be set in a pending state. When a rebalancing is executed, after fees are deduced using the old fee values, then change them to the pending ones.

Consider creating a separate function to update the fee watermark PPS value and date in the same manner as described above.

The function setAssetVaults should only be called on severe vault changes, consider limiting its use as much as possible.

Umami Team: Acknowledged.

The protocol has two different types of hooks:

• Hooks calling arbitrary addresses passed by users after a request of theirs gets executed/canceled.
• Protocol hooks that get evoked when a request gets queued or a rebalance gets opened/closed.

CLOSE_REBALANCE_HOOK is called before deposits are unpaused, which may restrict the callback from certain operations and limit its potential behavior.

Consider calling the protocol hook after the rebalance period gets closed similar to how it gets called in openRebalancePeriod, just before the rebalance gets opened.

Umami Team: Acknowledged.

Before depositing on GMX, the amount of asset vault native asset to expend to acquire the required amount of GM is calculated: uint256 assetAmountRequired = _previewGmMint(markets[i], gmSharesRequired[i], assetToMintFrom);

The assetAmountRequired can possibly be zero (post-internal netting), resulting in a GMX revert upon deposit creation with EmptyDepositAmounts. Because the mints are done in a for-loop, a failure in one market will cause all others to fail. Consequently, the cycle will fail and the necessary GMI shares will not be minted.

Carefully select the weights and target allocations such that function _previewGmMint does not return a zero amount.

Umami Team: The issue was resolved in commit c07078c and 4467ccd.

When calculating the performance and management fee, division occurs before multiplication, leading to some precision loss, which makes the fee less than it should be.

Perform all division after all multiplication.

Umami Team: Acknowledged.

Both AggregateVaultHelper::_getVaultGmi and LibAggregateVaultUtils::getVaultGmi implement the same functionality, with the only difference being how they fetch from storage.

Reuse the same functionality to avoid duplicative code.

Umami Team: The issue was resolved in commit ea5e0e7.

It is possible for the sum of the two asset vaults’ GMI allocations to be less than the total supply of GMI. As a result, a small portion of GMI is unaccounted for in the Asset Vaults which may lead to issues such as needing to expend more asset funds to reach the target allocation.

Clearly document this precision loss.

Umami Team: Acknowledged.

It is possible for the TVL Of a Vault != PPS * totalSupply because of precision loss when calculating the price per share. This may lead to slight differences in the amounts withdrawn and the shares received for a deposit, although this is preferable compared to having the PPS * totalSupply potentially exceeding the TVL.

Clearly document this precision loss.

Umami Team: The issue was resolved in commit b919f11.

During a deposit it is checked that the shares to be minted is not 0: require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");

However, it is still possible for a user to mint 0 shares with a non-zero deposit due to the precision loss that occurs in uint256 shares = assetsSansFees * (10 ** decimals) / pps; Consequently, a user may deposit a small amount of funds but receive 0 shares.

Consider enforcing a minimum deposit amount and/or validating the request upon execution.

Umami Team: The issue was resolved in commit 54ada0e.

In both the mintGmTokens and burnGmTokens functions, a minimum output amount is set to 0. This means that regardless of how many tokens are returned on a mint or burn, the execution will be completed successfully.

However, this poses a problem as any amount lost will negatively affect the vault's TVL, which would, in turn, impact the value of the users' shares.

Although some value loss is inevitable when making deposits or withdrawals on GMX v2, precautions should be taken to limit how much value can be lost.

This is especially important considering that the price impact can take out an unexpected amount of funds, and without a defined minimum output amount, price impact can remove a significant amount of value from the mint or burn, leading to a loss of funds for the users.

Use a non-zero minimum output amount for both minting and burning to limit the users' loss.

Umami Team: Acknowledged.

In the rebalanceGmi function, if isOppositeDirection is true, it attempts to swap one token for the other to rebalance before the minting and burning processes. This is achieved by swapping whichever token is at a surplus for the other.

Any swap will experience slippage resulting in the output amount being different than the input amount. The issue is that when updating the _current[] as well as calling the function _commitGmiDeltaProportions for each of USDC and ETH, only internalNet is used.

This will lead to a skew in the accounting because the actual delta of the output token will be different than that of the input token.

This skew will lead to the wrong amount being minted/burned as the cycle function continues, as well as an undervaluing of whichever asset was the input token at the expense of the other token.

Change the state of the outputted token based on the returned output amount of the swap, instead of the inputted amount. This will align the actual token balance proportions with what is stored in the state, preventing misvaluing of assets and inaccurate minting/burning.

Umami Team: We do this so the vault receiving the swap is responsible for all fees incurred from the swap (slippage and trading fee).

When calculating the withdrawal fee, the management fee component is incorrectly deducted only if the current vault price per share (PPS) is higher than the watermark PPS.

This condition is required for the performance fee, not for the management fee. This behavior is inconsistent with the way the management fee is deducted when rebalancing and leads to fewer fees for the protocol overall.

Calculate the management fee where there is a profit, regardless of the current vault price per share.

Umami Team: We take the performance and management fee on regular intervals at rebalance time. On withdrawal we only take them if there has been a profit since we last took them.

When a rebalance occurs, and the protocol intends to mint GM tokens, it will deposit either WETH or USDC into GMX via the mintGmTokens function.

After the deposit is created, the GMX Keeper will execute the order, and on completion, send the GM tokens back to the Umami protocol. If the order is canceled or fails on execution for any reason, the GMX keeper will return the long or short token to Umami.

Because the function _fulfilMintRequest will loop through all of the _mintRequest, if one did not succeed then the whole transaction will revert due to the following require statement if (!depositRequestDetails.success) revert RequestNotSucceded();.

Anytime the depositExecutionon GMX v2 fails or anytime an order is canceled, those funds will require manual intervention. There are a variety of reasons both maliciously and unintended that can cause an order to fail upon execution.

For example, the max deposit cap can be exceeded during execution and not creation, as well as congestion on the network leading to the execution of the order taking place beyond the max block limit.

Carefully monitor the status of a rebalance and fix state inconsistencies with handlers when necessary.

Umami Team: The keeper runs a simulation before sending all the transactions on chain but the possibility of them still failing after simulations is always there because of the state changes on chain between simulation block and rebalance block. If any of the requests fail it’d require manual intervention.

In the afterDepositExecution and afterWithdrawalExecution functions in the catch block, arbitrary bytes are loaded into memory from the arbitrary request callback.

Although the callback is limited in gas expenditure by the executionGasAmountCallback, the arbitrary callback contract can cause the keeper to expend much more gas than expected by reverting with a large amount of revert bytes which are then subsequently loaded into memory.

Memory expansion costs a quadratic amount of gas and malicious revert bytes can lead to the keeper expending hundreds of thousands or even millions of additional unexpected gas units. Such an expenditure can cost the keepers a significant amount over a period of time.

Do not accept revert bytes from the callback contract.

Umami Team: The eth_call is going to fail and keeper will continue on with the next one without actually sending any transaction and causing any loss of funds. The gas field for the transaction will be set to the base gas units executionGasAmount + callback gas units executionGasAmountCallback which will make it go OOG.

There are several inconsistencies with contracts that have the functionality needed to be called on its own or delegate-called into. Functions that work only when delegate called-into require the onlyDelegateCall modifier will not return an invalid result when called into.

• For the GmxV2Handler contract:
• getDepositRequestDetails and getWithdrawRequestDetails need the onlyDelegateCall modifier
• For the VaultFees contract:
• getDepositFee, getVaultRebalanceFees, and getWithdrawalFee need the onlyDelegateCall modifier
• consider making the following public functions internal, since they are called only from within the contract and also work only when delegate-called into: _getVaultDepositFee, _getVaultWithdrawalFee, getVaultTVL, and getVaultPPS
• For the AggregateVaultHelper contract:
• getTotalNotional needs the onlyDelegateCall modifier
• all functions from the AggregateVaultViews contract require the onlyDelegateCall modifier but with the sole exception of vaultToAssetVaultIndex, all other functions the not used by the AggregateVaultHelper contract.

Add an onlyDelegateCall modifier to the mentioned functions and implement the other suggested changes. Consider making vaultToAssetVaultIndex an internal function in the AggregateVaultHelper contract and removing the AggregateVaultViews contract completely.

If AggregateVaultViews is to be kept for on-chain reading of values through multicall, the onlyDelegateCall modifier must be added to all of its functions.

Umami Team: Acknowledged.