ParaSpace contest - nicobevi's results

Lines of code

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

Vulnerability details

Function removeFeeder() on NFTFloorOracle.sol does not validate that the sender has the required role DEFAULT_ADMIN_ROLE allowing anyone to call this function successfully. This could generate unexpected behaviours on the oracle (DDOS attacks blocking feeder updates)

   /// @notice Allows owner to remove feeder.
    /// @param _feeder feeder to remove
    function removeFeeder(address _feeder)
        external
        onlyWhenFeederExisted(_feeder)
    {
        _removeFeeder(_feeder);
    }

Add the validation to this function

    /// @notice Allows owner to remove feeder.
    /// @param _feeder feeder to remove
    function removeFeeder(address _feeder)
        external
        onlyWhenFeederExisted(_feeder)
        onlyRole(DEFAULT_ADMIN_ROLE)
    {
        _removeFeeder(_feeder);
    }

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/3e4e2e4e1322482dcdc6d342f8799cd44a3e42f5/paraspace-core/contracts/misc/NFTFloorOracle.sol#L236-L248 https://github.com/code-423n4/2022-11-paraspace/blob/3e4e2e4e1322482dcdc6d342f8799cd44a3e42f5/paraspace-core/contracts/misc/NFTFloorOracle.sol#L195-L216

Vulnerability details

If an asset is whitelisted in the contract but any oracle has returned a floor price yet, then the call to getPrice() would return a price 0.

This opens a time window when an attacker could take advantage of the assets with price 0.

Calls to getPrice() should revert in case that the oracle has no price records yet to avoid any unexpected behaviour until at least one price is registered in the contract.

POC: Foundry test

    // SPDX-License-Identifier: UNLICENSED
    pragma solidity 0.8.10;

    import "forge-std/Vm.sol";
    import "forge-std/Test.sol";
    import "forge-std/console.sol";

    import {NFTFloorOracle} from "../../../src/paraspace/misc/NFTFloorOracle.sol";

    contract NFTFloorOracleTest is Test {
        NFTFloorOracle private target;

        address[] private feeders;
        address[] private invalidFeeders;

        address[] private assets;
        address[] private invalidAssets;

        address private admin;

        address private attacker;

        function setUp() public {
            admin = makeAddr("admin");

            feeders.push(makeAddr("feeder1"));
            feeders.push(makeAddr("feeder2"));
            feeders.push(makeAddr("feeder3"));
            feeders.push(makeAddr("feeder4"));

            invalidFeeders.push(makeAddr("invalidFeeder1"));

            assets.push(makeAddr("asset1"));
            assets.push(makeAddr("asset2"));

            invalidAssets.push(makeAddr("invalidAsset1"));

            target = new NFTFloorOracle();
            target.initialize(admin, feeders, assets);
        }

        function test() public {
            address asset = assets[0];

            uint256 assetPrice = target.getPrice(asset);
            assertEq(assetPrice, 0, "Returned price is 0 for asset");
        }
    }

Solution: Add a validation in the getPrice() function

    /// @param _asset The nft contract
    /// @return price The most recent price on chain
    function getPrice(address _asset) external view override returns (uint256 price) {
        uint256 updatedAt = assetPriceMap[_asset].updatedAt;
        require(updatedAt > 0, "NFTOracle: asset without price"); // ADD THIS LINE
        require((block.number - updatedAt) <= config.expirationPeriod, "NFTOracle: asset price expired");
        return assetPriceMap[_asset].twap;
    }

Remove empty constructor

Where

https://github.com/code-423n4/2022-11-paraspace/blob/3e4e2e4e1322482dcdc6d342f8799cd44a3e42f5/paraspace-core/contracts/misc/marketplaces/LooksRareAdapter.sol#L23

https://github.com/code-423n4/2022-11-paraspace/blob/3e4e2e4e1322482dcdc6d342f8799cd44a3e42f5/paraspace-core/contracts/misc/marketplaces/SeaportAdapter.sol#L23

https://github.com/code-423n4/2022-11-paraspace/blob/3e4e2e4e1322482dcdc6d342f8799cd44a3e42f5/paraspace-core/contracts/misc/marketplaces/X2Y2Adapter.sol#L24

./misc/NFTFloorOracle.sol

Funcionality on initialize function can be moved to the constructo (since there is no proxies or upgradeability functions associated with this contract).

    /// @notice Allow contract creator to set admin and updaters
    /// @param _admin The admin who can change roles
    /// @param _feeders The initial updaters
    /// @param _assets The initial nft assets
    function initialize(
        address _admin,
        address[] memory _feeders,
        address[] memory _assets
    ) public initializer {
        _addAssets(_assets);
        _addFeeders(_feeders);
        _setupRole(DEFAULT_ADMIN_ROLE, _admin);
        //still need to grant update_role to admin for emergency call
        _setupRole(UPDATER_ROLE, _admin);
        _setConfig(EXPIRATION_PERIOD, MAX_DEVIATION_RATE);
    }

change it to

    /// @notice Allow contract creator to set admin and updaters
    /// @param _admin The admin who can change roles
    /// @param _feeders The initial updaters
    /// @param _assets The initial nft assets
    constructor(
        address _admin,
        address[] memory _feeders,
        address[] memory _assets
    ) {
        _addAssets(_assets);
        _addFeeders(_feeders);
        _setupRole(DEFAULT_ADMIN_ROLE, _admin);
        //still need to grant update_role to admin for emergency call
        _setupRole(UPDATER_ROLE, _admin);
        _setConfig(EXPIRATION_PERIOD, MAX_DEVIATION_RATE);
    }

and remove the Initializable.sol dependency

Duplicated verification onlyWhenFeederExisted() on removeFeeder() call.

Modifier onlyWhenFeederExisted() is being called twice when removeFeeder() is executed. Once from the function itself and one more time in _removeFeeder() resulting in gas wasted.

Remove the validation from removeFeeder() function

    /// @notice Allows owner to remove feeder.
    /// @param _feeder feeder to remove
    function removeFeeder(address _feeder)
        external
        // onlyWhenFeederExisted(_feeder) # remove this line
    {
        _removeFeeder(_feeder);
    }

Duplicated verification onlyWhenAssetExisted() on removeAsset() call.

Modifier onlyWhenAssetExisted() is being called twice when removeAsset() is executed. Once from the function itself and one more time in _removeAsset() resulting in gas wasted.

Remove the validation from removeAsset() function

    /// @notice Allows owner to remove asset.
    /// @param _asset asset to remove
    function removeAsset(address _asset)
        external
        onlyRole(DEFAULT_ADMIN_ROLE)
        // onlyWhenAssetExisted(_asset) # remove this line
    {
        _removeAsset(_asset);
    }