ParaSpace contest - Jeiwan's results

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/NFTFloorOracle.sol#L331-L334

Vulnerability details

Impact

The NFTFloorOracle admin may not be able to remove a malicious or a hacked feeder due to the flawed feeder indexes updating. The malicious feeder may harm the protocol before the admin adds another feeder, which will unlock removal of the malicious one.

Proof of Concept

The NFTFloorOracle contract maintains a list of allowed feeders who provide prices to the contract. Feeders are stored in two data structures: the feeders array and the feederPositionMap mapping (NFTFloorOracle.sol#L76-L81). The values in the mapping have the index key to link feeders from the mapping to their position in the array (NFTFloorOracle.sol#L311-L313):

feeders.push(_feeder);
feederPositionMap[_feeder].index = uint8(feeders.length - 1);
feederPositionMap[_feeder].registered = true;

When a feeder is removed, it's being replaced by the last feeder in the array. However, that feeder's index is not updated in the mapping (NFTFloorOracle.sol#L330-L335):

uint8 feederIndex = feederPositionMap[_feeder].index;
if (feederIndex >= 0 && feeders[feederIndex] == _feeder) {
    feeders[feederIndex] = feeders[feeders.length - 1];
    feeders.pop();
}
delete feederPositionMap[_feeder];

This causes denial of service when trying to remove the feeder that was previously placed at a removed feeder's position. To unlock the removal of the feeder, the admin would need to add another feeder:

// paraspace-core/test/_oracle_nft_floor_price.spec.ts
it("doesn't update feeder index when a feeder is removed [AUDIT]", async () => {
  const { nftFloorOracle } = testEnv;

  expect(await nftFloorOracle.getFeederSize()).to.eq(7);

  const feeder1 = await nftFloorOracle.feeders(1);
  const feeder6 = await nftFloorOracle.feeders(6); // last feeder

  await nftFloorOracle.removeFeeder(feeder1);

  // feeder6 was placed at the feeder1's position
  expect(await nftFloorOracle.feeders(1)).to.eq(feeder6);

  // Cannot remove feeder6 because its index is still 6 and feeders.length == 6
  await expect(
    nftFloorOracle.removeFeeder(feeder6)
  ).to.be.revertedWith("Array accessed at an out-of-bounds or negative index");

  // Had to add a new feeder to be able to remove feeder6
  await nftFloorOracle.addFeeders([feeder1]);
  expect(await nftFloorOracle.feeders(6)).to.eq(feeder1);

  await nftFloorOracle.removeFeeder(feeder6);

  // However, feeder6 is still in the feeders array
  expect(await nftFloorOracle.feeders(1)).to.eq(feeder6);
});

Exploit Scenario

1. The admin of NFTFloorOracle detects a malicious or a hacked feeder.
2. The malicious feeder was previously placed at the index of a previously removed feeder in the feeders array.
3. The admin calls the removeFeeder function to remove the malicious feeder and gets their transaction reverted.
4. Before the admin figures out what causes the revert and manages to add another feeder to remove the malicious one, the malicious feeder make more damage to the protocol.

Tools Used

Manual review

Recommended Mitigation Steps

Consider this change:

--- a/paraspace-core/contracts/misc/NFTFloorOracle.sol
+++ b/paraspace-core/contracts/misc/NFTFloorOracle.sol
@@ -330,6 +330,7 @@ contract NFTFloorOracle is Initializable, AccessControl, INFTFloorOracle {
         uint8 feederIndex = feederPositionMap[_feeder].index;
         if (feederIndex >= 0 && feeders[feederIndex] == _feeder) {
             feeders[feederIndex] = feeders[feeders.length - 1];
+            feederPositionMap[feeders[feederIndex]].index = feederIndex;
             feeders.pop();
         }
         delete feederPositionMap[_feeder];

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/NFTFloorOracle.sol#L284

Vulnerability details

Impact

Due to an overflow of the assets index a wrong asset can be removed from the assets array, which may impair integration and off-chain monitors that read the array to get the list of supported assets.

Proof of Concept

NFTFloorOracle maintains the list of asset for which feeders are allowed to report prices. Assets are stored in two data structures: the assets array and the assetFeederMap mapping (NFTFloorOracle.sol#L83-L88). The values in the mapping have the index key to link assets from the mapping to their position in the array (NFTFloorOracle.sol#L282-L284):

assetFeederMap[_asset].registered = true;
assets.push(_asset);
assetFeederMap[_asset].index = uint8(assets.length - 1);

However, the maximal value of the index is only 255. Moreover, when an asset is removed the assets array is not shrinked (NFTFloorOracle.sol#L300-L303):

uint8 assetIndex = assetFeederMap[_asset].index;
delete assets[assetIndex];
delete assetPriceMap[_asset];
delete assetFeederMap[_asset];

Thus, for the index to overflow, the contract needs to have 256 assets historically, not currently. It seems realistic that over multiple years the contract will have more than 256 assets, including removed ones.

// paraspace-core/test/_oracle_nft_floor_price.spec.ts
it("allows asset index to overflow [AUDIT]", async () => {
  const { nftFloorOracle } = testEnv;

  const numToAddress = (num) => '0x' + num.toString(16).padStart(40, '0');

  // There are 10 assets, so we'll add 246 more.
  const length = 246;
  const assets = Array.from({ length }, (_, i) => numToAddress(0x31337 + i));

  await nftFloorOracle.addAssets(assets);

  // The last asset has index 255.
  const lastAsset = assets[assets.length - 1];
  expect((await nftFloorOracle.assetFeederMap(lastAsset)).index).to.eq(255);

  // Removing an asset won't reduce the index.
  await nftFloorOracle.removeAsset(lastAsset);

  // Adding a new asset to overflow the index.
  const newAsset = numToAddress(0x31337 + length + 1);
  await nftFloorOracle.addAssets([newAsset]);
  expect((await nftFloorOracle.assetFeederMap(newAsset)).index).to.eq(0);

  const firstAsset = await nftFloorOracle.assets(0);
  expect(firstAsset).to.eq("0xD6C850aeBFDC46D7F4c207e445cC0d6B0919BDBe");

  // Removing the last asset...
  await nftFloorOracle.removeAsset(newAsset);

  // ...however, the first asset was removed from the array.
  expect(await nftFloorOracle.assets(0)).to.eq("0x0000000000000000000000000000000000000000");

  // The first asset is still registered though.
  expect(await (await nftFloorOracle.assetFeederMap(firstAsset)).registered).to.be.true;
  expect(await (await nftFloorOracle.assetFeederMap(newAsset)).registered).to.be.false;
});

Tools Used

Manual review

Recommended Mitigation Steps

Consider using a type with a higher maximal value for the index (e.g. uint16), or consider shrinking the assets array when an asset is removed (similarly to the feeders array: NFTFloorOracle.sol#L332-L333).

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/NFTFloorOracle.sol#L405-L407 https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/NFTFloorOracle.sol#L361-L364

Vulnerability details

Impact

A first feeder that reports a price to an asset may intentionally or mistakenly report a price that's significantly deviated from the real one. Due to the max price deviation check other feeders won't be able to report correct prices.

Proof of Concept

NFTFloorOracle allows trusted feeders to set prices of NFT tokens. The setPrice function accumulates prices from multiple feeders (the minimal number of feeders is 3) and lets the first feeder to dictate the price, bypassing the deviation and the median price checks (NFTFloorOracle.sol#L361-L364, NFTFloorOracle.sol#L404-L407):

//first price is always valid
if (_priorTwap == 0 || _updatedAt == 0) {
    return true;
}

//first time just use the feeding value
if (assetPriceMap[_asset].twap == 0) {
    return (true, _twap);
}

This allows a first feeder to mistakenly or intentionally set an unrealistic price and disable prices reporting for other feeders

// paraspace-core/test/_oracle_nft_floor_price.spec.ts
it("allows first feeder to disrupt prices reporting [AUDIT]", async () => {
  const { nftFloorOracle, users: [user1, user2] } = testEnv;

  const maxUint256 = "0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff";

  await nftFloorOracle.connect(user1.signer).setPrice(mockToken.address, maxUint256);
  expect(await nftFloorOracle.getPrice(mockToken.address)).to.eq(maxUint256);

  await expect(
    nftFloorOracle.connect(user2.signer).setPrice(mockToken.address, parseEther("10").toString())
  ).to.be.revertedWith("Arithmetic operation underflowed or overflowed outside of an unchecked block");
});

Another scenario is when the first price is deviated from the real price by more than the max price deviation setting:

// paraspace-core/test/_oracle_nft_floor_price.spec.ts
i("allows first feeder to disrupt prices reporting [AUDIT]", async () => {
  const { nftFloorOracle, users: [user1, user2] } = testEnv;

  // Max price deviation is 20x
  expect((await nftFloorOracle.config()).maxPriceDeviation).to.eq("2000");

  // Setting initial price to 25x of the real price.
  const realPrice = parseEther("10");
  const initialPrice = realPrice.mul("25");

  await nftFloorOracle.connect(user1.signer).setPrice(mockToken.address, initialPrice);
  expect(await nftFloorOracle.getPrice(mockToken.address)).to.eq(initialPrice);

  await expect(
    nftFloorOracle.connect(user2.signer).setPrice(mockToken.address, realPrice)
  ).to.be.revertedWith("NFTOracle: invalid price data");
});

It also goes without saying that an invalid price will disrupt collateral value calculation of affected NFT tokens.

Tools Used

Manual review

Recommended Mitigation Steps

Consider adding sanity checks to the initial price. Also, consider setting the initial price to the median of prices reported by at least MIN_ORACLES_NUM oracles.

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/tokenization/libraries/MintableERC721Logic.sol#L248 https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/tokenization/libraries/MintableERC721Logic.sol#L107

Vulnerability details

Impact

A malicious actor can disallow supplying of Uniswap V3 NFT tokens as collateral for any user. This can be exploited as a front-running attack that disallows a borrower to provide more Uniswap V3 LP tokens as collateral and save their collateral from being liquidated.

Proof of Concept

The protocol allows users to provide Uniswap V3 NFT tokens as collateral. Since these tokens can be minted freely, a malicious actor may provide a huge number of such tokens as collateral and impose high gas consumption by the calculateUserAccountData function of GenericLogic (which calculates user's collateral and debt value). Potentially, this can lead to out of gas errors during collateral/debt values calculation. To protect against this attack, a limit on the number of Uniswap V3 NFTs a user can provide as collateral was added (NTokenUniswapV3.sol#L33-L35):

constructor(IPool pool) NToken(pool, true) {
    _ERC721Data.balanceLimit = 30;
}

The limit is enforced during Uniswap V3 NToken minting and transferring (MintableERC721Logic.sol#L247-L248, MintableERC721Logic.sol#L106-L107, MintableERC721Logic.sol#L402-L414):

uint64 newBalance = oldBalance + uint64(tokenData.length);
_checkBalanceLimit(erc721Data, ATOMIC_PRICING, newBalance);

uint64 newRecipientBalance = oldRecipientBalance + 1;
_checkBalanceLimit(erc721Data, ATOMIC_PRICING, newRecipientBalance);

function _checkBalanceLimit(
    MintableERC721Data storage erc721Data,
    bool ATOMIC_PRICING,
    uint64 balance
) private view {
    if (ATOMIC_PRICING) {
        uint64 balanceLimit = erc721Data.balanceLimit;
        require(
            balanceLimit == 0 || balance <= balanceLimit,
            Errors.NTOKEN_BALANCE_EXCEEDED
        );
    }
}

While protecting from the attack mentioned above, this creates a new attack vector: a malicious actor can send many Uniswap V3 NTokens to a victim and lock them out of adding more Uniswap V3 NTokens as collateral. This attack is viable and cheap because Uniswap V3 NFTs can have 0 liquidity, thus the attacker would only need to pay transaction fees, which are cheap on L2 networks.

Exploit Scenario

1. Bob provides Uniswap V3 NFTs as collateral on Paraspace and borrows other tokens.
2. Due to extreme market conditions, Bob's health factor is getting closer to the liquidation threshold.
3. Bob, while being an active liquidity provider on Uniswap, supplies another Uniswap V3 NFT as a collateral.
4. Alice runs liquidation and front-running bots. One of her bots notices that Bob is trying to increase his collateral value with a Uniswap V3 NFT.
5. Alice's bot mints multiple Uniswap V3 NFTs using the same asset tokens by removing all liquidity from tokens after they were minted.
6. Alice's bot supplies the newly minted NFTs on behalf of Bob. Bob's balance of Uniswap V3 NFTs reaches the maximal allowed.
7. Bob's transaction to add another Uniswap V3 NFT as collateral fails due to the balance limit being reached.
8. While Bob is trying to figure out what's happened and before he provides collateral in other tokens or withdraws the empty NTokens, Alice's bot liquidates Bob's debt.

// paraspace-core/test/_uniswapv3_position_control.spec.ts
it("allows to fill balanceLimit of another user cheaply [AUDIT]", async () => {
  const {
    users: [user1, user2],
    dai,
    weth,
    nftPositionManager,
    nUniswapV3,
    pool
  } = testEnv;

  // user1 has 1 token initially.
  expect(await nUniswapV3.balanceOf(user1.address)).to.eq("1");

  // Set the limit to 5 tokens so the test runs faster.
  let totalTokens = 5;
  await waitForTx(await nUniswapV3.setBalanceLimit(totalTokens));

  const userDaiAmount = await convertToCurrencyDecimals(dai.address, "10000");
  const userWethAmount = await convertToCurrencyDecimals(weth.address, "10");
  await fund({ token: dai, user: user2, amount: userDaiAmount });
  await fund({ token: weth, user: user2, amount: userWethAmount });
  let nft = nftPositionManager.connect(user2.signer);
  await approveTo({ target: nftPositionManager.address, token: dai, user: user2 });
  await approveTo({ target: nftPositionManager.address, token: weth, user: user2 });
  const fee = 3000;
  const tickSpacing = fee / 50;
  const lowerPrice = encodeSqrtRatioX96(1, 10000);
  const upperPrice = encodeSqrtRatioX96(1, 100);
  await nft.setApprovalForAll(nftPositionManager.address, true);
  await nft.setApprovalForAll(pool.address, true);
  const MaxUint128 = DRE.ethers.BigNumber.from(2).pow(128).sub(1);

  let daiAvailable, wethAvailable;

  // user2 is going to mint 4 Uniswap V3 NFTs using the same amount of DAI and WETH.
  // After each mint, user2 removes all liquidity from a token and uses it to mint
  // next token.
  for (let tokenId = 2; tokenId <= totalTokens; tokenId++) {
    daiAvailable = await dai.balanceOf(user2.address);
    wethAvailable = await weth.balanceOf(user2.address);

    await mintNewPosition({
      nft: nft,
      token0: dai,
      token1: weth,
      fee: fee,
      user: user2,
      tickSpacing: tickSpacing,
      lowerPrice,
      upperPrice,
      token0Amount: daiAvailable,
      token1Amount: wethAvailable,
    });

    const liquidity = (await nftPositionManager.positions(tokenId)).liquidity;

    await nft.decreaseLiquidity({
      tokenId: tokenId,
      liquidity: liquidity,
      amount0Min: 0,
      amount1Min: 0,
      deadline: 2659537628,
    });

    await nft.collect({
      tokenId: tokenId,
      recipient: user2.address,
      amount0Max: MaxUint128,
      amount1Max: MaxUint128,
    });

    expect((await nftPositionManager.positions(tokenId)).liquidity).to.eq("0");
  }

  // user2 supplies the 4 UniV3 NFTs to user1.
  const tokenData = Array.from({ length: totalTokens - 1 }, (_, i) => {
    return {
      tokenId: i + 2,
      useAsCollateral: true
    }
  });
  await waitForTx(
    await pool
      .connect(user2.signer)
      .supplyERC721(
        nftPositionManager.address,
        tokenData,
        user1.address,
        0,
        {
          gasLimit: 12_450_000,
        }
      )
  );

  expect(await nUniswapV3.balanceOf(user2.address)).to.eq(0);
  expect(await nUniswapV3.balanceOf(user1.address)).to.eq(totalTokens);

  // user1 tries to supply another UniV3 NFT but fails, since the limit has
  // already been reached.
  await fund({ token: dai, user: user1, amount: userDaiAmount });
  await fund({ token: weth, user: user1, amount: userWethAmount });
  nft = nftPositionManager.connect(user1.signer);
  await mintNewPosition({
    nft: nft,
    token0: dai,
    token1: weth,
    fee: fee,
    user: user1,
    tickSpacing: tickSpacing,
    lowerPrice,
    upperPrice,
    token0Amount: userDaiAmount,
    token1Amount: userWethAmount,
  });

  await expect(
    pool
      .connect(user1.signer)
      .supplyERC721(
        nftPositionManager.address,
        [{ tokenId: totalTokens + 1, useAsCollateral: true }],
        user1.address,
        0,
        {
          gasLimit: 12_450_000,
        }
      )
  ).to.be.revertedWith("120");  //ntoken balance exceed limit.

  expect(await nUniswapV3.balanceOf(user1.address)).to.eq(totalTokens);

  // The cost of the attack was low. user2's balance of DAI and WETH
  // hasn't changed, only the rounding error of Uniswap V3 was subtracted.
  expect(await dai.balanceOf(user2.address)).to.eq("9999999999999999999996");
  expect(await weth.balanceOf(user2.address)).to.eq("9999999999999999996");
});

Tools Used

Manual review

Recommended Mitigation Steps

Consider disallowing supplying Uniswap V3 NFTs that have 0 liquidity and removing the entire liquidity from tokens that have already been supplied. Setting a minimal required liquidity for a Uniswap V3 NFT will make this attack more costly, however it won't remove the attack vector entirely.

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/marketplaces/LooksRareAdapter.sol#L51-L54 https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/logic/MarketplaceLogic.sol#L564-L565

Vulnerability details

Impact

Users won't be able to buy NFTs from LooksRare via the Paraspace Marketplace and pay with WETH when MakerOrder currency is set to WETH.

Proof of Concept

The Paraspace Marketplace allows users to buy NFTs from third-party marketplaces (LooksRare, Seaport, X2Y2) using funds borrowed from Paraspace. The mechanism of buying tokens requires a MakerOrder: a data structure that's created by the seller and posted on a third-party exchange and that contains all the information about the order. Besides other fields, MakerOrder contains the currency field, which sets the currency the buyer is willing to receive payment in (OrderTypes.sol#L20):

struct MakerOrder {
    ...
    address currency; // currency (e.g., WETH)
    ...
}

While WETH is supported by LooksRare as a currency of orders, the LooksRare adapter of Paraspace converts it to the native (e.g. ETH) currency: if order's currency is WETH, the currency of consideration is set to the native currency (LooksRareAdapter.sol#L49-L62):

ItemType itemType = ItemType.ERC20;
address token = makerAsk.currency;
if (token == weth) {
    itemType = ItemType.NATIVE;
    token = address(0);
}
consideration[0] = ConsiderationItem(
    itemType,
    token,
    0,
    makerAsk.price, // TODO: take minPercentageToAsk into account
    makerAsk.price,
    payable(takerBid.taker)
);

When users call the buyWithCredit function, they provide credit parameters: token address and amount (besides others) (PoolMarketplace.sol#L71-L76, DataTypes.sol#L296-L303):

function buyWithCredit(
    bytes32 marketplaceId,
    bytes calldata payload,
    DataTypes.Credit calldata credit,
    uint16 referralCode
) external payable virtual override nonReentrant {

struct Credit {
    address token;
    uint256 amount;
    bytes orderId;
    uint8 v;
    bytes32 r;
    bytes32 s;
}

Deep inside the buyWithCredit function, isETH flag is set when the credit token specified by user is address(0) (MarketplaceLogic.sol#L564-L566):

vars.isETH = params.credit.token == address(0);
vars.creditToken = vars.isETH ? params.weth : params.credit.token;
vars.creditAmount = params.credit.amount;

Finally, before giving a credit, consideration type and credit token are checked (MarketplaceLogic.sol#L388-L392):

require(
    item.itemType == ItemType.ERC20 ||
        (vars.isETH && item.itemType == ItemType.NATIVE),
    Errors.INVALID_ASSET_TYPE
);

This is what happens when a user tries to buy an NFT from LooksRare and pays with WETH as the maker order requires:

1. User calls buyWithCredit and sets credit token to WETH.
2. The currency of the consideration is changed to the native currency, and the consideration token is set to address(0).
3. var.isETH is not set since the credit token is WETH, not address(0).
4. The consideration type and credit token check fails because the type of the consideration is NATIVE but var.isETH is not set.
5. As a result, the call to buyWithCredit reverts and the user cannot buy a token while correctly using the API.

it("fails when trying to buy a token on LooksRare with WETH [AUDIT]", async () => {
  const {
    doodles,
    pool,
    weth,
    users: [maker, taker, middleman],
  } = await loadFixture(testEnvFixture);
  const payNowNumber = "8";
  const creditNumber = "2";
  const payNowAmount = await convertToCurrencyDecimals(
    weth.address,
    payNowNumber
  );
  const creditAmount = await convertToCurrencyDecimals(
    weth.address,
    creditNumber
  );
  const startAmount = payNowAmount.add(creditAmount);
  const nftId = 0;
  // mint WETH to offer
  await mintAndValidate(weth, payNowNumber, taker);
  // middleman supplies DAI to pool to be borrowed by offer later
  await supplyAndValidate(weth, creditNumber, middleman, true);
  // maker mint mayc
  await mintAndValidate(doodles, "1", maker);
  // approve
  await waitForTx(
    await weth.connect(taker.signer).approve(pool.address, payNowAmount)
  );

  await expect(
    executeLooksrareBuyWithCredit(
      doodles,
      weth as MintableERC20,
      startAmount,
      creditAmount,
      nftId,
      maker,
      taker
    )
  ).to.be.revertedWith('93'); // invalid asset type for action.
});

Tools Used

Manual review

Recommended Mitigation Steps

Consider removing the WETH to NATIVE conversion in the LooksRare adapter. Alternatively, consider converting WETH to ETH seamlessly, without forcing users to send ETH instead of WETH when maker order requires WETH.

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/ParaSpaceOracle.sol#L128

Vulnerability details

Impact

The deprecated interface doesn't provide additional data that can make integration with Chainlink more reliable. This can potentially lead the protocol to be more sensitive to disruptions or non-standard behavior of Chainlink oracles.

Proof of Concept

The getAssetPrice function of the ParaSpaceOracle oracle uses a deprecated Chainlink interface (ParaSpaceOracle.sol#L126-L129):

IEACAggregatorProxy source = IEACAggregatorProxy(assetsSources[asset]);
if (address(source) != address(0)) {
    price = uint256(source.latestAnswer());
}

As per the Chainlink documentation, the latestAnswer function is deprecated.

This interface doesn't allow to check the timestamp of when the latest price was reported and ensure that the price is not stale. Also, the Chainlink data feed aggregator provides minimal and maximal price values per each supported token to improve validation of prices returned by oracles.

Tools Used

Manual review

Recommended Mitigation Steps

Consider using the latestRoundData function instead of latestAnswer and consider following the Monitoring Data Feeds recommendations by Chainlink.

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/logic/SupplyLogic.sol#L380-L383

Vulnerability details

Impact

In the worst scenario, users may be liquidated due to an NFT token not being automatically enabled as collateral on transfer. In the majority of cases, users may be confused that some of their NFT tokens are not enabled as collateral.

Proof of Concept

Whenever a user supplies ERC20 or ERC721 tokens, the tokens get automatically enabled as collateral (if the user hadn't had these tokens before supplying) (SupplyLogic.sol#L110-L123, SupplyLogic.sol#L147-L155):

bool isFirstSupply = IPToken(reserveCache.xTokenAddress).mint(
    msg.sender,
    params.onBehalfOf,
    params.amount,
    reserveCache.nextLiquidityIndex
);

if (isFirstSupply) {
    userConfig.setUsingAsCollateral(reserve.id, true);
    emit ReserveUsedAsCollateralEnabled(
        params.asset,
        params.onBehalfOf
    );
}

bool isFirstSupplyCollateral = (oldCollateralizedBalance == 0 &&
    newCollateralizedBalance > 0);
if (isFirstSupplyCollateral) {
    userConfig.setUsingAsCollateral(reserveId, true);
    emit ReserveUsedAsCollateralEnabled(
        params.asset,
        params.onBehalfOf
    );
}

This also happens when a user receives PTokens (SupplyLogic.sol#L500-L509):

if (params.balanceToBefore == 0) {
    DataTypes.UserConfigurationMap
        storage toConfig = usersConfig[params.to];

    toConfig.setUsingAsCollateral(reserveId, true);
    emit ReserveUsedAsCollateralEnabled(
        params.asset,
        params.to
    );
}

However, this doesn't happen when NTokens are received (SupplyLogic.sol#L561-L567):

if (params.balanceFromBefore == 1) {
    fromConfig.setUsingAsCollateral(reserveId, false);
    emit ReserveUsedAsCollateralDisabled(
        params.asset,
        params.from
    );
}

// @audit should also enable collateral for receiver if `params.balanceToBefore == 0`

Consider this scenario:

1. Due to a market condition, Bob is close to being liquidated.
2. Bob sends an NToken from his another account to increase his collateral value. The current Bob's account doesn't hold any NTokens from this collection.
3. Bob thinks that he's safe now because he knows that collateral is enabled automatically on PToken transfers and NFT supplies.
4. Bob gets liquidated while having the NToken in the account. But since the token wasn't enabled as collateral, its value wasn't counted.
5. Even though Bob can enable collateral manually, he still might not do that expecting that it was enabled automatically.

// paraspace-core/_pool_core_erc721_supply.spec.ts
it("doesn't enable collateral on ERC721 receiving [AUDIT]", async () => {
  const {
    users: [user1, user2],
    bayc,
    pool,
    protocolDataProvider,
    nBAYC
  } = await loadFixture(testEnvFixture);

  // Ensure user1 has 0 collateral value initially.
  let [totalCollateralOfUser1, , , , ,] = await pool.getUserAccountData(user1.address);
  expect(totalCollateralOfUser1).to.eq("0");

  // user1 supplies an NFT as collateral and gets it enabled.
  await supplyAndValidate(bayc, "1", user1, true);
  expect(await nBAYC.ownerOf(0)).to.eq(user1.address);
  let [, , , , , , isCollateralEnabledForUser1] = await protocolDataProvider.getUserReserveData(bayc.address, user1.address);
  expect(isCollateralEnabledForUser1).to.be.true;

  // The NToken provides its value as collateral.
  [totalCollateralOfUser1, , , , ,] = await pool.getUserAccountData(user1.address);
  expect(totalCollateralOfUser1).to.eq("101000000000000000000");

  // user1 transfer the NToken to user2.
  await nBAYC.connect(user1.signer)["safeTransferFrom(address,address,uint256)"](user1.address, user2.address, 0);

  // The NToken wasn't enabled as collateral for user2.
  let [, , , , , , isCollateralEnabledForUser2] = await protocolDataProvider.getUserReserveData(bayc.address, user2.address);
  expect(isCollateralEnabledForUser2).to.be.false;

  // The NToken is not counted as collateral of user2.
  let [totalCollateralOfUser2, , , , ,] = await pool.getUserAccountData(user2.address);
  expect(totalCollateralOfUser2).to.eq("0");

  // However, user2 is the owner of the NToken.
  expect(await nBAYC.ownerOf(0)).to.eq(user2.address);
});

Tools Used

Manual review

Recommended Mitigation Steps

Consider enabling NToken collateral on receival, similarly to how it's done for PTokens.