Revert Lend - 14si2o_Flint's results

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L301-L320

Vulnerability details

Impact

The following functions are in breach of ERC4526 standard.

1. maxDeposit & maxMint
   • MUST factor in both global and user-specific limits, like if deposits are entirely disabled (even temporarily) it MUST return 0.
   • MUST return the maximum amount of assets deposit would allow to be deposited for receiver and not cause a revert, which MUST NOT be higher than the actual maximum that would be accepted (it should underestimate if necessary). This assumes that the user has infinite assets, i.e. MUST NOT rely on balanceOf of asset.

Proof of Concept

maxDeposit and maxMint

Both functions do not factor in all global and user-specific limits and thus return a value that will cause a revert.

    /// @inheritdoc IERC4626
    function maxDeposit(address) external view override returns (uint256) {
        (, uint256 lendExchangeRateX96) = _calculateGlobalInterest();
        uint256 value = _convertToAssets(totalSupply(), lendExchangeRateX96, Math.Rounding.Up);
        if (value >= globalLendLimit) {
            return 0;
        } else {
            return globalLendLimit - value;
        }
    }

    /// @inheritdoc IERC4626
    function maxMint(address) external view override returns (uint256) {
        (, uint256 lendExchangeRateX96) = _calculateGlobalInterest();
        uint256 value = _convertToAssets(totalSupply(), lendExchangeRateX96, Math.Rounding.Up);
        if (value >= globalLendLimit) {
            return 0;
        } else {
            return _convertToShares(globalLendLimit - value, lendExchangeRateX96, Math.Rounding.Down);
        }
    }

Both functions check for the global globalLendLimit limit and correctly return 0. However, they do not take into account the other global limit MAX_DAILY_LEND_INCREASE_X32 of 10%.

In the deposit function we have the below check which reverts if the amount exceeds the daily lend limit.

        if (assets > dailyLendIncreaseLimitLeft) { 
            revert DailyLendIncreaseLimit();
        } else {
            dailyLendIncreaseLimitLeft -= assets;
        }

Both maxDeposit and maxMint return an amount that assumes it can deposit an amount up to the global lend limit, which will 100% cause a revert since it exceeds the daily lend limit.

Tools Used

Manual Review

Recommended Mitigation Steps

Since the daily lend limit is always smaller then the global lend limit, it suffices to replace the limit in both functions.

NOTE: this is an edge case, but the use of setLimits by an emergencyAdmin in case of emergency can also lead to a "paused" state by setting the minLoanSize > dailyLendLimit. This should perhaps also be taken into account when mitigating this issue.

    /// @inheritdoc IERC4626
    function maxDeposit(address) external view override returns (uint256) {
        (, uint256 lendExchangeRateX96) = _calculateGlobalInterest();
        uint256 value = _convertToAssets(totalSupply(), lendExchangeRateX96, Math.Rounding.Up);
-        if (value >= globalLendLimit) {
+        if (value >= dailyLendIncreaseLimitLeft) {
            return 0;
        } else {
            return globalLendLimit - value;
        }
    }
    

    /// @inheritdoc IERC4626
    function maxMint(address) external view override returns (uint256) {
        (, uint256 lendExchangeRateX96) = _calculateGlobalInterest();
        uint256 value = _convertToAssets(totalSupply(), lendExchangeRateX96, Math.Rounding.Up);
-        if (value >= globalLendLimit) {
+        if (value >= dailyLendIncreaseLimitLeft) {
            return 0;
        } else {
            return _convertToShares(globalLendLimit - value, lendExchangeRateX96, Math.Rounding.Down);
        }
    }
    

Assessed type

ERC4626

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Oracle.sol#L395-L423 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Oracle.sol#L359-L374 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/transformers/AutoCompound.sol#L126-L153 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/automators/Automator.sol#L139-L162

Vulnerability details

Impact

The use of slot0 to obtain sqrtPriceX96 is heavily discouraged as it can easily manipulated. Attackers can acquire huge funds through flash loans and shift the slot0 by doing large swaps on Uniswap.

There are 4 instances of this throughout the protocol. In two of those, the sqrtPriceX96 is simply stored but not used. However, in the 3rd instance it is used to check price difference with chainlink feed and in 4th instance to calculate slippage protection. The last 2 instances are cases which could be subject to price manipulation.

Proof of Concept

In V3Oracle:_initalizeState, the sqrtPriceX96 is set in the PositionState struct but is never accessed.

    function _initializeState(uint256 tokenId) internal view returns (PositionState memory state) {
        (
            ,
            ,
            address token0,
            address token1,
            uint24 fee,
            int24 tickLower,
            int24 tickUpper,
            uint128 liquidity,
            uint256 feeGrowthInside0LastX128,
            uint256 feeGrowthInside1LastX128,
            uint128 tokensOwed0,
            uint128 tokensOwed1
        ) = nonfungiblePositionManager.positions(tokenId);
        state.tokenId = tokenId;
        state.token0 = token0;
        state.token1 = token1;
        state.fee = fee;
        state.tickLower = tickLower;
        state.tickUpper = tickUpper;
        state.liquidity = liquidity;
        state.feeGrowthInside0LastX128 = feeGrowthInside0LastX128;
        state.feeGrowthInside1LastX128 = feeGrowthInside1LastX128;
        state.tokensOwed0 = tokensOwed0;
        state.tokensOwed1 = tokensOwed1;
        state.pool = _getPool(token0, token1, fee);
        (state.sqrtPriceX96, state.tick,,,,,) = state.pool.slot0(); 
    }

In AutoCompound:execute the slot0 sqrtPriceX96 is set in the ExecuteParams struct, but is never used.

            // if a swap is requested - check TWAP oracle
            if (amountIn > 0) {
                IUniswapV3Pool pool = _getPool(state.token0, state.token1, state.fee);
                (state.sqrtPriceX96, state.tick,,,,,) = pool.slot0(); 

                // how many seconds are needed for TWAP protection
                uint32 tSecs = TWAPSeconds;
                if (tSecs > 0) {
                    if (!_hasMaxTWAPTickDifference(pool, tSecs, state.tick, maxTWAPTickDifference)) {
                        // if there is no valid TWAP - disable swap
                        amountIn = 0;
                    }
                }
                // if still needed - do swap
                if (amountIn > 0) {
                    // no slippage check done - because protected by TWAP check
                    (state.amountInDelta, state.amountOutDelta) = _poolSwap(
                        Swapper.PoolSwapParams(
                            pool, IERC20(state.token0), IERC20(state.token1), state.fee, params.swap0To1, amountIn, 0
                        )
                    );
                    state.amount0 =
                        params.swap0To1 ? state.amount0 - state.amountInDelta : state.amount0 + state.amountOutDelta;
                    state.amount1 =
                        params.swap0To1 ? state.amount1 + state.amountOutDelta : state.amount1 - state.amountInDelta;
                }
            }

In V3Oracle:_getReferencePoolPriceX96, slot0 is used whenever twapSeconds == 0. Which is the case every time _checkPoolPriceX96 is called. Since the explicit goal of this function is to check against price manipulation attacks, it is ill-advised to use an input which very weak to price manipulation.

    function _getReferencePoolPriceX96(IUniswapV3Pool pool, uint32 twapSeconds) internal view returns (uint256) {
        uint160 sqrtPriceX96;
        // if twap seconds set to 0 just use pool price
        if (twapSeconds == 0) {
            (sqrtPriceX96,,,,,,) = pool.slot0(); 
        } else {
            uint32[] memory secondsAgos = new uint32[](2);
            secondsAgos[0] = 0; // from (before)
            secondsAgos[1] = twapSeconds; // from (before)
            (int56[] memory tickCumulatives,) = pool.observe(secondsAgos); // pool observe may fail when there is not enough history available (only use pool with enough history!)
            int24 tick = int24((tickCumulatives[0] - tickCumulatives[1]) / int56(uint56(twapSeconds)));
            sqrtPriceX96 = TickMath.getSqrtRatioAtTick(tick);
        }

        return FullMath.mulDiv(sqrtPriceX96, sqrtPriceX96, Q96);
    }
    function _checkPoolPrice(address token0, address token1, uint24 fee, uint256 derivedPoolPriceX96) internal view {
        IUniswapV3Pool pool = _getPool(token0, token1, fee);
        uint256 priceX96 = _getReferencePoolPriceX96(pool, 0);
        _requireMaxDifference(priceX96, derivedPoolPriceX96, maxPoolPriceDifference);
    }

In Automator:_validateSwap slot0 sqrtPriceX96 is used to determine the amountOutMin so this can be clearly influences by manipulating slot0 price. It is also returned from the function, but the sqrtPriceX96 output is never used whenever _validateSwap is called.

    function _validateSwap(
        bool swap0For1,
        uint256 amountIn,
        IUniswapV3Pool pool,
        uint32 twapPeriod,
        uint16 maxTickDifference,
        uint64 maxPriceDifferenceX64
    ) internal view returns (uint256 amountOutMin, int24 currentTick, uint160 sqrtPriceX96, uint256 priceX96) {
        // get current price and tick
        (sqrtPriceX96, currentTick,,,,,) = pool.slot0();

        // check if current tick not too far from TWAP
        if (!_hasMaxTWAPTickDifference(pool, twapPeriod, currentTick, maxTickDifference)) {
            revert TWAPCheckFailed();
        }

        // calculate min output price price and percentage
        priceX96 = FullMath.mulDiv(sqrtPriceX96, sqrtPriceX96, Q96);
        if (swap0For1) {
            amountOutMin = FullMath.mulDiv(amountIn * (Q64 - maxPriceDifferenceX64), priceX96, Q96 * Q64);
        } else {
            amountOutMin = FullMath.mulDiv(amountIn * (Q64 - maxPriceDifferenceX64), Q96, priceX96 * Q64);
        }
    }

In the first 2 instances the variable is set but never used, so there is no impact. This alone would be a Low finding since it is redundant code with a price input source that can be manipulated.

In the 3rd and 4th case, there a clear attack vector where price manipulation can either be used to influence a price manipulation check or manipulate the amountOutMin of a swap.

Tools Used

Manual Review

Recommended Mitigation Steps

Replace slot.0 with the Uniswap TWAP as source of price information.

Assessed type

Uniswap

[L-01] Valid collateralValueLimitFactorX32 value will cause a revert

In the _updateAndCheckCollateral function, there are 2 checks to make sure that the collateralValueLimitFactorX32 does not exceed type(uint32).max. This is because this maximal value represents a collateral value limit of 100%.

However in the actual checks, < is used instead of <=. Meaning a valid value of 100% (type(uint32).max) would cause a revert.

Even though setting a value of 100% is very unlikely, it remains a valid input and thus should not cause a revert.

                if (
-                    collateralValueLimitFactorX32 < type(uint32).max
+                    collateralValueLimitFactorX32 <= type(uint32).max    
                        && _convertToAssets(tokenConfigs[token0].totalDebtShares, debtExchangeRateX96, Math.Rounding.Up) > lentAssets * collateralValueLimitFactorX32 / Q32
                ) {
                    revert CollateralValueLimit();
                }
                collateralValueLimitFactorX32 = tokenConfigs[token1].collateralValueLimitFactorX32;
                if (
-                    collateralValueLimitFactorX32 < type(uint32).max
+                    collateralValueLimitFactorX32 <= type(uint32).max  
                        && _convertToAssets(tokenConfigs[token1].totalDebtShares, debtExchangeRateX96, Math.Rounding.Up)
                            > lentAssets * collateralValueLimitFactorX32 / Q32
                ) {
                    revert CollateralValueLimit();
                }

[L-02] No slippage protection in Vault:liquidate

In the Vault liquidate function, the internal function _sendPositionValue is called which calls nonfungiblePositionManager.decreaseLiquidity with 100% slippage tolerance. Meaning MEV bots will take advantage and the liquidity returned will be either extremely small or non-existing.

This should a High finding, but it is mitigated by another finding. The _sendPositionValue does not return the decreased liquidity + fees, but only the fees. Which means in 90%+ of the cases, the amount0 and amount1 returned will be smaller than params.amount0Min and params.amount1Min and thus cause a SlippageError revert.

    function liquidate(){
        // send promised collateral tokens to liquidator
        (amount0, amount1) =
            _sendPositionValue(params.tokenId, state.liquidationValue, state.fullValue, state.feeValue, msg.sender);

        if (amount0 < params.amount0Min || amount1 < params.amount1Min) {
            revert SlippageError();
        }
    }
    function _sendPositionValue(
        uint256 tokenId,
        uint256 liquidationValue,
        uint256 fullValue,
        uint256 feeValue,
        address recipient
    ) internal returns (uint256 amount0, uint256 amount1) {
        uint128 liquidity;
        uint128 fees0;
        uint128 fees1;

        // if full position is liquidated - no analysis needed
        if (liquidationValue == fullValue) {
            (,,,,,,, liquidity,,,,) = nonfungiblePositionManager.positions(tokenId);
            fees0 = type(uint128).max;
            fees1 = type(uint128).max;
        } else {
            (,,, liquidity,,, fees0, fees1) = oracle.getPositionBreakdown(tokenId);

            // only take needed fees
            if (liquidationValue < feeValue) { 
                liquidity = 0;
                fees0 = uint128(liquidationValue * fees0 / feeValue);
                fees1 = uint128(liquidationValue * fees1 / feeValue);
            } else {
                // take all fees and needed liquidity
                fees0 = type(uint128).max; 
                fees1 = type(uint128).max; 
                liquidity = uint128((liquidationValue - feeValue) * liquidity / (fullValue - feeValue));
            }
        }

        if (liquidity > 0) {
            nonfungiblePositionManager.decreaseLiquidity(
                INonfungiblePositionManager.DecreaseLiquidityParams(tokenId, liquidity, 0, 0, block.timestamp)
            );
        }

        (amount0, amount1) = nonfungiblePositionManager.collect(
            INonfungiblePositionManager.CollectParams(tokenId, recipient, fees0, fees1)
        );
    }

[L-03] Inconsistent Lending rounding in view functions

The vaultInfo function calculates the total lent by Rounding Up _convertToAssets.

    function vaultInfo()
        external
        view
        override
        returns (
            uint256 debt,
            uint256 lent,
            uint256 balance,
            uint256 available,
            uint256 reserves,
            uint256 debtExchangeRateX96,
            uint256 lendExchangeRateX96
        )
    {
        (debtExchangeRateX96, lendExchangeRateX96) = _calculateGlobalInterest();
        (balance, available, reserves) = _getAvailableBalance(debtExchangeRateX96, lendExchangeRateX96);

        debt = _convertToAssets(debtSharesTotal, debtExchangeRateX96, Math.Rounding.Up);
        lent = _convertToAssets(totalSupply(), lendExchangeRateX96, Math.Rounding.Up);
    }

Yet the lendInfo function calculates the lend by Rounding Down _convertTaAssets.

    function lendInfo(address account) external view override returns (uint256 amount) {
        (, uint256 newLendExchangeRateX96) = _calculateGlobalInterest();
        amount = _convertToAssets(balanceOf(account), newLendExchangeRateX96, Math.Rounding.Down);
    }

As a result, the sum of all loans requested through lendInfo will be smaller than vaultInfo. The difference is minute, but I recommend changing the rounding of vaultInfo in order to be consistent.

[L-04] Liquidaty does not need to be calculated when fees == liquidationValue

• In the _sendPositionValue function, a check is performed to see whether the fees cover the liquidation value. If not, liquidity is calculated and decreased. However, in the check < is used instead of <=, meaning liquidity will be calculated when fees == liquidationValue.

This is unnecessary so I recommend it being changed.

            // only take needed fees
-            if (liquidationValue < feeValue) {
+            if (liquidationValue <= feeValue) {  
                liquidity = 0;
                fees0 = uint128(liquidationValue * fees0 / feeValue);
                fees1 = uint128(liquidationValue * fees1 / feeValue);

[L-05] Limit set to low in _requireMaxDifference

The _requireMaxDifference function checks for price manipulation by comparing a calculated difference with a maxDifference and reverting when it exceeds.

Yet in the if statement we see that >= is used instead of >, meaning a price difference equal to the maxDifference will cause a revert. Since the maxDifference should be an valid value, I recommend the following change:

    function _requireMaxDifference(uint256 priceX96, uint256 verifyPriceX96, uint256 maxDifferenceX10000)
        internal
        pure
    {
        uint256 differenceX10000 = priceX96 > verifyPriceX96 ? (priceX96 - verifyPriceX96) * 10000 / priceX96 : (verifyPriceX96 - priceX96) * 10000 / verifyPriceX96; 
        // if too big difference - revert
-        if (differenceX10000 >= maxDifferenceX10000) {
+        if (differenceX10000 > maxDifferenceX10000) {
            revert PriceDifferenceExceeded();
        }
    }

[L-06] onERC721Received not compliant with EIP721

onERC721Received is missing the "operator" named input param. This has no actual effect but is not in line with the EIP721 standard.

    function onERC721Received(address, address from, uint256 tokenId, bytes calldata data)
       external
       override
       returns (bytes4) 

[L-07] _sendPositionValue does not return correct amounts

The _sendPositionValue function is supposed to return the total amount of collateral tokens transferred to the liquidator, but it only returns the fees.

    function liquidate(){
        // send promised collateral tokens to liquidator
        (amount0, amount1) =
            _sendPositionValue(params.tokenId, state.liquidationValue, state.fullValue, state.feeValue, msg.sender);

        if (amount0 < params.amount0Min || amount1 < params.amount1Min) {
            revert SlippageError();
        }
    }
    function _sendPositionValue(
        uint256 tokenId,
        uint256 liquidationValue,
        uint256 fullValue,
        uint256 feeValue,
        address recipient
    ) internal returns (uint256 amount0, uint256 amount1) {
        uint128 liquidity;
        uint128 fees0;
        uint128 fees1;

        // if full position is liquidated - no analysis needed
        if (liquidationValue == fullValue) {
            (,,,,,,, liquidity,,,,) = nonfungiblePositionManager.positions(tokenId);
            fees0 = type(uint128).max;
            fees1 = type(uint128).max;
        } else {
            (,,, liquidity,,, fees0, fees1) = oracle.getPositionBreakdown(tokenId);

            // only take needed fees
            if (liquidationValue < feeValue) { 
                liquidity = 0;
                fees0 = uint128(liquidationValue * fees0 / feeValue);
                fees1 = uint128(liquidationValue * fees1 / feeValue);
            } else {
                // take all fees and needed liquidity
                fees0 = type(uint128).max; 
                fees1 = type(uint128).max; 
                liquidity = uint128((liquidationValue - feeValue) * liquidity / (fullValue - feeValue));
            }
        }

        if (liquidity > 0) {
            nonfungiblePositionManager.decreaseLiquidity(
                INonfungiblePositionManager.DecreaseLiquidityParams(tokenId, liquidity, 0, 0, block.timestamp)
            );
        }

        (amount0, amount1) = nonfungiblePositionManager.collect(
            INonfungiblePositionManager.CollectParams(tokenId, recipient, fees0, fees1)
        );
    }

[NC-01] sqrtPriceX96 is set but never used

There are several instances where sqrtPriceX96 is set in a struct or returned from a function but not used. This should be cleaned up since unforeseen side-effect could lead to serious security vulnerabilities.

• V3Oracle:_initalizeState, the sqrtPriceX96 is set in the PositionState struct but is never accessed.
• AutoCompound:execute the slot0 sqrtPriceX96 is set in the ExecuteParams struct, but is never used.
• Automator:_validateSwap slot0 sqrtPriceX96 is returned from the function, but the sqrtPriceX96 output is never used whenever _validateSwap is called.

[NC-02] Incorrect naming and Natspec of loanInfo

The _requireMaxDifference states Natspec & naming this returns information about one loan.

But function code clearly takes all loans against one collateral position, and returns variables based on the sum of loans on a a position. Thereby giving a user erroneous understanding of their actual situation.

I recommend changing the natspec and naming to correctly reflect the code.

[NC-03] lacking permit2 functions

• withdraw and redeem are the only functions to not have a permit2 version. Even though the use-case is rare, for completeness those version should be added.

[NC-04] Incorrect code comments

In two place in V3Vault, code comments are made suggesting the "vault itself" can perform an action. These are outdated comments dating from a change in logic dating several weeks ago and should be removed for clarity.

        // only the owner of the loan, the vault itself or any approved caller can call this 
        if (loanOwner != msg.sender && !transformApprovals[loanOwner][tokenId][msg.sender]) {
            revert Unauthorized();
        }

        // if not in transfer mode - must be called from owner or the vault itself
        if (!isTransformMode && tokenOwner[tokenId] != msg.sender && address(this) != msg.sender) {
            revert Unauthorized();
        }

Analysis of Revert Lend

1. Description Revert Lend

Revert have been developing analytical tools and dashboards to simplify and facility interaction with AMM protocols since June 2022. It is their stated belief that AMM protocols will play a pivotal role in the financial crypto markets in the coming years and they wish to help retails invests interact with these complex and sometimes obtuse protocols. These tools are mainly focused on Uniswap v3.

Revert Lend is their newest initiative, an ERC4626 lending protocol that takes a unique approach by allowing the Uniswap v3 NFT position to be used as collateral to facilitate the acquisition of ERC20 token loans, while allowing the users to retain control and management of their capital within Uniswap v3 Pools. As a consequence, it becomes possible to unify all the liquidity provided from all accepted tokens into one pool.

Another rare aspect of the protocol is the variable interest rate, which is completely dependent on the ebb and flow of the market.

2. Approach taken for this contest

Time spent on this audit: 11 days

Day 1: 7h35

• Reading the whitepaper and documentation
• Studying Uniswap V3, ERC4626
• Researching hacks of similar protocols
• Creating giant checklist of possible issues

Day 2-3: 13h37

• First pass through the code, adding questions to note.md while I read.
• Creating hand-drawn functional diagrams of all functions available to normal users in Excalidraw.

Day 4-6 : 17h50

• Second Pass through the code

Day 7-8: 11h06

• Go through notes one-by-one answering all questions and adding to preliminary findings
• Finalising preliminary findings
• Trying to test as many as I can in Foundry

Day 9-11: 20h55

• Writing reports & analysis

3. Architecture

In order to properly understand the architecture of the protocol, it is neccessary to understand its history.

Revert started back in June of 2022 with a mission of building actionable analytics for liquidity providers in AMM protcols, with a focus on Uniswap. To this end they developed the initiator and over time they added new functionalities and products such as V3Utils, Auto-Compound, Auto-Exist, Leverage-Transformer and Auto-Range. Revert Lend is their newest product which aims to introduce an ERC4626 vault.

As such, the architure should not be seen as a master drawing of "the Architect", but as many layers of architectural drawings, each becoming more complex and integrating what came before. As ingenious as it may be, the more custom functionality that has to be developed to allow everthing to fit together, the higher the chances of something being overlooked and exploited.

Architectural Risks

This becomes apparant when we evaluate how the V3Vault interacts with the automated contracts through tranform().

For AutoCompound and AutoRange:

• user calls executeWithVault from the transformer
• this calls Ivault(vault).transform()
• the vault calls back execute() to the transformer

For AutoExit,FlashloanLiquidator,LeverageTransformer and V3Utils:

• user calls transform() from the vault.

I could find no explanation anywhere in the documentation for having 2 different patterns. In itself this is not a security risk, but the chance of their being an oversight and a risk today or in future incarnations of the protocol, increases exponentially with each additional pattern.

Another issue is the multiplication of similar functionalities across the different contracts. When we look at Uniswap decreaseLiquidity, we can see that every contract besides FlashloanLiquidator can call this on a position. Again, in itself not a security risk, but the effort required to make sure that all instances are correctly configured and there exists no difference that be exploited, increases exponentially with each added similar functionality.

Recommandations

1. Streamline and perhaps externalise (ITransform) the access pattern between the vault and transformers so that there is only one correct way in which the communication between the two can happen.

2. Re-factor some of the existing contracts to reduce multiplication of similar functionality.

4. Main contracts and functionality

For each contract we will give a short description, an overview of the most important functions available to users and a custom function diagram to visualise internal and external calls. Furthermore, any findings will be mentioned with the relevant function.

Please refer to below Legend to understand the diagrams.

V3Vault.sol:

An IERC4626 compliant contract that manages one ERC20 asset for lending and borrowing. It accepts Uniswap v3 positions as collateral where these positions are composed of any 2 tokens which each have been configured to have a collateralFactor > 0.

External view functions:

• vaultInfo()
  • This functions retrieves the global information about the vault.
  • It makes use of the following internal functions:
    • _calculateGlobalInterest()
    • _getAvailableBalance()
    • _convertToAssets() x2

• lendInfo():
  • Here the lending information for a specified account is retrieved
  • It makes use of the following internal functions:
    • _calculateGlobalInterest()
    • _convertToAssets()
    • When we compare vaultInfo and lendInfo, we can see that the rounding for calculating the lending information is different. Due to this, the total lent from vaultInfo will be greater then the sum of lendInfo from all accounts. This could be considered a breaking of a fundamental invariant, as detailed in finding [L-03] Inconsistent Lending rounding in view functions.

• loanInfo():
  • The tokenId, which is the corresponding Uniswap V3 position, is used as input to retrieve the details of a loan.
  • It makes use of the following internal functions:
    • _calculateGlobalInterest()
    • _convertToAssets()
    • _checkLoanIsHealthy()
    • _calculateLiquidation()
    • When we look at the NatSpec and the naming of the function, it would seem obvious that you will receive the information for one specific loan. The code however, tells a different story. It calculate the debt as the sum of ALL loans taken against a position and calculate the health threshold on this global figure. This will gives users a completely erroneous understanding of their situation. This is further detailed in finding [NC-01] Incorrect naming and Natspec of loanInfo.

IERC4626 overriden external view functions:

• totalAssets()
  • Note that totalAssets makes use of balanceOf(address(this)), which opens an important attack vector of inflation attacks as detailed in finding H- Inflation attack due to the absence of dead shares and the reliance on balanceOf.
• convertToShares()
  • It makes use of the following internal functions:
    • _calculateGlobalInterest()
    • _convertToShares()
• convertToAssets()
  • It makes use of the following internal functions:
    • _calculateGlobalInterest()
    • _convertToAssets()
• maxDeposit()
  • It makes use of the following internal functions:
    • _calculateGlobalInterest()
    • _convertToAssets()
• maxMint()
  • It makes use of the following internal functions:
    • _calculateGlobalInterest()
    • _convertToShares()
• maxWithdraw()
  • It makes use of the following internal functions:
    • _calculateGlobalInterest()
    • _convertToAssets()
• maxRedeem()
  • balanceOf(owner)
• previewDeposit()
  • It makes use of the following internal functions:
    • _calculateGlobalInterest()
    • _convertToShares()
• previewMint()
  • It makes use of the following internal functions:
    • _calculateGlobalInterest()
    • _convertToAssets()
• previewWithdraw()
  • It makes use of the following internal functions:
    • _calculateGlobalInterest()
    • _convertToShares()
• previewRedeel()
  • It makes use of the following internal functions:
    • _calculateGlobalInterest()
    • _convertToShares()

IERC4626 overriden external functions:

• deposit(assets, receiver)
  • It makes use of the following internal functions:
    • _deposit()
• mint(shares, receiver)
  • It makes use of the following internal functions:
    • _deposit()
• deposit(shares, receiver, permitData)
  • It makes use of the following internal functions:
    • _deposit()
• mint(shares, receiver, permitData)
  • It makes use of the following internal functions:
    • _deposit()

• withdraw(assets, receiver, owner)
  • It makes use of the following internal functions:
    • _withdraw()
• redeem(shares, receiver, owner)
  • It makes use of the following internal functions:
    • _withdraw()
  • Note that withdraw and redeem are the only functions to not have a permit2 version. Even though the use-case is rare, for completeness those version should be added. More information in finding [NC-03] lacking permit2 functions

External functions:

• create()
  • The Uniswap V3 NFT position is transferred to the Vault contract and ownership is set to msg.sender or recipient if he is defined through onERC721Received.
  • Note that the onERC721Received is inconsistent with the EIP721 standard, as detailin finding [L-06] onERC721Received not compliant with EIP721.

• createWithPermit()
  • The permit version of the abovementioned create() function.
  • Ownership of position is transferred to the Vault contract and set to msg.sender or recipient if he is defined through onERC721Received
• approveTransform()
  • A user can approve automated agents (tranformers/automators) which will allow them to call the transform function.

• transform()
  • This function is the entry point for all the automated tools developed by the Revert protocol. Users approve these tools through approveTransform, which allows them to call transform to perform any user actions in an automated fashion.
  • It makes use of the following internal functions:
    • _updateGlobalInterest()
    • _convertToAssets()
    • _requireLoanIsHealthy()
  • Note that the code comments for "Unauthorized" check are incorrect, as detailed in finding [NC-04] Incorrect code comments.

• borrow()
  • A pivotal function in any lending protocol, the borrow functions allow a user to borrow assets with the uniswap position as collateral.
  • It makes use of the following internal functions:
    • _updateGlobalInterest()
    • _resetDailyDebtIncreaseLimit()
    • _convertToShares()
    • _updateAndCheckCollateral()
    • _convertToAssets()

• decreaseLiquidityAndCollect
  • A user can decrease the liquidity of a given position and resultant assets and potential fees.
  • It is of remark that transformers are not allowed to use this function since they can call the methods directly on the nonFungiblePositionManager.
  • It makes use of the following internal functions:
    • _updateGlobalInterest()
    • _convertToAssets()
    • _requireLoanIsHealthy()

• repay(tokenId, amount, IsShare)
  • Used to repay borrowed tokens. Can be denominated in assets or shares.
  • It makes use of the following internal functions:
    • _repay()
• repay(tokenId, amount, IsShare, permitData)
  • The permit version of repay.
  • Used to repay borrowed tokens. Can be denominated in assets or shares.
  • It makes use of the following internal functions:

    • _repay()

• liquidate()
  • This function is used to liquidate unhealthy loans.
  • Transformers cannot call this fucntion directly.
  • It makes use of the following internal functions:
    • _updateGlobalInterest()
    • _convertToAssets()
    • _checkLoanIsHealthy()
    • _calculateLiquidation()
    • _handleReserveLiquidation()
    • _sendPositionValue()
    • _cleanupLoan()
    • Note that the liquidate function has a major flaw in that it only transfers fees from the owner to the liquidator and not decreased liquidity. This causes the liquidator to not obtain the liquidity he paid for and the owner receives part of the liquidity he should have lost. More details can be found in finding H-Liquidation only transfers fees to liquidator, while part or all of the decreased liquidity is transferred back to the defaulting owner.
    • Also note that the decreaseLiquidity call is performed with 100% slippage tolerance, which can cause the liquidity returned to be close to zero. As detailed in finding [L-02] No slippage protection in Vault:liquidate

V3Oracle.sol:

This is contract is in V3Vault to calculate the value of positions. It is the main vector of obtaining price data and uses both Chainlink aswell as Uniswap v3 TWAP. Futhermore, it also provides emergency fallback mode.

getValue

• The function obtais value and prices of a Uniswap v3 LP Posiotion in specified token. It uses the configured oracles and verifies price on a second oracle. This the main function used by V3Vault functions to obtain price data.
• It makes use of the following functions:
  • getPositionBreakDown()
  • _getReferenceTokenPriceX96()
  • _checkPoolPrice()
• Note that a minor issue exists in _requireMaxDifference, which is called by _checkPoolPrice, as detailed in finding [L-05] Limit set to low in _requireMaxDifference.

getPositionBreakDown

• it returns a breakdown of a Uniswap v3 position (tokens and fee tier, liquidity, current liquidity amounts, uncollected fees).
• It makes use of the following internal functions:
  • _initializeState()
  • _getAmounts()

V3Utils.sol:

Stateless contract with utility functions for Uniswap V3 positions.

executeWithPermit

• This function calls execute with EIP712 permit.
• It makes use of the following functions:
  • execute()

execute

• This functions executes the provided instructions by pulling approved NFT instead of direct safeTransferFrom.
• It can make use of the following internal functions:

  • _decreaseLiquidity()

  • _collectFees()

  • _swapAndIncrease()

  • _swapAndMint()

  • _routerSwap()

  • _transferToken()

swap

• This function swaps amountIn for tokenOut - returning at least minAmountOut.
• It makes use of the following internal functions:
  • _prepareAddPermit2()
  • _prepareAddApproved()
  • _routerSwap()
  • _transferToken()

• This function performs 1 or 2 swaps from swapSourceToken to token0 and token1 and adds as much as possible liquidity to a newly minted position.
• It makes use of the following internal functions:
  • _prepareAddPermit2()
  • _prepareAddApproved()
  • _swapAndMint()

• This function performs 1 or 2 swaps from swapSourceToken to token0 and token1 and adds as much as possible liquidity to any existing position.
• It makes use of the following internal functions:
  • _prepareAddPermit2()
  • _prepareAddApproved()
  • _swapAndincrease()

AutoExit.sol:

This automator contract allows a v3 position to be automatically removed (limit order) or to be swapped to the opposite token (stop loss order) when it reaches a certain tick. The execution of the optimized swap is delegated to a revert controlled bot (operator) using an externalswap router.

execute

• This can only be from a configured operator account. Furthermore, the swap need to be executed within the maximum price difference allowed from the current pool price.
• It makes use of the following internal functions:
  • _getPool()
  • _decreaseFullLiquidityAndCollect()
  • _validateSwap()
  • _routerSwap()
  • _transferToken()

FlashloanLiquidator.sol:

• A helper contract which allows atomic liquidation and needed swaps by using Uniswap v3 Flashloan.

liquidate

• This function liquidates a loan from the V3Vault.
• It makes use of the following functions:
  • flashLoanPool.flash()
  • This causes uniswapV3FlashCallback() to be invoked

LeverageTransformer.sol:

• This contract offers functionality to leverage or deleverage Uniswap v3 positions in one transaction.

leverageUp

• The liquidity of a Uniswap v3 position is increased by this function.
• It can only be called through the transform function in V3Vault.sol
• It makes use of the following functions:
  • IVault(msg.sender).borrow()
  • _routerSwap()

leverageDown

• The liquidity of a Uniswap v3 position is decreased by this function.
• It can only be called through the transform function in V3Vault.sol
• It makes use of the following functions:
  • _routerSwap()
  • IVault(msg.sender).repay()

AutoCompound.sol:

This contract allows an approved operator of AutoCompound to compound a position. When called from outside the vault, the positions need to be approved, when called inside, the owner needs to approve the position to be transformed by the contract.

executeWithVault

• The token position in the vault is adjusted through the transform function. It can only be called from the configured operator account or from the vault.
• It makes use of the following function:
  • IVault(vault).transform()

execute

• This adjusts the token directly and only be called from the configured operator account or by the vault through transform.
• It makes use of the following internal functions:
  • _getPool()
  • _hasMaxTWAPTickDifference()
  • _poolSwap()
  • _checkApprovals()
  • _setBalance()
  • _increaseBalance()

AutoRange.sol

This contract allows an approved operator of AutoRange to change the range for the configured position. If called inside Vault, it will use the transform method. If outside, the positions need to be approved for the contract and configures with configToken function.

executeWithVault

• The token position in the vault is adjusted through the transform function. It can only be called from the configured operator account or from the vault.
• It makes use of the following function:
  • IVault(vault).transform()

execute

• This adjusts the token directly and only be called from the configured operator account or by the vault through transform.
• It makes use of the following internal functions:
  • _decreaseFullLiquidityAndCollect()
  • _getPool()
  • _getTickSpacing()
  • _routerSwap()
  • _transferToken()

5. Codebase Quality

On the whole, I evaluate the quality of Revert Lend codebase to be "Good". The contract and function design are clearly well though out, access control is properly implemented and the various standards are well implemented. Some improvements on attention on detail would be advisable and there are architectural complexities. Details are explained below:

6. Centralization Risks

The protocol defines 2 privileged roles: Owner and EmergencyAdmin.

The Owner is set to a Multisig and Timelock according to the contest README and has the following rights:

• In V3Oracle:
  • setTokenConfig
  • setEmergencyAdmin
  • setMaxPoolPriceDifference
• In V3Vault:
  • withdrawReserves
  • setTransformer
  • setLimits
  • setReserveFactor
  • setReserveProtectionFactor
  • setTokenConfig
  • setEmergencyAdmin
• In AutoCompound:
  • setReward

The main issue with the owner design is that all changes are One-Step operations. Regardless whether it is changing the owner itself or setting a token configuration, even the tiniest mistake could case major damage to the protocol. Either by setting the owner to a random address or wrongly setting twap seconds or a token address, which would give hackers an immediate and easy attack vector to drain the protocol, a single mistake is devastating.

I would recommend the implementation of a two-step approval process for the most critical of operations. Also, the contest README states that the owner is a multisig subject to a Timelock, but nothing of the sorts can be seen in the contract code.

The EmergencyAdmin is set to a Multisig according to the contest README and has the following rights:

• In V3Oracle:
  • setOracleMode
• In V3Vault:
  • setLimits

The emergencyAdmin can essentially pause the protocol by setting the limits to 0 and it can change the oracle from TWAP to Chainlink or change the verification mode. However, if issues are found that are not affected by these limits (liquidation, transformer misbehaving), then the emergencyAdmin does not have the powers to take action.

Instead of a custom role, I would recommend to implement the standardised pattern of pause/unpause, and adding the whenNotPaused modifier to all functions which change state in the protocol. Thus allowing the admin to freeze the entire protocol in case of emergency.

7. Systemic & Integration Risks

1. Reliance on double Oracle.

• The protocols obtain the price information from either Chainlink or Uniswap TWAP and uses the other to verify the price against manipulation. In itself an excellent design but it does mean that the oracle will malfunction if either of the oracle sources malfunctions. This has happened before and even though it is certainly not a reason to not use double verification, it is a risk that should be acknowledged.
• Note that proper configuration is also of paramount importance, since the currect code will malfunction due to the absence of a L2 sequencer check when deploying on Arbitrum.

2. Off-chain Router

• Many of the transformers make use of an off-chain router to execute swaps. The router is outside the scope of the audit and we cannot evaluate it's design or assess it's security. As such, if the router were to go off-line or be compromised, the entire protocol will be compromised.

3. Integrating ERC4626 Vault with Transformers

• Transformers are treated as trusted actors and can effect state-changing calls on the Vault. In itself, this is not a problem. However, as noted above in the architectural risks part, the multiplication of similar functionality exponentially increases the risk of a security oversight.
• Futhermore, the current design allows future transformers, which might have security issues or unforeseen side-effects when interacting with the vault, to be quickly added as a trusted source.

4. Lack of Upgradeability

• The absence of any upgradeability leaves the protocol powerless when critical bugs and errors are found. While it is true that upgrades can also introducing security issues (Euler hack is a famous example), bugs are as certain as death & taxes, so functionality to resolve these should be implemented.
• I would recommend implementing the UUPS proxy pattern since this allows the protocol to resolve bugs and burn the upgradeability once the protocol has been live for a significant amount of time and the likelyhood of new bugs becomes minute.

5. Transient Storage

• The protocol intents (code comment) to use transient storage for the transformedTokenId variable, which is critical for guarding against reentrancy. This could a problem since it is a new and fairly unexplored functionality. Since some theoretical reentrancy attacks are already being discussed, I would suggest much prudence in implementing this.

Time spent:

71 hours