ParaSpace contest - Josiah's results

Lines of code

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

Vulnerability details

Impact

When the admin calls removeFeeder(), it will internally call _removeFeeder() that will have the feeder removed from feeders before getting its UPDATE_ROLE revoked. In the midst of doing this, the protocol forgets to update index of feederPositionMap belonging to the last feeder (e.g. feeder X for reference) of feeders who has now taken over the position of the removed feeder. This could lead to function error when eventually feederX will need to be removed too.

Proof of Concept

NFTFloorOracle.sol#L326-L338

    function _removeFeeder(address _feeder)
        internal
        onlyWhenFeederExisted(_feeder)
    {
        uint8 feederIndex = feederPositionMap[_feeder].index;
        if (feederIndex >= 0 && feeders[feederIndex] == _feeder) {
            feeders[feederIndex] = feeders[feeders.length - 1];
            feeders.pop();
        }
        delete feederPositionMap[_feeder];
        revokeRole(UPDATER_ROLE, _feeder);
        emit FeederRemoved(_feeder);
    }

From the code block above, if (feederIndex >= 0 && feeders[feederIndex] == _feeder) will revert when attempting to remove feederX because you are now accessing an out of bound element in feeders

Tools Used

Manual

Recommended Mitigation Steps

It is recommended updating correctly the index of feederPositionMap belonging to feederX that has replaced the removed feeder.