ParaSpace contest - shark's results

Lines of code

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

Vulnerability details

Impact

In NFTFloorOracle.sol, the function removeFeeder() is intended to be called only by the owner. However, anyone could call removeFeeder() to remove a feeder. As such, an attacker could continually call removeFeeder() to remove all feeders. This could cause chaos to any contracts that depend on these feeders.

Proof of Concept

File: NFTFloorOracle.sol Line 167-172

As seen below, there is nothing checking if the caller is the owner

165    /// @notice Allows owner to remove feeder.
166    /// @param _feeder feeder to remove
167    function removeFeeder(address _feeder)
168        external
169        onlyWhenFeederExisted(_feeder)
170    {
171        _removeFeeder(_feeder);
172    }

Tools Used

No tools used.

Recommended Mitigation Steps

Add the onlyRole(DEFAULT_ADMIN_ROLE) modifier.

For example:

    function removeFeeder(address _feeder)
        external
        onlyWhenFeederExisted(_feeder)
        onlyRole(DEFAULT_ADMIN_ROLE)
    {
        _removeFeeder(_feeder);
    }

1. Redundant Conditional Check

File: NFTFloorOracle.sol Line 331

        if (feederIndex >= 0 && feeders[feederIndex] == _feeder) {

Because feederIndex is type uint8, it can never go below zero. As such, the check feederIndex >= 0 is redundant

Instead, you can refactor to the following:

        if (feeders[feederIndex] == _feeder) {

2. Typo mistakes

File: AuctionLogic.sol Line 34

tsatr -> start

     * @notice Function to tsatr auction on an ERC721 of a position if its ERC721 Health Factor drops below 1.

File: ReserveLogic.sol Line 163, Line 261

Duplicated "reserve reserve"

     * @param reserve The reserve reserve to be updated

3. Use delete to clear variables instead of zero assignment

A better way to indicate that you are clearing a variable is to use the delete keyword.

For example:

File: MarketplaceLogic.sol Line 409 - 410

            price = 0;
            downpayment = 0;

The above could be refactored to:

            delete price;
            delete downpayment;

4. Events are missing old value

Consider having events that update state variables emit both the old and new values.

Here are some instances of this issue:

File ParaSpaceOracle.sol Line 62

File: ParaSpaceOracle.sol Line 111

5. Missing Zero address/value checks

Zero address/value checks should be implemented for security. However, such checks are missing in many contracts.

Here are some instances:

File: ParaSpaceOracle.sol Line 49-63

File: UniswapV3OracleWrapper.sol Line 24-26

File: DefaultReserveAuctionStrategy.sol Line 50-64

6. Avoid the use of _setupRole() as it is deprecated

As said in the OpenZeppelin's docs, "This function is deprecated in favor of _grantRole."

There are 3 instances of this issue:

File: NFTFloorOracle.sol (Line 104, Line 106, Line 314)

104:         _setupRole(DEFAULT_ADMIN_ROLE, _admin);
...
106:        _setupRole(UPDATER_ROLE, _admin);
...
314:        _setupRole(UPDATER_ROLE, _feeder);

Consider replacing _setupRole() with _grantRole():

104:         _grantRole(DEFAULT_ADMIN_ROLE, _admin);
...
106:        _grantRole(UPDATER_ROLE, _admin);
...
314:        _grantRole(UPDATER_ROLE, _feeder);

7. Unresolved TODOs

Consider resolving open TODOs before deployment.

Here are some examples of this issue:

File: UniswapV3OracleWrapper.sol Line 238

File: MarketplaceLogic.sol Line 442

File: LooksRareAdapter.sol Line 59

8. Limit line length

To comply with the Solidity Style Guide, lines should be limited to 120 characters max.

For example, the instance below is 243 characters past the recommended limit

File: LiquidationLogic.sol Line 603

9. Unused imports

Contracts that are imported and not used anywhere in the code are most likely an accident due to incomplete refactoring. Such imports can lead to confusion by readers.

For example, here are some instances:

File: X2Y2Adapter.sol (Line 6, Line 7, Line 8, Line 10, Line 14, Line 16

6: import {OrderTypes} from "../../dependencies/looksrare/contracts/libraries/OrderTypes.sol";

7: import {SeaportInterface} from "../../dependencies/seaport/contracts/interfaces/SeaportInterface.sol";

8: import {ILooksRareExchange} from "../../dependencies/looksrare/contracts/interfaces/ILooksRareExchange.sol";

10: import {SignatureChecker} from "../../dependencies/looksrare/contracts/libraries/SignatureChecker.sol";

14: import {IERC1271} from "../../dependencies/openzeppelin/contracts/IERC1271.sol";

16: import {PoolStorage} from "../../protocol/pool/PoolStorage.sol";

File: LooksRareAdapter.sol (Line 7, Line 9, Line 13, Line 15)

7: import {SeaportInterface} from "../../dependencies/seaport/contracts/interfaces/SeaportInterface.sol";

9: import {SignatureChecker} from "../../dependencies/looksrare/contracts/libraries/SignatureChecker.sol";

13: import {IERC1271} from "../../dependencies/openzeppelin/contracts/IERC1271.sol";

15: import {PoolStorage} from "../../protocol/pool/PoolStorage.sol";

File: SeaportAdapter.sol (Line 6, Line 8, Line 9, Line 10, Line 13, Line 15)

6: import {OrderTypes} from "../../dependencies/looksrare/contracts/libraries/OrderTypes.sol";

8: import {ILooksRareExchange} from "../../dependencies/looksrare/contracts/interfaces/ILooksRareExchange.sol";

9: import {SignatureChecker} from "../../dependencies/looksrare/contracts/libraries/SignatureChecker.sol";

10: import {ConsiderationItem} from "../../dependencies/seaport/contracts/lib/ConsiderationStructs.sol";

13: import {IERC1271} from "../../dependencies/openzeppelin/contracts/IERC1271.sol";

15: import {PoolStorage} from "../../protocol/pool/PoolStorage.sol";