Frax Ether Liquid Staking contest - RockingMiles's results

Duplicates in array

    You allow in some arrays to have duplicates. Sometimes you assumes there are no duplicates in the array.
    

Code instances:

OperatorRegistry.addValidator pushed validator without checking if already exist (other similar code samples are checking if exists, therefore I think it might be medium)

Not verified owner

    owner param should be validated to make sure the owner address is not address(0).
    Otherwise if not given the right input all only owner accessible functions will be unaccessible.
    
    

Code instance:

    Owned.sol.nominateNewOwner _owner

Named return issue

Users can mistakenly think that the return value is the named return, but it is actually the actualreturn statement that comes after. To know that the user needs to read the code and is confusing. Furthermore, removing either the actual return or the named return will save gas.

Code instances:

    sfrxETH.sol, mintWithSignature
    sfrxETH.sol, depositWithSignature

Add a timelock

To give more trust to users: functions that set key/critical variables should be put behind a timelock.

Code instances:

    https://github.com/code-423n4/2022-09-frax/tree/main/src/OperatorRegistry.sol#L202
    https://github.com/code-423n4/2022-09-frax/tree/main/src/ERC20/ERC20PermitPermissionedMint.sol#L94
    https://github.com/code-423n4/2022-09-frax/tree/main/src/OperatorRegistry.sol#L181

Unbounded loop on array that can only grow can lead to DoS

A malicious attacker that is also a protocol owner can push unlimitedly to an array, that some function loop over this array. If increasing the array size enough, calling the function that does a loop over the array will always revert since there is a gas limit. This is a Med Risk issue since it can lead to DoS with a reasonable chance of having untrusted owner or even an owner that did a mistake in good faith.

Code instances:

    ERC20PermitPermissionedMint.sol (L83): Unbounded loop on the array minters_array that can be publicly pushed by ['addMinter'] and can't be pulled
    OperatorRegistry.sol (L113): Unbounded loop on the array validators that can be publicly pushed by ['addValidator', 'removeValidator'] and can't be pulled

Not verified input

external / public functions parameters should be validated to make sure the address is not 0. Otherwise if not given the right input it can mistakenly lead to loss of user funds.

Code instances:

    ERC20PermitPermissionedMint.sol.minter_burn_from b_address
    ERC20PermitPermissionedMint.sol.setTimelock _timelock_address
    ERC20PermitPermissionedMint.sol.addMinter minter_address
    ERC20PermitPermissionedMint.sol.minter_mint m_address
    OperatorRegistry.sol.setTimelock _timelock_address
    ERC20PermitPermissionedMint.sol.removeMinter minter_address
    sfrxETH.sol.depositWithSignature receiver
    sfrxETH.sol.mintWithSignature receiver

Unnecessary equals boolean

Boolean variables can be checked within conditionals directly without the use of equality operators to true/false.

Code instances:

    ERC20PermitPermissionedMint.sol, 68: require(minters[minter_address] == false, "Address already exists");
    ERC20PermitPermissionedMint.sol, 78: require(minters[minter_address] == true, "Address nonexistant");

State variables that could be set immutable

In the following files there are state variables that could be set immutable to save gas.

Code instance:

    DOMAIN_SEPARATOR in SigUtils.sol

Caching array length can save gas

Caching the array length is more gas efficient. This is because access to a local variable in solidity is more efficient than query storage / calldata / memory. We recommend to change from:

for (uint256 i=0; i<array.length; i++) { ... }

to:

uint len = array.length  
for (uint256 i=0; i<len; i++) { ... }

Code instance:

    ERC20PermitPermissionedMint.sol, minters_array, 84

Prefix increments are cheaper than postfix increments

Prefix increments are cheaper than postfix increments. Further more, using unchecked {++x} is even more gas efficient, and the gas saving accumulates every iteration and can make a real change There is no risk of overflow caused by increamenting the iteration index in for loops (the ++i in for (uint256 i = 0; i < numIterations; ++i)). But increments perform overflow checks that are not necessary in this case. These functions use not using prefix increments (++x) or not using the unchecked keyword:

Code instances:

    just change to unchecked: OperatorRegistry.sol, i, 84
    just change to unchecked: OperatorRegistry.sol, i, 63
    change to prefix increment and unchecked: ERC20PermitPermissionedMint.sol, i, 84

Unnecessary index init

In for loops you initialize the index to start from 0, but it already initialized to 0 in default and this assignment cost gas. It is more clear and gas efficient to declare without assigning 0 and will have the same meaning:

Code instances:

    OperatorRegistry.sol, 84
    ERC20PermitPermissionedMint.sol, 84
    OperatorRegistry.sol, 63

Rearrange state variables

You can change the order of the storage variables to decrease memory uses.

Code instance:

In deployGoerli.s.sol,rearranging the storage fields can optimize to: 4 slots from: 5 slots. The new order of types (you choose the actual variables): 1. bytes 2. address 3. uint32 4. address 5. address

Inline one time use functions

The following functions are used exactly once. Therefore you can inline them and save gas and improve code clearness.

Code instances:

    sfrxETH.sol, beforeWithdraw
    SigUtils.sol, getStructHash