PoolFactory Manual Review Findings

PFY-01M: Inexistent Protection of Pause State

Type	Severity	Location
Standard Conformity		PoolFactory.sol:L47-L57

Description:

The createNewPool function should be disallowed if the contract is in a paused state.

Impact:

The paused state of the contract solely protects whether pool creators are included or removed from the system which appears counter-intuitive.

Example:

Copy
47function createNewPool(
48    //address wbnbAddress,
49    address treasuryAddress,
50    address poolOwnerAddress,
51    address signerAddress,
52    address pricingModelRegistry,
53    uint256[] memory fees,
54    address[] memory tokenAs,
55    address[] memory tokenBs,
56    uint256[] memory pricingModelIds
57) external noDelegateCall override returns (IPool pool) {

Recommendation:

We advise the whenNotPaused modifier to be properly applied to the createNewPool function ensuring that it cannot be invoked in an emergency scenario.

Alleviation:

The whenNotPaused modifier is now properly applied to the createNewPool call.

PFY-02M: Unutilized Registry Address

Type	Severity	Location
Logical Fault		PoolFactory.sol:L59

Description:

The PoolFactory contains a registry address that is evaluated during createNewPool invocations and is set once, however, it remains unutilized by the Pool implementation.

Impact:

It is presently possible to create arbitrary pools with any configuration which can cause them to significantly misbehave if misconfigured.

Example:

Copy
47function createNewPool(
48    //address wbnbAddress,
49    address treasuryAddress,
50    address poolOwnerAddress,
51    address signerAddress,
52    address pricingModelRegistry,
53    uint256[] memory fees,
54    address[] memory tokenAs,
55    address[] memory tokenBs,
56    uint256[] memory pricingModelIds
57) external noDelegateCall override returns (IPool pool) {
58    require(treasuryToPool[treasuryAddress] == address(0), "Treasury binded to other pool");
59    require(registry != address(0), "Registry not available");
60    require(isPoolCreator[msg.sender], "Pool creator is not whitelisted");
61
62    // pool = new Pool(
63    //     treasuryAddress,
64    //     poolOwnerAddress,
65    //     signerAddress,
66    //     pricingModelRegistry,
67    //     fees,
68    //     tokenAs,
69    //     tokenBs,
70    //     pricingModelIds
71    // );
72    pool = PoolDeployer.deploy(
73        treasuryAddress,
74        poolOwnerAddress,
75        signerAddress,
76        pricingModelRegistry,
77        fees,
78        tokenAs,
79        tokenBs,
80        pricingModelIds
81    );
82    Ownable(address(pool)).transferOwnership(poolOwnerAddress);
83
84    pools[address(pool)] = true;
85    treasuryToPool[treasuryAddress] = address(pool);
86    poolArray.push(address(pool));
87
88    emit PoolCreated(treasuryAddress, poolOwnerAddress, address(pool));
89    return pool;
90}

Recommendation:

We advise it to be properly utilized as we assume it is meant to point to the pricing registry.

Alleviation:

The createNewPool no longer accepts the pricing model registry as an argument and utilizes the existing contract-level registry instead, increasing the operational security of pools created by the PoolFactory and alleviating this exhibit.

PFY-03M: Inexistent Exposure of Pause Functionality

Type	Severity	Location
Standard Conformity		PoolFactory.sol:L15

Description:

The PoolFactory contract inherits the Pausable implementation yet does not expose the Pausable::_pause and Pausable::_unpause methods to a privileged user of the PoolFactory.

Impact:

While the contract is meant to make use of the Pausable dependency to protect the addPoolCreator, and removePoolCreator functions, the modifiers are ineffectual as the contract can never be paused.

Example:

Copy
15contract PoolFactory is INativeFactory, Ownable, NoDelegateCall, Pausable  {

Recommendation:

We advise the functions to be properly exposed via corresponding pause and unpause function implementations that also apply the necessary access control to protect pausing functionality.

Alleviation:

The Pausable methods are now exposed using homonym functions (pause & unpause) protected with the onlyOwner modifier.