search

code-423n4 / 2022-02-jpyc-findings

1 stars 0 forks source link

QA Report #24

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

1 State variable shadowing inside functions

Ownable.owner has supreme power in this contract. There are many functions that have local variables using the same name as owner, hence shadowing this important state variable.

Functions that have this issue are, in FiatTokenV1, 

allowance
_approve
_increaseAllowance
_decreaseAllowance
_permit

To mitigate this issue, change local variable name from owner to _owner

2. NatSpec Comments missing @return

Functions that have this issue are, in FiatTokenV1,

isMinter
balanceOf

3 Typos

 Blocklist => Blacklist 
 Blocklistable => Blacklistable 
 Blocklister => Blacklister

4 Reimplementation of OpenZeppline ERC20, Pausable

ERC20 standard is reimplemented here. I wonder why OZ implementation of ERC20 is not used.

5 Use AccessControl instead of Ownable

There are many roles in this contract, including minter, masterMinter, owner, Pauser, rescuer and functions require more granular role-based access control. Thus, @openzeppelin/contracts/access/AccessControl.sol could be more suitable and simpler implementation than using @openzeppelin/contracts/access/Ownable.sol.

6 Not using the latest version of UUPSUpgradeable

The latest version of UUPSUpgradeable contract is v4.5 and the contract uses v4.4.1

7 Inherit @openzeppelin/contracts-upgradeable library

It could be less risky in inheriting the upgradeable version of openzeppelin library, particularly initializable, ERC20Upgradeable, 'PausableUpgradeable', 'OwnableUpgradeable'. Upgradability pattern could be complex and prone to attacks.

8 public function can be external

Instances are FiatTokenV1.initialize, FiatTokenV2.initialize

thurendous commented 2 years ago

2 is valid. Thanks.

thurendous commented 2 years ago

Although some of the issues you mentioned we will not fix, thank you for pointing out.

thurendous commented 2 years ago

  1. NatSpec Comments missing @return

  2. Not using the latest version of UUPSUpgradeable

Fixed and thanks.

Final change can be viewed here.