Inconsistency between `isMinter()` and `denyMinter()`

[code423n4]

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Frankencoin.sol#L290-L295 https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Frankencoin.sol#L148-L157

Vulnerability details

Bug Description

The Frankencoin contract determines if users are valid minters using 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];
}

A user is a valid minter if block.timestamp is greater than or equal to minters[user].

This is inconsistent with the denyMinter() function, which can be called by vetoers to prevent suggested users from becoming minters:

Frankencoin.sol#L148-L157

/**
* Qualified pool share holders can deny minters during the application period.
* Calling this function is relatively cheap thanks to the deletion of a storage slot.
*/
function denyMinter(address _minter, address[] calldata _helpers, string calldata _message) override external {
    if (block.timestamp > minters[_minter]) revert TooLate();
    // Code to prevent _minter from becoming a valid minter 
}

As seen from above, denyMinter() can be called as long as block.timestamp is less than or equal to minters[user].

Therefore, when minters[user] == block.timestamp, denyMinter() still works although isMinter() returns true.

Impact

If a suggested user is vetoed at literally the last second (block.timestamp == minters[user]), the user might still be able to call minter functions, such as mint() or burnFrom().

Proof of Concept

Consider the following scenario:

• Alice calls suggestMinter() with _applicationPeriod = 10 days to suggest herself as minter.
• 10 days pass, until minters[alice] == block.timestamp. During this period, no one has called denyMinter() to veto Alice's suggestion.
• Bob, a vetoer with sufficient votes, calls denyMinter() at the last second to prevent Alice from becoming a minter.
• Alice front-runs Bob's transaction, calling mint() to mint Frankencoin for herself.
• If Alice's transaction is executed before Bob's transaction in the same block, her call to mint() will succeed.

The following Foundry test demonstrates the scenario above:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "forge-std/Test.sol";
import "../contracts/Frankencoin.sol";

contract denyMinter_POC is Test {
    uint256 constant MIN_APPLICATION_PERIOD = 10 days;

    Frankencoin zchf;
    Equity reserve;

    address ALICE = address(0x1);
    address BOB = address(0x2);

    function setUp() public {
        // Setup contracts
        zchf = new Frankencoin(MIN_APPLICATION_PERIOD);
        reserve = Equity(address(zchf.reserve()));

        // Make BOB a valid vetoer
        vm.startPrank(BOB);
        zchf.suggestMinter(BOB, 0, 0, "");
        zchf.mint(BOB, 1000 ether);
        zchf.transferAndCall(address(reserve), 1000 ether, "");
        vm.stopPrank();

        // Give ALICE 1000 Frankencoin
        deal(address(zchf), ALICE, 1000 ether);
    }

    function test_denyMinterIsInconsistent() public {
        // BOB is a valid vetoer        
        reserve.checkQualified(BOB, new address[](0));

        // ALICE suggests herself as minter
        vm.startPrank(ALICE);
        zchf.suggestMinter(ALICE, MIN_APPLICATION_PERIOD, zchf.MIN_FEE(), "");

        // No one vetoes ALICE until minters[ALICE] == block.timestamp
        skip(MIN_APPLICATION_PERIOD);
        assertEq(zchf.minters(ALICE), block.timestamp);

        // ALICE is now a minter and can call minterOnly functions
        assertTrue(zchf.isMinter(ALICE));
        zchf.mint(ALICE, 1000 ether);
        vm.stopPrank();

        // However, BOB can still veto ALICE using denyMinter() 
        vm.prank(BOB);
        zchf.denyMinter(ALICE, new address[](0), "");

        // Now, ALICE is no longer a minter
        assertFalse(zchf.isMinter(ALICE));
    }
}

Recommendation

Change denyMinter() to also revert when block.timestamp == minters[_minter]:

Frankencoin.sol#L152-L153

    function denyMinter(address _minter, address[] calldata _helpers, string calldata _message) override external {
-      if (block.timestamp > minters[_minter]) revert TooLate();
+      if (block.timestamp >= minters[_minter]) revert TooLate();

Alternatively, make suggested users become minters only after minters[_minter] has passed:

Frankencoin.sol#L293-L295

    function isMinter(address _minter) override public view returns (bool){
-      return minters[_minter] != 0 && block.timestamp >= minters[_minter];
+      return minters[_minter] != 0 && block.timestamp > minters[_minter];
    }

[c4-pre-sort]

0xA5DF marked the issue as duplicate of #370

[c4-judge]

hansfriese changed the severity to QA (Quality Assurance)

[c4-judge]

hansfriese marked the issue as grade-a

[c4-judge]

This previously downgraded issue has been upgraded by hansfriese

[c4-judge]

hansfriese changed the severity to QA (Quality Assurance)