0xmuxyz - There is possibility that leave a contract "uninitialized" and that contract will be used for malicious attack

[sherlock-admin]

0xmuxyz

high

There is possibility that leave a contract "uninitialized" and that contract will be used for malicious attack

Summary

• The vulnerability is leaving the implementation contract uninitialized , which allows a malicious attacker to call initialize() function to upgrade the implementation contract. That allow malicious attacker to upgrade malicious contract as a new implementation contract and steal fund locked in contract by executing bad functions.

Vulnerability Detail

• The vulnerability is leaving the implementation contract (ex. TimeLockNonTransferablePool.sol) uninitialized , which allows a malicious attacker to call initialize() function to upgrade the implementation contract. As a result, an attacker can insert any attack contract that includes bad functionalities into a parameter of new implementation contract. Then, once an attacker is successful to update the implementation contract, they can execute the bad functions.

Impact

• A malicious attacker can steal fund locked in contract by executing bad functions such as a function that withdraw all locked fund, etc.

Code Snippet

• https://github.com/sherlock-audit/2022-10-merit-circle/blob/main/merit-liquidity-mining/contracts/TimeLockNonTransferablePool.sol#L6-L20

  contract TimeLockNonTransferablePool is TimeLockPool {
  function initialize(
      string memory _name,
      string memory _symbol,
      address _depositToken,
      address _rewardToken,
      address _escrowPool,
      uint256 _escrowPortion,
      uint256 _escrowDuration,
      uint256 _maxBonus,
      uint256 _maxLockDuration,
      uint256[] memory _curve
  ) public initializer {
      __TimeLockPool_init(_name, _symbol, _depositToken, _rewardToken, _escrowPool, _escrowPortion, _escrowDuration, _maxBonus, _maxLockDuration, _curve);
  }

Tool used

• Manual Review

Recommendation

• To avoid leaving a contract uninitialized, you should invoke the _disableInitializers() function in the constructor to automatically lock it when it is deployed:

• For example, in the TimeLockNonTransferablePool.sol, _disableInitializers() should be added with constructor like below. https://github.com/sherlock-audit/2022-10-merit-circle/blob/main/merit-liquidity-mining/contracts/TimeLockNonTransferablePool.sol#L6-L20

  contract TimeLockNonTransferablePool is TimeLockPool {
  
  //@dev - _disableInitializers() should be added to avoid leaving a contract uninitialized.
  constructor() {
      _disableInitializers();
  }
  
  function initialize(
      string memory _name,
      string memory _symbol,
      address _depositToken,
      address _rewardToken,
      address _escrowPool,
      uint256 _escrowPortion,
      uint256 _escrowDuration,
      uint256 _maxBonus,
      uint256 _maxLockDuration,
      uint256[] memory _curve
  ) public initializer {
      __TimeLockPool_init(_name, _symbol, _depositToken, _rewardToken, _escrowPool, _escrowPortion, _escrowDuration, _maxBonus, _maxLockDuration, _curve);
  }