Revert Lend - crypticdefense's results

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L329-L331 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L378-L381 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L940-L942

Vulnerability details

Impact

V3Vault::maxRedeem does not follow ERC4626 EIP.

Proof of Concept

The ERC4626 EIP states that maxRedeem "MUST factor in both global and user-specific limits, like if redemption is entirely disabled (even temporarily) it MUST return 0."

Looking at V3Vault::maxRedeem, it simply returns the balance of the owner address:

    /// @inheritdoc IERC4626
    function maxRedeem(address owner) external view override returns (uint256) {
        return balanceOf(owner);
    }

This implementation is incorrect because it is possible that redemption can be disabled. Looking at V3Vault::redeem:

    /// @inheritdoc IERC4626
    function redeem(uint256 shares, address receiver, address owner) external override returns (uint256) {
        (uint256 assets,) = _withdraw(receiver, owner, shares, true);
        return assets;
    }

Which makes a call to _withdraw:

    function _withdraw(address receiver, address owner, uint256 amount, bool isShare)
        internal
        returns (uint256 assets, uint256 shares)
    {
        (uint256 newDebtExchangeRateX96, uint256 newLendExchangeRateX96) = _updateGlobalInterest();

        if (isShare) {
            shares = amount;
            assets = _convertToAssets(amount, newLendExchangeRateX96, Math.Rounding.Down);
        } else {
            assets = amount;
            shares = _convertToShares(amount, newLendExchangeRateX96, Math.Rounding.Up);
        }

        // if caller has allowance for owners shares - may call withdraw
        if (msg.sender != owner) {
            _spendAllowance(owner, msg.sender, shares);
        }

        (, uint256 available,) = _getAvailableBalance(newDebtExchangeRateX96, newLendExchangeRateX96);
@>      if (available < assets) {
            revert InsufficientLiquidity();
        }

        // fails if not enough shares
        _burn(owner, shares);
        SafeERC20.safeTransfer(IERC20(asset), receiver, assets);

        // when amounts are withdrawn - they may be deposited again
        dailyLendIncreaseLimitLeft += assets;

        emit Withdraw(msg.sender, receiver, owner, assets, shares);
    }

Here, we can see that if the available assets are less than the assets calculated for redemption, then redeem() will revert due to insufficient liquidity. Therefore, redemption will be disabled if there is not enough liquidity available. maxRedeem() does not account for this (it must return 0 if redemption is disabled, even temporarily), thus does not follow ERC4626 standard.

Tools Used

Manual Review

Recommended Mitigation Steps

Deploy a check within maxRedeem() to return 0 if redemption is disabled due to insufficient liquidity:

    function maxRedeem(address owner) external view override returns (uint256) {
+    (debtExchangeRateX96, lendExchangeRateX96) = _calculateGlobalInterest();    
+    uint256 assets = _convertToAssets(balanceOf(owner), lendExchangeRateX96, Math.Rounding.Down);
+    (, uint256 available,) = _getAvailableBalance(debtExchangeRateX96, lendExchangeRateX96);
+       if (available < assets) {
+           return 0;
+       }
        return balanceOf(owner);
    }

Assessed type

ERC4626

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/automators/Automator.sol#L139-L162 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/automators/AutoExit.sol#L162-L179 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/transformers/AutoRange.sol#L157-L189

Vulnerability details

Impact

In multiple instances throughout the protocol, pool.slot0 is used to calculate the sqrtPriceX96 and currentTick.

The problem is that there is a known issue with slot0, where it can be easily manipulated through MEV bots and flashloans. If slot0 is used for sensitive information, this can lead to vulnerabilities. In the case of the Revert Lend protocol, the sqrtPriceX96 is used for calculating slippage when swapping in the AutoRange.sol and AutoCompound.sol contracts.

An attacker, by manipulating slot0, can sandwich attack the swaps and profit.

Proof of Concept

Lets look at Automater::_validateSwap:

    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);
        }
    }

Here, we can see that slot0 is used to get the sqrtPriceX96 (current price) and currentTick (tick for TWAP check). The sqrtPriceX96 is subsequently used to calculate the amountOutMin, which is one of the values returned.

Now let's look at AutoExit::_execute lines 162-169:

            // checks if price in valid oracle range and calculates amountOutMin
@>          (state.amountOutMin,,,) = _validateSwap(
                !state.isAbove,
                state.swapAmount,
                state.pool,
                TWAPSeconds,
                maxTWAPTickDifference,
                state.isAbove ? config.token1SlippageX64 : config.token0SlippageX64
            );

            (state.amountInDelta, state.amountOutDelta) = _routerSwap(
                Swapper.RouterSwapParams(
                    state.isAbove ? IERC20(state.token1) : IERC20(state.token0),
                    state.isAbove ? IERC20(state.token0) : IERC20(state.token1),
                    state.swapAmount,
@>                  state.amountOutMin,
                    params.swapData
                )
            );

Here, the Automater::_validateSwap function is called to calculate the slippage for the swap. This is also done in the AutoRange::execute function:

        // check oracle for swap
@>      (state.amountOutMin, state.currentTick,,) = _validateSwap(
            params.swap0To1,
            params.amountIn,
            state.pool,
            TWAPSeconds,
            maxTWAPTickDifference,
            params.swap0To1 ? config.token0SlippageX64 : config.token1SlippageX64
        );

        if (
            state.currentTick < state.tickLower - config.lowerTickLimit
                || state.currentTick >= state.tickUpper + config.upperTickLimit
        ) {
            int24 tickSpacing = _getTickSpacing(state.fee);
            int24 baseTick = state.currentTick - (((state.currentTick % tickSpacing) + tickSpacing) % tickSpacing);

            // check if new range same as old range
            if (
                baseTick + config.lowerTickDelta == state.tickLower
                    && baseTick + config.upperTickDelta == state.tickUpper
            ) {
                revert SameRange();
            }

            (state.amountInDelta, state.amountOutDelta) = _routerSwap(
                Swapper.RouterSwapParams(
                    params.swap0To1 ? IERC20(state.token0) : IERC20(state.token1),
                    params.swap0To1 ? IERC20(state.token1) : IERC20(state.token0),
                    params.amountIn,
@>                  state.amountOutMin,
                    params.swapData
                )
            );

As mentioned, an attacker can manipulate the sqrtPriceX96, thus manipulating the slippage, and perform a sandwich attack (front-run) on the swaps. This will lead to a loss of funds for the protocol, and therefore users.

Tools Used

Manual Review.

Recommended Mitigation Steps

Avoid using slot0 and instead use Uniswap TWAP.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L372-L381 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L877-L952 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L1150-L1165 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L1167-L1195 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/InterestRateModel.sol#L46-L75

Vulnerability details

Impact

V3Vault::withdraw and V3Vault::redeem allows users to burn shares in return for assets, through a call to the internal _withdraw function. Withdrawals in this contract are instantaneous. An attacker can view the withdraw or redeem calls in the mempool and perform a sandwich attack by calling deposit before the withdraw/redeem call, and then call withdraw/redeem for themselves.

This attack can be done in many ways, such as by using flashbots. The profits will be instantaneous due to changes in exchange rates, effectively stealing user funds.

Proof of Concept

When a user deposits or withdraws, the exchange rate is calculated through the same function call _updateGlobalInterest, which proceeds to update the exchange rate by calling _calculateGlobalInterest.

The available assets are calculated, and used to calculate the new rates.

V3Vault::_calculateGlobalInterest

        (, uint256 available,) = _getAvailableBalance(oldDebtExchangeRateX96, oldLendExchangeRateX96);

        uint256 debt = _convertToAssets(debtSharesTotal, oldDebtExchangeRateX96, Math.Rounding.Up);

        (uint256 borrowRateX96, uint256 supplyRateX96) = interestRateModel.getRatesPerSecondX96(available, debt);

InterestRateModel::getRatesPerSecondX96

    function getRatesPerSecondX96(uint256 cash, uint256 debt)
        public
        view
        override
        returns (uint256 borrowRateX96, uint256 supplyRateX96)
    {
@>       uint256 utilizationRateX96 = getUtilizationRateX96(cash, debt);

        if (utilizationRateX96 <= kinkX96) {
            borrowRateX96 = (utilizationRateX96 * multiplierPerSecondX96 / Q96) + baseRatePerSecondX96;
        } else {
            uint256 normalRateX96 = (kinkX96 * multiplierPerSecondX96 / Q96) + baseRatePerSecondX96;
            uint256 excessUtilX96 = utilizationRateX96 - kinkX96;
            borrowRateX96 = (excessUtilX96 * jumpMultiplierPerSecondX96 / Q96) + normalRateX96;
        }

        supplyRateX96 = utilizationRateX96 * borrowRateX96 / Q96;
    }

Note that cash is what was calculated as available assets.

InterestRateModel::getUtilizationRateX96

    function getUtilizationRateX96(uint256 cash, uint256 debt) public pure returns (uint256) {
        if (debt == 0) {
            return 0;
        }
        return debt * Q96 / (cash + debt);
    }

As you can see from above, the lower the cash is, the higher the rate returned will be. Withdrawal will lower cash because available assets will be lower, thus the exchange rate will be greater.

Therefore, withdrawing will increase the rate. A user can see a pending withdrawal in the mempool, knowing the rate will be increased, can perform the following:

Lets say the newLendExchangeRateX96 is 1.2 (arbitrary value. exact decimals, etc. doesn't matter in this example).

1. Deposit token A amount 1000, minting them 1000 / 1.2 = 833 shares.
2. Pending withdrawal now executes, increasing the exchange rate to 1.4.
3. Attacker proceeds to call withdraw 833 shares => transferring them 833 x 1.4 = 1166 amount of token A.

Attacker profits.

Tools Used

Manual Review.

Recommended Mitigation Steps

Perhaps it's best to not allow instant withdrawls. Consider using a queue.

Assessed type

Other