JPYC contest - Dravee's results

QA Report

Table of Contents:

• QA Report
  • Foreword
  • Summary
  • File: UUPSUpgradeable.sol
    • modifier onlyProxy()
      • Missing UUPSUpgradeable: prefix on revert string Function must be called through delegatecall
      • Missing UUPSUpgradeable: prefix on revert string Function must be called through active proxy
  • File: Blocklistable.sol
    • function isBlocklisted()
      • Missing comment @return bool
  • File: FiatTokenV1.sol
    • Storage
      • Related data should be grouped in a struct
    • function initialize()
      • Front-Runnable initialize()
    • function minterAllowance()
      • Missing comment @return uint256
    • function isMinter()
      • Missing comment @return bool
    • function balanceOf()
      • Missing comment @return uint256
  • File: FiatTokenV2.sol
    • Storage
      • Related data should be grouped in a struct
    • function initialize()
      • Front-Runnable initialize()
    • function minterAllowance()
      • Missing comment @return uint256
    • function isMinter()
      • Missing comment @return bool
    • function balanceOf()
      • Missing comment @return uint256
    • function _approve()
      • Wrong revert string prefix (1)
      • Wrong revert string prefix (2)
    • function transferFrom()
      • Wrong revert string prefix
    • function _transfer()
      • Wrong revert string prefix (1)
      • Wrong revert string prefix (2)
      • Wrong revert string prefix (3)
    • function _decreaseAllowance()
      • Wrong revert string prefix
    • modifier onlyWhitelister()
      • Wrong revert string prefix
    • modifier checkWhitelist()
      • Wrong revert string prefix
    • function isWhitelisted()
      • Missing comment @return bool
    • function updateWhitelister()
      • Wrong revert string prefix
  • File: Ownable.sol
    • function transferOwnership()
      • transferOwnership should be two step process

Foreword

• @audit tags

The code is annotated at multiple places with //@audit comments to pinpoint the issues. Please, pay attention to them for more details.

Summary

• The pragmas used are not the same everywhere:

proxy/ERC1967Proxy.sol:4:pragma solidity ^0.8.0;
proxy/Proxy.sol:4:pragma solidity ^0.8.0;
upgradeability/draft-IERC1822.sol:4:pragma solidity ^0.8.0;
upgradeability/ERC1967Upgrade.sol:4:pragma solidity 0.8.11;
upgradeability/UUPSUpgradeable.sol:4:pragma solidity 0.8.11;
util/Address.sol:4:pragma solidity ^0.8.0;
util/Context.sol:4:pragma solidity 0.8.11;
util/ECRecover.sol:26:pragma solidity 0.8.11;
util/EIP712.sol:25:pragma solidity 0.8.11;
util/IERC20.sol:4:pragma solidity ^0.8.0;
util/SafeERC20.sol:4:pragma solidity ^0.8.0;
util/StorageSlot.sol:3:pragma solidity ^0.8.0;
v1/AbstractFiatTokenV1.sol:25:pragma solidity 0.8.11;
v1/Blocklistable.sol:25:pragma solidity 0.8.11;
v1/EIP2612.sol:25:pragma solidity 0.8.11;
v1/EIP3009.sol:25:pragma solidity 0.8.11;
v1/EIP712Domain.sol:25:pragma solidity 0.8.11;
v1/FiatTokenV1.sol:25:pragma solidity 0.8.11;
v1/Ownable.sol:4:pragma solidity 0.8.11;
v1/Pausable.sol:26:pragma solidity 0.8.11;
v1/Rescuable.sol:25:pragma solidity 0.8.11;
v2/FiatTokenV2.sol:25:pragma solidity 0.8.11;
v2/FiatTokenV2test.sol:3:pragma solidity 0.8.11;

• The initialize() methods in FiatTokenV1 and FiatTokenV2 are front-runnable and call the default implementation of the custom Ownable's _transferOwnership, which doesn't use access control
• I suggest using a 2-step ownership transfer process
• Some @return comments were missing on functions
• There's an inconsistency on the prefix usage in revert strings (either it's missing, or it's a remnant from a copy-paste)
• Minters-info in FiatTokenV1 and FiatTokenV2 should be grouped in a struct

File: UUPSUpgradeable.sol

modifier onlyProxy()

32:     modifier onlyProxy() {
33:         require(
34:             address(this) != __self,
35:             "Function must be called through delegatecall" // missing "UUPSUpgradeable: " prefix (in all of the solution;, a prefix is used)
36:         );
37:         require(
38:             _getImplementation() == __self,
39:             "Function must be called through active proxy" // missing "UUPSUpgradeable: " prefix
40:         );
41:         _;
42:     }

Missing UUPSUpgradeable: prefix on revert string Function must be called through delegatecall

Missing UUPSUpgradeable: prefix on revert string Function must be called through active proxy

In the whole solution, revert strings are prefixed with the contract's name. Here, it's missing.

File: Blocklistable.sol

function isBlocklisted()

64:     /**
65:      * @dev Checks if account is blocklisted
66:      * @param _account The address to check //@audit missing @return bool
67:      */
68:     function isBlocklisted(address _account) external view returns (bool) {
69:         return blocklisted[_account];
70:     }

Missing comment @return bool

File: FiatTokenV1.sol

Storage

Related data should be grouped in a struct

For maps that use the same key value: having separate fields is error prone (like in case of deletion or future new fields).
In this contract, those 2 maps use the same key:

File: FiatTokenV1.sol
59:     mapping(address => bool) internal minters;
60:     mapping(address => uint256) internal minterAllowed;

Proof:

  333:         minters[minter] = true;
  334:         minterAllowed[minter] = minterAllowedAmount;
...
  349:         minters[minter] = false;
  350:         minterAllowed[minter] = 0;

I'd suggest these 2 related data get grouped in a struct, let's name it MinterInfo:

struct MinterInfo {  
    bool minters;  
    uint256 minterAllowed;  
}  

And it would be used as a state variable in this manner:

mapping(address => MinterInfo) minterInfo;  

It would be less error-prone, more readable, and it would be possible to delete all related fields with a simple delete minterInfo[address].

function initialize()

Front-Runnable initialize()

Even if the contract is Ownable and the default owner is the deployer:

File: Ownable.sol
28:     constructor() {
29:         _transferOwnership(_msgSender());
30:     }

there's no access-control implemented on initialize():

File: FiatTokenV1.sol
40: contract FiatTokenV1 is
41:     Ownable,
...
068:     function initialize(
069:         string memory tokenName,
070:         string memory tokenSymbol,
071:         string memory tokenCurrency,
072:         uint8 tokenDecimals,
073:         address newMasterMinter,
074:         address newPauser,
075:         address newBlocklister,
076:         address newOwner
077:     ) public {
078:         require(!initialized, "FiatToken: contract is already initialized");
079:         require(
080:             newMasterMinter != address(0),
081:             "FiatToken: new masterMinter is the zero address"
082:         );
083:         require(
084:             newPauser != address(0),
085:             "FiatToken: new pauser is the zero address"
086:         );
087:         require(
088:             newBlocklister != address(0),
089:             "FiatToken: new blocklister is the zero address"
090:         );
091:         require(
092:             newOwner != address(0),
093:             "FiatToken: new owner is the zero address"
094:         );
095: 
096:         name = tokenName;
097:         symbol = tokenSymbol;
098:         currency = tokenCurrency;
099:         decimals = tokenDecimals;
100:         masterMinter = newMasterMinter;
101:         pauser = newPauser;
102:         blocklister = newBlocklister;
103:         _transferOwnership(newOwner);
104:         blocklisted[address(this)] = true;
105:         DOMAIN_SEPARATOR = EIP712.makeDomainSeparator(name, "1");
106:         CHAIN_ID = block.chainid;
107:         NAME = name;
108:         VERSION = "1";
109:         initialized = true;
110:     }

Furthermore, as this method calls L103: _transferOwnership(newOwner);, it's possible for the front-runner to claim ownership, as this is calling the default implementation without access-control:

File: Ownable.sol
56:     /**
57:      * @dev Transfers ownership of the contract to a new account (`newOwner`).
58:      * Internal function without access restriction.
59:      */
60:     function _transferOwnership(address newOwner) internal virtual {
61:         address oldOwner = _owner;
62:         _owner = newOwner;
63:         emit OwnershipTransferred(oldOwner, newOwner);
64:     }

I suggest adding the onlyOwner modifier to the initialize() method. There's exactly the same issue on FiatTokenV2.sol

function minterAllowance()

Missing comment @return uint256

163:     /**
164:      * @dev Get minter allowance for an account
165:      * @param minter The address of the minter //@audit missing @return uint256
166:      */
167:     function minterAllowance(address minter) external view returns (uint256) {
168:         return minterAllowed[minter];
169:     }

function isMinter()

Missing comment @return bool

171:     /**
172:      * @dev Checks if account is a minter
173:      * @param account The address to check //@audit missing @return bool
174:      */
175:     function isMinter(address account) external view returns (bool) {
176:         return minters[account];
177:     }

function balanceOf()

Missing comment @return uint256

202:     /**
203:      * @dev Get token balance of an account
204:      * @param account address The account //@audit missing @return uint256
205:      */
206:     function balanceOf(address account)
207:         external
208:         view
209:         override
210:         returns (uint256)
211:     {
212:         return balances[account];
213:     }

File: FiatTokenV2.sol

Storage

Related data should be grouped in a struct

The same suggestion as in FiatTokenV1.sol applies here for grouping minters and minterAllowed in a struct.

function initialize()

Front-Runnable initialize()

Same as FiatTokenV1.sol: Front-Runnable initialize()

function minterAllowance()

Missing comment @return uint256

File: FiatTokenV2.sol
170:     /**
171:      * @dev Get minter allowance for an account
172:      * @param minter The address of the minter //@audit missing @return uint256
173:      */
174:     function minterAllowance(address minter) external view returns (uint256) {
175:         return minterAllowed[minter];
176:     }

function isMinter()

Missing comment @return bool

File: FiatTokenV2.sol
178:     /**
179:      * @dev Checks if account is a minter
180:      * @param account The address to check //@audit missing @return bool
181:      */
182:     function isMinter(address account) external view returns (bool) {
183:         return minters[account];
184:     }

function balanceOf()

Missing comment @return uint256

File: FiatTokenV2.sol
209:     /**
210:      * @dev Get token balance of an account
211:      * @param account address The account //@audit missing @return uint256
212:      */
213:     function balanceOf(address account)
214:         external
215:         view
216:         override
217:         returns (uint256)
218:     {
219:         return balances[account];
220:     }

function _approve()

File: FiatTokenV2.sol
248:     function _approve(
249:         address owner,
250:         address spender,
251:         uint256 value
252:     ) internal override {
253:         require(owner != address(0), "ERC20: approve from the zero address"); //@audit replace prefix from "ERC20:" to "FiatToken:"
254:         require(spender != address(0), "ERC20: approve to the zero address"); //@audit replace prefix from "ERC20:" to "FiatToken:"
255:         allowed[owner][spender] = value;
256:         emit Approval(owner, spender, value);
257:     }

Wrong revert string prefix (1)

ERC20: approve from the zero address should be FiatToken: approve from the zero address

Wrong revert string prefix (2)

ERC20: approve to the zero address should be FiatToken: approve to the zero address

function transferFrom()

Wrong revert string prefix

File: FiatTokenV2.sol
280:         require(
281:             value <= allowed[from][msg.sender],
282:             "ERC20: transfer amount exceeds allowance" //@audit replace prefix from "ERC20:" to "FiatToken:"
283:         );

ERC20: transfer amount exceeds allowance should be FiatToken: transfer amount exceeds allowance

function _transfer()

File: FiatTokenV2.sol
314:     function _transfer(
315:         address from,
316:         address to,
317:         uint256 value
318:     ) internal override {
319:         require(from != address(0), "ERC20: transfer from the zero address"); //@audit replace prefix from "ERC20:" to "FiatToken:"
320:         require(to != address(0), "ERC20: transfer to the zero address"); //@audit replace prefix from "ERC20:" to "FiatToken:"
321:         require(
322:             value <= balances[from],
323:             "ERC20: transfer amount exceeds balance" //@audit replace prefix from "ERC20:" to "FiatToken:"
324:         );
325: 
326:         balances[from] = balances[from] - value;
327:         balances[to] = balances[to] + value;
328:         emit Transfer(from, to, value);
329:     }

Wrong revert string prefix (1)

ERC20: transfer from the zero address should be FiatToken: transfer from the zero address

Wrong revert string prefix (2)

ERC20: transfer to the zero address should be FiatToken: transfer to the zero address

Wrong revert string prefix (3)

ERC20: transfer amount exceeds balance should be FiatToken: transfer amount exceeds balance

function _decreaseAllowance()

Wrong revert string prefix

File: FiatTokenV2.sol
280:         require(
281:             value <= allowed[from][msg.sender],
282:             "ERC20: transfer amount exceeds allowance" //@audit replace prefix from "ERC20:" to "FiatToken:"
283:         );

ERC20: decreased allowance below zero should be FiatToken: decreased allowance below zero

modifier onlyWhitelister()

File: FiatTokenV2.sol
610:     modifier onlyWhitelister() {
611:         require(
612:             msg.sender == whitelister,
613:             "Whitelistable: caller is not the whitelister" //@audit replace prefix from "Whitelistable:" to "FiatToken:"
614:         );
615:         _;
616:     }

Wrong revert string prefix

Whitelistable: caller is not the whitelister should be FiatToken: caller is not the whitelister

modifier checkWhitelist()

File: FiatTokenV2.sol
623:     modifier checkWhitelist(address _account, uint256 _value) {
624:         if (_value > 100000 * 10 ** 18) {
625:             require(
626:                 whitelisted[_account],
627:                 "Whitelistable: account is not whitelisted"//@audit replace prefix from "Whitelistable:" to "FiatToken:"
628:             );
629:         }
630:         _;
631:     }

Wrong revert string prefix

Whitelistable: account is not whitelisted should be FiatToken: account is not whitelisted

function isWhitelisted()

File: FiatTokenV2.sol
633:     /**
634:      * @dev Checks if account is whitelisted
635:      * @param _account The address to check //@audit missing @return bool
636:      */
637:     function isWhitelisted(address _account) external view returns (bool) {
638:         return whitelisted[_account];
639:     }

Missing comment @return bool

function updateWhitelister()

Wrong revert string prefix

Whitelistable: new whitelister is the zero address should be FiatToken: new whitelister is the zero address

File: Ownable.sol

function transferOwnership()

transferOwnership should be two step process

47:     /**
48:      * @dev Transfers ownership of the contract to a new account (`newOwner`).
49:      * Can only be called by the current owner.
50:      */
51:     function transferOwnership(address newOwner) public virtual onlyOwner {
52:         require(newOwner != address(0), "Ownable: new owner is the zero address");
53:         _transferOwnership(newOwner); //@audit should be 2 step
54:     }
55: 
56:     /**
57:      * @dev Transfers ownership of the contract to a new account (`newOwner`).
58:      * Internal function without access restriction.
59:      */
60:     function _transferOwnership(address newOwner) internal virtual {
61:         address oldOwner = _owner;
62:         _owner = newOwner;
63:         emit OwnershipTransferred(oldOwner, newOwner);
64:     }

The current ownership transfer process checks the new owner is not the zero address and proceeds to write the new owner's address into the owner's state variable. If the nominated EOA account is not a valid account, it is entirely possible the owner may accidentally transfer ownership to an uncontrolled account, breaking all functions with the onlyOwner() modifier. Lack of two-step procedure for critical operations leaves them error-prone. if the address is incorrect, the new address will take on the functionality of the new role immediately.

I suggest implementing a two step process where the owner nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of ownership to fully succeed. This ensures the nominated EOA account is a valid and active account.

Gas Report

Table of Contents:

• Gas Report
  • Foreword
  • File: ERC1967Proxy.sol
    • function _implementation()
      • Using both named returns and a return statement isn't necessary
  • File: UUPSUpgradeable.sol
    • function upgradeToAndCall()
    • Use calldata instead of memory for external functions where the function argument is read-only
  • File: Blocklistable.sol
    • function updateBlocklister()
      • Emitting a storage value (blocklister vs _newBlocklister)
  • File: EIP3009.sol
    • function _requireValidAuthorization()
      • Non-strict inequalities are cheaper than strict ones (1)
      • Non-strict inequalities are cheaper than strict ones (2)
  • File: FiatTokenV1.sol
    • function initialize()
      • Use tokenName instead of name
    • function mint()
      • Unchecked block L146
    • function transferFrom()
      • Unchecked block L276
      • Cache allowedfrom
    • function _transfer()
      • Unchecked block L316
      • Cache balances[from]
    • function burn()
      • Unchecked block L372
    • function updateMasterMinter()
      • Emitting a storage value (masterMinter vs _newMasterMinter)
    • function _decreaseAllowance()
      • Unchecked block L449
      • Cache allowedowner
  • File: FiatTokenV2.sol
    • function initialize()
      • Use tokenName instead of name
    • function mint()
      • Unchecked block L153
    • function transferFrom()
      • Unchecked block L285
      • Cache allowedfrom
    • function _transfer()
      • Unchecked block L326
      • Cache balances[from]
    • function burn()
      • Unchecked block L382
    • function updateMasterMinter()
      • Emitting a storage value (masterMinter vs _newMasterMinter)
    • function _decreaseAllowance()
      • Unchecked block L460
      • Cache allowedowner
    • function updateWhitelister()
      • Emitting a storage value (whitelister vs _newWhitelister)
  • General recommendations
    • Variables
      • No need to explicitly initialize variables with default values
      • Pre-increments cost less gas compared to post-increments
    • Comparisons
      • > 0 is less efficient than != 0 for unsigned integers (with proof)
    • Visibility
    • Errors
      • Reduce the size of error messages (Long revert Strings)
      • Use Custom Errors instead of Revert Strings to save Gas

Foreword

• Storage-reading optimizations

The code can be optimized by minimising the number of SLOADs. SLOADs are expensive (100 gas) compared to MLOADs/MSTOREs (3 gas). In the paragraphs below, please see the @audit-issue tags in the pieces of code's comments for more information about SLOADs that could be saved by caching the mentioned storage variables in memory variables.

• Unchecking arithmetics operations that can't underflow/overflow

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation, or the operation doesn't depend on user input), some gas can be saved by using an unchecked block: https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

• @audit tags

The code is annotated at multiple places with //@audit comments to pinpoint the issues. Please, pay attention to them for more details.

File: ERC1967Proxy.sol

function _implementation()

File: ERC1967Proxy.sol
30:     function _implementation() internal view virtual override returns (address impl) {
31:         return ERC1967Upgrade._getImplementation(); //@audit unused named returns
32:     }

Using both named returns and a return statement isn't necessary

Removing unused named returns variables can reduce gas usage (MSTOREs/MLOADs) and improve code clarity. To save gas and improve code quality: consider using only one of those.

File: UUPSUpgradeable.sol

function upgradeToAndCall()

File: UUPSUpgradeable.sol
095:     function upgradeToAndCall(address newImplementation, bytes memory data) //@audit can be calldata
096:         external
097:         payable
098:         virtual
099:         onlyProxy
100:     {
101:         _authorizeUpgrade(newImplementation);
102:         _upgradeToAndCallUUPS(newImplementation, data, true);
103:     }

Use calldata instead of memory for external functions where the function argument is read-only

Here, bytes memory data should be bytes calldata data

File: Blocklistable.sol

function updateBlocklister()

File: Blocklistable.sol
90:     function updateBlocklister(address _newBlocklister) external onlyOwner {
91:         require(
92:             _newBlocklister != address(0),
93:             "Blocklistable: new blocklister is the zero address"
94:         );
95:         blocklister = _newBlocklister;
96:         emit BlocklisterChanged(blocklister); //@audit emitting storage value
97:     }

Emitting a storage value (blocklister vs _newBlocklister)

I suggest going from

96:         emit BlocklisterChanged(blocklister);

to

96:         emit BlocklisterChanged(_newBlocklister);

File: EIP3009.sol

function _requireValidAuthorization()

File: EIP3009.sol
219:     function _requireValidAuthorization(
220:         address authorizer,
221:         bytes32 nonce,
222:         uint256 validAfter,
223:         uint256 validBefore
224:     ) private view {
225:         require(
226:             block.timestamp > validAfter, //@audit make it inclusive
227:             "FEIP3009: authorization is not yet valid"
228:         );
229:         require(
230:             block.timestamp < validBefore,//@audit make it inclusive
231:             "EIP3009: authorization is expired"
232:         );
233:         _requireUnusedAuthorization(authorizer, nonce);
234:     }

Non-strict inequalities are cheaper than strict ones (1)

It's possible to save 3 gas on block.timestamp > validAfter by making it inclusive: block.timestamp >= validAfter. I believe it wouldn't change much functionally here.

Non-strict inequalities are cheaper than strict ones (2)

It's possible to save 3 gas on block.timestamp < validBefore by making it inclusive: block.timestamp <= validBefore. I believe it wouldn't change much functionally here.

File: FiatTokenV1.sol

function initialize()

Use tokenName instead of name

This can save 2 SLOADs (around 200 gas). Replace:

File: FiatTokenV1.sol
96:         name = tokenName;
...
105:         DOMAIN_SEPARATOR = EIP712.makeDomainSeparator(name, "1"); //@audit name SLOAD 
106:         CHAIN_ID = block.chainid;
107:         NAME = name;//@audit name SLOAD

with

105:         DOMAIN_SEPARATOR = EIP712.makeDomainSeparator(tokenName, "1"); //@audit tokenName MLOAD 
...
107:         NAME = tokenName;//@audit tokenName MLOAD

Additionally, EIP712.makeDomainSeparator() signature is as such:

File: EIP712.sol
40:     function makeDomainSeparator(string memory name, string memory version)

As the first argument is a memory and not a storage, passing the storage name instead of the memory tokenName would imply another copy in memory

function mint()

File: FiatTokenV1.sol
127:     function mint(address _to, uint256 _amount)
...
139:         require(
140:             _amount <= mintingAllowedAmount,
...
146:         minterAllowed[msg.sender] = mintingAllowedAmount - _amount;//@audit should be unchecked (see L140)

Unchecked block L146

This line can't underflow due to L140. Therefore, it should be wrapped in an unchecked block.

function transferFrom()

File: FiatTokenV1.sol
258:     function transferFrom(
...
271:         require(
272:             value <= allowed[from][msg.sender], //@audit allowed[from][msg.sender] SLOAD 1
273:             "ERC20: transfer amount exceeds allowance"
274:         );
275:         _transfer(from, to, value);
276:         allowed[from][msg.sender] = allowed[from][msg.sender] - value; //@audit should be unchecked (see L272) //@audit allowed[from][msg.sender] SLOAD 2

Unchecked block L276

This line can't underflow due to L272. Therefore, it should be wrapped in an unchecked block.

Cache allowed[from][msg.sender]

Caching this in memory can save around 1 SLOAD. This is similar to an already implemented optimization in L361: function burn() for uint256 balance = balances[msg.sender]

function _transfer()

File: FiatTokenV1.sol
304:     function _transfer(
...
311:         require(
312:             value <= balances[from],//@audit SLOAD 1
313:             "ERC20: transfer amount exceeds balance"
314:         );
315: 
316:         balances[from] = balances[from] - value; //@audit should be unchecked (see L312) //@audit SLOAD 2
317:         balances[to] = balances[to] + value;

Unchecked block L316

This line can't underflow due to L312. Therefore, it should be wrapped in an unchecked block.

Cache balances[from]

Caching this in memory can save around 1 SLOAD. This is similar to an already implemented optimization in L361: function burn() for uint256 balance = balances[msg.sender]

function burn()

File: FiatTokenV1.sol
361:     function burn(uint256 _amount)
...
369:         require(balance >= _amount, "FiatToken: burn amount exceeds balance");
370: 
371:         totalSupply_ = totalSupply_ - _amount;
372:         balances[msg.sender] = balance - _amount; //@audit should be unchecked (see L369)

Unchecked block L372

This line can't underflow due to L369. Therefore, it should be wrapped in an unchecked block.

function updateMasterMinter()

File: FiatTokenV1.sol
377:     function updateMasterMinter(address _newMasterMinter) external onlyOwner {
378:         require(
379:             _newMasterMinter != address(0),
380:             "FiatToken: new masterMinter is the zero address"
381:         );
382:         masterMinter = _newMasterMinter;
383:         emit MasterMinterChanged(masterMinter); //@audit emitting storage value
384:     }

Emitting a storage value (masterMinter vs _newMasterMinter)

I suggest going from

383:         emit MasterMinterChanged(masterMinter);

to

383:         emit MasterMinterChanged(_newMasterMinter);

function _decreaseAllowance()

File: FiatTokenV1.sol
440:     function _decreaseAllowance(
...
445:         require(
446:             decrement <= allowed[owner][spender],//@audit SLOAD 1
447:             "ERC20: decreased allowance below zero"
448:         );
449:         _approve(owner, spender, allowed[owner][spender] - decrement); //@audit should be unchecked (see L446) //@audit SLOAD 2
450:     }

Unchecked block L449

This line can't underflow due to L446. Therefore, it should be wrapped in an unchecked block.

Cache allowed[owner][spender]

Caching this in memory can save around 1 SLOAD. This is similar to an already implemented optimization in L361: function burn() for uint256 balance = balances[msg.sender]

File: FiatTokenV2.sol

function initialize()

Use tokenName instead of name

This can save 2 SLOADs (around 200 gas). The explanation is the same as Use tokenName instead of name

function mint()

File: FiatTokenV2.sol
133:     function mint(address _to, uint256 _amount)
...
146:         require(
147:             _amount <= mintingAllowedAmount,
...
153:         minterAllowed[msg.sender] = mintingAllowedAmount - _amount; //@audit should be unchecked (see L147)

Unchecked block L153

This line can't underflow due to L147. Therefore, it should be wrapped in an unchecked block.

function transferFrom()

File: FiatTokenV2.sol
266:     function transferFrom(
...
280:         require(
281:             value <= allowed[from][msg.sender],//@audit allowed[from][msg.sender] SLOAD 1
...
285:         allowed[from][msg.sender] = allowed[from][msg.sender] - value; //@audit should be unchecked (see L281) //@audit allowed[from][msg.sender] SLOAD 2

Unchecked block L285

This line can't underflow due to L281. Therefore, it should be wrapped in an unchecked block.

Cache allowed[from][msg.sender]

Caching this in memory can save around 1 SLOAD.

function _transfer()

File: FiatTokenV2.sol
314:     function _transfer(
...   
321:         require(
322:             value <= balances[from],//@audit SLOAD 1
323:             "ERC20: transfer amount exceeds balance" 
...
326:         balances[from] = balances[from] - value; //@audit should be unchecked (see L322) //@audit SLOAD 2

Unchecked block L326

This line can't underflow due to L322. Therefore, it should be wrapped in an unchecked block.

Cache balances[from]

Caching this in memory can save around 1 SLOAD.

function burn()

File: FiatTokenV2.sol
371:     function burn(uint256 _amount)
...
379:         require(balance >= _amount, "FiatToken: burn amount exceeds balance");
...
382:         balances[msg.sender] = balance - _amount; //@audit should be unchecked (see L379)

Unchecked block L382

This line can't underflow due to L379. Therefore, it should be wrapped in an unchecked block.

function updateMasterMinter()

File: FiatTokenV2.sol
387:     function updateMasterMinter(address _newMasterMinter) external onlyOwner {
388:         require(
389:             _newMasterMinter != address(0),
390:             "FiatToken: new masterMinter is the zero address"
391:         );
392:         masterMinter = _newMasterMinter;
393:         emit MasterMinterChanged(masterMinter);
394:     }

Emitting a storage value (masterMinter vs _newMasterMinter)

I suggest going from

393:         emit MasterMinterChanged(masterMinter);

to

393:         emit MasterMinterChanged(_newMasterMinter);

function _decreaseAllowance()

File: FiatTokenV2.sol
451:     function _decreaseAllowance(
452:         address owner,
453:         address spender,
454:         uint256 decrement
455:     ) internal override {
456:         require(
457:             decrement <= allowed[owner][spender],//@audit SLOAD 1
458:             "ERC20: decreased allowance below zero" 
459:         );
460:         _approve(owner, spender, allowed[owner][spender] - decrement); //@audit should be unchecked (see L457) //@audit SLOAD 2
461:     }

Unchecked block L460

This line can't underflow due to L457. Therefore, it should be wrapped in an unchecked block.

Cache allowed[owner][spender]

Caching this in memory can save around 1 SLOAD.

function updateWhitelister()

File: FiatTokenV2.sol
659:     function updateWhitelister(address _newWhitelister) external onlyOwner {
660:         require(
661:             _newWhitelister != address(0),
662:             "Whitelistable: new whitelister is the zero address"
663:         );
664:         whitelister = _newWhitelister;
665:         emit WhitelisterChanged(whitelister); //@audit emitting storage value
666:     }

Emitting a storage value (whitelister vs _newWhitelister)

I suggest going from

665:         emit WhitelisterChanged(whitelister);

to

665:         emit WhitelisterChanged(_newWhitelister);

General recommendations

Variables

No need to explicitly initialize variables with default values

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {

Instances include:

v1/FiatTokenV1.sol:58:    uint256 internal totalSupply_ = 0;
v1/Pausable.sol:49:    bool public paused = false;
v2/FiatTokenV2.sol:58:    uint256 internal totalSupply_ = 0;

I suggest removing explicit initializations for default values.

Pre-increments cost less gas compared to post-increments

Comparisons

> 0 is less efficient than != 0 for unsigned integers (with proof)

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)

Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you're in a require statement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706

I suggest changing > 0 with != 0 here:

v1/FiatTokenV1.sol:136:        require(_amount > 0, "FiatToken: mint amount not greater than 0");
v1/FiatTokenV1.sol:368:        require(_amount > 0, "FiatToken: burn amount not greater than 0");
v2/FiatTokenV2.sol:143:        require(_amount > 0, "FiatToken: mint amount not greater than 0");
v2/FiatTokenV2.sol:378:        require(_amount > 0, "FiatToken: burn amount not greater than 0");

Also, please enable the Optimizer.

Visibility

Errors

Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Strings > 32 bytes:

upgradeability/ERC1967Upgrade.sol:45:        require(Address.isContract(newImplementation), "ERC1967: new implementation is not a contract");
upgradeability/ERC1967Upgrade.sol:92:                require(slot == _IMPLEMENTATION_SLOT, "ERC1967Upgrade: unsupported proxiableUUID");
upgradeability/ERC1967Upgrade.sol:94:                revert("ERC1967Upgrade: new implementation is not UUPS");
upgradeability/UUPSUpgradeable.sol:35:            "Function must be called through delegatecall"
upgradeability/UUPSUpgradeable.sol:39:            "Function must be called through active proxy"
upgradeability/UUPSUpgradeable.sol:51:            "UUPSUpgradeable: must not be called through delegatecall"
util/Address.sol:59:        require(success, "Address: unable to send value, recipient may have reverted");
util/Address.sol:114:        return functionCallWithValue(target, data, value, "Address: low-level call with value failed");
util/Address.sol:129:        require(address(this).balance >= value, "Address: insufficient balance for call");
util/Address.sol:143:        return functionStaticCall(target, data, "Address: low-level static call failed");
util/Address.sol:157:        require(isContract(target), "Address: static call to non-contract");
util/Address.sol:170:        return functionDelegateCall(target, data, "Address: low-level delegate call failed");
util/Address.sol:184:        require(isContract(target), "Address: delegate call to non-contract");
util/ECRecover.sol:62:            revert("ECRecover: invalid signature 's' value");
util/ECRecover.sol:66:            revert("ECRecover: invalid signature 'v' value");
util/SafeERC20.sol:55:            "SafeERC20: approve from non-zero to non-zero allowance"
util/SafeERC20.sol:76:            require(oldAllowance >= value, "SafeERC20: decreased allowance below zero");
util/SafeERC20.sol:96:            require(abi.decode(returndata, (bool)), "SafeERC20: ERC20 operation did not succeed");
util/StorageSlot.sol:23: *         require(Address.isContract(newImplementation), "ERC1967: new implementation is not a contract");
v1/Blocklistable.sol:47:            "Blocklistable: caller is not the blocklister"
v1/Blocklistable.sol:59:            "Blocklistable: account is blocklisted"
v1/Blocklistable.sol:93:            "Blocklistable: new blocklister is the zero address"
v1/EIP3009.sol:145:        require(to == msg.sender, "EIP3009: caller must be the payee");
v1/EIP3009.sol:208:            "EIP3009: authorization is used or canceled"
v1/EIP3009.sol:227:            "FEIP3009: authorization is not yet valid"
v1/EIP3009.sol:231:            "EIP3009: authorization is expired"
v1/FiatTokenV1.sol:78:        require(!initialized, "FiatToken: contract is already initialized");
v1/FiatTokenV1.sol:81:            "FiatToken: new masterMinter is the zero address"
v1/FiatTokenV1.sol:85:            "FiatToken: new pauser is the zero address"
v1/FiatTokenV1.sol:89:            "FiatToken: new blocklister is the zero address"
v1/FiatTokenV1.sol:93:            "FiatToken: new owner is the zero address"
v1/FiatTokenV1.sol:116:        require(minters[msg.sender], "FiatToken: caller is not a minter");
v1/FiatTokenV1.sol:135:        require(_to != address(0), "FiatToken: mint to the zero address");
v1/FiatTokenV1.sol:136:        require(_amount > 0, "FiatToken: mint amount not greater than 0");
v1/FiatTokenV1.sol:141:            "FiatToken: mint amount exceeds minterAllowance"
v1/FiatTokenV1.sol:158:            "FiatToken: caller is not the masterMinter"
v1/FiatTokenV1.sol:245:        require(owner != address(0), "ERC20: approve from the zero address");
v1/FiatTokenV1.sol:246:        require(spender != address(0), "ERC20: approve to the zero address");
v1/FiatTokenV1.sol:273:            "ERC20: transfer amount exceeds allowance"
v1/FiatTokenV1.sol:309:        require(from != address(0), "ERC20: transfer from the zero address");
v1/FiatTokenV1.sol:310:        require(to != address(0), "ERC20: transfer to the zero address");
v1/FiatTokenV1.sol:313:            "ERC20: transfer amount exceeds balance"
v1/FiatTokenV1.sol:368:        require(_amount > 0, "FiatToken: burn amount not greater than 0");
v1/FiatTokenV1.sol:369:        require(balance >= _amount, "FiatToken: burn amount exceeds balance");
v1/FiatTokenV1.sol:380:            "FiatToken: new masterMinter is the zero address"
v1/FiatTokenV1.sol:447:            "ERC20: decreased allowance below zero"
v1/Ownable.sol:52:        require(newOwner != address(0), "Ownable: new owner is the zero address");
v1/Pausable.sol:63:        require(msg.sender == pauser, "Pausable: caller is not the pauser");
v1/Pausable.sol:89:            "Pausable: new pauser is the zero address"
v1/Rescuable.sol:50:        require(msg.sender == _rescuer, "Rescuable: caller is not the rescuer");
v1/Rescuable.sol:75:            "Rescuable: new rescuer is the zero address"
v2/FiatTokenV2.sol:84:        require(!initialized, "FiatToken: contract is already initialized");
v2/FiatTokenV2.sol:87:            "FiatToken: new masterMinter is the zero address"
v2/FiatTokenV2.sol:91:            "FiatToken: new pauser is the zero address"
v2/FiatTokenV2.sol:95:            "FiatToken: new blocklister is the zero address"
v2/FiatTokenV2.sol:99:            "FiatToken: new owner is the zero address"
v2/FiatTokenV2.sol:122:        require(minters[msg.sender], "FiatToken: caller is not a minter");
v2/FiatTokenV2.sol:142:        require(_to != address(0), "FiatToken: mint to the zero address");
v2/FiatTokenV2.sol:143:        require(_amount > 0, "FiatToken: mint amount not greater than 0");
v2/FiatTokenV2.sol:148:            "FiatToken: mint amount exceeds minterAllowance"
v2/FiatTokenV2.sol:165:            "FiatToken: caller is not the masterMinter"
v2/FiatTokenV2.sol:253:        require(owner != address(0), "ERC20: approve from the zero address");
v2/FiatTokenV2.sol:254:        require(spender != address(0), "ERC20: approve to the zero address");
v2/FiatTokenV2.sol:282:            "ERC20: transfer amount exceeds allowance"
v2/FiatTokenV2.sol:319:        require(from != address(0), "ERC20: transfer from the zero address");
v2/FiatTokenV2.sol:320:        require(to != address(0), "ERC20: transfer to the zero address");
v2/FiatTokenV2.sol:323:            "ERC20: transfer amount exceeds balance"
v2/FiatTokenV2.sol:378:        require(_amount > 0, "FiatToken: burn amount not greater than 0");
v2/FiatTokenV2.sol:379:        require(balance >= _amount, "FiatToken: burn amount exceeds balance");
v2/FiatTokenV2.sol:390:            "FiatToken: new masterMinter is the zero address"
v2/FiatTokenV2.sol:458:            "ERC20: decreased allowance below zero"
v2/FiatTokenV2.sol:613:            "Whitelistable: caller is not the whitelister"
v2/FiatTokenV2.sol:627:                "Whitelistable: account is not whitelisted"
v2/FiatTokenV2.sol:662:            "Whitelistable: new whitelister is the zero address" 

I suggest shortening the revert strings to fit in 32 bytes, or that using custom errors as described next.

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Instances include:

78 results - 14 files

contracts/upgradeability/ERC1967Upgrade.sol:
  45:         require(Address.isContract(newImplementation), "ERC1967: new implementation is not a contract");
  92:                 require(slot == _IMPLEMENTATION_SLOT, "ERC1967Upgrade: unsupported proxiableUUID");

contracts/upgradeability/UUPSUpgradeable.sol:
  33:         require(
  37:         require(
  49:         require(

contracts/util/Address.sol:
   56:         require(address(this).balance >= amount, "Address: insufficient balance");
   59:         require(success, "Address: unable to send value, recipient may have reverted");
  129:         require(address(this).balance >= value, "Address: insufficient balance for call");
  130:         require(isContract(target), "Address: call to non-contract");
  157:         require(isContract(target), "Address: static call to non-contract");
  184:         require(isContract(target), "Address: delegate call to non-contract");

contracts/util/ECRecover.sol:
  71:         require(signer != address(0), "ECRecover: invalid signature");

contracts/util/SafeERC20.sol:
  53:         require(
  76:             require(oldAllowance >= value, "SafeERC20: decreased allowance below zero");
  96:             require(abi.decode(returndata, (bool)), "SafeERC20: ERC20 operation did not succeed");

contracts/util/StorageSlot.sol:
  23:  *         require(Address.isContract(newImplementation), "ERC1967: new implementation is not a contract");

contracts/v1/Blocklistable.sol:
  45:         require(
  57:         require(
  91:         require(

contracts/v1/EIP2612.sol:
  70:         require(deadline >= block.timestamp, "EIP2612: permit is expired");
  80:         require(

contracts/v1/EIP3009.sol:
  111:         require(
  145:         require(to == msg.sender, "EIP3009: caller must be the payee");
  157:         require(
  188:         require(
  206:         require(
  225:         require(
  229:         require(

contracts/v1/FiatTokenV1.sol:
   78:         require(!initialized, "FiatToken: contract is already initialized");
   79:         require(
   83:         require(
   87:         require(
   91:         require(
  116:         require(minters[msg.sender], "FiatToken: caller is not a minter");
  135:         require(_to != address(0), "FiatToken: mint to the zero address");
  136:         require(_amount > 0, "FiatToken: mint amount not greater than 0");
  139:         require(
  156:         require(
  245:         require(owner != address(0), "ERC20: approve from the zero address");
  246:         require(spender != address(0), "ERC20: approve to the zero address");
  271:         require(
  309:         require(from != address(0), "ERC20: transfer from the zero address");
  310:         require(to != address(0), "ERC20: transfer to the zero address");
  311:         require(
  368:         require(_amount > 0, "FiatToken: burn amount not greater than 0");
  369:         require(balance >= _amount, "FiatToken: burn amount exceeds balance");
  378:         require(
  445:         require(

contracts/v1/Ownable.sol:
  43:         require(owner() == _msgSender(), "Ownable: caller is not the owner");
  52:         require(newOwner != address(0), "Ownable: new owner is the zero address");

contracts/v1/Pausable.sol:
  55:         require(!paused, "Pausable: paused");
  63:         require(msg.sender == pauser, "Pausable: caller is not the pauser");
  87:         require(

contracts/v1/Rescuable.sol:
  50:         require(msg.sender == _rescuer, "Rescuable: caller is not the rescuer");
  73:         require(

contracts/v2/FiatTokenV2.sol:
   84:         require(!initialized, "FiatToken: contract is already initialized");
   85:         require(
   89:         require(
   93:         require(
   97:         require(
  122:         require(minters[msg.sender], "FiatToken: caller is not a minter");
  142:         require(_to != address(0), "FiatToken: mint to the zero address");
  143:         require(_amount > 0, "FiatToken: mint amount not greater than 0");
  146:         require(
  163:         require(
  253:         require(owner != address(0), "ERC20: approve from the zero address"); 
  254:         require(spender != address(0), "ERC20: approve to the zero address"); 
  280:         require(
  319:         require(from != address(0), "ERC20: transfer from the zero address"); 
  320:         require(to != address(0), "ERC20: transfer to the zero address");     
  321:         require(
  378:         require(_amount > 0, "FiatToken: burn amount not greater than 0");
  379:         require(balance >= _amount, "FiatToken: burn amount exceeds balance");
  388:         require(
  456:         require(
  611:         require(
  625:             require(
  660:         require(

I suggest replacing revert strings with custom errors.