Revert Lend - Ocean_Sky's results

Lines of code

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

Vulnerability details

Detailed Explanation

To explain this fully, let's discuss this in 3 parts, 1st part will be about creating, then 2nd is borrowing and the last part will be liquidation process.

1st Part: Create In the beginning, borrower need to transfer first the Uniswap V3 NFT position into the protocol. This will serve as the collateral when the borrower decided to make a loan against it. Please remember in this function that the recipient of NFT position (needed in liquidation part) is included in user parameter input.

File: V3Vault.sol
459:     function create(uint256 tokenId, address recipient) external override {
460:         nonfungiblePositionManager.safeTransferFrom(msg.sender, address(this), tokenId, abi.encode(recipient));
461:     }

Once the protocol receives the NFT position, function onERC721received will be executed. One of the parameter here is bytes calldata data, this is the data of recipient address from function create. In line 503 and 507 below, the recipient address will be assigned as tokenID owner.

File: V3Vault.sol
488:     function onERC721Received(address, address from, uint256 tokenId, bytes calldata data)
489:         external
490:         override
491:         returns (bytes4)
492:     {
493:         // only Uniswap v3 NFTs allowed - sent from other contract
494:         if (msg.sender != address(nonfungiblePositionManager) || from == address(this)) {
495:             revert WrongContract();
496:         }
497: 
498:         (uint256 debtExchangeRateX96, uint256 lendExchangeRateX96) = _updateGlobalInterest();
499: 
500:         if (transformedTokenId == 0) {
501:             address owner = from;
502:             if (data.length > 0) {
503:                 owner = abi.decode(data, (address));
504:             }
505:             loans[tokenId] = Loan(0);
506: 
507:             _addTokenToOwner(owner, tokenId);
508:             emit Add(tokenId, owner, 0);

2nd Part: Borrow In this part, borrow function has a requirement to check if the token ID owner is the same with msg.sender (line 620-21) so it can proceed to borrow successfully. If the borrower want to pass this checking, he should assigned the recipient in the create function the msg.sender itself.

File: V3Vault.sol
609:     function borrow(uint256 tokenId, uint256 assets) external override {
610:         bool isTransformMode =
611:             transformedTokenId > 0 && transformedTokenId == tokenId && transformerAllowList[msg.sender];
612: 
613:         (uint256 newDebtExchangeRateX96, uint256 newLendExchangeRateX96) = _updateGlobalInterest();
614: 
615:         _resetDailyDebtIncreaseLimit(newLendExchangeRateX96, false);
616: 
617:         Loan storage loan = loans[tokenId];
618: 
619:         // if not in transfer mode - must be called from owner or the vault itself
620:         if (!isTransformMode && tokenOwner[tokenId] != msg.sender && address(this) != msg.sender) {
621:             revert Unauthorized();
622:         }
623: 

The token ID owner here will be the recipient of the loan borrowed (line 658).

File: V3Vault.sol
654:         address owner = tokenOwner[tokenId];
655: 
656:         // fails if not enough asset available
657:         // if called from transform mode - send funds to transformer contract
658:         SafeERC20.safeTransfer(IERC20(asset), isTransformMode ? msg.sender : owner, assets);

3rd Part: Liquidation If the position nft's assessed collateral value falls below the position's accrued debt, this will trigger the liquidation of the position. There is just one part of the liquidation process that is worth to check in relation to the "token ID owner" in which we discussed on creating and borrowing part. Remember in creating part, borrower can deposit collateral and assign the recipient address as token ID owner in the parameter. This is the same procedure in liquidation, the recipient of token ID after liquidation is the token ID owner.

File: V3Vault.sol
801:         address owner = tokenOwner[params.tokenId];
802: 
803:         // disarm loan and send remaining position to owner
804:         _cleanupLoan(params.tokenId, state.newDebtExchangeRateX96, state.newLendExchangeRateX96, owner);

_cleanup loan details below: look at line 1150, this is the transfer of nft token ID to the "token owner"

File: V3Vault.sol
1142:     // cleans up loan when it is closed because of replacement, repayment or liquidation
1143:     // send the position in its current state to owner
1144:     function _cleanupLoan(uint256 tokenId, uint256 debtExchangeRateX96, uint256 lendExchangeRateX96, address owner)
1145:         internal
1146:     {
1147:         _removeTokenFromOwner(owner, tokenId);
1148:         _updateAndCheckCollateral(tokenId, debtExchangeRateX96, lendExchangeRateX96, loans[tokenId].debtShares, 0);
1149:         delete loans[tokenId];
1150:         nonfungiblePositionManager.safeTransferFrom(address(this), owner, tokenId);
1151:         emit Remove(tokenId, owner);
1152:     }

There is just one big issue here, the recipient/token ID owner assigned here can be a smart contract with no onERC721Received function to be called on by the nonfungiblePositionManager through safeTransferFrom. Since there is no function to be called on, this will result to error and the entire liquidate process will be reverted.

function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) public virtual {
        transferFrom(from, to, tokenId);
        ERC721Utils.checkOnERC721Received(_msgSender(), from, to, tokenId, data);
    }

There is also a one big question here on why the token ID owner happened to not have onERC721Received function despite able to deposit nft position in the first place? Yes this is possible because when the nft position is minted to the token ID owner, the nonfungiblePositionManager use the _mint function in which it doesn't check the onERC721Received function. It only checks whether the recipient is address 0.

function _mint(address to, uint256 tokenId) internal {
        if (to == address(0)) {
            revert ERC721InvalidReceiver(address(0));
        }
        address previousOwner = _update(to, tokenId, address(0));
        if (previousOwner != address(0)) {
            revert ERC721InvalidSender(address(0));
        }
    }

Impact

Borrower can avoid the liquidation in perpetuity, in which the loan basically becomes bad debt to the protocol.

Proof of Concept

Let us illustrate here a possible scenario on how the malicious borrower can DOS the liquidation process.

Pre-borrowing activities 1.) Alice , a uniswap v3 position owner created a pool with risky assets. 2.) Alice inflated the price by depositing more on the pool and encouraging more innocent users to invest or exchange in the pool. This will bring the value of pool/nft position in higher value.

Borrowing activities 3.) When Alice already have a highly valued nft position through pump scheme, she will deposit one of her nft positions to become collateral to the protocol using the smart contract with no onERC721received function. 4.) Alice will received the more stable assets such as USDC through loan in exchange of highly valued risky collateral.

Post-borrowing activities 5.) Alice will dump the whole other positions in uniswap v3 bringing the value of the pool/nft position basically worthless. 6.) Liquidation will be triggered but will fail because the nft position can't be transferred to the token ID owner due to missing onERC721received function. 7.) At the end, Alice will possess more stable assets and the protocol has bad debt.

Just to remind that the double oracle setup here can't do anything to stop these activities. The TWAP and Chainlink oracles will just follow what is happening in the market.

Tools Used

Manual Review

Recommended Mitigation Steps

Separate the transfer of remaining NFT position to the owner of token ID from the actual liquidation operation. Let the owner of token ID claim the NFT on his own and not be automatically transferred to them.

Assessed type

DoS