Revert Lend - Aymen0909's results

Lines of code

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

Vulnerability details

Issue Description

The onERC721Received function is invoked whenever the vault contract receives a Uniswap V3 position ERC721 token. This can happen either when an owner creates a new position or when a transformation occurs.

For this issue, we'll focus on the second case, specifically when a position is going through a transformation, which creates a new position token. In such a case, we have tokenId != oldTokenId, and the else block is run, as shown below:

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

    if {
        ...
    } else {
        uint256 oldTokenId = transformedTokenId;

        // if in transform mode - and a new position is sent - current position is replaced and returned
        if (tokenId != oldTokenId) {
            address owner = tokenOwner[oldTokenId];

            // set transformed token to new one
            transformedTokenId = tokenId;

            // copy debt to new token
            loans[tokenId] = Loan(loans[oldTokenId].debtShares);

            _addTokenToOwner(owner, tokenId);
            emit Add(tokenId, owner, oldTokenId);

            // clears data of old loan
            _cleanupLoan(oldTokenId, debtExchangeRateX96, lendExchangeRateX96, owner);

            //@audit can reenter with onERC721Received and call repay or borrow to call _updateAndCheckCollateral twice and manipulate collateral token configs

            // sets data of new loan
            _updateAndCheckCollateral(
                tokenId, debtExchangeRateX96, lendExchangeRateX96, 0, loans[tokenId].debtShares
            );
        }
    }

    return IERC721Receiver.onERC721Received.selector;
}

We should note that the _cleanupLoan function does return the old position token to the 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);
}

The issue that can occur is that the _cleanupLoan is invoked before the _updateAndCheckCollateral call. So, a malicious owner can use the onERC721Received callback when receiving the old token to call the borrow function, which makes changes to loans[tokenId].debtShares and calls _updateAndCheckCollateral. When the call resumes, the V3Vault.onERC721Received function will call _updateAndCheckCollateral again, resulting in incorrect accounting of internal token configs debt shares (tokenConfigs[token0].totalDebtShares & tokenConfigs[token1].totalDebtShares) and potentially impacting the vault borrowing process negatively.

Proof of Concept

Let's use the following scenario to demonstrate the issue:

Before starting, we suppose the following states:

• tokenConfigs[token0].totalDebtShares = 10000
• tokenConfigs[token1].totalDebtShares = 15000

1. Bob has previously deposited a UniswapV3 position (which uses token0 and token1) with tokenId = 12 and borrowed loans[tokenId = 12].debtShares = 1000 debt shares.

2. Bob calls the transform function to change the range of his position using the AutoRange transformer, which mints a new ERC721 token tokenId = 20 for the newly arranged position and sends it to the vault.

3. Upon receiving the new token, the V3Vault.onERC721Received function is triggered. As we're in transformation mode and the token ID is different, the second else block above will be executed.

4. V3Vault.onERC721Received will copy loan debt shares to the new token, so we'll have loans[tokenId = 20].debtShares = 1000.

5. Then V3Vault.onERC721Received will invoke the _cleanupLoan function to clear the data of the old loan and transfer the old position token tokenId = 12 back to Bob.

   5.1. _cleanupLoan will also call _updateAndCheckCollateral function to change oldShares = 1000 --> newShares = 0 (remove old token shares), resulting in:

   • tokenConfigs[token0].totalDebtShares = 10000 - 1000 = 9000
   • tokenConfigs[token1].totalDebtShares = 15000 - 1000 = 14000

6. Bob, upon receiving the old position token, will also use the ERC721 onERC721Received callback to call the borrow function. He will borrow 200 debt shares against his new position token tokenId = 20.

   6.1. The borrow function will update the token debt shares from loans[tokenId = 20].debtShares = 1000 to: loans[tokenId = 20].debtShares = 1000 + 200 = 1200 (assuming the position is healthy).

   6.2. The borrow function will also invoke the _updateAndCheckCollateral function to change oldShares = 1000 --> newShares = 1200 for tokenId = 20, resulting in:

   • tokenConfigs[token0].totalDebtShares = 9000 + 200 = 9200
   • tokenConfigs[token1].totalDebtShares = 14000 + 200 = 14200

7. Bob's borrow call ends, and the V3Vault.onERC721Received call resumes. _updateAndCheckCollateral gets called again, changing oldShares = 0 --> newShares = 1200 (as the borrow call changed the token debt shares), resulting in:

   • tokenConfigs[token0].totalDebtShares = 9200 + 1200 = 10400
   • tokenConfigs[token1].totalDebtShares = 14200 + 1200 = 15400

Now, let's assess what Bob managed to achieve by taking a normal/honest transformation process (without using the onERC721Received callback) and then a borrow operation scenario:

• Normally, when V3Vault.onERC721Received is called, it shouldn't change the internal token configs debt shares (tokenConfigs[token0].totalDebtShares & tokenConfigs[token1].totalDebtShares). After a normal V3Vault.onERC721Received, we should still have:

  • tokenConfigs[token0].totalDebtShares = 10000
  • tokenConfigs[token1].totalDebtShares = 15000

• Then, when Bob borrows 200 debt shares against the new token, we should get:

  • tokenConfigs[token0].totalDebtShares = 10000 + 200 = 10200
  • tokenConfigs[token1].totalDebtShares = 15000 + 200 = 15200

We observe that by using the onERC721Received callback, Bob managed to increase the internal token configs debt shares (tokenConfigs[token0].totalDebtShares & tokenConfigs[token1].totalDebtShares) by 200 debt shares more than expected.

This means that Bob, by using this attack, has manipulated the internal token configs debt shares, making the vault believe it has 200 additional debt shares. Bob can repeat this attack multiple times until he approaches the limit represented by collateralValueLimitFactorX32 and collateralValueLimitFactorX32 multiplied by the amount of asset lent as shown below:

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

Then, when other borrowers try to call the borrow function, it will revert because _updateAndCheckCollateral will trigger the CollateralValueLimit error, thinking there is too much debt already. However, this is not the case, as the internal token configs debt shares have been manipulated (increased) by an attacker (Bob).

This attack is irreversible because there is no way to correct the internal token configs debt shares (tokenConfigs[token0].totalDebtShares & tokenConfigs[token1].totalDebtShares), and the vault will remain in that state, not allowing users to borrow, resulting in no interest being accrued and leading to financial losses for the lenders and the protocol.

Impact

A malicious attacker could use the AutoRange transformation process to manipulate the internal token configs debt shares, potentially resulting in:

• Fewer loans being allowed by the vault than expected.
• A complete denial-of-service (DOS) for all borrow operations.

Tools Used

Manual review, VS Code

Recommended Mitigation

The simplest way to address this issue is to ensure that the onERC721Received function follows the Correctness by Construction (CEI) pattern, as follows:

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

    if {
        ...
    } else {
        uint256 oldTokenId = transformedTokenId;

        // if in transform mode - and a new position is sent - current position is replaced and returned
        if (tokenId != oldTokenId) {
            address owner = tokenOwner[oldTokenId];

            // set transformed token to new one
            transformedTokenId = tokenId;

            // copy debt to new token
            loans[tokenId] = Loan(loans[oldTokenId].debtShares);

            _addTokenToOwner(owner, tokenId);
            emit Add(tokenId, owner, oldTokenId);

--          // clears data of old loan
--          _cleanupLoan(oldTokenId, debtExchangeRateX96, lendExchangeRateX96, owner);

            // sets data of new loan
            _updateAndCheckCollateral(
                tokenId, debtExchangeRateX96, lendExchangeRateX96, 0, loans[tokenId].debtShares
            );

++          // clears data of old loan
++          _cleanupLoan(oldTokenId, debtExchangeRateX96, lendExchangeRateX96, owner);
        }
    }

    return IERC721Receiver.onERC721Received.selector;
}

Assessed type

Context

Lines of code

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

Vulnerability details

Issue Description

The vault applies the maximum daily limit for lending and borrowing as an emergency backstop in case of a massive exploit in the protocol, as stated in the whitepaper. The role of these limits is to ensure that the daily amount lent or borrowed does not exceed a certain percentage of the total assets lent to the vault previously (currently set at 10%).

The two variables tracking these limits are dailyLendIncreaseLimitLeft and dailyDebtIncreaseLimitLeft, which are updated on a daily basis by the two functions _resetDailyLendIncreaseLimit and _resetDailyDebtIncreaseLimit, respectively.

function _resetDailyLendIncreaseLimit(
    uint256 newLendExchangeRateX96,
    bool force
) internal {
    // daily lend limit reset handling
    uint256 time = block.timestamp / 1 days;
    if (force || time > dailyLendIncreaseLimitLastReset) {
        //@audit lendIncreaseLimit is wrong
        uint256 lendIncreaseLimit = (_convertToAssets(
            totalSupply(),
            newLendExchangeRateX96,
            Math.Rounding.Up
        ) * (Q32 + MAX_DAILY_LEND_INCREASE_X32)) / Q32;
        dailyLendIncreaseLimitLeft = dailyLendIncreaseLimitMin >
            lendIncreaseLimit
            ? dailyLendIncreaseLimitMin
            : lendIncreaseLimit;
        dailyLendIncreaseLimitLastReset = time;
    }
}

function _resetDailyDebtIncreaseLimit(
    uint256 newLendExchangeRateX96,
    bool force
) internal {
    // daily debt limit reset handling
    uint256 time = block.timestamp / 1 days;
    if (force || time > dailyDebtIncreaseLimitLastReset) {
        //@audit debtIncreaseLimit is wrong
        uint256 debtIncreaseLimit = (_convertToAssets(
            totalSupply(),
            newLendExchangeRateX96,
            Math.Rounding.Up
        ) * (Q32 + MAX_DAILY_DEBT_INCREASE_X32)) / Q32;
        dailyDebtIncreaseLimitLeft = dailyDebtIncreaseLimitMin >
            debtIncreaseLimit
            ? dailyDebtIncreaseLimitMin
            : debtIncreaseLimit;
        dailyDebtIncreaseLimitLastReset = time;
    }
}

The issue, however, is that both _resetDailyLendIncreaseLimit and _resetDailyDebtIncreaseLimit calculate the daily increase limit incorrectly. They both use the following formula (we will demonstrate for _resetDailyLendIncreaseLimit, but the same issue holds for _resetDailyDebtIncreaseLimit):

uint256 lendIncreaseLimit = (_convertToAssets(
            totalSupply(),
            newLendExchangeRateX96,
            Math.Rounding.Up
        ) * (Q32 + MAX_DAILY_LEND_INCREASE_X32)) / Q32;
dailyLendIncreaseLimitLeft = dailyLendIncreaseLimitMin >
            lendIncreaseLimit
            ? dailyLendIncreaseLimitMin
            : lendIncreaseLimit;

As shown in the code above instead of taking just a percentage of the total assets lent to the vault (MAX_DAILY_DEBT_INCREASE_X32 <=> 10%), the function takes the full amount of assets lent plus that percentage (lendIncreaseLimit). Then, it compares it with the minimal daily increase limit (dailyLendIncreaseLimitMin) and takes the maximum of both, max(lendIncreaseLimit, dailyLendIncreaseLimitMin).

This logic implies that the daily limit will never be applied, as lendIncreaseLimit will always be bigger than dailyLendIncreaseLimitMin, and it will, of course, be bigger than what can be deposited in one day because lendIncreaseLimit represents the total asset amount lent in the vault from its creation, and it's also incremented by 10% of that total amount.

So the _resetDailyLendIncreaseLimit function will always set dailyLendIncreaseLimitLeft to the lendIncreaseLimit value, which is far bigger than the 10% intended by the protocol, and thus users will have no daily lending limit imposed, and the protocol won't have its expected emergency backstop.

The same issue is present when calculating the daily debt limit in the _resetDailyDebtIncreaseLimit function.

It should be noted that the only times when those limits would work as expected are at the beginning of the vault, as the total asset amount lent will still be less than dailyLendIncreaseLimitMin, and thus this would be the daily limit.

Proof of Concept

Let's use the following scenario to demonstrate the issue:

• Assume the following state in the vault:

  totalAssetLent = _convertToAssets(totalSupply(), newLendExchangeRateX96, Math.Rounding.Up) = 10000 (variable created for simplicity to represent the total asset amount lent)

  dailyLendIncreaseLimitMin = 500

• When _resetDailyLendIncreaseLimit is called, it will calculate the following:

  lendIncreaseLimit = totalAssetLent * (Q32 + MAX_DAILY_LEND_INCREASE_X32) / Q32

  As we know MAX_DAILY_LEND_INCREASE_X32 ~ 10%, we'll get:

  lendIncreaseLimit = 11000

  dailyLendIncreaseLimitLeft = max(dailyLendIncreaseLimitMin, lendIncreaseLimit) = max(500, 11000) = 11000

• So, this gives us a daily limit of 11000, which is bigger than the actual total asset lent in the vault, and thus the daily limit won't really work as expected.

• If the calculation were done correctly, we should have had:

  lendIncreaseLimit = (totalAssetLent * MAX_DAILY_LEND_INCREASE_X32) / Q32 = 1000

  dailyLendIncreaseLimitLeft = max(500, 1000) = 1000

  Which represents the 10% daily limit that was actually intended.

Impact

Incorrect daily lend and debt calculation in both _resetDailyLendIncreaseLimit and _resetDailyDebtIncreaseLimit functions will allow users to lend and borrow much more than intended by the protocol and will disable the daily limit safety mechanism.

Tools Used

Manual review, VS Code

Recommended Mitigation

To address this issue, both _resetDailyLendIncreaseLimit and _resetDailyDebtIncreaseLimit functions must calculate the daily increase limit correctly, which should be a percentage of the total assets lent to the vault. The following modifications must be implemented:

function _resetDailyLendIncreaseLimit(
    uint256 newLendExchangeRateX96,
    bool force
) internal {
    // daily lend limit reset handling
    uint256 time = block.timestamp / 1 days;
    if (force || time > dailyLendIncreaseLimitLastReset) {
        uint256 lendIncreaseLimit = (_convertToAssets(
            totalSupply(),
            newLendExchangeRateX96,
            Math.Rounding.Up
--      ) * (Q32 + MAX_DAILY_LEND_INCREASE_X32)) / Q32;
++      ) * (MAX_DAILY_LEND_INCREASE_X32)) / Q32;
        dailyLendIncreaseLimitLeft = dailyLendIncreaseLimitMin >
            lendIncreaseLimit
            ? dailyLendIncreaseLimitMin
            : lendIncreaseLimit;
        dailyLendIncreaseLimitLastReset = time;
    }
}

function _resetDailyDebtIncreaseLimit(
    uint256 newLendExchangeRateX96,
    bool force
) internal {
    // daily debt limit reset handling
    uint256 time = block.timestamp / 1 days;
    if (force || time > dailyDebtIncreaseLimitLastReset) {
        uint256 debtIncreaseLimit = (_convertToAssets(
            totalSupply(),
            newLendExchangeRateX96,
            Math.Rounding.Up
--      ) * (Q32 + MAX_DAILY_DEBT_INCREASE_X32)) / Q32;
++      ) * (MAX_DAILY_DEBT_INCREASE_X32)) / Q32;
        dailyDebtIncreaseLimitLeft = dailyDebtIncreaseLimitMin >
            debtIncreaseLimit
            ? dailyDebtIncreaseLimitMin
            : debtIncreaseLimit;
        dailyDebtIncreaseLimitLastReset = time;
    }
}

Assessed type

Error

Lines of code

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

Vulnerability details

Issue Description

The _deposit function is invoked in both the deposit and mint functions when a user wants to lend assets. This function is intended to ensure that the total amount of assets lent does not exceed the protocol limit globalLendLimit.

function _deposit(
    address receiver,
    uint256 amount,
    bool isShare,
    bytes memory permitData
) internal returns (uint256 assets, uint256 shares) {
    ...

    _mint(receiver, shares);

    //@audit must convert totalSupply() to assets before comparing with globalLendLimit
    if (totalSupply() > globalLendLimit) {
        revert GlobalLendLimit();
    }

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

In the provided code snippet, the _deposit function checks the totalSupply() against the globalLendLimit limit. However, totalSupply() represents the lenders' share amount and does not represent the actual asset amount lent. It must first be converted to assets using the _convertToAssets function.

This mistake is evident because in the maxDeposit function, the correct check is implemented:

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

Because the _deposit function performs the wrong check, it will allow more assets to be lent in the protocol. This is due to the fact that the lending exchange rate lastLendExchangeRateX96 will be greater than 1 (due to interest accrual), and so we will always have totalSupply() < _convertToAssets(totalSupply(), lendExchangeRateX96, Math.Rounding.Up) (the only case this might not hold is when there is a significant bad debt after liquidation, which would not occur under normal circumstances).

Impact

Incorrect global lending checking in the _deposit function will result in more assets being lent than allowed by the protocol.

Tools Used

Manual review, VS Code

Recommended Mitigation

To address this issue, the totalSupply() must be converted to an asset amount before checking it against globalLendLimit, as is done in the maxDeposit function:

function _deposit(
    address receiver,
    uint256 amount,
    bool isShare,
    bytes memory permitData
) internal returns (uint256 assets, uint256 shares) {
    ...

    _mint(receiver, shares);

++  //@audit must convert totalSupply() to assets before comparing with globalLendLimit
++  uint256 value = _convertToAssets(totalSupply(), newLendExchangeRateX96, Math.Rounding.Up);
++  if (value > globalLendLimit) {
--  if (totalSupply() > globalLendLimit) {
        revert GlobalLendLimit();
    }

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

Assessed type

Error

Lines of code

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

Vulnerability details

Issue Description

The V3Vault contract is intended to adhere to the standards outlined in EIP4626 regarding the tokenized token architecture. However, it has been found that V3Vault deviates from the directives specified in EIP4626 in the following ways:

1. The maxWithdraw & maxRedeem functions returns the withdrawal amount (or redeemable share) based uniquely on the user shares balance balanceOf(owner) (converted to asset in maxWithdraw):

function maxWithdraw(
    address owner
) external view override returns (uint256) {
    (, uint256 lendExchangeRateX96) = _calculateGlobalInterest();
    return
        _convertToAssets(
            balanceOf(owner),
            lendExchangeRateX96,
            Math.Rounding.Down
        );
}

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

And both functions don't take into account the available asset limit imposed in the _withdraw function (which is the internal function invoked by withdraw & redeem):

function _withdraw(
    address receiver,
    address owner,
    uint256 amount,
    bool isShare
) internal returns (uint256 assets, uint256 shares) {
    ...

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

    ...
}

According to EIP4626, the maxWithdraw & maxRedeem functions must factor in both global and user-specific limits, and in our case the available asset limit mentioned above is a global limit imposed by the vault on all withdrawals and thus it should be taken into account in maxWithdraw & maxRedeem.

2. In a similar way, the maxDeposit & maxMint functions compute the the amount of asset that can be deposited (or shares minted) and both functions ensure that the globalLendLimit limit is respected:

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

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

But when depositing asset or minting shares, there is another deposit limit imposed by the vault in the _deposit function (which is the internal function invoked by deposit & mint functions), it's the daily lend limit represented by dailyLendIncreaseLimitLeft:

function _deposit(
        address receiver,
        uint256 amount,
        bool isShare,
        bytes memory permitData
    ) internal returns (uint256 assets, uint256 shares) {
        ...

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

        ...
    }

According to EIP4626, the maxDeposit & maxMint functions must also factor in both global and user-specific limits, and in this case the daily asset lending limit dailyLendIncreaseLimitLeft is a global limit imposed by the vault on all deposits (or mints).

Due to both aforementioned issues, V3Vault contract fail to fully comply with EIP4626 standards which may pose difficulties or even render it impossible for external protocols to build on top of it or integrate it into their business logic.

As for example an external protocol contract, could in its internal logic call maxWithdraw to get his max withdrawal amount but when he tries to withdraw that amount the withdraw function might revert due to insufficient balance available. And the same issue can occur in the deposit/mint case.

Impact

The inability of external protocols to integrate with the V3Vault contract due to its lack of full compliance with EIP4626 poses a potential hindrance to its adoption and interoperability.

Tools Used

Manual review, VS Code

Recommended Mitigation

A straightforward solution to address the aforementioned issues is as follows:

1. Modify the maxWithdraw & maxRedeem function to check against the vault true available balance.

2. In both maxDeposit & maxMint functions, check that the deposited asset amount will be within the daily asset lending limit dailyLendIncreaseLimitLeft.

Assessed type

ERC4626

Lines of code

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

Vulnerability details

Issue Description

When a borrower debt increase above his position collateral value he becomes liquidatable, and any user can call the liquidate function to liquidate it, to determine if a loan is healthy or not the _checkLoanIsHealthy function is used.

In the liquidate function, the liquidator must provide LiquidateParams as input which will include the liquidated position debt shares LiquidateParams.debtShares and the function will check this value against the current position debt value loans[params.tokenId].debtShares and will revert if those two values are different as shown below:

function liquidate(
    LiquidateParams calldata params
) external override returns (uint256 amount0, uint256 amount1) {
    // liquidation is not allowed during transformer mode
    if (transformedTokenId > 0) {
        revert TransformNotAllowed();
    }

    LiquidateState memory state;

    (
        state.newDebtExchangeRateX96,
        state.newLendExchangeRateX96
    ) = _updateGlobalInterest();

    uint256 debtShares = loans[params.tokenId].debtShares;
    //@audit could repay small amounts to stop liquidation
    if (debtShares != params.debtShares) {
        revert DebtChanged();
    }

    ...
}

A borrower can exploit this debt change check to avoid getting liquidated. When the borrower sees that their position has become liquidatable, they can front-run the liquidation call and make a call to the repay function to repay a very small amount of their debt (could be as small as 1 wei) and because the repay function doesn't include any position healthiness check it will allow the borrower call and will decrease the current position debt value loans[tokenId].debtShares by that small amount.

When the liquidator's call goes through, it will revert because the debt has changed, the borrower can repeat this process as many times as the he wants in order to keep his unhealthy position and avoid liquidation.

This issue can result in the accrual of bad debt in the protocol, as a user can practically avoid the liquidation of their unhealthy position forever at no cost (repaying each time a small amount of asset).

Impact

Users can avoid liquidation by triggering the debt change check in the liquidate function, resulting in the accrual of bad debt in the protocol and potentially causing a loss of funds for all the lenders and the protocol.

Proof of Concept

Let's use the following scenario to demonstrate the issue:

• Bob has previously deposited UniswapV3 position tokenId = 12 and borrowed 1000e18 asset against it which generated loans[tokenId].debtShares = 1200e18 debt shares.

• After a while Bob position collateral value dropped below his debt value and thus his position is now liquidatable.

• Alice notices that and calls liquidate function to liquidate Bob with providing LiquidateParams.tokenId = 12 and LiquidateParams.debtShares = 1200e18.

• Bob front-runs Alice's call and calls repay function, repaying a small amount the 1 wei of asset. This activates will change has debt shares to loans[tokenId].debtShares = 1199999999999999999999.

• Alice's liquidation call goes through now, but it reverts because Bob debt shares have changed.

• Bob can repeat this process many times, and he is able to successfully avoid liquidation by repaying only a very small amount of asset.

Tools Used

Manual review, VS Code

Recommended Mitigation

To solve this issue, you should remove the debt changed check as the liquidator provided debt shares amount is not included later on in the calculation of the liquidation cost.

Assessed type

Context

Lines of code

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

Vulnerability details

Issue Description

The protocol uses a maximum daily debt increase limit (MDDI) represented by the dailyDebtIncreaseLimitLeft state to control daily debt variations. Whenever a new loan is created, this state is increased by the corresponding loan amount. Similarly, when someone repays their loan, it decreases by the corresponding repaid amount to allow new loans within a 24-hour period.

However, there is an instance where this limit is not applied: in the liquidate function. The liquidation process allows a liquidator to repay an unhealthy borrower's loan and receive a liquidation bonus from the user's collateral.

function liquidate(
    LiquidateParams calldata params
) external override returns (uint256 amount0, uint256 amount1) {
    ...

    debtSharesTotal -= debtShares;

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

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

    address owner = tokenOwner[params.tokenId];

    //@audit didn't increase dailyDebtIncreaseLimitLeft

    // disarm loan and send remaining position to owner
    _cleanupLoan(
        params.tokenId,
        state.newDebtExchangeRateX96,
        state.newLendExchangeRateX96,
        owner
    );

    ...
}

Because liquidation represents a decrease in debt shares, it should trigger an increase in the dailyDebtIncreaseLimitLeft state, as is the case with a normal repayment call. However, the liquidate function doesn't update dailyDebtIncreaseLimitLeft. Consequently, even though loans are being repaid through liquidation, it doesn't allow more loans to be taken on that day.

This issue is significant because in major DeFi protocols that rely on collateralized debt positions (CDPs), liquidations occur frequently, especially if the protocol has a large total value locked (TVL). The failure to increase dailyDebtIncreaseLimitLeft after liquidation will prevent the creation of additional loans, resulting in less interest being accrued daily and eventually leading to financial losses for lenders and the protocol.

Note: Although the impact of this issue may seem small, as it only prevents some loans on a daily basis, the cumulative effect over time can result in substantial financial losses for both lenders and the protocol.

Impact

The vault will see a reduction in interest accrued for lenders because the liquidate function fails to increase dailyDebtIncreaseLimitLeft, thereby preventing the creation of many loans.

Tools Used

Manual review, VS Code

Recommended Mitigation

To address this issue, the liquidate function must increase the maximum daily debt increase limit dailyDebtIncreaseLimitLeft as follows:

function liquidate(
    LiquidateParams calldata params
) external override returns (uint256 amount0, uint256 amount1) {
    ...

    debtSharesTotal -= debtShares;

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

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

    address owner = tokenOwner[params.tokenId];

++  // when amounts are repaid - they may be borrowed again
++  dailyDebtIncreaseLimitLeft += state.liquidatorCost;

    // disarm loan and send remaining position to owner
    _cleanupLoan(
        params.tokenId,
        state.newDebtExchangeRateX96,
        state.newLendExchangeRateX96,
        owner
    );

    ...
}

Assessed type

Context

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L360-L393 https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L881-L891 https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L924-L932

Vulnerability details

Issue Description

In order to deposit assets in V3Vault or to withdraw previously lent assets one of the following functions must be called: deposit, mint, withdraw and redeem.

In all this functions the exchange lending rate lastLendExchangeRateX96 in used to calculated the assets/shares amount that must be given to the users, this is visible in both _deposit and _withdraw functions (which are internally invoked by all the aforementioned functions):

function _deposit(
    address receiver,
    uint256 amount,
    bool isShare,
    bytes memory permitData
) internal returns (uint256 assets, uint256 shares) {
    ...

    (, uint256 newLendExchangeRateX96) = _updateGlobalInterest();

    _resetDailyLendIncreaseLimit(newLendExchangeRateX96, false);

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

    ...
}

function _withdraw(
    address receiver,
    address owner,
    uint256 amount,
    bool isShare
) internal returns (uint256 assets, uint256 shares) {
    ...

    (
        uint256 newDebtExchangeRateX96,
        uint256 newLendExchangeRateX96
    ) = _updateGlobalInterest();

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

    ...
}

This lending rate lastLendExchangeRateX96 is variable, it will increase on a second basis while the interest in accrued and may decrease if there is an underwater liquidation (bad debt socialized on all lenders which's handled by _handleReserveLiquidation), because the lending rate is variable it means that when a user submit a tx either by calling deposit or mint to lend assets or withdraw, redeem to withdraw lent asset, he might experience a slippage when the tx is pending in the mempool (as the protocol will be deployed on any EVM) and might receive less assets or shares from the vault.

To illustrate this issue, consider the following scenarios:

- Scenario 1:

1- In the vault, we have the lending rate lastLendExchangeRateX96 = 1.2 * Q96

2- Bob wants to deposit 1000 asset tokens and he's expecting to get approximately 830 shares back from the vault.

3- Bob calls the deposit function.

4- Bob deposit tx remains in the mempool pending execution for some time, and in that time the lending rate did increase lastLendExchangeRateX96 = 1.28 * Q96.

5- When Bob tx goes through he gets minted (1000 * Q96)/(1.28 * Q96) = 781.25 shares which muck less than the ~830 shares that Bob was expecting

So Bob did experience a slippage which make him lose ~50 shares, and the same issue would happen if Bob did call the mint function instead.

- Scenario 2:

1- In the vault, we have the lending rate lastLendExchangeRateX96 = 1.2 * Q96

2- Bob wants to withdraw 1000 asset tokens and he's expecting to burn approximately 830 shares from his balance.

3- Bob calls the withdraw function.

4- While Bob deposit tx remains in the mempool pending execution, a liquidation happens and it's an underwater liquidation and as reserve couldn't cover it fully, a bad debt was generated and lending rate did decrease lastLendExchangeRateX96 = 1.1 * Q96.

5- When Bob tx goes through he will get (1000 * Q96)/(1.1 * Q96) = 909 shares burned from his balance in order to receive his 1000 asset tokens which more that Bob was expecting to burn

Bob did again experience a slippage which make him spend ~70 shares more that what he was intending to withdraw 1000 assets.

In both scenarios, Bob did lose funds due to a change in the lending rate lastLendExchangeRateX96 while his tx was pending in the mempool. And because it's stated in the contest ReadMe that the protocol is expected to be deployed on any EVM compatible chain like Ethereum, Polygon which use tx mempool, this kind of issue might happen frequently to many users.

Impact

The deposit, mint, withdraw and redeem functions lack slippage protection, which can lead to users losing funds when trying to lend or withdraw asset from the V3Vault, and this due to the changes in the the lending rate lastLendExchangeRateX96 after users submit their transactions to the mempool.

Tools Used

Manual review, VS Code

Recommended Mitigation

The deposit, mint, withdraw and redeem functions should include slippage protection parameters provided by the users (either minimum amount received for deposit & redeem function or a maximum amount (shares) used in mint, withdraw).

Assessed type

Context