Users Uniswap Positions could be locked forever in Vault, because of Missing Input Validation

[c4-bot-1]

Lines of code

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

Vulnerability details

Impact

User LP Position could be forever locked in V3Vault, if the user mistakingly set the call data in create() to the zero address. It is possible to misunderstand the create() function and set the recipient to the x0 address, which will set the owner of this Position as the zero address in the vault, potentially locking the position in vault forever.

Proof of Concept

This issue is possible because, the onERC721Received() function in the V3Vault contract, doesn't check if the recipient is the zero address, and if it is, it sets the owner of the position in vault as the zero address.

To run the test please add the following test to test/integration/V3Vault.t.sol :

    function testCreatePositionToZeroAddress() external {
        vm.prank(TEST_NFT_ACCOUNT);
        NPM.approve(address(vault), TEST_NFT);
        vm.prank(TEST_NFT_ACCOUNT);
        vault.create(TEST_NFT,address(0));
        assertTrue(vault.ownerOf(TEST_NFT)== address(0));
        assertTrue(NPM.ownerOf(TEST_NFT)==address(vault));
        console.log("NFT Lost Forever");
        console.log("Owner of NFT in NPM: ",NPM.ownerOf(TEST_NFT));
        console.log("Owner of NFT in Vault: ",vault.ownerOf(TEST_NFT));
    }

The result of the test being executed:

Ran 1 test for test/integration/V3Vault.t.sol:V3VaultIntegrationTest
[PASS] testCreatePositionToZeroAddress() (gas: 186268)
Logs:
  NFT Lost Forever
  Owner of NFT in NPM:  0xF62849F9A0B5Bf2913b396098F7c7019b51A820a
  Owner of NFT in Vault:  0x0000000000000000000000000000000000000000

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 530.37ms (929.12µs CPU time)

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

Tools Used

Foundry, manual review

Recommended Mitigation Steps

To mitigate this issue, it is recomended to add a check in the onERC721Received() and to set the owner of the cdp to msg.sender (from in onERC721Received()) if the recipient is the zero address. A possible solution, would be something like this:

    function onERC721Received(address, address from, uint256 tokenId, bytes calldata data)
        external
        override
        returns (bytes4)
    {
        // only Uniswap v3 NFTs allowed - sent from other contract
        if (msg.sender != address(nonfungiblePositionManager) || from == address(this)) {
            revert WrongContract();
        }

        (uint256 debtExchangeRateX96, uint256 lendExchangeRateX96) = _updateGlobalInterest();

        if (transformedTokenId == 0) {
            address owner = from;
            if (data.length > 0) {
--                owner = abi.decode(data, (address));
++                address recipient = abi.decode(data, (address));
++                owner = (recipient == 0 ? owner : recipient);
            }
            loans[tokenId] = Loan(0);

            _addTokenToOwner(owner, tokenId);
            emit Add(tokenId, owner, 0);
        } else {
            uint256 oldTokenId = transformedTokenId;

Assessed type

Invalid Validation

[c4-pre-sort]

0xEVom marked the issue as insufficient quality report

[c4-judge]

jhsagd76 marked the issue as unsatisfactory: Out of scope

[jhsagd76]

user error