ParaSpace contest - hyh's results

Lines of code

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

Vulnerability details

NFTFloorOracle's _addAsset() and _addFeeder() truncate the assets and feeders arrays indices to 255, both using uint8 index field in the corresponding structures and performing uint8(assets.length - 1) truncation on the new element addition.

2^8 - 1 looks to be too tight as an all time element count limit. It can be realistically surpassed in a couple years time, especially given multi-asset and multi-feeder nature of the protocol. This way this isn't a theoretical unsafe truncation, but an accounting malfunction that is practically reachable given long enough system lifespan, without any additional requirements as asset/feeder turnaround is a going concern state of the system.

Impact

Once truncation start corrupting the indices the asset/feeder structures will become incorrectly referenced and removal of an element will start to remove another one, permanently breaking up the structures.

This will lead to inability to control these structures and then to Oracle malfunction. This can lead to collateral mispricing. Setting the severity to be medium due to prerequisites.

Proof of Concept

feederPositionMap and assetFeederMap use uint8 indices:

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

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

struct FeederPosition {
    // if feeder registered or not
    bool registered;
    // index in feeder list
    uint8 index;
}

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

    /// @dev feeder map
    // feeder address -> index in feeder list
    mapping(address => FeederPosition) private feederPositionMap;

    ...

    /// @dev Original raw value to aggregate with
    // the NFT contract address -> FeederRegistrar which contains price from each feeder
    mapping(address => FeederRegistrar) public assetFeederMap;

On entry removal both assets array length do not decrease:

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

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

On the contrary, feeders array is being decreased:

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

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

I.e. assets array element is set to zero with delete, but not removed from the array.

This means that assets will only grow over time, and will eventually surpass 2^8 - 1 = 255. That's realistic given that assets here are NFTs, whose variety will increase over time.

Once this happen the truncation will start to corrupt the indices:

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

    function _addAsset(address _asset)
        internal
        onlyWhenAssetNotExisted(_asset)
    {
        assetFeederMap[_asset].registered = true;
        assets.push(_asset);
        assetFeederMap[_asset].index = uint8(assets.length - 1);
        emit AssetAdded(_asset);
    }

This can happen with feeders too, if the count merely surpass 255 with net additions:

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

    function _addFeeder(address _feeder)
        internal
        onlyWhenFeederNotExisted(_feeder)
    {
        feeders.push(_feeder);
        feederPositionMap[_feeder].index = uint8(feeders.length - 1);
        feederPositionMap[_feeder].registered = true;
        _setupRole(UPDATER_ROLE, _feeder);
        emit FeederAdded(_feeder);
    }

This will lead to _removeAsset() and _removeFeeder() clearing another assets/feeders as the assetFeederMap[_asset].index and feederPositionMap[_feeder].index become broken being truncated. It will permanently mess the structures.

Recommended Mitigation Steps

As a simplest measure consider increasing the limit to 2^32 - 1:

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

    function _addAsset(address _asset)
        internal
        onlyWhenAssetNotExisted(_asset)
    {
        assetFeederMap[_asset].registered = true;
        assets.push(_asset);
-       assetFeederMap[_asset].index = uint8(assets.length - 1);
+       assetFeederMap[_asset].index = uint32(assets.length - 1);
        emit AssetAdded(_asset);
    }

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

    function _addFeeder(address _feeder)
        internal
        onlyWhenFeederNotExisted(_feeder)
    {
        feeders.push(_feeder);
-       feederPositionMap[_feeder].index = uint8(feeders.length - 1);
+       feederPositionMap[_feeder].index = uint32(feeders.length - 1);
        feederPositionMap[_feeder].registered = true;
        _setupRole(UPDATER_ROLE, _feeder);
        emit FeederAdded(_feeder);
    }

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

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

struct FeederPosition {
    // if feeder registered or not
    bool registered;
    // index in feeder list
-   uint8 index;
+   uint32 index;
}

Also, consider actually removing assets array element in _removeAsset() via the usual moving of the last element as it's done in _removeFeeder().

Lines of code

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

Vulnerability details

assetFeederMap mapping has elements of the FeederRegistrar structure type, that contains nested feederPrice mapping. When an asset is being removed with _removeAsset(), its assetFeederMap entry is deleted with the plain delete assetFeederMap[_asset] operation, which leaves feederPrice mapping intact.

This way if the asset be added back after removal its old prices will be reused via _combine() given that their timestamps are fresh enough and the corresponding feeders stay active.

Impact

Old prices can be irrelevant in the big enough share of cases. The asset can be removed due to its internal issues that are usually coupled with price disruptions, so it is reasonable to assume that recent prices of a removed asset can be corrupted for the same reason that caused its removal.

Nevertheless these prices will be used as if they were added for this asset after its return. Recency logic will work as usual, so the issue is conditional on config.expirationPeriod being substantial enough, which might be the case for all illiquid assets.

Net impact is incorrect valuation of the corresponding NFTs, that can lead to the liquidation of the healthy accounts, which is the permanent loss of the principal funds for their owners. However, due to prerequisites placing the severity to be medium.

Proof of Concept

_removeAsset() will delete the assetFeederMap entry, setting its elements to zero:

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

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

Notice, that assetFeederMap is the mapping with FeederRegistrar elements:

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

    /// @dev Original raw value to aggregate with
    // the NFT contract address -> FeederRegistrar which contains price from each feeder
    mapping(address => FeederRegistrar) public assetFeederMap;

FeederRegistrar is a structure with a nested feederPrice mapping.

feederPrice nested mapping will not be deleted with delete assetFeederMap[_asset]:

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

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

Per operation docs So if you delete a struct, it will reset all members that are not mappings and also recurse into the members unless they are mappings:

https://docs.soliditylang.org/en/latest/types.html#delete

This way, if the asset be added back its feederPrice mapping will be reused.

On addition of this old _asset only its assetFeederMap[_asset].index be renewed:

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

    function _addAsset(address _asset)
        internal
        onlyWhenAssetNotExisted(_asset)
    {
        assetFeederMap[_asset].registered = true;
        assets.push(_asset);
        assetFeederMap[_asset].index = uint8(assets.length - 1);
        emit AssetAdded(_asset);
    }

This means that the old prices will be immediately used for price construction, given that config.expirationPeriod is big enough:

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

    function _combine(address _asset, uint256 _twap)
        internal
        view
        returns (bool, uint256)
    {
        FeederRegistrar storage feederRegistrar = assetFeederMap[_asset];
        uint256 currentBlock = block.number;
        //first time just use the feeding value
        if (assetPriceMap[_asset].twap == 0) {
            return (true, _twap);
        }
        //use memory here so allocate with maximum length
        uint256 feederSize = feeders.length;
        uint256[] memory validPriceList = new uint256[](feederSize);
        uint256 validNum = 0;
        //aggeregate with price from all feeders
        for (uint256 i = 0; i < feederSize; i++) {
            PriceInformation memory priceInfo = feederRegistrar.feederPrice[
                feeders[i]
            ];
            if (priceInfo.updatedAt > 0) {
                uint256 diffBlock = currentBlock - priceInfo.updatedAt;
                if (diffBlock <= config.expirationPeriod) {
                    validPriceList[validNum] = priceInfo.twap;
                    validNum++;
                }
            }
        }
        if (validNum < MIN_ORACLES_NUM) {
            return (false, assetPriceMap[_asset].twap);
        }
        _quickSort(validPriceList, 0, int256(validNum - 1));
        return (true, validPriceList[validNum / 2]);
    }

Recommended Mitigation Steps

Consider clearing the current list of prices on asset removal, for example:

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

    function _removeAsset(address _asset)
        internal
        onlyWhenAssetExisted(_asset)
    {
+       FeederRegistrar storage feederRegistrar = assetFeederMap[_asset];
+       uint256 feederSize = feeders.length;
+       for (uint256 i = 0; i < feederSize; i++) {
+           delete feederRegistrar.feederPrice[feeders[i]];
+       }
        uint8 assetIndex = feederRegistrar.index;
        delete assets[assetIndex];
        delete assetPriceMap[_asset];
        delete assetFeederMap[_asset];
        emit AssetRemoved(_asset);
    }