Core Contracts

Summary

The LendingPool contract allows users to deposit RAACHouse NFTs as collateral via depositNFT and later withdraw them using withdrawNFT or have them transferred during liquidation via finalizeLiquidation. However, both functions use for loops to iterate over the user's nftTokenIds array. Over time, as more NFTs are deposited through depositNFT (which simply pushes each tokenId into the array), this array can grow very large. The unbounded looping in withdrawNFT and finalizeLiquidation can then lead to out-of-gas errors, effectively causing a denial of service (DoS) for users with many deposited NFTs.

Vulnerability Details

How It Begins

1. NFT Deposit Process:

   In depositNFT, each deposited NFT is appended to the user's nftTokenIds array:

   function depositNFT(uint256 tokenId) external nonReentrant whenNotPaused {

   // update state

   ReserveLibrary.updateReserveState(reserve, rateData);

   ​

   if (raacNFT.ownerOf(tokenId) != msg.sender) revert NotOwnerOfNFT();

   ​

   UserData storage user = userData[msg.sender];

   if (user.depositedNFTs[tokenId]) revert NFTAlreadyDeposited();

   ​

   user.nftTokenIds.push(tokenId);

   user.depositedNFTs[tokenId] = true;

   ​

   raacNFT.safeTransferFrom(msg.sender, address(this), tokenId);

   ​

   emit NFTDeposited(msg.sender, tokenId);

   }

2. Inefficient Removal in withdrawNFT:

   When a user withdraws an NFT, the contract iterates over the entire nftTokenIds array to locate the tokenId and then swaps it with the last element before popping it:

   // @info: denial of service could occur over time

   for (uint256 i = 0; i < user.nftTokenIds.length; i++) {

   if (user.nftTokenIds[i] == tokenId) {

   user.nftTokenIds[i] = user.nftTokenIds[user.nftTokenIds.length - 1];

   user.nftTokenIds.pop();

   break;

   }

   }

   As the array grows, this loop can consume significant gas.

3. Batch Processing in finalizeLiquidation:

   Similarly, during liquidation finalization, the contract loops over the entire nftTokenIds array to transfer each NFT to the Stability Pool:

   for (uint256 i = 0; i < user.nftTokenIds.length; i++) {

   uint256 tokenId = user.nftTokenIds[i];

   user.depositedNFTs[tokenId] = false;

   raacNFT.transferFrom(address(this), stabilityPool, tokenId);

   }

   delete user.nftTokenIds;

   When a user has deposited many NFTs, this operation becomes prohibitively expensive in gas.

How the Issue Arises

An attacker or even an honest user who frequently deposits NFTs can grow their nftTokenIds array arbitrarily large. When they (or the Stability Pool, in the case of liquidation) later trigger a withdrawal or liquidation finalization, the for loop iterates over a massive array. The gas required for these operations may exceed the block gas limit, effectively causing a denial of service for that user.

Proof of Concept

Test Suite Walkthrough and Scenario Example

1. Scenario Example:

   • NFT Deposits:
     Alice deposits reserve assets into the LendingPool and then proceeds to deposit 3,292 NFTs (each with a small house price of 10e18) as collateral. This causes her nftTokenIds array to grow to 3,292 elements.

   • Withdrawal/Finalization DoS:
     When Alice (or the Stability Pool in the case of liquidation) later calls withdrawNFT or finalizeLiquidation, the function iterates over the large array. As demonstrated in the test, the 3,293rd call runs out of gas, effectively locking her funds.

2. Test Suite Code:

Output Log

How to Run the Test

1. Create a Foundry Project:

   forge init my-foundry-project

2. Place Contract Files:
   Ensure that all contracts (LendingPool.sol, RAACNFT.sol, RAACHousePrices.sol, etc.) are placed in the src directory.

3. Create Test Directory:
   Create a test directory adjacent to src and add the test file (e.g., PoolsTest.t.sol).

4. Run the Test:

Impact

• Denial of Service (DoS):
  As the nftTokenIds array grows over time, the gas required to loop through it increases. This can lead to out-of-gas errors when executing withdrawNFT or finalizeLiquidation, preventing users from withdrawing their NFTs or finalizing liquidations.

• User Lockout and Liquidity Issues:
  Affected users could be locked out of retrieving their collateral, potentially causing broader liquidity and operational problems within the protocol.

• Scalability Concerns:
  This vulnerability hinders the protocol’s ability to scale, as increased NFT deposits directly correlate with higher gas consumption during critical operations.

Tools Used

• Manual Review

• Foundry (Forge)

Recommendations

To mitigate the denial-of-service vulnerability, the NFT management logic should be optimized. One effective solution is to maintain an additional mapping that tracks each NFT's index in the nftTokenIds array. This allows constant-time removal without iterating over the entire array.

Recommended Diff

Update in depositNFT

Add a mapping to track the index of each NFT in the user's array (e.g., nftTokenIdIndex in the UserData struct). Then update the deposit function as follows:

Update in withdrawNFT

Replace the for loop with a constant-time removal mechanism:

Update in finalizeLiquidation

For batch processing NFT removals, implement a removal limit and track the batch process with a flag. For example, add a state variable (e.g., batchRemovalInProgress) and a mapping to track progress, then limit the number of removals per transaction:

Additionally, lock all functions that depend on NFT state (such as new deposits, withdrawals, or borrow actions) while batchRemovalInProgress[userAddress] is true to ensure consistency.

2. Integrate Function Locks During Batch Removal

Before processing any NFT-related operation, check if a batch removal is in progress for the user:

Apply this check in depositNFT, withdrawNFT, and any other functions that interact with the NFT array.

After implementing these changes, rerun the test suite and conduct additional tests to verify that:

• NFT removals occur in constant time.

• Batch processing limits gas usage.

• Functions are properly locked until all NFTs are removed.

By implementing these optimizations and batch removal controls, the protocol will prevent DoS scenarios arising from unbounded loops and improve both efficiency and scalability.