Slither fixes

[petejkim]

Fixed issues detected by Slither

Issues found were minor:

• Local variable shadowing
• Unnecessary boolean equality
• Mark public functions that aren't called internally as external

INFO:Detectors:
FiatTokenProxy.constructor(address)._implementation (FiatTokenProxy.sol#39) shadows:
	- UpgradeabilityProxy._implementation() (upgradeability/UpgradeabilityProxy.sol#71-77) (function)
	- Proxy._implementation() (upgradeability/Proxy.sol#49) (function)
FiatTokenV1.initialize(string,string,string,uint8,address,address,address,address)._owner (FiatTokenV1.sol#68) shadows:
	- Ownable._owner (Ownable.sol#38) (state variable)
FiatTokenV1.allowance(address,address).owner (FiatTokenV1.sol#171) shadows:
	- Ownable.owner() (Ownable.sol#58-60) (function)
UpgradedFiatTokenNewFieldsNewLogicTest.initialize(string,string,string,uint8,address,address,address,address,bool,address,uint256)._owner (test/UpgradedFiatTokenNewFieldsNewLogicTest.sol#48) shadows:
	- Ownable._owner (Ownable.sol#38) (state variable)
DummyERC20.constructor(string,string,uint256).name (test/DummyERC20.sol#32) shadows:
	- ERC20.name() (@openzeppelin/contracts/token/ERC20/ERC20.sol#64-66) (function)
DummyERC20.constructor(string,string,uint256).symbol (test/DummyERC20.sol#33) shadows:
	- ERC20.symbol() (@openzeppelin/contracts/token/ERC20/ERC20.sol#72-74) (function)
AdminUpgradeabilityProxy.constructor(address)._implementation (upgradeability/AdminUpgradeabilityProxy.sol#71) shadows:
	- UpgradeabilityProxy._implementation() (upgradeability/UpgradeabilityProxy.sol#71-77) (function)
	- Proxy._implementation() (upgradeability/Proxy.sol#49) (function)
UpgradedFiatTokenNewFieldsTest.initialize(string,string,string,uint8,address,address,address,address,bool,address,uint256)._owner (test/UpgradedFiatTokenNewFieldsTest.sol#48) shadows:
	- Ownable._owner (Ownable.sol#38) (state variable)
UpgradeabilityProxy.constructor(address)._implementation (upgradeability/UpgradeabilityProxy.sol#58) shadows:
	- UpgradeabilityProxy._implementation() (upgradeability/UpgradeabilityProxy.sol#71-77) (function)
	- Proxy._implementation() (upgradeability/Proxy.sol#49) (function)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#local-variable-shadowingINFO:Detectors:

INFO:Detectors:
FiatTokenV1.onlyMinters() (FiatTokenV1.sol#102-108) compares to a boolean constant:
	-require(bool,string)(minters[msg.sender] == true,FiatToken: caller is not a minter) (FiatTokenV1.sol#103-106)
Blacklistable.notBlacklisted(address) (Blacklistable.sol#57-63) compares to a boolean constant:
	-require(bool,string)(blacklisted[_account] == false,Blacklistable: account is blacklisted) (Blacklistable.sol#58-61)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#boolean-equality

INFO:Detectors:
rescuer() should be declared external:
	- Rescuable.rescuer() (Rescuable.sol#43-45)
setCompleted(uint256) should be declared external:
	- Migrations.setCompleted(uint256) (Migrations.sol#42-44)
upgrade(address) should be declared external:
	- Migrations.upgrade(address) (Migrations.sol#46-49)
mint(address,uint256) should be declared external:
	- FiatTokenV1.mint(address,uint256) (FiatTokenV1.sol#117-140)
minterAllowance(address) should be declared external:
	- FiatTokenV1.minterAllowance(address) (FiatTokenV1.sol#157-159)
isMinter(address) should be declared external:
	- FiatTokenV1.isMinter(address) (FiatTokenV1.sol#165-167)
allowance(address,address) should be declared external:
	- FiatTokenV1.allowance(address,address) (FiatTokenV1.sol#174-181)
	- ERC20.allowance(address,address) (@openzeppelin/contracts/token/ERC20/ERC20.sol#123-125)
totalSupply() should be declared external:
	- FiatTokenV1.totalSupply() (FiatTokenV1.sol#186-188)
	- ERC20.totalSupply() (@openzeppelin/contracts/token/ERC20/ERC20.sol#96-98)
balanceOf(address) should be declared external:
	- FiatTokenV1.balanceOf(address) (FiatTokenV1.sol#194-196)
	- ERC20.balanceOf(address) (@openzeppelin/contracts/token/ERC20/ERC20.sol#103-105)
approve(address,uint256) should be declared external:
	- FiatTokenV1.approve(address,uint256) (FiatTokenV1.sol#202-213)
	- ERC20.approve(address,uint256) (@openzeppelin/contracts/token/ERC20/ERC20.sol#134-137)
transferFrom(address,address,uint256) should be declared external:
	- ERC20.transferFrom(address,address,uint256) (@openzeppelin/contracts/token/ERC20/ERC20.sol#151-155)
	- FiatTokenV1.transferFrom(address,address,uint256) (FiatTokenV1.sol#222-250)
transfer(address,uint256) should be declared external:
	- FiatTokenV1.transfer(address,uint256) (FiatTokenV1.sol#258-276)
	- ERC20.transfer(address,uint256) (@openzeppelin/contracts/token/ERC20/ERC20.sol#115-118)
configureMinter(address,uint256) should be declared external:
	- FiatTokenV1.configureMinter(address,uint256) (FiatTokenV1.sol#284-294)
removeMinter(address) should be declared external:
	- FiatTokenV1.removeMinter(address) (FiatTokenV1.sol#301-310)
burn(uint256) should be declared external:
	- FiatTokenV1.burn(uint256) (FiatTokenV1.sol#318-332)
updateMasterMinter(address) should be declared external:
	- FiatTokenV1.updateMasterMinter(address) (FiatTokenV1.sol#334-341)
initialize(string,string,string,uint8,address,address,address,address,bool,address,uint256) should be declared external:
	- UpgradedFiatTokenNewFieldsNewLogicTest.initialize(string,string,string,uint8,address,address,address,address,bool,address,uint256) (test/UpgradedFiatTokenNewFieldsNewLogicTest.sol#40-64)
setNewAddress(address) should be declared external:
	- UpgradedFiatTokenNewFieldsNewLogicTest.setNewAddress(address) (test/UpgradedFiatTokenNewFieldsNewLogicTest.sol#78-80)
pause() should be declared external:
	- Pausable.pause() (Pausable.sol#70-73)
unpause() should be declared external:
	- Pausable.unpause() (Pausable.sol#78-81)
updatePauser(address) should be declared external:
	- Pausable.updatePauser(address) (Pausable.sol#86-93)
transferOwnership(address) should be declared external:
	- Ownable.transferOwnership(address) (Ownable.sol#81-88)
isBlacklisted(address) should be declared external:
	- Blacklistable.isBlacklisted(address) (Blacklistable.sol#69-71)
blacklist(address) should be declared external:
	- Blacklistable.blacklist(address) (Blacklistable.sol#77-80)
unBlacklist(address) should be declared external:
	- Blacklistable.unBlacklist(address) (Blacklistable.sol#86-89)
updateBlacklister(address) should be declared external:
	- Blacklistable.updateBlacklister(address) (Blacklistable.sol#91-98)
initialize(string,string,string,uint8,address,address,address,address,bool,address,uint256) should be declared external:
	- UpgradedFiatTokenNewFieldsTest.initialize(string,string,string,uint8,address,address,address,address,bool,address,uint256) (test/UpgradedFiatTokenNewFieldsTest.sol#40-64)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#public-function-that-could-be-declared-as-external

The following issues can be safely ignored:

INFO:Detectors:
Proxy._delegate(address) (upgradeability/Proxy.sol#57-88) uses assembly
        - INLINE ASM None (upgradeability/Proxy.sol#59-87)
AdminUpgradeabilityProxy._admin() (upgradeability/AdminUpgradeabilityProxy.sol#144-151) uses assembly
        - INLINE ASM None (upgradeability/AdminUpgradeabilityProxy.sol#148-150)
AdminUpgradeabilityProxy._setAdmin(address) (upgradeability/AdminUpgradeabilityProxy.sol#157-164) uses assembly
        - INLINE ASM None (upgradeability/AdminUpgradeabilityProxy.sol#161-163)
UpgradeabilityProxy._implementation() (upgradeability/UpgradeabilityProxy.sol#71-77) uses assembly
        - INLINE ASM None (upgradeability/UpgradeabilityProxy.sol#74-76)
UpgradeabilityProxy._setImplementation(address) (upgradeability/UpgradeabilityProxy.sol#92-104) uses assembly
        - INLINE ASM None (upgradeability/UpgradeabilityProxy.sol#101-103)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#assembly-usage

INFO:Detectors:
Low level call in AdminUpgradeabilityProxy.upgradeToAndCall(address,bytes) (upgradeability/AdminUpgradeabilityProxy.sol#128-139):
        - (success) = address(this).call{value: msg.value}(data) (upgradeability/AdminUpgradeabilityProxy.sol#136)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#low-level-calls

[eztierney]

I don't think we should rename owner -> contractOwner

• the benefit is fairly low (I have not heard of any end user confusion from this issue)
• there may be existing code relying on the old name
• the event & transfer methods were unchanged and would be inconsistent with the new name

In general I think we should have a very high bar for changing the name or signature of any existing properties/methods in an upgrade.

[eztierney]

Also, it would be great if changes like this could be broken up into separate PRs in the future - it is a lot easier to review small focused changes (e.g. a PR that adds external to a bunch of functions).

[petejkim]

It's mostly for developer confusion rather than user confusion (variable shadowing), but I do agree that we should have sufficient unit test coverage and stringent reviews so in practice it never actually causes a problem. I can revert the rename and keep the other changes. Sounds good?

[eztierney]

yeah, that sounds good

[eztierney]

Look good.

To confirm my understanding the public -> external change will change how arguments are referenced in the generated code, but should have no effect on how the functions are invoked? So existing callers should be unaffected.

[petejkim]

Oops accidentally re-requested a review.

[petejkim]

Existing callers should be unaffected, but I will add a test to be safe.