ParaSpace contest - pzeus's results

Lines of code

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

Vulnerability details

Impact

There is no modifier to check if the msg.caller has the required role in order to execute the function. Therefore, removeFeeder() could be called by everyone and setting a feeder of their liking and potentially to compromise critically the whole protocol

Proof of Concept

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

Tools Used

Manual review

Recommended Mitigation Steps

Add onlyRole(DEFAULT_ADMIN_ROLE) modifer to the removeFeeder() method

Issue

In general, missing a lot of dev and not only comments that will suit greatly to all auditors, future audits or even new developers that will be potentially onboarded Although, this does not have any impact on the protocol itself, I think this will smoothen up a lot of things in the furure

Examples

Some examples are SeaportAdapter.sol and X2Y2Adapter.sol Also, in a lot of cases, the function behaviour and params explanation are placed inside the interface contracts of the contracts that are implementing those functionalities. This makes the whole process uncomfortable and annoying

There are also examples of no comments on complex math calculations DefaultReserveAuctionStrategy.sol

Recommendation

Add missing useful comments where needed

Issue

Unnecessary onlyWhenFeederExisted(_feeder) modifier used in the internal _removeFeeder() function. The same modifier is used inside the external removeFeeder() which calls the internal _removeFeeder() function

Example

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

Recommendation

Remove onlyWhenFeederExisted(_feeder) modifier from the internal _removeFeeder() function since it is only called by the external removeFeeder()

Issue

Unnecessary onlyWhenAssetExisted(_asset) modifier used in the internal _removeAsset() function. The same modifier is used inside the external _removeAsset() which calls the internal _removeAsset() function

Example

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

Recommendation

Remove onlyWhenAssetExisted(_asset) modifier from the internal _removeAsset() function since it is only called by the external removeAsset()

Issue

There are getters which can be reused inside a contract

Examples

ParaSpaceOracle.sol 1 & 2

Recommendation

Use getSourceOfAsset() on every assetsSources[asset] occurence inside ParaSpaceOracle.sol contract and getFallbackOracle() on every address(_fallbackOracle) occurrence

Issue

There are a lot of places where an event should be emitted in order to point that some action was executed. Since using the events we can consider whether or not something should happen on the front-end or search for an information by event indexed values, there should be emitted events on important protocol actions

Examples

PoolParameters.sol 1, 2, 3, 4, 5 & 6

Recommendation

Declare events and use them at the proper places

Issue

There are missing access modifiers on public/exernal methods. In these cases, it can result in exposing customers data or some important protocol variables

Examples

PoolParameters.sol 1 & 2 PoolLogic.sol - 1

Recommendation

Use access modifiers in order to restrict who can call the functions