`Frankencoin.denyMinter` and `Frankencoin.isMinter` functions contradict each other when `block.timestamp` and `minters[_minter]` are equal

[code423n4]

Lines of code

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

Vulnerability details

Impact

When block.timestamp and minters[_minter] are equal, a suggested minter can be denied by calling the following Frankencoin.denyMinter function because block.timestamp > minters[_minter] is false.

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

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

However, when block.timestamp and minters[_minter] are equal, calling the following Frankencoin.isMinter function also returns true for the suggested minter since minters[_minter] != 0 and block.timestamp >= minters[_minter] are both true; this means that such suggested minter can actually mint ZCHF tokens immediately at that moment.

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

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

Therefore, the Frankencoin.denyMinter and Frankencoin.isMinter functions contradict each other when block.timestamp and minters[_minter] are equal. At block.timestamp that equals minters[_minter], if a suggested minter is considered as approved, it should be allowed to mint; yet, a qualified pool share holder can frontrun the minter's Frankencoin.mint transaction by calling the Frankencoin.denyMinter function to block such minter from minting. On the other hand, at block.timestamp that equals minters[_minter], if a suggested minter is deniable, it should be allowed to be denied; yet, such minter can frontrun a qualified pool share holder's Frankencoin.denyMinter transaction by calling the Frankencoin.mint function to mint some ZCHF tokens before it gets denied. In the former case, the suggested minter cannot mint when it should be able to so the suggested minter is DOS'ed unexpectedly. In the latter case, the suggested minter is able to mint when it should not be, which can be more severe than the former case if the minter is malicious and should be prevented from minting any ZCHF tokens.

Proof of Concept

Please replace https://github.com/code-423n4/2023-04-frankencoin/blob/main/test/PluginVetoTests.ts#L59-L68 with the following code in test\PluginVetoTests.ts. Both Frontrunning Frankencoin.mint transaction by calling Frankencoin.denyMinter function at block.timestamp that equals minters[_minter] and Frontrunning Frankencoin.denyMinter transaction by calling Frankencoin.mint function at block.timestamp that equals minters[_minter] tests will pass to demonstrate the described scenarios.

        // it("Participant suggests minter", async () => {
        //     let applicationPeriod = await ZCHFContract.MIN_APPLICATION_PERIOD();
        //     let applicationFee = await ZCHFContract.MIN_FEE();
        //     let msg = "DCHF Bridge"
        //     await mockXCHF.connect(accounts[1]).approve(ZCHFContract.address, applicationFee);
        //     let balance = await ZCHFContract.balanceOf(accounts[1].address);
        //     expect(balance).to.be.greaterThan(applicationFee);
        //     await expect(ZCHFContract.connect(accounts[1]).suggestMinter(bridgeSygnum.address, applicationPeriod, 
        //         applicationFee, msg)).to.emit(ZCHFContract, "MinterApplied");
        // });

        it("Frontrunning Frankencoin.mint transaction by calling Frankencoin.denyMinter function at block.timestamp that equals minters[_minter]", async () => {
            let applicationPeriod = await ZCHFContract.MIN_APPLICATION_PERIOD();
            let applicationFee = await ZCHFContract.MIN_FEE();
            let msg = "DCHF Bridge"
            await mockXCHF.connect(accounts[1]).approve(ZCHFContract.address, applicationFee);
            await ZCHFContract.connect(accounts[1]).suggestMinter(bridgeSygnum.address, applicationPeriod * 10, applicationFee, msg);

            // simulate that block.timestamp and minters[_minter] are equal
            await ethers.provider.send('evm_increaseTime', [8639999]); 
            await network.provider.send("evm_mine");

            // a qualified pool share holder frontruns suggested minter's Frankencoin.mint transaction by calling Frankencoin.denyMinter function to deny such suggested minter
            await expect(
                ZCHFContract.connect(accounts[0]).denyMinter(bridgeSygnum.address, [], "sygnum denied")
            ).to.emit(ZCHFContract, "MinterDenied");

            const amount = floatToDec18(1_000);
            await mockDCHF.connect(accounts[1]).approve(bridgeSygnum.address, amount);

            // after such frontrunning, suggested minter's Frankencoin.mint function call reverts because suggested minter is already denied
            await expect(
                bridgeSygnum.connect(accounts[1])["mint(uint256)"](amount)
            ).to.be.revertedWithCustomError(ZCHFContract, "NotMinter");
        });

        it("Frontrunning Frankencoin.denyMinter transaction by calling Frankencoin.mint function at block.timestamp that equals minters[_minter]", async () => {
            let applicationPeriod = await ZCHFContract.MIN_APPLICATION_PERIOD();
            let applicationFee = await ZCHFContract.MIN_FEE();
            let msg = "DCHF Bridge"
            await mockXCHF.connect(accounts[1]).approve(ZCHFContract.address, applicationFee);
            await ZCHFContract.connect(accounts[1]).suggestMinter(bridgeSygnum.address, applicationPeriod * 10, applicationFee, msg);

            const ZCHFBalanceBefore = await ZCHFContract.balanceOf(accounts[1].address);

            // simulate that block.timestamp and minters[_minter] are equal
            await ethers.provider.send('evm_increaseTime', [8639998]); 
            await network.provider.send("evm_mine");

            const amount = floatToDec18(1_000);
            await mockDCHF.connect(accounts[1]).approve(bridgeSygnum.address, amount);

            // suggested minter frontruns a qualified pool share holder's Frankencoin.denyMinter transaction by calling Frankencoin.mint function to mint some ZCHF tokens
            await bridgeSygnum.connect(accounts[1])["mint(uint256)"](amount);

            // simulate that block.timestamp and minters[_minter] are equal
            await ethers.provider.send('evm_increaseTime', [-1]); 
            await network.provider.send("evm_mine");

            // after such frontrunning, qualified pool share holder's Frankencoin.denyMinter function call is too late for denying suggested minter
            //   since some ZCHF tokens have already be minted by the suggested minter even though it is now denied
            await ZCHFContract.connect(accounts[0]).denyMinter(bridgeSygnum.address, [], "sygnum denied");

            const isbridgeSygnumMinter = await ZCHFContract.isMinter(bridgeSygnum.address);
            expect(isbridgeSygnumMinter).to.be.false;

            const ZCHFBalanceAfter = await ZCHFContract.balanceOf(accounts[1].address);
            expect(ZCHFBalanceAfter.sub(ZCHFBalanceBefore)).to.equal(amount);
        });

Tools Used

VSCode

Recommended Mitigation Steps

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Frankencoin.sol#L152-L157 can be updated to the following code:

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

or

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Frankencoin.sol#L293-L295 can be updated to the following code:

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

but not both.

[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)