Revert Lend - wangxx2026's results

Lines of code

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

Vulnerability details

Impact

Can be blocked from being liquidated

Proof of Concept

With the liquidate method. we can see that the logic of liquidation is: 1.Check if the tokenId is in an unhealthy state 2.Calculate the cost of liquidation 3.If there is a bad debt, let the lender share it 4.from the liquidator to pull the liquidation needs of the fee 5.Send the reward to the liquidator 6.Return the liquidated tokenId to the owner

The problem occurs when the tokenId is returned to the owner. the return code is implemented in the _cleanupLoan method with the following code:

    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); // @audit-issue If the owner is a contract and does not support onERC721Received, it will revert, resulting in a failure of the liquidation.
        emit Remove(tokenId, owner);
    }

And the owner can be set by the recipient parameter in the create method. Then without any checking, set the recipient to owner in onERC721Received method. the implementation is as follows: https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L429-L477

    function onERC721Received(address, address from, uint256 tokenId, bytes calldata data)
        external
        override
        returns (bytes4)
    {
...
        if (transformedTokenId == 0) {
            address owner = from;
            if (data.length > 0) {
                owner = abi.decode(data, (address)); // @audit-issue Get the specified parameter as owner
            }
            loans[tokenId] = Loan(0);

            _addTokenToOwner(owner, tokenId); // @audit-issue Set as owner
            emit Add(tokenId, owner, 0);
        } else {
...
        }

        return IERC721Receiver.onERC721Received.selector;
    }

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

    function _addTokenToOwner(address to, uint256 tokenId) internal {
        ownedTokensIndex[tokenId] = ownedTokens[to].length;
        ownedTokens[to].push(tokenId);
        tokenOwner[tokenId] = to; // @audit-issue Store the correspondence between tokenid and owner.
    }

The implementation of getting the owner of the liquidation return tokenId is as follows: https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L741-L744

        address owner = tokenOwner[params.tokenId]; // @audit-issue The owner may be a contract.

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

Tools Used

Manual Review

Recommended Mitigation Steps

It is recommended that only the EOA address can be used as the owner. If the owner is a contract address, it is undesirable to check whether it supports onERC721Received. If the owner is a contract, the controller of the contract can control when to return normally and when to return abnormally, which also prevents it from being liquidated.

Assessed type

ERC721

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#L910-L914

Vulnerability details

Impact

maxDeposit, maxMint, maxWithdraw, maxRedeem methods are not fully implemented according to the ERC4642 standard, which may cause issues with integration in third-party dapps.

Proof of Concept

1、The maxDeposit method is not fully compliant with the standard.

The maxDeposit implementation is as follows:

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

Global deposit limits are supported, but not daily limits.

In the _deposit method, we can see the implementation of the daily limit https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L906-L914

    function _deposit(address receiver, uint256 amount, bool isShare, bytes memory permitData)
        internal
        returns (uint256 assets, uint256 shares)
    {
...
        if (totalSupply() > globalLendLimit) { // @audit-issue Global Limit
            revert GlobalLendLimit();
        }

        if (assets > dailyLendIncreaseLimitLeft) { // @audit-issue Daily Limit
            revert DailyLendIncreaseLimit();
        } else {
            dailyLendIncreaseLimitLeft -= assets;
        }

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

Required by the ERC4642 standard: https://eips.ethereum.org/EIPS/eip-4626#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.

2、maxMint and maxDeposit have the same problem

Only global limits, not daily limits

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

3、maxWithdraw Only limits the amount of money a user can withdraw, not the amount available to the vault.

Limit on available balance in _withdraw method _withdraw

    function _withdraw(address receiver, address owner, uint256 amount, bool isShare)
        internal
        returns (uint256 assets, uint256 shares)
    {
...
        (, uint256 available,) = _getAvailableBalance(newDebtExchangeRateX96, newLendExchangeRateX96);
        if (available < assets) { // @audit-issue limit available
            revert InsufficientLiquidity();
        }
...
    }

4、maxRedeem and maxWithdraw have the same problem

    function maxRedeem(address owner) external view override returns (uint256) {
        return balanceOf(owner); // @audit-issue Limit only own balance
    }

Tools Used

Manual Review

Recommended Mitigation Steps

The maximum limit returned by these four methods is the same as in the implementation.

Assessed type

ERC4626

Lines of code

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

Vulnerability details

Impact

Forced limit updates can result in unexpected increases in the day's limit

Proof of Concept

In the setLimits method, we can see that updating the dailyLendIncreaseLimitMin, dailyDebtIncreaseLimitMin will do a limit force update limit.

    function setLimits(
        uint256 _minLoanSize,
        uint256 _globalLendLimit,
        uint256 _globalDebtLimit,
        uint256 _dailyLendIncreaseLimitMin,
        uint256 _dailyDebtIncreaseLimitMin
    ) external {
...
        // force reset daily limits with new values
        _resetDailyLendIncreaseLimit(newLendExchangeRateX96, true); // @audit-issue  Updating Deposit Limits 
        _resetDailyDebtIncreaseLimit(newLendExchangeRateX96, true); // @audit-issue  Update Debt Limit   
...
        );
    }

However, the forced limit update does not take into account the amount already used on that day, which will result in the updated limit being equal to the amount used on that day + the updated limit.

Take the _resetDailyLendIncreaseLimit method as an example, which is implemented 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;
            dailyLendIncreaseLimitLeft =
                dailyLendIncreaseLimitMin > lendIncreaseLimit ? dailyLendIncreaseLimitMin : lendIncreaseLimit;
            dailyLendIncreaseLimitLastReset = time;
        }
    }

For example, in the case where lendIncreaseLimit is less than dailyLendIncreaseLimitMin, and the deposit limit for today was originally 1 million, with 500,000 already used. Now, if dailyLendIncreaseLimitMin is changed to 800,000, the updated limit for today would be 1.3 million. However, the correct result should be that there is only 300,000 remaining.

_resetDailyDebtIncreaseLimit has the same problem

Tools Used

Manual Review

Recommended Mitigation Steps

Record the amount already used for the day in the variable todayUsed, and when forcing an update

dailyLendIncreaseLimitLeft -= todayUsed.

Then set todayUsed to 0 when updating the limit normally.

Assessed type

Math