It is unsafe to rely on logic that underlying token have permit function implemented. It is considered good practice to use ERC20 standard modification that contains logic of safe calling of permit function. This is so, because of possibility of existence of phantom function that will be used when contract calls permit and, as result, incorrect logic of usage of such function.
2. indexed parameters of events
It is reasonable to use indexed for some of the parameters of the events EmergencyERC20Recovered, TimelockChanged, ValidatorsSwapped, MinterAdded, MinterRemoved, TimelockChanged, OwnerNominated, OwnerChanged.
3. possibility of arbitrary call for owner account
It is reasonable to add allowance for the owner account to call any contract by call/delegate call on behalf of contract. As we can see owner already have an ability to withdraw any amount of ETH or any ERC20 token, so it will be good to give owner rights to call any logic and to not have many special recover functions.
4. external minter_burn_from and minter_mint functions
minter_burn_from and minter_mint functions declared as public, but it is reasonable to delcare them as external, since they are not used throught internal calls.
5. clearValidatorArray function DoS
There is a function clearValidatorArray in OperatorRegistry contract. It clears all the validator array, so for each of the validators it writes to the corresponding storage slot a zero value. In case of big amount of validators there will be no chance to call this function due to very big amount of gas to be used, which actually can be even greater than the block gas limit.
1. usage of non-ERC20 standard function "permit"
It is unsafe to rely on logic that underlying token have
permitfunction implemented. It is considered good practice to use ERC20 standard modification that contains logic of safe calling ofpermitfunction. This is so, because of possibility of existence of phantom function that will be used when contract callspermitand, as result, incorrect logic of usage of such function.2. indexed parameters of events
It is reasonable to use
indexedfor some of the parameters of the eventsEmergencyERC20Recovered,TimelockChanged,ValidatorsSwapped,MinterAdded,MinterRemoved,TimelockChanged,OwnerNominated,OwnerChanged.3. possibility of arbitrary call for owner account
It is reasonable to add allowance for the owner account to call any contract by
call/delegate callon behalf of contract. As we can see owner already have an ability to withdraw any amount of ETH or any ERC20 token, so it will be good to give owner rights to call any logic and to not have many specialrecoverfunctions.4. external minter_burn_from and minter_mint functions
minter_burn_fromandminter_mintfunctions declared aspublic, but it is reasonable to delcare them asexternal, since they are not used throught internal calls.5. clearValidatorArray function DoS
There is a function
clearValidatorArrayinOperatorRegistrycontract. It clears all the validator array, so for each of the validators it writes to the corresponding storage slot a zero value. In case of big amount of validators there will be no chance to call this function due to very big amount of gas to be used, which actually can be even greater than the block gas limit.