code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Frankencoin.sol#L83

Vulnerability details

Impact

Detailed description of the impact of this finding. Anyone can suggest an unremovable minter when the total supply is 0.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. As you can see from the code anybody can call with _applicationPeriod = 0, _applicationFee = 0 when totalSupply is 0

   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);
   }

contracts/Frankencoin.sol#L83 This means that created minter will be unremovable due to block.timestamp > minters[_minter]

   function denyMinter(address _minter, address[] calldata _helpers, string calldata _message) override external {
      if (block.timestamp > minters[_minter]) revert TooLate();
      reserve.checkQualified(msg.sender, _helpers);
      delete minters[_minter];
      emit MinterDenied(_minter, _message);
   }

contracts/Frankencoin.sol#L152

Tools Used

Manual

Recommended Mitigation Steps

Add minters in constructor, which should be added without fee and less than MIN_APPLICATION_PERIOD

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

   constructor(uint256 _minApplicationPeriod, calldata minters) ERC20(18){
      MIN_APPLICATION_PERIOD = _minApplicationPeriod;
      reserve = new Equity(this);
      // initialize minters
      ...
   }

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as duplicate of #921

c4-judge commented 1 year ago

hansfriese changed the severity to QA (Quality Assurance)