QA Report

[code423n4]

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 (

---

---