JPYC contest - Rhynorater's results

Missing Modifier on burn Function in FiatTokenV2.sol

When a burn is performed, conceptually what is happening is that tokens are being transferred to address(0). Therefore, according to the documentation supplied with this finding, the checkWhitelist(msg.sender, _amount) should be applied as it is in mint.

Additionally, the emit Transfer(msg.sender, address(0), _amount); call indicates to external tools that a transfer has taken place. An external tool might assume that msg.sender has been whitelisted for 100000+ transfers based on the fact that an event was emitted that suggests they have this capability.

Code in question:

    function burn(uint256 _amount)
        external
        whenNotPaused
        onlyMinters
        notBlocklisted(msg.sender)//@audit Check whitelist here since this is technically emittings a Transfer event to address(0)
    {
        uint256 balance = balances[msg.sender];
        require(_amount > 0, "FiatToken: burn amount not greater than 0");
        require(balance >= _amount, "FiatToken: burn amount exceeds balance");

        totalSupply_ = totalSupply_ - _amount;
        balances[msg.sender] = balance - _amount;
        emit Burn(msg.sender, _amount);
        emit Transfer(msg.sender, address(0), _amount);
    }

Informational - Note on 100_000+ Token Transfers

I'm sure this has been thought off, but an attacker desiring to send more than 100_000 tokens can simply send two transactions of 50_000 in its stead. If you wish to prevent large transfers of money without approval, perhaps it would make sense to keep track of how much is being transferred by a specific address within a certain timeframe. For example, each address can transfer a max of 100_000 per day or something of the like.

Use of Constants Improves Readability and Gas Performance

In the checkWhitelist function of the FiatTokenV2.sol file, the following function can be found:

    modifier checkWhitelist(address _account, uint256 _value) {
        if (_value > 100000 * 10 ** 18) {//@audit Gas Optimization This should be moved to constant or 1e18
            require(
                whitelisted[_account],
                "Whitelistable: account is not whitelisted"
            );
        }
        _;
    }

Inside the if statement we have two things: a constant 100000 which is unexplained, and a 10**18 expression. Moving the 10**18 function to 1e18 or a constant would save on gas and be more readable.

Additionally, the use of 10000 instead of a constant like WHITELIST_LOWERBOUND or a similar constant would be more readable.