Some arbitrary feeders will not be removable, even by admin.

[code423n4]

Lines of code

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

Vulnerability details

Description

NFTFloorOracle receives data from different "feeders". They are added using addFeeders() and removed by removeFeeder().

Feeders are managed by two data structures. feeders is an array, each element in the address of the feeder. feederPositionMap maps between feeder address to its position.

    /// @dev All feeders
    address[] public feeders;
    /// @dev feeder map
    // feeder address -> index in feeder list
    mapping(address => FeederPosition) private feederPositionMap;

    struct FeederPosition {
    // if feeder registered or not
    bool registered;
    // index in feeder list
    uint8 index;
}

Let's see how _removeFeeder is implemented:

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);
}

To remove feeder, we take its index from postion map, and store in that index the current last feeder in the feeder array. Then the last element is popped, like in a classic swap and pop. The issue is that the feederPositionMap entry for the last element in the array, which was just moved somewhere else, will still have the index pointing to the previous original location. As a result, the feeder can no longer by removed, breaking an important safety guarantee of the system that all feeders can be removed if they misbehave.

Impact

Some arbitrary feeders will not be removable, even by admin.

Proof of Concept

Copy the test below to NFTFloorOracle.t.sol:

function testAddRemoveFeeders() public {
    address[] memory newUpdaters = new address[](2);
    newUpdaters[0] = 0x0000000000000000000000000000000000000005;
    newUpdaters[1] = 0x0000000000000000000000000000000000000006;
    //add new updaters and remove old ones
    cheats.startPrank(admin);
    _contract.addFeeders(newUpdaters);
    _contract.removeFeeder(newUpdaters[1]);
    _contract.removeFeeder(newUpdaters[0]);
    _contract.addFeeders(newUpdaters);
    _contract.removeFeeder(newUpdaters[0]);
    _contract.removeFeeder(newUpdaters[1]);
}

The test shows that feeders can be removed from high to low, but will revert when removed from low to high. This is because when removing the high feeder, the swap logic will swap it with itself, so index variable of other feeders is not harmed.

Tools Used

Manual audit

Recommended Mitigation Steps

Change the _removeFeeder function as below:

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

At this point, the test will run successfully.

[c4-judge]

dmvt marked the issue as duplicate of #47

[c4-judge]

dmvt changed the severity to 3 (High Risk)

[c4-judge]

dmvt marked the issue as satisfactory