Aycozzynfada - Incorrect instruction for Skipping initialization in Broker.sol will lead issues during deployment

[sherlock-admin4]

Aycozzynfada

High

Incorrect instruction for Skipping initialization in Broker.sol will lead issues during deployment

Summary

The broker.sol is an implementation contract which is set to be called through a proxy during deployment, To enable this, the contracts inherits an initializable contract from the celo library. The issue lies in the fact that the broker.sol can be inaccurately set during deployment because of an incorrect instruction/documentation to the constructor.
The instruction says to set "bool test" to true to skip implementation whereas setting test to true will rather leave contract vulnerable to subsequent initialization by a malicious actor. This is feasible because the inherited library allows initialization to be skipped only if test is set to false.

Root Cause

The root cause of this issue is the incorrect documentation in the child implementation contract’s constructor. The documentation states that setting test to true will skip initialization, whereas the actual logic in the parent Initializable contract from the celo library skips initialization only if test is false.

Internal pre-conditions

-The child implementation contract inherits from the Initializable contract from the celo library
Here is the inherited library https://github.com/celo-org/celo-monorepo/blob/master/packages/protocol/contracts/common/Initializable.sol

pragma solidity >=0.5.13 <0.9.0;

contract Initializable {
  bool public initialized;

  modifier initializer() {
    require(!initialized, "contract already initialized");
    initialized = true;
    _;
  }

  constructor(bool testingDeployment) public {
    if (!testingDeployment) {    //if testing is set to true from broker .sol,  initilization is not skipped.
      initialized = true;
    }
  }
}

An aware party during deployment adheres to the instruction and sets Bool test to true

here is the documentation on the constructor for broker.sol
https://github.com/sherlock-audit/2024-10-mento-update/blob/main/mento-core/contracts/swap/Broker.sol#L55-L58

  /**
   * @notice Sets initialized == true on implementation contracts.
   * @param test Set to true to skip implementation initialization.
   */
  constructor(bool test) Initializable(test) {}

External pre-conditions

No response

Attack Path

-A developer or user reads the incorrect documentation and sets test to true in the constructor, expecting the initialization to be skipped.
-A developer or user reads the incorrect documentation and sets test to true in the constructor, expecting the initialization to be skipped, meanwhile initialization wasn't skipped for implementation contract
-A malicious actor calls the broker.sol contract after deployment and initializes the contract with wrong data rendering the protocol useless and leading to possible loss for user funds

Impact

-The contract will be incorrectly initialized, leading to potential functional issues when capitalized by malicious actor

PoC

No response

Mitigation

Update the documentation in the child implementation contract’s constructor to accurately reflect the behavior of the Initializable contract.

/**
 * @notice Sets initialized == true on implementation contracts.
 * @param test Set to false to skip implementation initialization.
 */
constructor(bool test) Initializable(test) {}