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.
Asset removal leaks previous asset prices which will be used again when asset is readded.
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);
}
Manual audit
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.
The text was updated successfully, but these errors were encountered:
All reactions