Core Contracts

Summary

The LendingPool contract's withdrawNFT function allows users to withdraw deposited RAACHouse NFTs. However, the function contains an inefficient storage update mechanism when removing an NFT from the user's deposited NFTs array. Specifically, the function always iterates through the entire array to swap the target NFT with the last element and then pops it, even in cases where the NFT to be removed is the only element or already resides at the last index. This unnecessary operation increases gas costs and degrades the protocol's overall efficiency.

Vulnerability Details

How It Begins

1. Inefficient Removal of NFT:

   When a user withdraws an NFT, the function iterates over the user's nftTokenIds array:

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

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

   // @info: verbose assignment if user has only 1 nft or

   // if the removing nft resides at last index

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

   user.nftTokenIds.pop();

   break;

   }

   }

   This approach is unnecessarily verbose. In the scenario where the user has only one NFT or if the NFT to be removed is already the last element in the array, there is no need for a swap operation; a simple pop() would suffice.

2. Inefficiency and Gas Costs:

   The redundant swap operation consumes extra gas. Although it does not directly compromise the protocol's security, it results in higher transaction costs, particularly for users with multiple NFTs deposited. Over time and at scale, these inefficiencies can lead to substantial economic overhead for the protocol.

Proof of Concept

Test Suite Walkthrough and Scenario Example

1. Scenario Example:

   • Deposit Phase:
     Alice deposits reserve assets into the LendingPool and mints one RAACHouse NFT (tokenId 1) valued at 1000e18, which she then deposits as collateral.

   • Withdrawal Phase:
     When Alice calls withdrawNFT to retrieve her only NFT, the function unnecessarily performs a swap even though the NFT is already at the last index of the array.

   • Inefficiency Demonstrated:
     The test verifies that after withdrawal, the NFT array is empty. However, the redundant operations in the loop lead to increased gas consumption.

2. Test Suite Code:

   // SPDX-License-Identifier: MIT

   ​

   pragma solidity ^0.8.19;

   ​

   import {Test, console} from "forge-std/Test.sol";

   ​

   import {StabilityPool} from "../src/core/pools/StabilityPool/StabilityPool.sol";

   import {LendingPool} from "../src/core/pools/LendingPool/LendingPool.sol";

   import {DEToken} from "../src/core/tokens/DEToken.sol";

   import {RToken} from "../src/core/tokens/RToken.sol";

   import {DebtToken} from "../src/core/tokens/DebtToken.sol";

   import {RAACToken} from "../src/core/tokens/RAACToken.sol";

   import {RAACMinter} from "../src/core/minters/RAACMinter/RAACMinter.sol";

   import {RAACHousePrices} from "../src/core/primitives/RAACHousePrices.sol";

   import {RAACNFT} from "../src/core/tokens/RAACNFT.sol";

   ​

   import {CurveCrvUSDVaultMock} from "./mocks/CurveCrvUSDVaultMock.m.sol";

   import {crvUSDToken} from "../src/mocks/core/tokens/crvUSDToken.sol";

   import {ERC20Mock} from "../src/mocks/core/tokens/ERC20Mock.sol";

   ​

   import {ILendingPool} from "../src/interfaces/core/pools/LendingPool/ILendingPool.sol";

   import {PercentageMath} from "../src/libraries/math/PercentageMath.sol";

   import {WadRayMath} from "../src/libraries/math/WadRayMath.sol";

   ​

   contract PoolsTest is Test {

   using PercentageMath for uint256;

   using WadRayMath for uint256;

   // contracts

   ​

   StabilityPool stabilityPool;

   LendingPool lendingPool;

   DEToken deToken;

   RToken rToken;

   DebtToken debtToken;

   RAACToken raacToken;

   RAACMinter raacMinter;

   RAACHousePrices raacHousePrices;

   RAACNFT raacNft;

   CurveCrvUSDVaultMock curveCrvUsdVault;

   crvUSDToken crvUsdToken;

   ERC20Mock erc20Mock;

   ​

   // owners

   address STABILITY_POOL_OWNER = makeAddr("STABILITY_POOL_OWNER");

   address LENDING_POOL_OWNER = makeAddr("LENDING_POOL_OWNER");

   address DETOKEN_OWNER = makeAddr("DETOKEN_OWNER");

   address RTOKEN_OWNER = makeAddr("RTOKEN_OWNER");

   address DEBT_TOKEN_OWNER = makeAddr("DEBT_TOKEN_OWNER");

   address RAAC_TOKEN_OWNER = makeAddr("RAAC_TOKEN_OWNER");

   address RAAC_MINTER_DUMMY = makeAddr("RAAC_MINTER_DUMMY");

   address RAAC_HOUSE_PRICES_OWNER = makeAddr("RAAC_HOUSE_PRICES_OWNER");

   address RAAC_HOUSE_PRICES_ORACLE = makeAddr("RAAC_HOUSE_PRICES_ORACLE");

   address CRV_USD_TOKEN_OWENR = makeAddr("CRV_USD_TOKEN_OWENR");

   address CURVE_CRV_USD_VAULT_OWENR = makeAddr("CURVE_CRV_USD_VAULT_OWENR");

   address ERC20_MOCK_TOKEN_OWNER = makeAddr("ERC20_MOCK_TOKEN_OWNER");

   address RAAC_NFT_TOKEN_OWNER = makeAddr("RAAC_NFT_TOKEN_OWNER");

   address RAAC_MINTER_OWNER = makeAddr("RAAC_MINTER_OWNER");

   address RAAC_OWNER = makeAddr("RAAC_OWNER");

   ​

   // users

   address ALICE = makeAddr("ALICE");

   address BOB = makeAddr("BOB");

   address CHARLIE = makeAddr("CHARLIE");

   address DEVIL = makeAddr("DEVIL");

   ​

   // managers

   address MANAGER_1 = makeAddr("MANAGER_1");

   address MANAGER_2 = makeAddr("MANAGER_2");

   address MANAGER_3 = makeAddr("MANAGER_3");

   address MANAGER_4 = makeAddr("MANAGER_4");

   address MANAGER_5 = makeAddr("MANAGER_5");

   address MANAGER_6 = makeAddr("MANAGER_6");

   address MANAGER_7 = makeAddr("MANAGER_7");

   address MANAGER_8 = makeAddr("MANAGER_8");

   address MANAGER_9 = makeAddr("MANAGER_9");

   address MANAGER_10 = makeAddr("MANAGER_10");

   ​

   uint256 initialPrimeRate = 5e27;

   uint256 initialRaacSwapTaxRateInBps = 200; // 2%, 10000 - 100%

   uint256 initialRaacBurnTaxRateInBps = 150; // 1.5%, 10000 - 100%

   ​

   function setUp() public {

   vm.warp(block.timestamp + 1 days + 1);

   ​

   vm.startPrank(CRV_USD_TOKEN_OWENR);

   crvUsdToken = new crvUSDToken(CRV_USD_TOKEN_OWENR);

   vm.stopPrank();

   ​

   vm.startPrank(CURVE_CRV_USD_VAULT_OWENR);

   curveCrvUsdVault = new CurveCrvUSDVaultMock(address(crvUsdToken), CURVE_CRV_USD_VAULT_OWENR);

   vm.stopPrank();

   ​

   vm.startPrank(RTOKEN_OWNER);

   rToken = new RToken("R_Token_V1", "RTKNV1", RTOKEN_OWNER, address(crvUsdToken));

   vm.stopPrank();

   ​

   vm.startPrank(DETOKEN_OWNER);

   deToken = new DEToken("DE_Token_V1", "DETKNV1", DETOKEN_OWNER, address(rToken));

   vm.stopPrank();

   ​

   vm.startPrank(DEBT_TOKEN_OWNER);

   debtToken = new DebtToken("DEBT_TOKEN_V1", "DEBTKNV1", DEBT_TOKEN_OWNER);

   vm.stopPrank();

   ​

   vm.startPrank(ERC20_MOCK_TOKEN_OWNER);

   erc20Mock = new ERC20Mock("ERC20_MOCK_TOKEN", "ERC20MTKN");

   vm.stopPrank();

   ​

   vm.startPrank(RAAC_HOUSE_PRICES_OWNER);

   raacHousePrices = new RAACHousePrices(RAAC_HOUSE_PRICES_OWNER);

   vm.stopPrank();

   ​

   vm.startPrank(RAAC_NFT_TOKEN_OWNER);

   raacNft = new RAACNFT(address(erc20Mock), address(raacHousePrices), RAAC_NFT_TOKEN_OWNER);

   vm.stopPrank();

   ​

   vm.startPrank(RAAC_OWNER);

   raacToken = new RAACToken(RAAC_OWNER, initialRaacSwapTaxRateInBps, initialRaacBurnTaxRateInBps);

   vm.stopPrank();

   ​

   vm.startPrank(LENDING_POOL_OWNER);

   lendingPool = new LendingPool(

   address(crvUsdToken),

   address(rToken),

   address(debtToken),

   address(raacNft),

   address(raacHousePrices),

   initialPrimeRate

   );

   vm.stopPrank();

   ​

   vm.startPrank(STABILITY_POOL_OWNER);

   stabilityPool = new StabilityPool(STABILITY_POOL_OWNER);

   vm.stopPrank();

   ​

   vm.startPrank(RAAC_MINTER_OWNER);

   raacMinter = new RAACMinter(address(raacToken), address(stabilityPool), address(lendingPool), RAAC_MINTER_OWNER);

   vm.stopPrank();

   ​

   vm.startPrank(RAAC_OWNER);

   raacToken.setMinter(address(raacMinter));

   vm.stopPrank();

   ​

   vm.startPrank(LENDING_POOL_OWNER);

   lendingPool.setCurveVault(address(curveCrvUsdVault));

   vm.stopPrank();

   ​

   vm.startPrank(RTOKEN_OWNER);

   rToken.setReservePool(address(lendingPool));

   vm.stopPrank();

   ​

   vm.startPrank(DEBT_TOKEN_OWNER);

   debtToken.setReservePool(address(lendingPool));

   vm.stopPrank();

   ​

   vm.startPrank(STABILITY_POOL_OWNER);

   stabilityPool.initialize(

   address(rToken),

   address(deToken),

   address(raacToken),

   address(raacMinter),

   address(crvUsdToken),

   address(lendingPool)

   );

   vm.stopPrank();

   ​

   crvUsdToken.mint(address(lendingPool), 1000_000_000e18);

   }

   ​

   function testVerboseUserDataStorageUpdate() public {

   uint256 tokenId = 1;

   uint256 housePrice = 1000e18;

   ​

   vm.startPrank(RAAC_HOUSE_PRICES_OWNER);

   // set oracle

   raacHousePrices.setOracle(RAAC_HOUSE_PRICES_ORACLE);

   vm.stopPrank();

   ​

   vm.startPrank(RAAC_HOUSE_PRICES_ORACLE);

   // house price in USD

   raacHousePrices.setHousePrice(tokenId, housePrice);

   vm.stopPrank();

   ​

   // ensure alice has sufficient balance

   crvUsdToken.mint(ALICE, 1000e18);

   ​

   vm.startPrank(ALICE);

   crvUsdToken.approve(address(lendingPool), 1000e18);

   // deposit 1000e18 into lending pool to have some liquidity

   lendingPool.deposit(1000e18);

   vm.stopPrank();

   ​

   // alice pays in erc20 to mint nft

   erc20Mock.mint(ALICE, housePrice);

   ​

   vm.startPrank(ALICE);

   erc20Mock.approve(address(raacNft), housePrice);

   // mint 1 nft with tokenId 1

   raacNft.mint(tokenId, housePrice);

   vm.stopPrank();

   ​

   vm.startPrank(ALICE);

   raacNft.approve(address(lendingPool), tokenId);

   // deposit nft into lending pool

   lendingPool.depositNFT(tokenId);

   vm.stopPrank();

   ​

   (

   uint256[] memory nftTokenIds,

   uint256 scaledDebtBalance,

   uint256 userCollateralValue,

   bool isUserUnderLiquidation,

   uint256 liquidityIndex,

   uint256 usageIndex,

   uint256 totalLiquidity,

   uint256 totalUsage

   ) = lendingPool.getAllUserData(ALICE);

   ​

   console.log("After depositNFT... ");

   console.log("nftTokenIds: ", nftTokenIds[0]);

   console.log("scaledDebtBalance: ", scaledDebtBalance);

   console.log("userCollateralValue: ", userCollateralValue);

   console.log("isUserUnderLiquidation: ", isUserUnderLiquidation);

   console.log("liquidityIndex: ", liquidityIndex);

   console.log("usageIndex: ", usageIndex);

   console.log("totalLiquidity: ", totalLiquidity);

   console.log("totalUsage: ", totalUsage);

   ​

   vm.startPrank(ALICE);

   lendingPool.withdrawNFT(tokenId);

   vm.stopPrank();

   ​

   (

   nftTokenIds,

   scaledDebtBalance,

   userCollateralValue,

   isUserUnderLiquidation,

   liquidityIndex,

   usageIndex,

   totalLiquidity,

   totalUsage

   ) = lendingPool.getAllUserData(ALICE);

   ​

   console.log("After withdraw... ");

   console.log("scaledDebtBalance: ", scaledDebtBalance);

   console.log("userCollateralValue: ", userCollateralValue);

   console.log("isUserUnderLiquidation: ", isUserUnderLiquidation);

   console.log("liquidityIndex: ", liquidityIndex);

   console.log("usageIndex: ", usageIndex);

   console.log("totalLiquidity: ", totalLiquidity);

   console.log("totalUsage: ", totalUsage);

   ​

   assertEq(nftTokenIds.length, 0);

   }

   }

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:

   forge test --mt testVerboseUserDataStorageUpdate -vv

Impact

• Increased Gas Consumption:
  The inefficient array element removal leads to unnecessary swap operations even in trivial cases (e.g., when only one NFT is deposited or the NFT to be removed is already last). This results in higher gas costs per transaction.

• Reduced Protocol Efficiency:
  As gas costs rise, users may face increased fees, particularly in scenarios with multiple NFT deposits. This inefficiency can impact user experience and reduce the overall economic efficiency of the protocol.

• Scalability Concerns:
  If users deposit large arrays of NFTs, the gas overhead of iterating through the array for every withdrawal becomes more pronounced, potentially leading to scalability issues.

Tools Used

• Manual Review

• Foundry (Forge)

Recommendations

To optimize gas consumption and improve efficiency, modify the NFT removal logic in withdrawNFT to check if the NFT to be removed is already at the last index or if the user has only one NFT. In such cases, directly pop the NFT from the array without performing the swap operation.

Recommended Diff

After applying this patch, rerun the test suite to verify that NFT withdrawals correctly update storage with optimized gas usage.