Frax Ether Liquid Staking contest - delfin454000's results

Typos

---

ERC20PermitPermissionedMint.sol: L78

        require(minters[minter_address] == true, "Address nonexistant");

Change nonexistant to nonexistent

---

frxETHMinter.sol: L78-81

        uint256 sfrxeth_recieved = sfrxETHToken.deposit(msg.value, recipient);
        require(sfrxeth_recieved > 0, 'No sfrxETH was returned');

        return sfrxeth_recieved;

Change sfrxeth_recieved to sfrxeth_received in each case

---

frxETHMinter.sol: L176

    /// @notice Toggle allowing submites

Change submites to submits

---

xERC4626.sol: L43

    /// @notice Compute the amount of tokens available to share holders.

Change share holders to shareholders

---

---

Long single line comments

In theory, comments over 79 characters should wrap using multi-line comment syntax. Even if somewhat longer comments are acceptable and a scroll bar is provided, there are cases where very long comments interfere with readability. Below are five instances of extra-long comments whose readability could be improved through wrapping, as shown:

---

sfrxETH.sol: L31-39

/** @dev Exchange rate between frxETH and sfrxETH floats, you can convert your sfrxETH for more frxETH over time.
    Exchange rate increases as the frax msig mints new frxETH corresponding to the staking yield and drops it into the vault (sfrxETH contract).
    There is a short time period, “cycles” which the exchange rate increases linearly over. This is to prevent gaming the exchange rate (MEV).
    The cycles are constant length, but calling syncRewards slightly into a would-be cycle keeps the same would-be endpoint (so cycle ends are every X seconds).
    Someone must call syncRewards, which queues any new frxETH in the contract to be added to the redeemable amount.
    sfrxETH adheres to ERC-4626 vault specs 
    Mint vs Deposit
    mint() - deposit targeting a specific number of sfrxETH out
    deposit() - deposit knowing a specific number of frxETH in */

Suggestion:

/** 
 @dev Exchange rate between frxETH and sfrxETH floats, you can convert
      your sfrxETH for more frxETH over time.
     Exchange rate increases as the frax msig mints new frxETH corresponding 
         to the staking yield and drops it into the vault (sfrxETH contract).
     There is a short time period, “cycles” which the exchange rate increases 
         linearly over — this is to prevent gaming the exchange rate (MEV).
     The cycles are constant length, but calling syncRewards slightly into a would-be cycle
         keeps the same would-be endpoint (so cycle ends are every X seconds).
     Someone must call syncRewards, which queues any new frxETH 
         in the contract to be added to the redeemable amount.
     sfrxETH adheres to ERC-4626 vault specs 
     Mint vs Deposit:
         mint() - deposit targeting a specific number of sfrxETH out
         deposit() - deposit knowing a specific number of frxETH in
*/

---

frxETHMinter.sol: L33-36

/// @notice Accepts user-supplied ETH and converts it to frxETH (submit()), and also optionally inline stakes it for sfrxETH (submitAndDeposit())
/** @dev Has permission to mint frxETH. 
    Once +32 ETH has accumulated, adds it to a validator, which then deposits it for ETH 2.0 staking (depositEther())
    Withhold ratio refers to what percentage of ETH this contract keeps whenever a user makes a deposit. 0% is kept initially */

Suggestion:

/// @notice Accepts user-supplied ETH and converts it to frxETH (submit()), 
     and also optionally inline stakes it for sfrxETH (submitAndDeposit()).
/** 
 @dev Has permission to mint frxETH. 
     Once +32 ETH has accumulated, adds it to a validator, which then 
         deposits it for ETH 2.0 staking (depositEther()).
     Withhold ratio refers to what percentage of ETH this contract keeps 
         whenever a user makes a deposit. 0% is kept initially.
*/

---

frxETHMinter.sol: L138-139

            // Make sure the validator hasn't been deposited into already, to prevent stranding an extra 32 eth
            // until withdrawals are allowed

Suggestion:

            // Make sure the validator hasn't been deposited into already, to prevent  
            //    stranding an extra 32 eth until withdrawals are allowed.

---

xERC4626.sol: L11-19

/** 
 @title  An xERC4626 Single Staking Contract
 @notice This contract allows users to autocompound rewards denominated in an underlying reward token. 
         It is fully compatible with [ERC4626](https://eips.ethereum.org/EIPS/eip-4626) allowing for DeFi composability.
         It maintains balances using internal accounting to prevent instantaneous changes in the exchange rate.
         NOTE: an exception is at contract creation, when a reward cycle begins before the first deposit. After the first deposit, exchange rate updates smoothly.

         Operates on "cycles" which distribute the rewards surplus over the internal balance to users linearly over the remainder of the cycle window.
*/

Suggestion:

/** 
 @title  An xERC4626 Single Staking Contract
 @notice This contract allows users to autocompound rewards denominated in an underlying reward token. 
     It is fully compatible with [ERC4626](https://eips.ethereum.org/EIPS/eip-4626),
         allowing for DeFi composability.
     It maintains balances using internal accounting to prevent instantaneous 
         changes in the exchange rate.
     NOTE: an exception is at contract creation, when a reward cycle begins before
         the first deposit. After the first deposit, exchange rate updates smoothly.
     Operates on "cycles" which distribute the rewards surplus over the 
         internal balance to users linearly over the remainder of the cycle window.
*/

---

xERC4626.sol: L77

    /// All surplus `asset` balance of the contract over the internal balance becomes queued for the next cycle.

Suggestion:

    /// All surplus `asset` balance of the contract over the internal balance 
    ///    becomes queued for the next cycle. 

---

---

Update sensitive terms in both comments and code

Terms incorporating "black," "white," "slave" or "master" are potentially problematic. Substituting more neutral terminology is becoming common practice.

---

ERC20PermitPermissionedMint.sol: L64

    // Adds whitelisted minters 

Suggestion: Change whitelisted to allowlisted or allowed

---

---

Missing @param statements

@param statements, which document parameters, are missing for most of the functions and constructors in the Frax Ether in-scope contracts (a @param statement is given for one function parameter: frxETHMinter.sol: L157). Below are examples of missing @param statements, however there are dozens more that need to be addressed.

function example:

sfrxETH.sol: L47-51

    /// @notice Syncs rewards if applicable beforehand. Noop if otherwise 
    function beforeWithdraw(uint256 assets, uint256 shares) internal override {
        super.beforeWithdraw(assets, shares); // call xERC4626's beforeWithdraw first
        if (block.timestamp >= rewardsCycleEnd) { syncRewards(); } 
    }

@param statements missing for assets and shares

constructor example:

sfrxETH.sol: L41-44

    /* ========== CONSTRUCTOR ========== */
    constructor(ERC20 _underlying, uint32 _rewardsCycleLength)
        ERC4626(_underlying, "Staked Frax Ether", "sfrxETH")
        xERC4626(_rewardsCycleLength)

@param statements missing for _underlying and _rewardsCycleLength

---

---

Inconsistent require string punctuation

While require messages may be punctuated as either "message" or 'message', the treatment should be consistent within a contest. Below is the only message surrounded by ' ' instead of " ":

frxETHMinter.sol: L79

        require(sfrxeth_recieved > 0, 'No sfrxETH was returned');

Suggestion: Change message to "No sfrxETH was returned"

---

---

Incorrect require statement syntax

frxETHMinter.sol: L160

        require (newRatio <= RATIO_PRECISION, "Ratio cannot surpass 100%");

Recommendation: Remove unneeded space between require and (

---

---

Use != 0 instead of > 0 in a require statement if variable is an unsigned integer (uint)

!= 0 should be used where possible since > 0 costs more gas

frxETHMinter.sol: L79

        require(sfrxeth_recieved > 0, 'No sfrxETH was returned');

Recommendation: Change sfrxeth_recieved > 0 to sfrxeth_recieved != 0

Note: The spelling error (recieved) was addressed under Typos in the QA Report (low / non-critical)

---

---

State variables should not be initialized to their default values

In particular, initializing uint variables to their default value of 0 is unnecessary and costs gas

---

frxETHMinter.sol: L50

        uint256 withheld_amt = 0;

Change to:

        uint256 withheld_amt;

---

---

Require revert string is too long

The revert string below can be shortened to 32 characters (as shown) to save gas

---

frxETHMinter.sol: L167

        require(amount <= currentWithheldETH, "Not enough withheld ETH in contract");

Change message to Not enough contract withheld ETH

---

---

Array length should not be looked up during every iteration of a for loop

Since calculating the array length costs gas, it's best to read the length of the array from memory before executing the loop, as demonstrated below

---

ERC20PermitPermissionedMint.sol: L84-89

        for (uint i = 0; i < minters_array.length; i++){ 
            if (minters_array[i] == minter_address) {
                minters_array[i] = address(0); // This will leave a null in the array and keep the indices the same
                break;
            }
        }

Suggestion:

        uint256 totalMintersLength = minters_array.length; 
        for (uint i = 0; i < totalMintersLength; i++){ 
            if (minters_array[i] == minter_address) {
                minters_array[i] = address(0); // This will leave a null in the array and keep the indices the same
                break;
            }
        }

---

Similarly for the following for loop:

OperatorRegistry.sol: L114-118

---

---

Use ++i instead of i++ to increase count in a for loop

Since use of i++ (or equivalent counter) costs more gas, it is better to use ++i

ERC20PermitPermissionedMint.sol: L84-89

        for (uint i = 0; i < minters_array.length; i++){ 
            if (minters_array[i] == minter_address) {
                minters_array[i] = address(0); // This will leave a null in the array and keep the indices the same
                break;
            }
        }

Suggestion:

        uint256 totalMintersLength = minters_array.length; 
        for (uint i = 0; i < totalMintersLength; ++i){ 
            if (minters_array[i] == minter_address) {
                minters_array[i] = address(0); // This will leave a null in the array and keep the indices the same
                break;
            }
        }

Note that I have included the previous correction (avoiding the array length look up during every for loop iteration) in the suggestion

---

---

Avoid use of default "checked" behavior in a for loop

Underflow/overflow checks are made every time ++i (or equivalent counter) is called. Such checks are unnecessary since i is already limited. Therefore, use `unchecked {++i} instead, as shown below:

ERC20PermitPermissionedMint.sol: L84-89

        for (uint i = 0; i < minters_array.length; i++){ 
            if (minters_array[i] == minter_address) {
                minters_array[i] = address(0); // This will leave a null in the array and keep the indices the same
                break;
            }
        }

Suggestion:

        uint256 totalMintersLength = minters_array.length; 
        for (uint i = 0; i < totalMintersLength;){ 
            if (minters_array[i] == minter_address) {
                minters_array[i] = address(0); // This will leave a null in the array and keep the indices the same
                break;

                unchecked {
                  ++i;
              }
            }
        }

Similarly for the following four for loops:

frxETHMinter.sol: L129-154

OperatorRegistry.sol: L63-65

OperatorRegistry.sol: L84-86

OperatorRegistry.sol: L114-117

---

---