Revert Lend - 0xDemon's results

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/457230945a49878eefdc1001796b10638c1e7584/src/V3Vault.sol#L359-L381 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L300-L320

Vulnerability details

Impact

Functionality is not working as expected

Proof of Concept

1. deposit, mint, withdraw, and redeem functions

As per EIP-4626 :

If implementors intend to support EOA account access directly, they should consider adding an additional function call for deposit/mint/withdraw/redeem with the means to accommodate slippage loss or unexpected deposit/withdrawal limits, since they have no other means to revert the transaction if the exact output amount is not achieved.

But in this codebase, the protocol does not follow this. There is no slippage protection for these four main functions which results in users receiving shares (deposit) less than they should and users receiving assets (withdraw) less than they should. Lets take a look these function in the codebase :

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

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

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

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

It can be seen from the codebase, when the user calls these four functions they will be passed to the internal _deposit and _withdraw functions, the difference is the isShares variable. Lets take a look those 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);
        }

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

    // withdraws lent tokens. can be denominated in token or share amount
    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
        dailyLendIncreaseLimitLeft += assets;

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

In the _deposit function, the protocol only takes into account the user's upper deposit limit with the DailyLendIncreaseLimit and GlobalLendLimit variables but does not take into account how many shares the user should receive when making a deposit. So when a user makes a deposit, he may receive shares less than the amount of the deposit he made.

Meanwhile, for the _withdraw function, the protocol only takes into account the amount of liquidity available when the user makes a withdrawal but does not take into account whether the assets received by the user correspond to the shares burned. So when a user makes a withdrawal, the user may receive assets less than they should be and burning more shares than necessary.

2. maxDeposit and maxMint functions

As per EIP-4626 :

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

But in the codebase, the protocol does not follow this. When the user calls these two functions, in certain circumstances the results of these two functions do not match what the user can do. Lets take a look these function in the codebase :

    /// @inheritdoc IERC4626
    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);
        }
    }

As seen in the functions above, the protocol only takes into account the upper limit using globalLendLimit, whereas in the codebase there is also a dailyLendIncreaseLimitLeft variable which functions to limit the number of deposits / mints made each day. When the user's dailyLendIncreaseLimitLeft value reaches the maximum value, the user cannot make deposits or mints. Thus, the maxDeposit and maxMint functions should have a return value = 0 because deposit and mint are in the disabled state.

Tools Used

Manual review

Recommended Mitigation Steps

1. Go through the standard and follow it all.
2. Add slippage protection

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

    /// @inheritdoc IERC4626
    function mint(uint256 shares, address receiver, uint256 minAssets) external override returns (uint256) {
        (uint256 assets,) = _deposit(receiver, shares, true, "");
        if (assets < minAssets) {
	        revert slippageProtection }
        return assets;
    }

    /// @inheritdoc IERC4626
    function withdraw(uint256 assets, address receiver, address owner, uint256 maxShares) external override returns (uint256) {
        (, uint256 shares) = _withdraw(receiver, owner, assets, false);
        if (shares > maxShares) {
	        revert slippageProtection }
        return shares;
    }

    /// @inheritdoc IERC4626
    function redeem(uint256 shares, address receiver, address owner, uint256 mintAssets) external override returns (uint256) {
        (uint256 assets,) = _withdraw(receiver, owner, shares, true);
        if (assets < minAssets) {
	        revert slippageProtection }
        return assets;
    }

2. Also pay attention to user specific variables

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

Assessed type

ERC4626

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L877-L917 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L954-L1014 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L685-L757

Vulnerability details

Impact

V3Vault.mint(), V3Vault.repay() and V3Vault.liquidate() using signatures to approve user assets. But in those functions, the asset value can change easily so the method can easily revert and can be DoS.

Proof of Concept

Because this is a similar case for both functions, for a deeper explanation i will use V3Vault.mint() with permitData as an example. When the user call the V3Vault.mint() function with permitData it will be passed to the internal function _deposit. _deposit gets the share amount as an input and calculates the asset amount from the share. Then, it approves the asset amount with permit method.

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

The signature is generated using the exact value of the expected asset amount calculated from the share amount, and the resulting asset amount depends on the exchange rate of current vault. newLendExchangeRateX96 will continue to update if a user makes a deposit or mint, so this value will continue to change which will have an impact on the amount of assets.

(, uint256 newLendExchangeRateX96) = _updateGlobalInterest();

    function _convertToAssets(uint256 shares, uint256 exchangeRateX96, Math.Rounding rounding)
        internal
        pure
        returns (uint256)
    {
        return shares.mulDiv(exchangeRateX96, Q96, rounding);
    }

The resulting asset amount can be different from the value of transaction start time. Thus, malicious actors can easily change newLendExchangeRateX96 so that the share exchange rate for assets will be different and the resulting assets will also be different. This causes, V3Vault.mint() with permitData mostly revert either intentionally or unintentionally and can be DoS by malicious actors.

Another instances :

1. V3Vault.repay() with permitData :

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

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

2. V3Vault.liquidate() with permitData

    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;
        if (debtShares != params.debtShares) {
            revert DebtChanged();
        }

        state.debt = _convertToAssets(debtShares, state.newDebtExchangeRateX96, Math.Rounding.Up);

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

        (state.liquidationValue, state.liquidatorCost, state.reserveCost) =
            _calculateLiquidation(state.debt, state.fullValue, state.collateralValue);

        // calculate reserve (before transfering liquidation money - otherwise calculation is off)
        if (state.reserveCost > 0) {
            state.missing =
                _handleReserveLiquidation(state.reserveCost, state.newDebtExchangeRateX96, state.newLendExchangeRateX96);
        }

        if (params.permitData.length > 0) {
            (ISignatureTransfer.PermitTransferFrom memory permit, bytes memory signature) =
                abi.decode(params.permitData, (ISignatureTransfer.PermitTransferFrom, bytes));
            permit2.permitTransferFrom(
                permit,
                ISignatureTransfer.SignatureTransferDetails(address(this), state.liquidatorCost),
                msg.sender,
                signature
            );
        } else {
            // take value from liquidator
            SafeERC20.safeTransferFrom(IERC20(asset), msg.sender, address(this), state.liquidatorCost);
        }

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

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

        emit Liquidate(
            params.tokenId,
            msg.sender,
            owner,
            state.fullValue,
            state.liquidatorCost,
            amount0,
            amount1,
            state.reserveCost,
            state.missing
        );
    }

Tools Used

Manual review

Recommended Mitigation Steps

Consider using upper bound amount of asset as input for permit instead of exact value.

Assessed type

ERC4626