Impossible to remove the operator at index 0 of operators array

[code423n4]

Handle

loop

Vulnerability details

Currently the operator at the 0 index can never be removed due to the require statement in removeOperator.

Impact

If this is an operator which is no longer supported, it can't be removed.

Proof of Concept

Code of removeOperator:

function removeOperator(bytes32 operator) external override onlyOwner {
        uint256 i = 0;
        while (operators[i] != operator) {
                i++;
        }
        require(i > 0, "NestedFactory::removeOperator: Cant remove non-existent operator");
        delete operators[i];
}

(https://github.com/code-423n4/2021-11-nested/blob/main/contracts/NestedFactory.sol#L79-L86)

Let's say the value bytes32(1) is at index 0 of the operators array. If we call removeOperator(bytes32(1)) the condition in the while loop will be false since bytes32(1) is equal to operators[0]. Since i does not get increased the require statement of i > 0 will fail since i is equal to 0. The error will state that a non-existant operator can't be removed, but the operator does exist and it's impossible to remove it from the array.

Recommended Mitigation Steps

Change the function code to something like this:

function removeOperator(bytes32 operator) external override onlyOwner {
        require(operators.length > 0, "NestedFactory::removeOperator: Cant remove non-existent operator");
        for (uint256 i = 0; i < operators.length; i++) {
            if (operators[i] == operator) {
                delete operators[i];
            }
        }  
    }

[maximebrugel]

Note

This issue also means that :

• You can't remove the last Operator.
• If is called with non-existant operator the while loop keeps going until gas limit is hit.

Every issues pointing this incorrect logic will be linked to this issue.

[maximebrugel]

Note that we should use pop to really remove the operator => #139

[alcueca]

Using #220 as the main instead