Confliction on double `initialize` functions front-run `minter`

[code423n4]

Handle

hagrid

Vulnerability details

When we deploy the UnlockDiscountTokenV2 contract to the any test network, it is possible to frontrun initialize function. Also, Remix IDE shows two different initialize method for this contract.

The first one is overridden: initialize(address _minter). And, the other one is default one for initialize function which is: initialize(string name, string symbol, uint8 decimals).

So, it is possible to execute second one before the first one initialize(address _minter). In this case, following lines will be eliminated:

 function initialize(address _minter) public override initializer()
  {
    ERC20MintableUpgradeable.initialize(_minter);
    ERC20DetailedUpgradeable.initialize('Unlock Discount Token', 'UDT', 18);
    __ERC20Permit_init(name());
  }

Therefore, contract will be initialized with wrong arguments. Current minter address will be address(0). As a result, it will be impossible to change minter address.

Proof of Concept

1. Deploy the UnlockDiscountTokenV2 contract to the Remix IDE.
2. Call initialize2() method.
3. Call initialize(string name, string symbol, uint8 decimals) method instead of initialize(address _minter).
4. Try to execute addMinter(address account) method.
5. It will fail. Minter address will stuck forever and it will be impossible to grant anybody to Minter role.

Tools Used

Remix IDE

Recommended Mitigation

Properly implement the overridden initialize function.

[julien51]

The UDT contract has already been deployed and initialized so I am not sure any of these are real issues anymore.

[0xleastwood]

As the sponsor has outlined, contracts have already been deployed and this does not pose any risk. However, I'll keep it open for future reference.