NestedFactory.removeOperator code doesn't correspond to it's logic

[code423n4]

Handle

hyh

Vulnerability details

Impact

Current implementation throws if first operator is to be deleted, i.e. operators[0] == operator, and doesn't throw when operator is not found, i.e. there is no i such that operators[i] == operator. This way an expected logic of throwing whenever operator isn't found in current list and deleting the one found otherwise doesn't take place.

This way

1. operators[0] cannot be deleted
2. if there is no requested operator in the operators list, an array bounds check violation will happen

Proof of Concept

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

Recommended Mitigation Steps

Function code needs to be updated, for example:

Now:

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

To be:

for (uint256 i = 0; i < operators.length; i++) {
	if (operators[i] == operator) {
		break;
	}
}
require(i < operators.length, "NestedFactory::removeOperator: Can't remove non-existent operator");
delete operators[i];

[maximebrugel]

Duplicated : #58

[alcueca]

Taking this as the main for the whole lot.