NestedFactory.addOperator/removeOperator have no effect until importOperators

[code423n4]

Lines of code

https://github.com/code-423n4/2022-02-nested/blob/main/contracts/FeeSplitter.sol#L155

Vulnerability details

Impact

addOperator/removeOperator being run alone don't have any effect, as the cache is used in operations, which is only updated when an implementation is added or removed via importOperators.

If an operation is added via addOperator, but importOperators isn't yet called, the cache doesn't include new operation and it's not usable.

More dangerously, when operator is removed with removeOperator, it is still can be used, even if rebuildCaches was called after that, as an operator is removed from cache only when implementation is zero, so until that is done via importOperators, the deleted operator can be still called by user facing functions.

If operator removal was linked to security considerations this opens up an attack surface, when a malicious user front runs importOperators, invoking the operator to be deleted as it’s still in the cache and can be accessed with callOperator.

This dependency was described among other points in code-423n4/2021-11-nested-findings#139, but isn't yet addressed, as the need of atomic run and now corrected removeOperator implementation were independent issues.

Proof of Concept

NestedFactory.addOperator/removeOperator only manage the operators array:

https://github.com/code-423n4/2022-02-nested/blob/main/contracts/NestedFactory.sol#L100-122

rebuildCaches remove operator from cache only after its implementation is set to zero:

https://github.com/code-423n4/2022-02-nested/blob/main/contracts/OperatorResolver.sol#L74

https://github.com/code-423n4/2022-02-nested/blob/main/contracts/abstracts/MixinOperatorResolver.sol#L36-45

importOperators is effectively adds and removes operators by changing their implementation:

https://github.com/code-423n4/2022-02-nested/blob/main/contracts/OperatorResolver.sol#L52-70

This way addOperator/removeOperator should only be run atomically with importOperators within one transaction to prevent front running.

Recommended Mitigation Steps

As both addOperator/removeOperator aren't effective alone, while addOperator/removeOperator and importOperators are both only owner run, and, given that this functions are basically two parts of one operation, consider add importOperators call to addOperator/removeOperator.

addOperator/removeOperator arguments to be expanded to include Operator structure to be set for the operator via importOperators.

[maximebrugel]

Duplicated : #38