Impermax V3

Currently the borrow function is at risk of cross-contract reentrancy. Consider the following scenario:

(1) ImpermaxV3Borrowable.borrow with borrowAmount > 0 with an NFTLP collateral that is enough to cover the borrow.

(2) Callback impermaxV3Borrow is called before tokenId's borrow balance is updated. At this point in time, the account's borrowed is 0. (3) Call ImpermaxV3Collateral.redeem with percentage=1e18 to get back the NFTLP. This is still in the context of the callback.

(4) require(IBorrowable(borrowable0).borrowBalance(tokenId) = 0 and require(IBorrowable(borrowable1).borrowBalance(tokenId) = 0 should pass successfully as the account wasn't updated yet.

(5) Back in ImpermaxV3Borrowable.borrow, canBorrow would be called, but the NFTLP position still has enough liquidity to cover the borrow and the getPositionData function would still return the proper values, even if Alice now holds the NFT instead of the ImpermaxV3Collateral contract.

(6) Ultimately, Alice borrowed a non-zero amount but also holds the entirety of her collateral NFTLP.

Ensure the ImpermaxV3Collateral contract holds the NFTLP collateral when there is an active borrow.

Impermax Team: The issue was resolved in commit d71e5b6.

Proof of concept: PoC

The auto-compounding feature should help borrowers be more capital efficient by automatically reinvesting fees earned on uniswap. As the caller of the reinvest function receives a bounty for doing the work, the position's collateral value decreases through auto-compounding.

The reinvest function does not implement safety checks for the position's health. Therefore, it is possible that calling the reinvest function leads to positions being liquidatable, which is definitely not in the borrower's interest and should therefore be prevented.

It is also possible to auto-compound liquidatable positions and therefore potentially make them go underwater.

Revert at the end of the reinvest flow if the position is liquidatable. It would also make sense to let borrowers decide up to which point they want to auto-compound so that reinvestors can not push them to the edge of liquidation.

Impermax Team: The issue was resolved in commit af10809.

The function oraclePriceSqrtX96 aims to calculate the oracle price by gathering the TWAP across all Uniswap pools with a specific (token0, token1) pair and calculating the mean.

The pre-condition for a successful call to oraclePriceSqrtX96 is that for each Uniswap pool in the poolsList, the consult function does not revert.

However, if a pool was just created, there would not be an observation that was ORACLE_T seconds ago and Uniswap's observeSingle would revert: "dev Reverts if an observation at or before the desired observation timestamp does not exist"

Ultimately, a user can DoS the calculation of oracle pricing for an entire period just by deploying a different fee tier, which would prevent positionData retrieval and DoS other areas of the codebase such as function isUnderwater and consequently restructureBadDebt.

Consider wrapping the call to Uniswap's observe in a try-catch and then adjusting the number of observations by how many calls were successful.

Impermax Team: The issue was resolved in commit 6f6ab04.

When a position’s debt is restructured, the debt is forgiven, the losses socialized, and the position remains in the system. In certain cases, the position can become liquidatable after restructuring and be liquidated, and other times the position can become fully healthy and remain.

This incongruent behavior may be unexpected for the protocol, since some positions will effectively get a bail-out and remain. This can be used by malicious borrower’s to keep lender’s funds locked up indefinitely.

Consider if previously underwater positions should remain in the system after restructuring and allow them to be liquidated.

Impermax Team: The issue was resolved in commit 92796e2.

Guardian Team: The owner of a position can frontrun the liquidator to call restructureBadDebt first so that the position is healthy again and then stuff the block so that the block gas limit is hit before the liquidator's transaction goes through. Afterwards the blockOfLastRestructureOrLiquidation check will fail.

The ImpermaxFactory contract allows for the creation of lending pools with a completely arbitrary NFTLP address. Consequently, a malicious user may create a lending pool with a malicious tokenized position which could lead to loss of user data and assets.

Validate that the NFTLP's passed into the ImpermaxFactory functions have been deployed through the respective tokenized position factory contracts.

Impermax Team: Acknowledged. Validation is done through the whitelisting process at a higher level (generally in the UI).

In the accrueInterest function, the interest calculation interestFactor.mul(_totalBorrows).div(1e18) can round down to zero for low-decimal tokens (such as USDC), particularly when the borrow rate or total borrows are low.

Specifically, when the product of interestFactor and _totalBorrows is less than 1e18, the division results in zero. This leads to a loss of interest for lenders, as the interestAccumulated is zero.

Furthermore, this can cause a discrepancy between totalBorrows and accountBorrows, since accountBorrows does not experience the same precision loss.

Use a higher precision for the rate values to accommodate tokens with fewer decimals, ensuring that the computed interest does not round to zero.

Impermax Team: Acknowledged.

The currentBorrowBalance and trackBorrow functions accrue interest and therefore update the totalBorrows which would influence the utilizationRate and therefore the borrowRate. However, they do not have the _update modifier and therefore do not update the borrowRate.

Therefore the system potentially deals with a stale borrowRate and the lenders receive less yield than they should. Furthermore, the system allows for interest gaming, since calling sync() every second will provide a different debt accumulated than calling sync() once over the same timespans.

For example, with a decreasing _kinkBorrowRate every sync the borrow rates will also decrease and cause less to be repaid over the borrow’s lifetime, which in turn is less yield for lenders.

Add the _update modifier to the currentBorrowBalance and trackBorrow functions. Furthermore, re-consider if the system show allow for varying sync times to lead to different costs on users.

Impermax Team: Acknowledged. We call calculateBorrowRate when there is a supply/demand change in the contract.

Interest should always be accrued before updating parameters that will impact the outcome of the next update to fairly calculate the accumulated interest.

The _setAdjustSpeed function in the BSetter contract for example can update the adjustSpeed variable that will influence the borrowRate, but it does not accrue interest before updating this variable.

Therefore the next time the borrowRate is updated it applies the updated adjustSpeed value on the whole timeElapsed since the last update while in reality a part of the timeElapsed should be calculated with the old adjustSpeed value to fairly update the borrowRate.

Accrue interest before updating parameters that will impact the outcome of interest related calculations.

Impermax Team: Acknowledged.

When the bounty is sent to the given bountyTo address with an ERC-777 token the receiver can re-enter the system. At this point, the fee growth values of the position were already reset, but the position had not received the liquidity yet.

Therefore the collateral value of the position is smaller than it is in reality. This could lead to the system seeing the position as liquidatable or underwater when the caller reenters the system.

Therefore a malicious actor can potentially abuse this state to either:

• Liquidate the position to steal from its owner
• Call restructureBadDebt to reduce the debt of the position and steal from the lenders (could be

abused by the owner of the position)

Transfer the bounty to the given bountyTo address after all state changes occurred.

Impermax Team: The issue was resolved in commit 33126d8.

When restructureBadDebt is called, the exchangeRate will immediately decrease due to the reduction in _totalBalance without a proportional reduction in pool token supply. A user can frontrun the restructuring to redeem before the exchange rate drop to avoid the penalty.

The LP exploiting this receives yield without risk while increasing the risk of all the other LP as the bad debt is socialized among all other LPs. Furthermore, the LP taking advantage of the stepwise jump in the exchange rate can then mint the same amount of PoolTokens for a fraction of the price.

Consider a 2 step deposit/redeem process.

Impermax Team: Acknowledged. I think the only real solution to this would be to introduce an unbonding period for redeem. But this would ruin the UX for a marginal benefit.

The ImpermaxV3Collateral contract utilizes a 30-minute TWAP to determine the value of collateral and debt during the liquidation process. This approach helps provide a stable price by averaging out short-term market fluctuations.

However, when the liquidated tokens are claimed, Uniswap uses the spot price. This discrepancy between the TWAP and the spot price can lead to differences in the expected versus actual value received by liquidators.

For instance, if the spot price is lower than the TWAP, a liquidator may receive fewer tokens than anticipated, resulting in a possible loss.

Clearly document potential discrepancies between TWAP and spot pricing so users understand the associated risks

Impermax Team: Acknowledged. Since liquidations will be handled by external contracts, all the PNL logic and liquidations strategy will be handled by the liquidator.

In the join function 1 wei is added to the resulting newFeeGrowthInside0LastX128 of the joined position to avoid fee over estimations.

However in cases where the newFeeGrowthInside0LastX128 result does not have precision loss this is an unnecessary addition. Instead rounding up can occur only when precision loss would occur.

Consider implementing tA0.add(tB0).add(newLiquidity - 1).div(newLiquidity) Instead of the existing tA0.add(tB0).div(newLiquidity).add(1) to accurately only round up when precision loss would occur.

Impermax Team: The issue was resolved in commit 5d38704.

The safetyMarginSqrt plays a crucial role when determining the liquidatable state of a position, as it's used to calculate the LOWEST and HIGHEST price as a factor of the oracle TWAP price.

Therefore, healthy position can become instantly liquidatable if this factor is increased by the admin. The same issue applies when updating the liquidationIncentive and liquidationFee.

Introduce a time lock feature so that these admin state changes include a delay, to allow users to take action before they are applied.

Impermax Team: Acknowledged.

Functions throughout the codebase allow for an input of zero, e.g. functions ImpermaxV3Borrowable.liquidate for repayAmount and ImpermaxV3Collateral.redeem for percentage.

This would ultimately leads to events being emitted when no actual actions have been taken on a Position.

Consider validating against zero inputs.

Impermax Team: Acknowledged.

There are multiple public/external functions with names starting with an underscore. Some examples:

• TokenizedUniswapV3Factory._setPendingAdmin
• CSetter._initialize
• BSetter._setReserveFactor

However, this naming style goes against the solidity convention for external/public vs internal/private functions, more details here:

https://docs.soliditylang.org/en/latest/style-guide.html#underscore-prefix-for-non-external-function

s-and-variables

Additionally, this can create confusion for the reader, or allow bugs to be introduced, declaring a function external when it's suppose to be private/internal.

Consider adapting to the function naming convention, according to the solidity docs shared above.

Impermax Team: Acknowledged.

Within the IUniswapV3AC01 the interface there are numerous functions where the return type does not match the expected return type.

For example, function PROTOCOL_SHARE() external view returns (address); expected an address to be returned in the interface definition but the PROTOCOL_SHARE is a uint.

Correct the interface definitions so the return types match.

Impermax Team: The issue was resolved in commit 8a88634.

• The borrow function allows to borrow up to the point of being liquidatable therefore a tiny price

change afterward will make the position liquidatable

• The system uses TWAP prices at the moment to calculate the value of collateral and debt

This could be problematic in black swan events when prices drop or rise very quickly. It could lead to a situation where the TWAP price is off by more than the safety margin + liquidation fees compared to the current price.

In this case, anyone would be able to borrow funds that are worth more than the deposited collateral, use these funds to buy more collateral, and repeat the process to drain the protocol.

There are multiple things that can decrease the likelihood of this edge case:

• Add a percentage buffer on top of the liquidation threshold so that users are not able to borrow up

to the point of being liquidatable

• Use a different price oracle that would be more accurate in such an edge case

Impermax Team: Acknowledged.

During liquidations, the NFTLP token will we split twice, one for the liquidator, one for the reservesManager. Even though each tokenId can be in different ranges, the reservesManager will end up with hundreds of NFTLPs that will later need to be redeemed to collect the underlying.

Consider redeeming the NFTLP to receive the underlying tokens.

Impermax Team: Acknowledged.

UniswapV3CollateralMath.newPosition validates that paSqrtX96 > Q32 and pbSqrtX96 < Q160 However, Q32 is below Uniswap's MIN_SQRT_RATIO and Q160 is above Uniswap's MAX_SQRT_RATIO, which leads to potentially invalid position objects being created.

Align the validation with Uniswap's min and max prices.

Impermax Team: The issue was resolved in commit 672084e.

The current implementation of ImpermaxV3Borrowable.liquidate allows users to partially repay the current borrowed balance. However, depending on the previous health state, a partial repay can make the position healthy post liquidation.

This means that both the protocol ad the user could have received more fees during a full liquidation.

If the user has both tokens borrowed from the pool, and based on the liquidity structure of the position, there are cases where repaying one of the tokens first will allow the liquidator to trigger a second liquidation with the other token.

Protocol should be aware of the fees they are missing out with partial liquidations. Additionally, document this behavior, so users are aware of the impact of partial liquidations.

Impermax Team: Acknowledged. Partial liquidations make us miss out on fees, but they make the protocol safer.

The createCollateral, createBorrowable0, and createBorrowable1 functions call the _getTokens function in the beginning without using the function's return values. Therefore, the call is redundant and can be removed.

Remove the redundant function calls. If there should be a check to ensure the two token addresses are set add a require statement instead.

Impermax Team: The issue was resolved in commit 74c85cf.

Due to the lagging nature of TWAP price, borrowers will have a time window to repay their loans, once they notice the price went against them, but the TWAP price has not catch up yet.

Therefore, liquidators will have a hard time finding liquidatable positions, as users will know ahead of time due to the pool price action.

Document this behavior to the users and liquidators.

Impermax Team: Acknowledged.

There is no zero check in the mint function of the TokenizedUniswapV3Position contract therefore zero liquidity positions are permitted.

It is also possible to split 0% of a position to create a new position with 0 liquidity. Such unexpected states should not occur and might lead to vulnerabilities with future updates.

Add zero checks to prevent the creation of zero liquidity positions.

Impermax Team: Acknowledged. Through the router it’s easier to create a leveraged position by first minting a position with 0 liquidity.

If multiple people call reinvest at the same time, the transactions of the users who paid less gas might be successful and the users have to pay the full gas costs, but the transaction does not have any necessary effect and the users will not receive a bounty:

• Bob calls reinvest
• Alice calls reinvest
• Bob paid more gas and the transaction went through first
• The position's fees are reinvested
• Bob receives a bounty for his work
• Alice's transaction went through and in between some fees were accrued on Uniswap
• A dust amount or even no fees are reinvested
• Alice does not receive a bounty or the bounty that does not compensate for her gas costs

This does also allow a griefing vector, a malicious actor could front-run the reinvest call on his position and split the NFTLP into two positions so that the reinvest caller wastes gas to reinvest a dust amount of fees.

Add a lastReinvest slippage check at the beginning of the reinvest function, or document that this function should be called over a contract that implements such a check.

Impermax Team: Acknowledged.

The protocol relies on a two-step process for most of its depositing functionality. Users send funds to the contract or a Uniswap position first, and then call a function that mints the corresponding tokens or positions.

This works well for those who use the periphery contracts, since both actions occur atomically. However, users who interact directly with the protocol’s core contracts may risk losing their funds if, for instance, the second transaction is frontrun.

Make it clear in the documentation that this risk exists and advise users to use the protocol’s periphery contracts.

Impermax Team: Acknowledged. No user should ever interact with the core directly.

TokenizedUniswapV2Position prevents a split percentage of 100% but TokenizedUniswapV3Position allows it.

Consider making the validation symmetrical.

Impermax Team: The issue was resolved in commit 2b519ec.

The current implementation of TokenizedUniswapV3Position contains a Position struct that stores position data by tokenId. However, this struct will not be compatible with the periphery contracts, and the unclaimedFees0 and unclaimedFees1 are unexpected returned params.

Be aware of this issue, and update the periphery contracts and avoid integration issues

Impermax Team: Acknowledged. We are aware of this, we will update the periphery once the audit for the core is completed.

The ERC721 mint function is used multiple times in the codebase instead of the safeMint function. If the receiver is a contract and is not capable of handling NFT-related actions and/or calling other functions in the system, the NFTs will be stuck.

Consider using the safeMint function instead of the mint function to make sure the receiver is capable of handling NFTs.

Impermax Team: The issue was resolved in commit 44b3100.

When calling the split function in the TokenizedUniswapV3Position contract, the receiver of the new NFT is always the owner.

The function call could come from an approved actor who wants to receive a part of the NFT which is not possible due to this hardcoded behavior

The approved actor is therefore allowed to receive the whole NFT but not a part of it.

Add a to parameter to the split function as in the mint function.

Impermax Team: Acknowledged.

Currently there isn't a minimum on the size of the borrow, which would require a very small collateral amount. In such a case, there may be not enough incentive for a liquidator to liquidate the borrower, and the protocol risks accumulation of bad debt.

Consider enforcing a minimum borrow size.

Impermax Team: Acknowledged.

A position is not liquidatable if the position is underwater (has acquired bad debt). At this point anyone needs to call restructureDebt to forgive the debt and make the position liquidatable again. This can lead to DoS when prices are around the position's bad debt threshold and fluctuate slightly.

A liquidator could call liquidate and between the creation of the transaction and its execution, the position could go underwater as the prices changed slightly and the liquidate call reverts.

Then the liquidator would call restructureDebt but between the creation of the transaction and its execution the position could be healthy enough again to be liquidated without forgiving debt as the prices changed slightly again causing the restructureDebt call to revert.

This is not a big problem but it could be a bad user experience and delay the liquidation process, a process that should happen as fast as possible to avoid further damage.

Merge the restructureDebt and liquidate flows into one function, so that the restructureDebt flow is executed if the position is underwater before continuing with the liquidate flow.

Impermax Team: Acknowledged. Before calling liquidate, the liquidator contract can check if the position is underwater. If it is, it can call restructureDebt before calling liquidate.

According to the CSetter, the minimum safety margin SAFETY_MARGIN_SQRT_MIN is 1e18.

With this setting, a user can borrow up to about ~1x post-liquidation collateral ratio and then become both liquidatable and underwater simultaneously, introducing bad debt into the collateral very quickly.

This also differs from the whitepaper constraints that the safetyMargin is between 150% and 250%.

Furthermore, in the case that the liquidation fee is zero, a liquidatable position owner can self-liquidate their position to take advantage of the discount instead of repaying their borrow, since self-liquidation would require less collateral for the same amount of assets due to the liquidationIncentive.

Firstly, adjust the minimum sqrt safety margin validation according to the whitepaper. Secondly, ensure that a liquidation fee is active.

Impermax Team: Acknowledged.