Revert Lend - y0ng0p3's results

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L360-L363 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L366-L369 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L372-L375 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L378-L381

Vulnerability details

The V3Vault::withdraw function withdraws the assets, parameter for the amount of asset, by burning shares in the V3Vault::_withdraw function subsequently called.

function withdraw(uint256 assets, address receiver, address owner) external override returns (uint256) {
        (, uint256 shares) = _withdraw(receiver, owner, assets, false);
        return shares;
    }

The V3Vault::deposit function deposits the assets, parameter for the amount of asset, by minting shares in the V3Vault::_deposit function subsequently called.

function deposit(uint256 assets, address receiver) external override returns (uint256) {
        (, uint256 shares) = _deposit(receiver, assets, false, "");
        return shares;
    }

The V3Vault::redeem function redeems a specific amount of shares, by burning them and withdrawing in the corresponding amount of assets in V3Vault::_withdraw function subsequently called.

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

The V3Vault::mint function mints the assets, parameter for the amount of asset, by minting shares in the V3Vault::_deposit function subsequently called.

function mint(uint256 shares, address receiver) external override returns (uint256) {
        (uint256 assets,) = _deposit(receiver, shares, true, "");
        return assets;
    }

It is theoretically possible for the deposit amount to mint shares more than the maximum mint-able amount, or for the withdraw amount to burn shares more than the maximum withdraw-able amount. It is also theoretically possible for the mint amount to deposit assets more than the maximum deposit-able amount, or for the redeem amount to withdraw assets more than the maximum withdraw-able amount.

However, there is neither protection against burning or minting too many user's shares for their specific assets, not protection against depositing or withdrawing too many user's assets for their specific amount of shares. Then the maxDeposit, maxMint, maxWithdraw and maxRedeem functions of V3Vault should be used to protect user's shares.

Proof of Concept

There is no check to validate if the minted/burned shares would exceed the maximum amount of shares that could've been minted/burned by V3Vault, nor check to validate if the deposited/withdrawn amount of assets would exceed the maximum amount of assets that could've been deposited/withdrawn.

However, the expression amount.mulDiv(Q96, exchangeRateX96, rounding) in the V3Vault::_convertToShares function could return a value more than the maximum mint-able or the maximum redeem-able amount.
As well, the expression shares.mulDiv(exchangeRateX96, Q96, rounding) in the V3Vault::_convertToAssets function could return a value more than the maximum deposit-able amount or the maximum withdraw-able amount.

This is possible if the vault get exploited or is severely under-collateralized.

User call V3Vault::withdraw() with assets = 1200. Normally, the burned shares should be 1000. However, there's an exploit in the vault which makes the shares burned to = 100_000, the entire user's shares.

Impact

Users can lose their entire shares when withdrawing and obtain more shares with depositing due to lack of amount burned check. Users can obtain more assets when redeeming due to lack of check on amount of withdrawn assets.

Tools Used

Manual review

Recommended Mitigation Steps

Include the maxMintable, maxDepositable, maxWithdrawable and maxRedeemable check in the V3Vault deposit, withdraw, redeem and mint corresponding functions.

Below an example :

-  function withdraw(uint256 assets, address receiver, address owner) external override returns (uint256) {
+  function withdraw(
+      uint256 assets, 
+      address receiver, 
+      address owner,
+      uint256 maxRedeem, 
+      uint256 maxWithdraw 
+  ) external override returns (uint256) {
+       if (assets > maxWithdraw) {
+           revert TooMuchAmount();
+       }
        (, uint256 shares) = _withdraw(receiver, owner, assets, false);
+      if (shares > maxRedeem) {
+       revert TooMuchAmount();
+      }
        return shares;
    }

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/457230945a49878eefdc1001796b10638c1e7584/src/V3Vault.sol#L352-L355 https://github.com/code-423n4/2024-03-revert-lend/blob/457230945a49878eefdc1001796b10638c1e7584/src/V3Vault.sol#L378-L381

Vulnerability details

The contest page explicitly mentioned that the V3Vault must conform with the ERC4626.

V3Vault: Should comply with ERC/EIP4626

Thus, issues related to EIP compliance should be considered valid in the context of this audit.

Let's consider the value returned by V3Vault::previewRedeem function be $asset_{preview}$ and the actual number of assets obtained from calling the V3Vault::redeem function be $asset_{actual}$.

The following specification of previewRedeem function is taken from ERC4626 specification:

Allows an on-chain or off-chain user to simulate the effects of their redeemption at the current block, given current on-chain conditions.

MUST return as close to and no more than the exact amount of assets that would be withdrawn in a redeem call in the same transaction. I.e. redeem should return the same or more assets as previewRedeem if called in the same transaction.

It mentioned that the redeem should return the same or more assets as previewRedeem if called in the same transaction, which means that it must always be $asset_{preview} \le asset_{actual}$.

This is the implementation of V3Vault::previewRedeem and V3Vault::redeem functions.

File: src/V3Vault.sol

function previewRedeem(uint256 shares) public view override returns (uint256) {
    (, uint256 lendExchangeRateX96) = _calculateGlobalInterest();
    return _convertToAssets(shares, lendExchangeRateX96, Math.Rounding.Down);
}

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

V3Vault::redeem() subsequently calls the V3Vault::_updateGlobalInterest function, which call the V3Vault::_calculateGlobalInterest function only once per block.

    function _updateGlobalInterest()
    internal
    returns (uint256 newDebtExchangeRateX96, uint256 newLendExchangeRateX96)
{
@@->// only needs to be updated once per block (when needed)
    if (block.timestamp > lastExchangeRateUpdate) {
        (newDebtExchangeRateX96, newLendExchangeRateX96) = _calculateGlobalInterest();
        lastDebtExchangeRateX96 = newDebtExchangeRateX96;
        lastLendExchangeRateX96 = newLendExchangeRateX96;
        lastExchangeRateUpdate = block.timestamp;
        emit ExchangeRateUpdate(newDebtExchangeRateX96, newLendExchangeRateX96);
    } else {
        newDebtExchangeRateX96 = lastDebtExchangeRateX96;
        newLendExchangeRateX96 = lastLendExchangeRateX96;
    }
}

Note that the V3Vault::redeem function does not call the V3Vault::previewRedeem function. This basically allows the two functions be called in different blocks, which means that global interest rates would be recalculated. Those values can be different between the call to V3Vault::previewRedeem() and the call to V3Vault::redeem().
It is possible that the V3Vault::redeem function might return fewer assets than the number of assets previewed by the V3Vault::previewRedeem ($asset_{preview} > asset_{actual}$), thus it does not conform to the specification.

Impact

It was understood from the protocol team that they anticipate external parties (ERC4626 compliant smart contracts) to integrate directly with the V3Vault (e.g., vault shares as collateral). Thus, the V3Vault must be ERC4626 compliant. Otherwise, the caller of the V3Vault::previewRedeem function might receive incorrect information, leading to the wrong action being executed.

Tools Used

Manual review

Recommended Mitigation Steps

Ensure that the actual value redeemed in V3Vault::redeem() is at least equal to the value that V3Vault::previewRedeem() would return in the same transaction ($asset_{preview} \le asset_{actual}$).

Alternatively, document that the previewRedeem and redeem functions deviate from the ERC4626 specification in the comments and/or documentation.

Assessed type

ERC4626

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/457230945a49878eefdc1001796b10638c1e7584/src/V3Oracle.sol#L363 https://github.com/code-423n4/2024-03-revert-lend/blob/457230945a49878eefdc1001796b10638c1e7584/src/automators/Automator.sol#L148

Vulnerability details

The V3Oracle::_getReferencePoolPriceX96 function uses uniswap slot0 price. slot0 price can be manipulated with flash loans.

The contract uses the instantaneous price from slot0 to get the pool price. The slot0 price is calculated from the ratios of the underlying assets. However, these ratios can be manipulated by buying/selling assets in the pool.

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

Thus any user can take a flashloan, use those funds to manipulate the price of the target asset in the pool, and then trigger V3Vault::borrow() to take a loan at good price.
All functionalities that use the slot0 price is vulnerable, this includes but not limited to borrowing, liquidate, transforming in V3Vault contract as well as validating swap in Automator contract since the same issue lies there too.

Impact

This vulnerability is closed when feedConfig.twapSeconds is set to a value different from 0, but since V3Oracle::_checkPoolPrice() always calls V3Oracle::_getReferencePoolPriceX96() with 0 as twapSeconds, and V3Oracle::_checkPoolPrice() is used in many places in the codebase, this vulnerability is very widespread and causes serious losses to the protocol.
The value of an LP token depends on the current market reserves which can be manipulated by an attacker via flashloans. Therefore, an attacker trading large amounts in the market can either increase or decrease the value of an LP token.
If the value decreases, they can try to liquidate users borrowing against their LP tokens. If the value increases, they can borrow against it and potentially receive an under-collateralized borrow this way, making a profit.

The exact profitability of such an attack depends on the AMM as the initial reserve manipulation and restoring the reserves later incurs fees and slippage. In constant-product AMMs like Uniswap it's profitable and several projects have already been exploited by this, like warp.finance.

Tools Used

Manual review

Recommended Mitigation Steps

Only use TWAP to get the value of sqrtPriceX96.
Use a minimal twap seconds when twapSeconds argument is set to 0.

    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();
++          twapSeconds = MIN_TWAP_SECONDS;
--      } 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);
    }

Assessed type

Oracle

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/transformers/AutoCompound.sol#L159-L172 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L1032-L1074

Vulnerability details

The protocol is using block.timestamp as the deadline argument while interacting with the Uniswap NFT Position Manager, which completely defeats the purpose of using a deadline.

Actions in the Uniswap NonfungiblePositionManager contract are protected by a deadline parameter to limit the execution of pending transactions. Functions that modify the liquidity of the pool check this parameter against the current block timestamp in order to discard expired actions.

These interactions with the Uniswap position are present throughout the code base, in particular and not only in the functions V3Utils::_swapAndMint, Automator::_decreaseFullLiquidityAndCollect, LeverageTransformer::leverageUp.. Those functions call their corresponding functions in the Uniswap Position Manager, providing the deadline argument with their own dealine argument.
In the other hand, AutoCompound::execute and V3Vault::_sendPositionValue functions provide block.timestamp as the argument for the deadline parameter in their call to the corresponding underlying Uniswap NonfungiblePositionManager contract.

File: src/transformers/AutoCompound.sol

// deposit liquidity into tokenId
if (state.maxAddAmount0 > 0 || state.maxAddAmount1 > 0) {
    _checkApprovals(state.token0, state.token1);


    (, state.compounded0, state.compounded1) = nonfungiblePositionManager.increaseLiquidity(
        INonfungiblePositionManager.IncreaseLiquidityParams(
@@->    params.tokenId, state.maxAddAmount0, state.maxAddAmount1, 0, 0, block.timestamp
        )
    );


    // fees are always calculated based on added amount (to incentivize optimal swap)
    state.amount0Fees = state.compounded0 * rewardX64 / Q64;
    state.amount1Fees = state.compounded1 * rewardX64 / Q64;
}

File: src/V3Vault.sol

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

Using block.timestamp as the deadline is effectively a no-operation that has no effect nor protection. Since block.timestamp will take the timestamp value when the transaction gets mined, the check will end up comparing block.timestamp against the same value (see https://github.com/Uniswap/v3-periphery/blob/697c2474757ea89fec12a4e6db16a574fe259610/contracts/base/PeripheryValidation.sol#L7).

Impact

Failure to provide a proper deadline value enables pending transactions to be maliciously executed at a later point. Transactions that provide an insufficient amount of gas such that they are not mined within a reasonable amount of time, can be picked by malicious actors or MEV bots and executed later in detriment of the submitter.
See this issue for an excellent reference on the topic (the author runs a MEV bot).

Tools Used

Manual review

Recommended Mitigation Steps

As done in the LeverageTransformer::leverageUp and V3Utils::_swapAndIncrease functions, add a deadline parameter to the AutoCompound::execute and V3Vault::_sendPositionValue functions and forward this parameter to the corresponding underlying call to the Uniswap NonfungiblePositionManager contract.

Assessed type

Other

Lines of code

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

Vulnerability details

The protocol missing the DEADLINE check at all in logic.
This is actually how uniswap implemented the deadline https://github.com/Uniswap/v2-periphery/blob/0335e8f7e1bd1e8d8329fd300aea2ef2f36dd19f/contracts/UniswapV2Router02.sol#L32-L76

// **** ADD LIQUIDITY ****
function _addLiquidity(
    address tokenA,
    address tokenB,
    uint amountADesired,
    uint amountBDesired,
    uint amountAMin,
    uint amountBMin
) internal virtual returns (uint amountA, uint amountB) {
    // create the pair if it doesn't exist yet
    if (IUniswapV2Factory(factory).getPair(tokenA, tokenB) == address(0)) {
        IUniswapV2Factory(factory).createPair(tokenA, tokenB);
    }
    (uint reserveA, uint reserveB) = UniswapV2Library.getReserves(factory, tokenA, tokenB);
    if (reserveA == 0 && reserveB == 0) {
        (amountA, amountB) = (amountADesired, amountBDesired);
    } else {
        uint amountBOptimal = UniswapV2Library.quote(amountADesired, reserveA, reserveB);
        if (amountBOptimal <= amountBDesired) {
            require(amountBOptimal >= amountBMin, 'UniswapV2Router: INSUFFICIENT_B_AMOUNT');
            (amountA, amountB) = (amountADesired, amountBOptimal);
        } else {
            uint amountAOptimal = UniswapV2Library.quote(amountBDesired, reserveB, reserveA);
            assert(amountAOptimal <= amountADesired);
            require(amountAOptimal >= amountAMin, 'UniswapV2Router: INSUFFICIENT_A_AMOUNT');
            (amountA, amountB) = (amountAOptimal, amountBDesired);
        }
    }
}

function addLiquidity(
    address tokenA,
    address tokenB,
    uint amountADesired,
    uint amountBDesired,
    uint amountAMin,
    uint amountBMin,
    address to,
    uint deadline
) external virtual override ensure(deadline) returns (uint amountA, uint amountB, uint liquidity) {
    (amountA, amountB) = _addLiquidity(tokenA, tokenB, amountADesired, amountBDesired, amountAMin, amountBMin);
    address pair = UniswapV2Library.pairFor(factory, tokenA, tokenB);
    TransferHelper.safeTransferFrom(tokenA, msg.sender, pair, amountA);
    TransferHelper.safeTransferFrom(tokenB, msg.sender, pair, amountB);
    liquidity = IUniswapV2Pair(pair).mint(to);
}

The point is the deadline check

modifier ensure(uint deadline) {
    require(deadline >= block.timestamp, 'UniswapV2Router: EXPIRED');
    _;
}

The deadline check ensure that the transaction can be executed on time and the expired transaction revert.

Impact

The transaction can be pending in mempool for a long and the trading activity is very time sensitive. Without deadline check, the trade transaction can be executed in a long time after the user submit the transaction, at that time, the trade can be done in a sub-optimal price, which harms user's position.

The deadline check ensure that the transaction can be executed on time and the expired transaction revert.

Tools Used

Manual review

Recommended Mitigation Steps

Consider adding deadline check like in the functions like withdraw and deposit.

Assessed type

Other