Revert Lend - nmirchev8'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

We can see that Uniswap NonfungiblePositionManager.sol uses _mint when opening a position for a user, instead of _safeMint (o.e receiver may be a contract, which is not IERC721Receiver). However in Revert Lend we always use safeTransferFrom when interacting with nft positions, which check whether to is able to receive NFT (if it is contract, it should implement IERC721Receiver). If this check is invalid, the whole transaction reverts. It is possible exploiter to create a contract, which can interact with nfts, but it is not IERC721Receiver. Example:

contract ExploiterContract{
	IERC721 public nftContract;
	address private owner;
	
	contructor(address _nftContract){
		owner = msg.sender;
		nftContract = IERC721(_nftContract);
	} 
	
	function setNftContract(address _nftContract) external{
		require(msg.sender == owner, "Unauthorized");
		nftContract = IERC721(_nftContract);
	}
	
  function executeActionOnNftContract(bytes calldata _data) external {
        require(msg.sender == owner, "Unauthorized");

        // Use low-level call to execute the function on nftContract
        (bool success, ) = address(nftContract).call(_data);
        require(success, "Function call failed");
  }
  
   function executeAnyActionOnAnyContract(address target,bytes calldata _data) external {
        require(msg.sender == owner, "Unauthorized");

        // Use low-level call to execute the function on nftContract
        (bool success, ) = target.call(_data);
        require(success, "Function call failed");
  }
}

This way ExploiterContract can approve V3Vault to tranfer NFT position to themself, and open a position, which is impossible to be liquidated, because the following line will always revert, because owner (to parameter) is contract, which does not implement IERC721Receiver: liquidate -> _cleanupLoan -> nftManager.safeTransferFrom Impact:

• 100% safe borrow position fo exploiter, which can never be liquidated
• Exploiter can deploy upgradable contract, which can be updated to support IERC721Receiver when/if borrower decide to repay loan.
• Bad dept generation, which cannot be removed with any of the liquidation methods, because anyway safeTransferFrom is called.

Proof of Concept

• See above example as exploiter contract

1. Possible scenario is Eve opening a nft position to USDC/WETH pool with tick between 3000 - 4000 USDC per WETH providing $1500 worth of WETH and $1500 worth of USDC.
2. She borrows 2500 USDC from Revert Lend by providing the above position.
3. She is hedging possibility to lose value from her position by having sure $2500
4. WETH price falls below $3000 and now her position is worth less than $2500, but she cannot be liquidated, because transaction will revert when protocol try to transfer her empty nft position.
5. This is bad, because she has stolen depositor's funds.

Tools Used

Manual Review

Recommended Mitigation Steps

• I would recommend using pull over push pattern for this specific case. In other word use transferFrom instead of safeTransferFrom when availability of the function is crucial:

    function _cleanupLoan(uint256 tokenId, uint256 debtExchangeRateX96, uint256 lendExchangeRateX96, address owner)
        internal
    {
        _removeTokenFromOwner(owner, tokenId);
        _updateAndCheckCollateral(tokenId, debtExchangeRateX96, lendExchangeRateX96, loans[tokenId].debtShares, 0);
        delete loans[tokenId];
-        nonfungiblePositionManager.safeTransferFrom(address(this), owner, tokenId);
+        nonfungiblePositionManager.transferFrom(address(this), owner, tokenId);
        emit Remove(tokenId, owner);
    }

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Inside liquidate function caller must provide debtShares of the position he is trying to liquidate. The same value is compared with loans[params.tokenId].debtShares and if it is different, the tx reverts. The problem is that borrower can front-run every liquidate attempt by repaying only 1 share, which would make this check fails. Having in mind that mainly shares would have 18 decimals, repaying 1 share could go on forever, or at least reach underwater position, which would be in cost of protocol reserves.

Proof of Concept

User A try to liquidate another user with LiquidateParams.debtShares = X: https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L685 Borrower B sees the tx and front-run it calling repay with shares = X - 1: https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L954 User A transaction revert, because debt.shares does not match: https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L695-L698 Above steps could continue along...

Tools Used

Manual Review

Recommended Mitigation Steps

Remove the whole check:

-        if (debtShares != params.debtShares) {
-            revert DebtChanged();
-        }

Assessed type

DoS