Revert Lend - FastChecker's results

Lines of code

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

Vulnerability details

Impact

If Max Daily Debt Increase (MDDI) is set much larger, the protocol may suffer destructive loss.

Proof of Concept

V3Vault.sol#__resetDailyLendIncreaseLimit is as follows.

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

L1250 and L1251 shows uint256 lendIncreaseLimit = _convertToAssets(totalSupply(), newLendExchangeRateX96, Math.Rounding.Up) * (Q32 + MAX_DAILY_LEND_INCREASE_X32) / Q32. That is, lendIncreaseLimit is set to 110% of the total assets amount. However, the actual intended amount is 10%. Therefore, lendIncreaseLimit becomes 11 times larger than intended. A similar error exists in the debtIncreaseLimit calculation in the VeVault.sol#_resetDailyDebtIncreaseLimit function.

Tools Used

Manual Review

Recommended Mitigation Steps

VeVault.sol#_resetDailyLendIncreaseLimit function is modified as follows.

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

VeVault.sol#_resetDailyDebtIncreaseLimit function is modified as follows.

    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_LEND_INCREASE_X32 / Q32;
            dailyDebtIncreaseLimitLeft =
                dailyDebtIncreaseLimitMin > debtIncreaseLimit ? dailyDebtIncreaseLimitMin : debtIncreaseLimit;
            dailyDebtIncreaseLimitLastReset = time;
        }
    }

Assessed type

Other

Lines of code

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

Vulnerability details

Vulnerability Details

When the V3Vault.sol#_withdraw and V3Vault.sol#_repay functions are called, dailyLendIncreaseLimitLeft and dailyDebtIncreaseLimitLeft are increased. However, if it is called before _withdraw and _repay are called, this increase becomes meaningless.

Impact

Even if the V3Vault.sol#_withdraw and V3Vault.sol#_repay functions are called, dailyLendIncreaseLimitLeft and dailyDebtIncreaseLimitLeft do not increase, so the protocol does not work as intended.

Proof of Concept

V3Vault.sol#_withdraw is as follows.

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

        // if caller has allowance for owners shares - may call withdraw
        if (msg.sender != owner) {
            _spendAllowance(owner, msg.sender, shares);
        }

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

        // fails if not enough shares
        _burn(owner, shares);
        SafeERC20.safeTransfer(IERC20(asset), receiver, assets);

        // when amounts are withdrawn - they may be deposited again
949:    dailyLendIncreaseLimitLeft += assets;

        emit Withdraw(msg.sender, receiver, owner, assets, shares);
    }

As you can see, increase dailylendIncreaselimitLeft by the asset amount in L949. However V3Vault.sol#_deposit is as follows.

    function _deposit(address receiver, uint256 amount, bool isShare, bytes memory permitData)
        internal
        returns (uint256 assets, uint256 shares)
    {
        (, uint256 newLendExchangeRateX96) = _updateGlobalInterest();

883:    _resetDailyLendIncreaseLimit(newLendExchangeRateX96, false);

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

        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
            );
        } else {
            // fails if not enough token approved
            SafeERC20.safeTransferFrom(IERC20(asset), msg.sender, address(this), assets);
        }

        _mint(receiver, shares);

        if (totalSupply() > globalLendLimit) {
            revert GlobalLendLimit();
        }

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

        emit Deposit(msg.sender, receiver, assets, shares);
    }

As you can see on the right, the dailyLendIncreaseLimitLeft function is called in L883. V3Vault.sol#_resetDailyLendIncreaseLimit is as follows.

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

Looking at the function above, the increase of dailyLendIncreaseLimitLeft in the withdraw performed before depositing when a new day begins is not reflected in the dailyLendIncreaseLimitleft control by L1249. That is, the increase will not be reflected in the dailyLendIncreaseLimitLeft control. The same problem exists in the repay and borrow functions.

Tools Used

Manual Review

Recommended Mitigation Steps

VeVault.sol#_withdraw function is modified as follows.

    function _withdraw(address receiver, address owner, uint256 amount, bool isShare)
        internal
        returns (uint256 assets, uint256 shares)
    {
        (uint256 newDebtExchangeRateX96, uint256 newLendExchangeRateX96) = _updateGlobalInterest();
+       _resetDailyLendIncreaseLimit(newLendExchangeRateX96, false);

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

        // if caller has allowance for owners shares - may call withdraw
        if (msg.sender != owner) {
            _spendAllowance(owner, msg.sender, shares);
        }

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

        // fails if not enough shares
        _burn(owner, shares);
        SafeERC20.safeTransfer(IERC20(asset), receiver, assets);

        // when amounts are withdrawn - they may be deposited again
        dailyLendIncreaseLimitLeft += assets;

        emit Withdraw(msg.sender, receiver, owner, assets, shares);
    }

VeVault.sol#_repay function is modified as follows.

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

        Loan storage loan = loans[tokenId];

        uint256 currentShares = loan.debtShares;

        uint256 shares;
        uint256 assets;

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

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

        if (assets > 0) {
            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
                );
            } else {
                // fails if not enough token approved
                SafeERC20.safeTransferFrom(IERC20(asset), msg.sender, address(this), assets);
            }
        }

        uint256 loanDebtShares = loan.debtShares - shares;
        loan.debtShares = loanDebtShares;
        debtSharesTotal -= shares;

        // when amounts are repayed - they maybe borrowed again
        dailyDebtIncreaseLimitLeft += assets;

        _updateAndCheckCollateral(
            tokenId, newDebtExchangeRateX96, newLendExchangeRateX96, loanDebtShares + shares, loanDebtShares
        );

        address owner = tokenOwner[tokenId];

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

        emit Repay(tokenId, msg.sender, owner, assets, shares);
    }

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The user cannot fully compensate for the debt of the NFT token by setting isShare = false.

Proof of Concept

The V3Vault.sol#_repay function is as follows.

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

SNIP...

        // fails if too much repayed
973:    if (shares > currentShares) {
            revert RepayExceedsDebt();
        }

SNIP...

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

        emit Repay(tokenId, msg.sender, owner, assets, shares);
    }

By L973, shares <= currentShares must be set. Next, if the user wants to liquidate all debtShares of the token, he or she must call the repay function by setting amount so that shares == currentShares. The user will preview the amount of asset needed to liquidate debt in full. However, even if the user previews the amount of asset needed to compensate for the debt of the token, this is not an accurate preview. The reason is as follows. Call the _updateGlobalinterest function in L955. Therefore, shares are calculated by newDebtExchangeRateX96, which is different from when preview, so when the user proceeds with repay, shares == currentShares is established. Therefore, the control passes to L1006. In this case, since loanDebtShares = loan.debtShares - shares, the size of loanDebtShares is very small, so the repay operation fails due to L1008.

Even if the user attempts to proceed with repay by setting amount sufficiently, this operation will fail due to L973.

In other words, the user cannot fully compensate for the debt of token by setting isShare = false.

Tools Used

Manual Review

Recommended Mitigation Steps

Modify the V3Vault.sol#_repay function as follows.

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

SNIP...

        // fails if too much repayed
        if (shares > currentShares) {
---         revert RepayExceedsDebt();
+++         shares = currentShares;
+++         assets = _convertToAssets(shares, newDebtExchangeRateX96, Math.Rounding.Up)
        }

SNIP...

    }

Assessed type

Other