JPYC contest - IllIllI's results

Forked files should have changelog

Your version of Pausable.sol does a good job of saying the exact source of the fork as well as most of the changes

/**
 * @notice Base contract which allows children to implement an emergency stop
 * mechanism
 * @dev Forked from https://github.com/OpenZeppelin/openzeppelin-contracts/blob/feb665136c0dae9912e08397c1a21c4af3651ef3/contracts/lifecycle/Pausable.sol
 * Modifications:
 * 1. Added pauser role, switched pause/unpause to be onlyPauser (6/14/2018)
 * 2. Removed whenNotPause/whenPaused from pause/unpause (6/14/2018)
 * 3. Removed whenPaused (6/14/2018)
 * 4. Switches ownable library to use ZeppelinOS (7/12/18)
 * 5. Remove constructor (7/13/18)
 * 6. Reformat, conform to Solidity 0.6 syntax and add error messages (5/13/20)
 * 7. Make public functions external (5/27/20)
 */

https://github.com/code-423n4/2022-02-jpyc/blob/cfc018384dd1d71febaa57f0576cb51f5d9c7e07/contracts/v1/Pausable.sol#L30-L42

There are a lot of other files that do not have such changelogs

$ find ./contracts -name "*.sol" | xargs grep -li openzeppelin | xargs grep -iL "forked from"
./contracts/proxy/ERC1967Proxy.sol
./contracts/proxy/Proxy.sol
./contracts/util/SafeERC20.sol
./contracts/util/Context.sol
./contracts/util/ECRecover.sol
./contracts/util/Address.sol
./contracts/util/IERC20.sol
./contracts/test/ERC20.sol
./contracts/test/IERC20Metadata.sol
./contracts/upgradeability/ERC1967Upgrade.sol
./contracts/upgradeability/UUPSUpgradeable.sol
./contracts/upgradeability/draft-IERC1822.sol
./contracts/v1/Ownable.sol

For example, Ownable has one of its functions removed, and all of the files have __gap storage variables added. Having a changelog will make upgrading again easier, and will make security reviews more thorough.

Not all contracts have added a __gap variable

AbstractFiatTokenV1.sol is missing a __gap storage variable. Consider adding one so that storage can be used in this contract in future versions https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/AbstractFiatTokenV1.sol

Update other sensitive terms

I see that there has been an attempt to get rid of sensitive terms such as 'blacklist' by using 'blocklist'. The contracts still use some of the others, and I would suggest changing 'whitelist' to 'allowlist', and 'masterMinter' to 'primaryMinter' or 'minterAdmin' https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v2/FiatTokenV2.sol

Using bools for storage incurs overhead

    // Booleans are more expensive than uint256 or any type that takes up a full
    // word because each write operation emits an extra SLOAD to first read the
    // slot's contents, replace the bits taken up by the boolean, and then write
    // back. This is the compiler's defense against contract upgrades and
    // pointer aliasing, and it cannot be disabled.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27

mapping(address => mapping(bytes32 => bool)) private _authorizationStates;         

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/EIP3009.sol#L53

bool internal initialized;             

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.sol#L54

mapping(address => bool) internal minters;           

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.sol#L59

bool internal initialized;             

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.altered.sol#L54

mapping(address => bool) internal minters;           

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.altered.sol#L59

bool public paused = false;           

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/Pausable.sol#L49

mapping(address => bool) internal blocklisted;           

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/Blocklistable.sol#L35

bool internal initialized;             

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v2/FiatTokenV2.sol#L54

mapping(address => bool) internal minters;           

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v2/FiatTokenV2.sol#L59

mapping(address => bool) internal whitelisted;           

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v2/FiatTokenV2.sol#L62

abi.encode() is less efficient than abi.encodePacked()

bytes memory data = abi.encode(           

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/EIP3009.sol#L102

bytes memory data = abi.encode(           

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/EIP3009.sol#L148

bytes memory data = abi.encode(           

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/EIP3009.sol#L183

bytes memory data = abi.encode(           

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/EIP2612.sol#L72

require() strings longer than 32 bytes cost extra gas

require(oldAllowance >= value, "SafeERC20decreased allowance below zero");                 

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/util/SafeERC20.sol#L76

require(abi.decode(returndata, (bool)), "SafeERC20ERC20 operation did not succeed");                 

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/util/SafeERC20.sol#L96

require(Address.isContract(newImplementation), "ERC1967new implementation is not a contract");                 

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/util/StorageSlot.sol#L23*

require(success, "Addressunable to send value, recipient may have reverted");               

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/util/Address.sol#L59

require(address(this).balance >= value, "Addressinsufficient balance for call");                 

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/util/Address.sol#L129

require(isContract(target), "Addressstatic call to non-contract");                   

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/util/Address.sol#L157

require(isContract(target), "Addressdelegate call to non-contract");                   

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/util/Address.sol#L184

require(Address.isContract(newImplementation), "ERC1967new implementation is not a contract");                 

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/upgradeability/ERC1967Upgrade.sol#L45

require(slot == _IMPLEMENTATION_SLOT, "ERC1967Upgradeunsupported proxiableUUID");                   

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/upgradeability/ERC1967Upgrade.sol#L92

require(to == msg.sender, "EIP3009caller must be the payee");                

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/EIP3009.sol#L145

require(!initialized, "FiatTokencontract is already initialized");                   

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.sol#L78

require(minters[msg.sender], "FiatTokencaller is not a minter");                  

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.sol#L116

require(_to != address(0), "FiatTokenmint to the zero address");                

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.sol#L135

require(_amount > 0, "FiatTokenmint amount not greater than 0");               

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.sol#L136

require(owner != address(0), "ERC20approve from the zero address");                

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.sol#L245

require(spender != address(0), "ERC20approve to the zero address");                

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.sol#L246

require(from != address(0), "ERC20transfer from the zero address");                

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.sol#L309

require(to != address(0), "ERC20transfer to the zero address");                

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.sol#L310

require(_amount > 0, "FiatTokenburn amount not greater than 0");               

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.sol#L368

require(balance >= _amount, "FiatTokenburn amount exceeds balance");                 

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.sol#L369

require(!initialized, "FiatTokencontract is already initialized");                   

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.altered.sol#L78

require(minters[msg.sender], "FiatTokencaller is not a minter");                  

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.altered.sol#L116

require(_to != address(0), "FiatTokenmint to the zero address");                

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.altered.sol#L135

require(_amount > 0, "FiatTokenmint amount not greater than 0");               

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.altered.sol#L136

require(owner != address(0), "ERC20approve from the zero address");                

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.altered.sol#L245

require(spender != address(0), "ERC20approve to the zero address");                

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.altered.sol#L246

require(from != address(0), "ERC20transfer from the zero address");                

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.altered.sol#L309

require(to != address(0), "ERC20transfer to the zero address");                

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.altered.sol#L310

require(_amount > 0, "FiatTokenburn amount not greater than 0");               

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.altered.sol#L368

require(balance >= _amount, "FiatTokenburn amount exceeds balance");                 

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.altered.sol#L369

require(msg.sender == pauser, "Pausablecaller is not the pauser");                

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/Pausable.sol#L63

require(newOwner != address(0), "Ownablenew owner is the zero address");               

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/Ownable.sol#L52

require(msg.sender == _rescuer, "Rescuablecaller is not the rescuer");                

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/Rescuable.sol#L50

require(!initialized, "FiatTokencontract is already initialized");                   

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v2/FiatTokenV2.sol#L84

require(minters[msg.sender], "FiatTokencaller is not a minter");                  

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v2/FiatTokenV2.sol#L122

require(_to != address(0), "FiatTokenmint to the zero address");                

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v2/FiatTokenV2.sol#L142

require(_amount > 0, "FiatTokenmint amount not greater than 0");               

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v2/FiatTokenV2.sol#L143

require(owner != address(0), "ERC20approve from the zero address");                

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v2/FiatTokenV2.sol#L253

require(spender != address(0), "ERC20approve to the zero address");                

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v2/FiatTokenV2.sol#L254

require(from != address(0), "ERC20transfer from the zero address");                

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v2/FiatTokenV2.sol#L319

require(to != address(0), "ERC20transfer to the zero address");                

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v2/FiatTokenV2.sol#L320

require(_amount > 0, "FiatTokenburn amount not greater than 0");               

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v2/FiatTokenV2.sol#L378

require(balance >= _amount, "FiatTokenburn amount exceeds balance");                 

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v2/FiatTokenV2.sol#L379

Using 1e18 rather than 10**18 saves gas

if (_value > 100000 * 10 ** 18) {       

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v2/FiatTokenV2.sol#L624

Functions not called by the contract itself should be external rather than public

function transferOwnership(address newOwner) public virtual onlyOwner {         

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/Ownable.sol#L51

function setName2(string memory _name) public {          

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v2/FiatTokenV2test.sol#L11

Using > 0 costs more gas than != 0 when used on uints in a require() statement

require(_amount > 0, "FiatTokenmint amount not greater than 0");       

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.sol#L136

require(_amount > 0, "FiatTokenburn amount not greater than 0");       

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.sol#L368

require(_amount > 0, "FiatTokenmint amount not greater than 0");       

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.altered.sol#L136

require(_amount > 0, "FiatTokenburn amount not greater than 0");       

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.altered.sol#L368

require(_amount > 0, "FiatTokenmint amount not greater than 0");       

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v2/FiatTokenV2.sol#L143

require(_amount > 0, "FiatTokenburn amount not greater than 0");       

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v2/FiatTokenV2.sol#L378

It costs more gas to initialize variables to zero than to let the default of zero be applied

uint256 internal totalSupply_ = 0;           

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.sol#L58

minterAllowed[minter] = 0;             

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.sol#L350

uint256 internal totalSupply_ = 0;           

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/FiatTokenV1.altered.sol#L58

uint256 internal totalSupply_ = 0;           

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v2/FiatTokenV2.sol#L58