ParaSpace contest - kaliberpoziomka8552's results

Lines of code

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

Vulnerability details

Impact

NFTOracle may get incorrect information because of incorrect management of feeders. Some feeders may never be removable (even if needed), and it is possible to have multiple feeders in the oracle, which cause the possibility of incorrect oracle functioning.

Proof of Concept

After feeder's removal with the function _removeFeeder(...) the feeder's index in feeders[] and feederPositionMap differ, if the removing feeder is the non-last element in list. In the feeders[] list last element's value is copied in the place of _feeder, and then the last element is removed from the list. The moved feeder address now have different index in the feeders[] list but that change is not updated in the mapping feederPositionMap[]. Consider scenario:

1. list feeders[] got 3 elements: {index=0, address=0x123}, {index=1, address=0x456}, {index=2, address=0x789}
2. feederPositionMap[] stores information: {address=0x123, index=0}, {address=0x456, index=1}, {address=0x789, index=2}
3. First element is removed by calling removeFeeder(0x123)
4. feeders[] list state: {index=0, address=0x789}, {index=1, address=0x456}
5. feederPositionMap[] mapping state: {address=0x456, index=1}, {address=0x789, index=2}

This state inconsistency creates two vulnerabilities (following given example):

1. The moved address 0x789 now cannot be removed from the feeders[] list, because it's index stored in a mapping feederPositionMap[] won't match the actual index in the list feeders[] (that is the requirement to remove feeder from the list: if (feederIndex >= 0 && feeders[feederIndex] == _feeder))
2. Mutliple feeders may be in the list. We can add the feeder 0x789 again to the feeders[] list by following sequence:

• removeFeeder(0x789) - the feeder won't be removed from feeders[] (because of the failed requirement), but will be removed from the feederPositionMap[]
• addFeeder(0x789) - this will work becuase the modifier onlyWhenFeederExisted checks if feeder was registered in the mapping feederPositionMap[]

Tools Used

Manual review

Recommended Mitigation Steps

When removing the feeder update the index of moved feeder in the mapping feederPositionMap[] with index of the feeder that is removed.

Lines of code

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

Vulnerability details

Impact

Removing feeders by an arbitrary entity may lead to incorrect functioning of the NFT Oracle. Removing all feeders would stop the functionality of the NFT Oracle, so new prices could not be set.

Proof of Concept

The function removeFeeder(...) (in NFTFloorOracle) has external visibility and no access controls, therefore the function may be called by any address.

Tools Used

Manual review

Recommended Mitigation Steps

Add modifier onlyRole(DEFAULT_ADMIN_ROLE) to the function removeFeeder(...) which would allow only owner to remove feeders.

Lines of code

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

Vulnerability details

Impact

The price of asset may be calculated incorrectly if given asset is added again after being removed. This creates risk of exposing incorrect price and in consequence harm protocol.

Proof of Concept

The function _removeAsset(...) removes asset from the mapping assetFeederMap[] with delete keyword. However, the delete keyword removes only: FeederRegistrar.registered, FeederRegistrar.index and FeederRegistrar.paused. The last member FeederRegistrar.feederPrice is not removed, because it is a mapping. The mapping feederPrice[] contains PriceInformation objects with information about the price. Below is the scenario in which the non-removed PriceInformation object may affect the price:

1. asset1 is added with function addAsset(asset1)
2. feeder1 sets price of asset1 by calling setPrice(asset1, twap1)
3. asset1 is removed with function removeAsset(asset1)
4. asset1 is added again
5. feeder2 sets price of asset1 with function setPrice(asset1, twap2).

After this sequence price of asset1 should consist only from twap2, but in fact the value twap1 will be included as well. This is because the twap1 price (stored in PriceInformation object) was not removed with the rest of data and is included to the median price in the function _combine(...), where median price is computed from all price values provided by feeders, here. The impact of issue is high, but it is not likely to happen., since the combination of actions is complex and must happen within the expirationPeriod which is 6h.

Tools Used

Manual review

Recommended Mitigation Steps

Consider removing all PriceInformation objects from the mapping FeederRegistrar.feederPrice[] when removing asset (FeederRegistrar object) from assetFeederMap.