code423n4 commented 1 year ago

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.

c4-judge commented 1 year ago

dmvt marked the issue as duplicate of #244