Revert Lend - grearlake's results

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Oracle.sol#L359-#L371

Vulnerability details

Vulnerability Detail

In V3Oracle contract, _getReferencePoolPriceX96() function is used by protocol to get average price

The problem is that in case if tickCumulatives[1] - tickCumulatives[0] is negative, then timeWeightedTick should be rounded down like Uniswap library

As result, in case if tickCumulatives[1] - tickCumulatives[0] is negative and (tickCumulatives[1] - tickCumulatives[0]) % twapSeconds != 0, tick will be bigger then it should be. Which opens possibility for arbitrage opportunities.

Impact

If tickCumulatives[1] - tickCumulatives[0] is negative and ((tickCumulatives[1] - tickCumulatives[0]) % twapSeconds != 0, then returned tick will be bigger than it should be.

Tools Used

Manual review

Recommended Mitigation Steps

Tick should be rounded down in that case:

  int24 tick = int24((tickCumulatives[1] - tickCumulatives[0]) / int56(uint56(twapSeconds)));
+  if ((tickCumulatives[1] - tickCumulatives[0]) < 0 && ((tickCumulatives[1] - tickCumulatives[0]) % twapSeconds != 0)) tick--;

Assessed type

Uniswap

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/automators/Automator.sol#L180-#L191

Vulnerability details

Vulnerability details

Function Automator#_getTWAPTick() is used to gets twap tick from pool history:

function _getTWAPTick(IUniswapV3Pool pool, uint32 twapPeriod) internal view returns (int24, bool) {
    uint32[] memory secondsAgos = new uint32[](2);
    secondsAgos[0] = 0; // from (before)
    secondsAgos[1] = twapPeriod; // from (before)

    // pool observe may fail when there is not enough history available
    try pool.observe(secondsAgos) returns (int56[] memory tickCumulatives, uint160[] memory) {
        return (int24((tickCumulatives[0] - tickCumulatives[1]) / int56(uint56(twapPeriod))), true); //<---
    } catch {
        return (0, false);
    }
}

But it do not round to negative infinity like in original uniswap function:

    // Always round to negative infinity
    if (tickCumulativesDelta < 0 && (tickCumulativesDelta % secondsAgo != 0)) arithmeticMeanTick--;

So it might return wrong result in _hasMaxTWAPTickDifference():

function _hasMaxTWAPTickDifference(IUniswapV3Pool pool, uint32 twapPeriod, int24 currentTick, uint16 maxDifference)
    internal
    view
    returns (bool)
{
    (int24 twapTick, bool twapOk) = _getTWAPTick(pool, twapPeriod);
    if (twapOk) {
        return twapTick - currentTick >= -int16(maxDifference) && twapTick - currentTick <= int16(maxDifference); //<---
    } else {
        return false;
    }
}

Which will make _validateSwap() return wrong result, in the worst case, it will be reverted despite it should not:

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

    if (!_hasMaxTWAPTickDifference(pool, twapPeriod, currentTick, maxTickDifference)) {  //<---
        revert TWAPCheckFailed();
    }
  .  .  .  .  .  .

Impact

In the worst case, swap will be unintentionally reverted when using _validateSwap() function

Tools Used

Manual review

Recommended Mitigation Steps

Adjust function like below:

function _getTWAPTick(IUniswapV3Pool pool, uint32 twapPeriod) internal view returns (int24, bool) {
    uint32[] memory secondsAgos = new uint32[](2);
    secondsAgos[0] = 0; // from (before)
    secondsAgos[1] = twapPeriod; // from (before)

    // pool observe may fail when there is not enough history available
    try pool.observe(secondsAgos) returns (int56[] memory tickCumulatives, uint160[] memory) {
  -     return (int24((tickCumulatives[0] - tickCumulatives[1]) / int56(uint56(twapPeriod))), true);
  +     if((tickCumulatives[0] - tickCumulatives[1]) < 0 && ((tickCumulatives[0] - tickCumulatives[1]) % twapPeriod != 0))) {
  +          return (int24((tickCumulatives[0] - tickCumulatives[1]) / int56(uint56(twapPeriod)) - 1), true);
  +     }
  +     else {
  +          return (int24((tickCumulatives[0] - tickCumulatives[1]) / int56(uint56(twapPeriod))), true);
  +     }
    } catch {
        return (0, false);
    }
}

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L550-#L602

Vulnerability details

Vulnerability details

When deploying V3Vault contract, minLoanSize is originally set to 0:

// minimal size of loan (to protect from non-liquidatable positions because of gas-cost)
uint256 public minLoanSize = 0;

And in the uniswap v3, minting position does not have any limitation in value: original code

Attacker can mint multiple positions with small amount, transfer them to V3Vault and use them to borrow. When the price change, and these positions is under-collateralzed, there is no incentive for anyone to liquidate them because cost of gas required to call them is more than value of token they receive back.

Impact

Protocol will suffer bad debt because no one is willing to liquidate them

Tools Used

Manual review

Recommended Mitigation Steps

Variable minLoanSize should be set when initalizing this contract.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L696-#L698

Vulnerability details

Vulnerability detail

In V3Vault.liquidate() function, there is a checking condition to make sure debtShares of position is equal to debtShares of input:

function liquidate(LiquidateParams calldata params) external override returns (uint256 amount0, uint256 amount1) {
    .  .  .  .  .
    if (debtShares != params.debtShares) {  //<---
        revert DebtChanged();
    }

In _repay() function, it is allowed for user to partial repay, user only need to partial repay amount that at least minLoanSize:

function _repay(uint256 tokenId, uint256 amount, bool isShare, bytes memory permitData) internal {
    .  .  .  .  .  .
    // if fully repayed
    if (currentShares == shares) {
        _cleanupLoan(tokenId, newDebtExchangeRateX96, newLendExchangeRateX96, owner);
    } else {
        // if resulting loan is too small - revert
        if (_convertToAssets(loanDebtShares, newDebtExchangeRateX96, Math.Rounding.Up) < minLoanSize) {  //<---
            revert MinLoanSize();
        }
    }

Which is set at 0 when deploy, but it can be changed later:

// minimal size of loan (to protect from non-liquidatable positions because of gas-cost)
uint256 public minLoanSize = 0;

So, the problem is user can front-running liquidate() function by repay at least minLoanSize amount, keep the position not liquidated although it is under-collateralized

Impact

Position are not able to be liquidated, although it is under-collateralized, which will lead to bad debt for protocol

Tools Used

Manual review

Recommended Mitigation Steps

Remove that checking position when liquidating, only rely-on current debtShares of the position to handle

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L973-#L975

Vulnerability details

Vulnerability detail

In the _repay() function, there is a checking condition that make sure that repay amount should not excess full amount that need to be repay:

function _repay(uint256 tokenId, uint256 amount, bool isShare, bytes memory permitData) internal {
    (uint256 newDebtExchangeRateX96, uint256 newLendExchangeRateX96) = _updateGlobalInterest();

    Loan storage loan = loans[tokenId];

    uint256 currentShares = loan.debtShares;

    uint256 shares;
    uint256 assets;

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

    // fails if too much repayed
    if (shares > currentShares) {  // <---
        revert RepayExceedsDebt();
    }
    .  .  .  .  .  .

But the problem is anyone can repay for any position, because there is no checking condition to restrict that only owner's position is able to repay. It only need to make sure repay amount is at least minLoanSize:

    if (currentShares == shares) {
        _cleanupLoan(tokenId, newDebtExchangeRateX96, newLendExchangeRateX96, owner);
    } else {
        // if resulting loan is too small - revert
        if (_convertToAssets(loanDebtShares, newDebtExchangeRateX96, Math.Rounding.Up) < minLoanSize) {  //<--
            revert MinLoanSize();
        }
    }

Which is set to 0 when deployment, although it can be changed later:

// minimal size of loan (to protect from non-liquidatable positions because of gas-cost)
uint256 public minLoanSize = 0;

So the attack is attacker will front-running anyone who willing to full repay for a position and repay dust amount for that position, which lead to revert when trying to repay full that position

Impact

User are not able to repay full for position.

Tools Used

Manual review

Recommended Mitigation Steps

Only allow owner of position to repay for that position:

function _repay(uint256 tokenId, uint256 amount, bool isShare, bytes memory permitData) internal {
+   address owner = tokenOwner[tokenId];
+   require(msg.sender == owner);

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Oracle.sol#L232 https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Oracle.sol#L362-#L364

Vulnerability details

Vulnerability details

In _getReferencePoolPriceX96() function, if twaapSeconds = 0, price will be fetched directly by using slot0() function:

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

This function with twapSeconds = 0 is called in the _checkPoolPrice(), which then used to check for price manipulate attack:

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

It is also called in _getTWAPPriceX96() function to fetch price:

function _getTWAPPriceX96(TokenConfig memory feedConfig) internal view returns (uint256 poolTWAPPriceX96) {
    // get reference pool price
    uint256 priceX96 = _getReferencePoolPriceX96(feedConfig.pool, feedConfig.twapSeconds); //<---
    .  .  .  .  .

It is called at _getReferenceTokenPriceX96(), with config is set by owner:

function _getReferenceTokenPriceX96(address token, uint256 cachedChainlinkReferencePriceX96) internal view returns (uint256 priceX96, uint256 chainlinkReferencePriceX96)
{
    if (token == referenceToken) {
        return (Q96, chainlinkReferencePriceX96);
    }

    TokenConfig memory feedConfig = feedConfigs[token]; //<--

    .  .  .  .  .  .

    if (usesTWAP) {
        uint256 twapPriceX96 = _getTWAPPriceX96(feedConfig);  //<--
        if (feedConfig.mode == Mode.CHAINLINK_TWAP_VERIFY) {
            verifyPriceX96 = twapPriceX96;
        } else {
            priceX96 = twapPriceX96;
        }
    }

In the setTokenConfig() function, there is no restriction in settings twapSeconds. So if twapSeconds of feedConfig is set to 0, attacker can manipulate price and bypass protection

Impact

Token price can be manipulated, manipulate protection is not work in that case

Tools Used

Manual review

Recommended Mitigation Steps

Add checking for twapSeconds in setTokenConfig() function, make sure it have at least some minutes to prevent TWAP manipulate attack

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L877-#L917 https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L920-#L952

Vulnerability details

Vulnerability details

The deposit/withdraw function in the V3Vault lacks a mechanism for users to express their minimum acceptable output. This deficiency exposes users to potential losses of their principal due to update of the rate

In the _deposit() function, amount and shares is directly converted to the other by calling _convertToAssets/_convertToShares() function, and it is rely on newLendExchangeRateX96. The update of newLendExchangeRateX96 can make user receive less shares/cost more amounts than they expected. This thing is similar in _withdraw() function. This scenario also can happen when user attempts to withdraw/deposit assets and their transaction is delayed due to low gas cost/network issue/reorg/ ..., which harming user

Impact

Users will loss assets due to the absence of slippage control in the withdraw/deposit function.

Tools Used

Manual review

Recommended Mitigation Steps

Add slippage checking when deposit/withdraw assets

Assessed type

Other