NestedFactory.FeeSplitter can be immediatly changed with current and future fees lost to a malicious Owner

[code423n4]

Handle

hyh

Vulnerability details

Impact

Malicious owner can substitute current FeeSplitter with another implementation, that will receive the system fees after that, surpassing current shareholders.

Proof of Concept

setFeeSplitter function allows for unlimited and immediate update of the FeeSplitter contract, which has control of fees collected and distributes them among shareholders.

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

Recommended Mitigation Steps

As NestedFactory.FeeSplitter holds shareholders list, its change should be either not permitted or Timelocked.

Either make feeSplitter immutable by allowing changing it only once: Now:

require(address(_feeSplitter) != address(0), "NestedFactory::setFeeSplitter: Invalid feeSplitter address");

To be:

require(address(feeSplitter) == address(0), "NestedFactory: feeSplitter is immutable");

Or, when FeeSplitter updates are planned without explicit Factory migration, Timelock the setFeeSplitter function.

[adrien-supizet]

As mentioned in the readme, those functions will be timelocked

[alcueca]

Dispute accepted.