V3Vault is not ERC-4626 compliant

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L301-L309 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#L323-L326 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L329-L331

Vulnerability details

Impact

Protocols that try to integrate with Revert Land, expecting V3Vault to be ERC-4626 compliant, will multiple issues that may damage Revert Land's brand and limit Revert Land's growth in the market.

Proof of Concept

All official ERC-4626 requirements are on their official page. Non-compliant methods are listed below along with why they are not compliant and coded POCs demonstrating the issues.

V3Vault::maxDeposit and V3Vault::maxMint

As specified in ERC-4626, both maxDeposit and maxMint must return the maximum amount that would allow to be used and not cause a revert. Also, if the deposits or mints are disabled at current moment, it MUST return 0.

ERC4626::maxDeposit Non-compliant Requirements

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).

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

ERC4626::maxMint Non-compliant Requirements

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).

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

However, in both V3Vault::maxDeposit or V3Vault::maxMint return the amount that cause a revert in deposit or mint. This is because they do not check if the amount exceeded the daily lend limit and if this is a case, it will cause a revert inside _deposit function (where it used in both deposit and mint):

src/V3Vault.sol:
915:         if (assets > dailyLendIncreaseLimitLeft) {
916:             revert DailyLendIncreaseLimit();
917:         } else {

Furthermore, when dailyLendIncreaseLimitLeft == 0 that means the deposits and mints are temporarily disabled, while both V3Vault::maxDeposit and V3Vault::maxMint could return amounts that is more than 0. Based on ERC4626 requirements, it MUST return 0 in this case.

Test Case (Foundry)

To run the POC, copy-paste it into V3Vault.t.sol:

    function testPOC_MaxDepositDoesNotReturnZeroWhenExceedsDailyLimit() public {
        uint256 dailyLendIncreaseLimitMin = vault.dailyLendIncreaseLimitMin();
        uint256 depositAmount = dailyLendIncreaseLimitMin;

        vm.startPrank(WHALE_ACCOUNT);
        USDC.approve(address(vault), depositAmount);
        vault.deposit(depositAmount, WHALE_ACCOUNT);

        //Should return 0 to comply to ERC-4626.
        assertNotEq(vault.maxDeposit(address(WHALE_ACCOUNT)), 0);

        USDC.approve(address(vault), 1);
        vm.expectRevert(IErrors.DailyLendIncreaseLimit.selector);
        vault.deposit(1, WHALE_ACCOUNT);

        vm.stopPrank();
    }

V3Vault::maxWithdraw and V3Vault::maxRedeem

As specified in ERC-4626, both maxWithdraw and maxRedeem must return the maximum amount that would allow to be  transferred from owner and not cause a revert. Also, if the withdrawals or redemption are disabled at current moment, it MUST return 0.

ERC4626::maxWithdraw Non-compliant Requirements

MUST return the maximum amount of assets that could be transferred from owner through withdraw and not cause a revert, which MUST NOT be higher than the actual maximum that would be accepted (it should underestimate if necessary).

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

ERC4626::maxRedeem Non-compliant Requirements

MUST return the maximum amount of shares that could be transferred from owner through redeem and not cause a revert, which MUST NOT be higher than the actual maximum that would be accepted (it should underestimate if necessary).

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

However, in both V3Vault::maxWithdraw or V3Vault::maxRedeem return the amount that cause a revert in withdraw or redeem. This is because they do not check if the amount exceeded the current available balance in the vault and if this is a case, it will cause a revert inside _withdraw function (where it used in both withdraw and redeem):

src/V3Vault.sol:
962:         (, uint256 available,) = _getAvailableBalance(newDebtExchangeRateX96, newLendExchangeRateX96);
963:         if (available < assets) {
964:             revert InsufficientLiquidity();
965:         }

Test Case (Foundry)

To run the POC, copy-paste it into V3Vault.t.sol:

    function testPOC_MaxWithdrawDoesNotReturnZeroWhenExceedsAvailableBalance() external {
        // maximized collateral loan
        _setupBasicLoan(true);

        uint256 amount = vault.maxRedeem(address(WHALE_ACCOUNT));

        (,,, uint256 available,,,) = vault.vaultInfo();

        //Should return available balance if it is less than owner balance to comply to ERC-4626.
        assertNotEq(vault.maxRedeem(address(WHALE_ACCOUNT)), available);

        vm.expectRevert(IErrors.InsufficientLiquidity.selector);
        vm.prank(WHALE_ACCOUNT);
        vault.redeem(amount, WHALE_ACCOUNT, WHALE_ACCOUNT);
    }

Tools Used

Manual review.

Recommended Mitigation Steps

To address the non-compliance issues, the following changes are recommended:

diff --git a/src/V3Vault.sol b/src/V3Vault.sol
index 64141ec..a25cebd 100644
--- a/src/V3Vault.sol
+++ b/src/V3Vault.sol
@@ -304,7 +304,12 @@ contract V3Vault is ERC20, Multicall, Ownable, IVault, IERC721Receiver, IErrors
         if (value >= globalLendLimit) {
             return 0;
         } else {
-            return globalLendLimit - value;
+            uint256 maxGlobalDeposit = globalLendLimit - value;
+            if (maxGlobalDeposit > dailyLendIncreaseLimitLeft) {
+                return dailyLendIncreaseLimitLeft;
+            } else {
+                return maxGlobalDeposit;
+            }
         }
     }

@@ -315,19 +320,37 @@ contract V3Vault is ERC20, Multicall, Ownable, IVault, IERC721Receiver, IErrors
         if (value >= globalLendLimit) {
             return 0;
         } else {
-            return _convertToShares(globalLendLimit - value, lendExchangeRateX96, Math.Rounding.Down);
+            uint256 maxGlobalDeposit = globalLendLimit - value;
+            if (maxGlobalDeposit > dailyLendIncreaseLimitLeft) {
+                return _convertToShares(dailyLendIncreaseLimitLeft, lendExchangeRateX96, Math.Rounding.Down);
+            } else {
+                return _convertToShares(maxGlobalDeposit, lendExchangeRateX96, Math.Rounding.Down);
+            }
         }
     }

     /// @inheritdoc IERC4626
     function maxWithdraw(address owner) external view override returns (uint256) {
-        (, uint256 lendExchangeRateX96) = _calculateGlobalInterest();
-        return _convertToAssets(balanceOf(owner), lendExchangeRateX96, Math.Rounding.Down);
+        uint256 ownerBalance = balanceOf(owner);
+        (uint256 debtExchangeRateX96, uint256 lendExchangeRateX96) = _calculateGlobalInterest();
+        (, uint256 available, ) = _getAvailableBalance(debtExchangeRateX96, lendExchangeRateX96);
+        if (available > ownerBalance) {
+            return _convertToAssets(ownerBalance, lendExchangeRateX96, Math.Rounding.Down);
+        } else {
+            return _convertToAssets(available, lendExchangeRateX96, Math.Rounding.Down);
+        }
     }

     /// @inheritdoc IERC4626
     function maxRedeem(address owner) external view override returns (uint256) {
-        return balanceOf(owner);
+        uint256 ownerBalance = balanceOf(owner);
+        (uint256 debtExchangeRateX96, uint256 lendExchangeRateX96) = _calculateGlobalInterest();
+        (, uint256 available, ) = _getAvailableBalance(debtExchangeRateX96, lendExchangeRateX96);
+        if (available > ownerBalance) {
+            return ownerBalance;
+        } else {
+            return available;
+        }
     }

• The modified maxDeposit function now correctly calculates the maximum deposit amount by considering both the global lend limit and the daily lend increase limit. If the calculated maximum global deposit exceeds the daily lend increase limit, the function returns the daily lend increase limit to comply with ERC-4626 requirements.

• Similarly, the modified maxMint ensures compliance with ERC-4626 by calculating the maximum mints amount for a given owner. It considers both the global lend limit and the daily lend increase limit as mentioned in maxDeposit.

• The modified maxWithdraw function now correctly calculates the maximum withdrawal amount for a given owner. It ensures that the returned value does not exceed the available balance in the vault. If the available balance is greater than the owner's balance, it returns the owner's balance, otherwise it return the available balance to prevent potential reverts during withdrawal transactions. This adjustment aligns with ERC-4626 requirements by ensuring that withdrawals do not cause unexpected reverts and accurately reflect the available funds for withdrawal.

• Similarly, the modified maxRedeem function ensures compliance with ERC-4626 by calculating the maximum redemption amount for a given owner. It considers both the owner's balance and the available liquidity in the vault as mentioned in maxWithdraw.

Assessed type

Other

[c4-pre-sort]

0xEVom marked the issue as high quality report

[c4-pre-sort]

0xEVom marked the issue as primary issue

[c4-sponsor]

kalinbas (sponsor) confirmed

[c4-judge]

jhsagd76 marked the issue as satisfactory

[c4-judge]

jhsagd76 marked the issue as selected for report

[kalinbas]

https://github.com/revert-finance/lend/pull/15