Anyone can become a minter for Frankencoin when `totalSupply` is 0

[code423n4]

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Frankencoin.sol#L84

Vulnerability details

Bug Description

In the Frankencoin contract, anyone can call the suggestMinter() function to propose a new minter:

Frankencoin.sol#L83-L90

function suggestMinter(address _minter, uint256 _applicationPeriod, uint256 _applicationFee, string calldata _message) override external {
    if (_applicationPeriod < MIN_APPLICATION_PERIOD && totalSupply() > 0) revert PeriodTooShort();
    if (_applicationFee < MIN_FEE  && totalSupply() > 0) revert FeeTooLow();
    if (minters[_minter] != 0) revert AlreadyRegistered();
    _transfer(msg.sender, address(reserve), _applicationFee);
    minters[_minter] = block.timestamp + _applicationPeriod;
    emit MinterApplied(_minter, _applicationPeriod, _applicationFee, _message);
}

Suggested users become an approved minter when the application period has passed, as seen in the isMinter() function:

Frankencoin.sol#L290-L295

/**
* Returns true if the address is an approved minter.
*/
function isMinter(address _minter) override public view returns (bool){
    return minters[_minter] != 0 && block.timestamp >= minters[_minter];
}

The issue lies in the following check in suggestMinter():

if (_applicationPeriod < MIN_APPLICATION_PERIOD && totalSupply() > 0) revert PeriodTooShort();

As the check above always passes when Frankencoin's total supply is 0, an attacker can call suggestMinter() with _applicationPeriod = 0 to instantly become an approved minter, allowing him to call minter functions.

Impact

Whenever Frankencoin's total supply is 0, a malicious attacker can instantly become an approved minter and call sensitive functions, such as mint() or burnFrom().

Additionally, this implementation is potentially vulnerable to front-running during deployment. If the deployer is meant to call suggestMinter() and then mint Frankencoin when the contract is first deployed, an attacker can front-run the deployer's transaction with his own call to suggestMinter() to become an approved minter first.

Recommendation

In the suggestMinter() function, remove the totalSupply() > 0 condition in both checks:

Frankencoin.sol#L83-L85

    function suggestMinter(address _minter, uint256 _applicationPeriod, uint256 _applicationFee, string calldata _message) override external {
-      if (_applicationPeriod < MIN_APPLICATION_PERIOD && totalSupply() > 0) revert PeriodTooShort();
-      if (_applicationFee < MIN_FEE  && totalSupply() > 0) revert FeeTooLow();
+      if (_applicationPeriod < MIN_APPLICATION_PERIOD) revert PeriodTooShort();
+      if (_applicationFee < MIN_FEE) revert FeeTooLow();

If this functionality exists to assign a trusted address as minter during deployment, consider assigning the trusted address as minter in the constructor instead:

Frankencoin.sol#L83-L85

-   constructor(uint256 _minApplicationPeriod) ERC20(18){
+   constructor(uint256 _minApplicationPeriod, address _minter) ERC20(18){
       MIN_APPLICATION_PERIOD = _minApplicationPeriod;
       reserve = new Equity(this);
+      minters[_minter] = block.timestamp;
    }

[c4-pre-sort]

0xA5DF marked the issue as primary issue

[luziusmeisser]

I have considered this suggested recommendation, but this comes with the disadvantage that the ZCHF address is not known at the deploy time of the minters and therefore cannot be set as a gas-efficient constant.

The assumption is that whoever deploys is fast enough to also set the first two minters. In case the deployer is frontrun, a new set of contracts with frontrun protection would need to be deployed, but I find this rather unlikely.

--> This is intended behavior and a calcuatated risk during initialization.

[c4-sponsor]

luziusmeisser marked the issue as sponsor disputed

[c4-judge]

hansfriese changed the severity to QA (Quality Assurance)

[c4-judge]

hansfriese marked the issue as grade-a