Downcast can lead to overflow and impact the functionality to remove an asset in `NFTFloorOracle`

[code423n4]

Lines of code

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

Vulnerability details

Impact

When adding a new asset, the index for the last asset will be saved and downcasted on L284. If the number of assets is bigger than 256, the downcasting operation will lead to a silent overflow. Consequentially, the incorrect index will be stored.

This can impact the functionality to remove an asset (because the index for the asset to be removed will be retrieved while deleting an asset).

Proof of concept

Take the following case:

1. Assume 256 items are added.
2. When adding item #257, the index saved will be zero, due to the overflow. E.g. uint8(257 - 1) = 0
3. While trying to remove asset #257, the asset with index 0 will be removed instead.

On the tests setup, 10 assets are added initialy. The first asset is DOODLE. The following POC demonstrates what would happen if 247 are added after the initial 10 items. Assume items from index 11 to 256 are random addresses (for demonstration), and the asset for index 257 is DAI.

import crypto from "crypto"; // To generate random Addresses
import {utils, constants} from "ethers"; // To get constants.AddressZero

  // Copying the following test case into `_oracle_nft_floor_price.spec.ts` should work.

  it("Adding more than 256 assets can break the functionality to remote assets", async () => {
    const { nftFloorOracle, dai} = testEnv;

    // Add assets from index 11 to 256.
    // Note that 10 assets are already added initially on the current tests setup. 
    const addresses: string[] = [];
    for (let i = 11; i <= 256; ++i) {
      // Generate a random address.
      addresses.push(`0x${crypto.randomBytes(20).toString("hex")}`);
    }
    try {
      await nftFloorOracle.addAssets(addresses);
    } catch (e) {}

    // Confirming that the first item is DOODLE.
    const doodleAddress = '0xD6C850aeBFDC46D7F4c207e445cC0d6B0919BDBe';
    expect(await nftFloorOracle.assets(0)).to.equal(doodleAddress);

    // Add DAI as item #257.
    try {
      await nftFloorOracle.addAssets([dai.address]);
    } catch (e) {}

    // Removing DAI will accidentaly remove DOODLE.
    await nftFloorOracle.removeAsset(dai.address);

    // This assertation will pass to demonstrate the bug.
    // First asset will no longer be DOODLE and it will become address zero.
    expect(await nftFloorOracle.assets(0)).to.equal(constants.AddressZero);
  });

Recommended Mitigation Steps

If it's expected for the number of assets to never exeed 256 items, consider adding a SafeCast function.

However, if the number is expected to go beyond 256 items, consider replacing uint8 with uint256 for the FeederRegistrar.

Note that the same overlflow problem is present for the add and remove feeders functionalities. Therefore, the FeederPosition should also use uint256 instead of uint8.

[c4-judge]

dmvt marked the issue as duplicate of #209

[c4-judge]

dmvt changed the severity to 3 (High Risk)

[c4-judge]

dmvt marked the issue as satisfactory