Submitted by IllIllI, also found by 0x1f8b, hyh, and pauliax
NestedFactory.sol#L99-L108
MixinOperatorResolver.sol#L30-L47
NestedFactory.sol#L110-L122
MixinOperatorResolver.sol#L49-L55
NestedFactory extends the MixinOperatorResolver contract which comes from the synthetix/MixinResolver.sol code base where the expectation is that isResolverCached() returns false until rebuildCache() is called and the cache is fully up to date. Due to a medium issue identified in a prior contest, the OperatorResolver.importOperators() step was made to be atomically combined with the NestedFactory.rebuildCache() step. However, the atomicity was not applied everywhere and the ability to add/remove operators from the NestedFactory also had other cache-inconsistency issues. There are four separate instances of operator tracking problems in this submission.
As with the prior issue, many core operations (such as NestedFactory.create() and NestedFactory.swapTokenForTokens()) are dependant on the assumption that the operatorCache cache is synced prior to these functions being executed, but this may not necessarily be the case. Unlike the prior issue which was about updates to the resolver not getting reflected in the cache, this issue is about changes to the factory not updating the cache.
This flow is not aware that the cache is not in sync
NestedFactory.sol#L99-L108
Even if removeOperator() calls rebuildCache() the function will still not work because resolverOperatorsRequired() only keeps track of remaining operators, and rebuildCache() currently has no way of knowing that an entry was removed from that array and that a corresponding entry from operatorCache needs to be removed too.
MixinOperatorResolver.sol#L30-L47
This flow is not aware that the cache is not in sync
NestedFactory.sol#L110-L122
This function, like removeOperator() is not able to tell that there is an operator that needs to be removed from resolverCache, causing the owner not to know a call to rebuildCache() is required to remove the operator
MixinOperatorResolver.sol#L49-L55
Add calls to rebuildCache() in addOperator() and removeOperator(), have INestedFactory also track operators that have been removed with a new array, and have isResolverCached() also check whether this new array is empty or not.
maximebrugel (Nested Finance) confirmed and commented:
