Ethena Labs - lsaudit's results

[QA-01] Lack of checking for address(0) when setting new minter

File: USDe.sol

  function setMinter(address newMinter) external onlyOwner {
    emit MinterUpdated(newMinter, minter);
    minter = newMinter;
  }

Function setMinter() does not check if minter is address(0).

[QA-02] Lack of checking for address(0) when creating USDeSilo

File: USDeSilo.sol

  constructor(address stakingVault, address usde) {
    STAKING_VAULT = stakingVault;
    USDE = IERC20(usde);
  }

constructor() does not verify if stakingVault and usde are not address(0).

[QA-03] Protocol does not allow to add additional users with DEFAULT_ADMIN_ROLE role

This issue is related to centralization risk (it should be also noted in the Analysis Report). Protocol does not allow to create additional admins.

File: SingleAdminAccessControl.sol

  function grantRole(bytes32 role, address account) public override onlyRole(DEFAULT_ADMIN_ROLE) notAdmin(role) {
    _grantRole(role, account);
  }

It's not possible to call grantRole() to set another admin, since grantRole() function implements notAdmin(role) modifier. This implies, that we cannot grant anyone with DEFAULT_ADMIN_ROLE.

[QA-04] addToBlacklist() should be better defined

In the current implementation - function addToBlacklist() set's target role to be either FULL_RESTRICTED_STAKER_ROLE or SOFT_RESTRICTED_STAKER_ROLE. Those roles are defined in the README.md:

Due to legal requirements, there's a SOFT_RESTRICTED_STAKER_ROLE and FULL_RESTRICTED_STAKER_ROLE. The former is for addresses based in countries we are not allowed to provide yield to, for example USA. 
Addresses under this category will be soft restricted. They cannot deposit USDe to get stUSDe or withdraw stUSDe for USDe. However they can participate in earning yield by buying and selling stUSDe on the open market.

FULL_RESTRCITED_STAKER_ROLE is for sanction/stolen funds, or if we get a request from law enforcement to freeze funds. Addresses fully restricted cannot move their funds, and only Ethena can unfreeze the address. 

Another function - removeFromBlacklist() - allows to revoke those, previously set roles.

By definition - addToBlacklist() and removeFromBlacklist() can be called by Blacklist Manager (modifier: onlyRole(BLACKLIST_MANAGER_ROLE)).

There is some inaccuracy in definition and description of these functions. According to the description - when addToBlacklist() is called on user - he should either be soft or full restricted. However, the current scenario does not take into consideration what will happen, when that user is one of the Blacklist Manager.

If Blacklist Manager was blacklisted by FULL_RESTRICTED_STAKER_ROLE role - he - by the definition of addToBlacklist(), should not be able to move his funds. However, since he's still Blacklist Manager, he can remove himself from the blacklist by calling removeFromBlacklist() on his/her own address. That way he/she won't be on Blacklist anymore.

There are multiple of ways to solve this issue. Whenever blacklisting active Blacklist Manager - either revoke his/her BLACKLIST_MANAGER_ROLE role - or, state in the documentation, that blacklisting active Blacklist Manager should be preceded by revoking that role.

Another idea would be adding additional check in removeFromBlacklist(): require(msg.sender != target, "Cannot remove themself"), to make sure, that user cannot remove himself/herself from the Blacklist.

[QA-5] Incorrect revert in verifyOrder() in EthenaMinting.sol

File: EthenaMinting.sol

    if (order.beneficiary == address(0)) revert InvalidAmount();

When order.beneficiary == address(0), function verifyOrder() reverts with InvalidAmount(). This revert is reserved for incorrect amounts. It should be used either InvalidAddress() or InvalidZeroAddress() instead.

[QA-6] User who's being added to the blacklist can front-run this operation

The main purpose of addToBlacklist() call is to add the user to the blacklist. When user is blacklisted (with FULL_RESTRICTED_STAKED_ROLE) - he basically cannot do anything. Address with this role can't burn his stUSDe tokens to unstake his USDe tokens, neither to transfer stUSDe tokens. His balance can be manipulated by the admin of StakedUSDe. However, when users notices in the mem-pool that addToBlacklist() is called - he can front-run it.

1. BLACKLIST_MANAGER_ROLE calls addToBlacklist(0xAAA, true)
2. User 0xAAA sees that transaction in the mem-pool
3. User 0xAAA front-runs that call and transfers stUSDe tokens to different address
4. Now addToBlacklist(0xAAA, true) is being called, 0xAAA is FULL_RESTRICTED_STAKER_ROLE, but he/she doesn't care, because he/she had already transferred their stUSDe tokens to different address

Ability to front-run addToBlacklist() allows to evade FULL_RESTRICTED_STAKER_ROLE.

[QA-7] SOFT_RESTRICTED_STAKER_ROLE can by bypassed

According to documentation - this might be a design choice - however, since this choice influences the security of the protocol - I think it should be contained in the QA Report.

Blacklisted address (with SOFT_RESTRICTED_STAKER_ROLE) cannot deposit funds. However, nothing stops that address for transferring their funds to a different (non-blacklisted) address and deposit from that new address. This makes this role easily to be bypassed.

According to docs, the purpose of this role seems to be just for some legal requirements/regulations:

Due to legal requirements, there's a SOFT_RESTRICTED_STAKER_ROLE and FULL_RESTRICTED_STAKER_ROLE. The former is for addresses based in countries we are not allowed to provide yield to, for example USA. Addresses under this category will be soft restricted.

However, I think that it might be a good idea, to make this role a little more restrictive.

[QA-8] Before adding/removing role, make sure that user already has that role

Whenever we _grantRole via addToBlacklist() function - there's no check if user already has that role. This implies, that it's possible to assign the same role to the user who already has that role.

Whenever we _revokeRole via removeFromBlacklist() function - there's no check if user really has that role. This implies, that it's possible to revoke a role from user who hasn't got that role (in that case, nothing will change, if user doesn't have a role and removeFromBlacklist() is being called on him - he still won't have that role).

File: StakedUSDe.sol

 function addToBlacklist(address target, bool isFullBlacklisting)
    external
    onlyRole(BLACKLIST_MANAGER_ROLE)
    notOwner(target)
  {
    bytes32 role = isFullBlacklisting ? FULL_RESTRICTED_STAKER_ROLE : SOFT_RESTRICTED_STAKER_ROLE;
    _grantRole(role, target);
  }

 (...)

  function removeFromBlacklist(address target, bool isFullBlacklisting)
    external
    onlyRole(BLACKLIST_MANAGER_ROLE)
    notOwner(target)
  {
    bytes32 role = isFullBlacklisting ? FULL_RESTRICTED_STAKER_ROLE : SOFT_RESTRICTED_STAKER_ROLE;
    _revokeRole(role, target);
  }

Whenever we grant role to target (by calling addToBlacklist()), we should check if target already hasn't that role. Whenever we revoke role from target (by calling removeFromBlacklist()), we should check if target already has that role.

[QA-9] Lack of event emission when adding to/removing from the blacklist

In file StakedUSDe.sol, functions addToBlacklist() and removeFromBlacklist() do not emit any events. Consider emitting events whenever user is being blacklisted/removed from the blacklist

[QA-10] Documentation and codebase defining DEFAULT_ADMIN_ROLE can be misleading

Documentation states, that:

The DEFAULT_ADMIN_ROLE, also our ethena multisig, is required to re-enable minting/redeeming. 
DEFAULT_ADMIN_ROLE also has the power to add/remove GATEKEEPERS,MINTER and REDEEMER.

which implies, that there's only one DEFAULT_ADMIN_ROLE across the whole protocol - and it's Ethena multisig.

The code base states, that we cannot grant additional DEFAULT_ADMIN_ROLE:

File: SingleAdminAccessControl.sol

/// @notice admin role cannot be granted externally
  /// @param role bytes32
  /// @param account address
  function grantRole(bytes32 role, address account) public override onlyRole(DEFAULT_ADMIN_ROLE) notAdmin(role) {
    _grantRole(role, account);
  }

which also implies, that there can only be one DEFAULT_ADMIN_ROLE.

However, when we look at EthenaMinting.sol:

File: EthenaMinting.sol

constructor(
    IUSDe _usde,
    address[] memory _assets,
    address[] memory _custodians,
    address _admin,
    uint256 _maxMintPerBlock,
    uint256 _maxRedeemPerBlock
  ) {
    if (address(_usde) == address(0)) revert InvalidUSDeAddress();
    if (_assets.length == 0) revert NoAssetsProvided();
    if (_admin == address(0)) revert InvalidZeroAddress();
    usde = _usde;

    _grantRole(DEFAULT_ADMIN_ROLE, msg.sender);

    for (uint256 i = 0; i < _assets.length; i++) {
      addSupportedAsset(_assets[i]);
    }

    for (uint256 j = 0; j < _custodians.length; j++) {
      addCustodianAddress(_custodians[j]);
    }

    // Set the max mint/redeem limits per block
    _setMaxMintPerBlock(_maxMintPerBlock);
    _setMaxRedeemPerBlock(_maxRedeemPerBlock);

    if (msg.sender != _admin) {
      _grantRole(DEFAULT_ADMIN_ROLE, _admin);
    }

we can spot, that actually, there could be two users with DEFAULT_ADMIN_ROLE.

The first one - _grantRole(DEFAULT_ADMIN_ROLE, msg.sender); - msg.sender who deployed the contract The second one - _grantRole(DEFAULT_ADMIN_ROLE, _admin); - _admin passed in _admin parameter

Please make sure to standardize the documentation - to make it straightforwardly clear that there could be more than one DEFAULT_ADMIN_ROLE in the whole protocol. The first DEFAULT_ADMIN_ROLE - msg.sender who called constructor(), the 2nd DEFAULT_ADMIN_ROLE - _admin address from the constructor().

[N-01] Typo in documentation

File: README.md

The primary functions used in this contract is `mint()` and `redeen()`. Users who call this contract are all within Ethena. 
When outside users wishes to mint or redeem, they perform an EIP712 signature based on an offchain price we provided to them. 
They sign the order and sends it back to Ethena's backend, where we run a series of checks and are the ones who take their signed order and put them on chain.

By design, Ethena will be the only ones calling `mint()`,`redeen()` and other functions in this contract.

Function redeen() should be renamed to redeem(). There's no redeen() function in the code base.

[N-02] SingleAdminAccessControl.sol is not fully compliant with ERC-5313

File: SingleAdminAccessControl.sol

  /**
   * @dev See {IERC5313-owner}.
   */
  function owner() public view virtual returns (address) {
    return _currentDefaultAdmin;
  }

According to code-section - owner() is required for ERC-5313. However, when we look at ERC-5313, it defines owner() as external - not public:

source: https://eips.ethereum.org/EIPS/eip-5313

Every contract compliant with this EIP MUST implement the EIP5313 interface.

// SPDX-License-Identifier: CC0-1.0
pragma solidity ^0.8.15;

/// @title EIP-5313 Light Contract Ownership Standard
interface EIP5313 {
    /// @notice Get the address of the owner    
    /// @return The address of the owner
    function owner() view external returns(address);
}

external (not public) is also in the Open Zeppelin's interface (which SingleAdminAccessControl.sol imports):

interface IERC5313 {
    /**
     * @dev Gets the address of the owner.
     */
    function owner() external view returns (address);
}

Recommendation: change public to external.

[N-03] Stick with one way of using curly-brackets for single-instruction if conditions

File: StackedUSDeV2

if (shares > maxRedeem(owner)) revert ExcessiveRedeemAmount();

File: StackedUSDeV2

 if (duration > MAX_COOLDOWN_DURATION) {
      revert InvalidCooldown();
    }

When if condition wants to execute a single instruction, that instruction does not need to be inside {}. E.g., those two syntax are valid:

if (CONDITION) INSTRUCTION;

if (CONDITION) {
  INSTRUCTION;
}

It's a good coding practice to stick to one way of if-syntax. In the above scenario, in StakedUSDeV2.sol - the first syntax is being used, however, line 127-129 use the second one.

[N-04] Don't use short forms/contractions when naming events

File: USDe.sol

revert CantRenounceOwnership();

Renaming event from CantRenounceOwnership() to CannotRenounceOwnership() will be much more readable.

File: StakedUSDe.sol

 if (target == owner()) revert CantBlacklistOwner();

Renaming event from CantBlacklistOwner() to CannotBlacklistOwner() will be much more readable.

[N-05] Documentation does not properly describe available roles

File: README.md

### Trusted Roles

- `USDe` minter - can mint any amount of `USDe` tokens to any address. Expected to be the `EthenaMinting` contract
- `USDe` owner - can set token `minter` and transfer ownership to another address
- `USDe` token holder - can not just transfer tokens but burn them and sign permits for others to spend their balance
- `StakedUSDe` admin - can rescue tokens from the contract and also to redistribute a fully restricted staker's `stUSDe` balance, as well as give roles to other addresses (for example the `FULL_RESTRICTED_STAKER_ROLE` role)

The Trusted Roles paragraph lists all available roles in the protocol. One of the mentioned role is:

`USDe` token holder - can not just transfer tokens but burn them and sign permits for others to spend their balance

However, token holder cannot be classified as "Trusted Role", since it can be anyone. The paragraph name suggests that "USDe token holder" is a Trusted Role. Consider changing the name of the paragraph to "Available Roles", since it lists not only Trusted Roles, but every roles implemented by the protocol.

[N-06] Protocol supports limited number of nonces

There's some deviation from how nonces are being stored and how nonces are being represented.

File: EthenaMinting

 mapping(address => mapping(uint256 => uint256)) private _orderBitmaps;

_orderBitmaps is a uint256 => uint256 mapping, in verifyNonce().

File: EthenaMinting.sol

uint256 invalidatorSlot = uint64(nonce) >> 8;

The max value of invalidatorSlot is (2**64 - 1 ) >> 8 = 72057594037927935.

There are much more available invalidator slots in _orderBitmaps, than the number of invalidator slots which can be derived from nonce. This may lead to some collision, e.g.: (1 and 18446744073709551617)

[G-01] Checking for revert in unstake() in StakedUSDeV2.sol can be moved up

File: StakedUSDeV2.sol

function unstake(address receiver) external {
    UserCooldown storage userCooldown = cooldowns[msg.sender];
    uint256 assets = userCooldown.underlyingAmount;

    if (block.timestamp >= userCooldown.cooldownEnd) {
      userCooldown.cooldownEnd = 0;
      userCooldown.underlyingAmount = 0;

      silo.withdraw(receiver, assets);
    } else {
      revert InvalidCooldown();
    }
  }

There's no need to get uint256 assets = userCooldown.underlyingAmount; before checking if block.timestamp >= userCooldown.cooldownEnd.

Please consider changing above code-snippet to:

function unstake(address receiver) external {
    UserCooldown storage userCooldown = cooldowns[msg.sender];
   
    if (block.timestamp >= userCooldown.cooldownEnd) {
      uint256 assets = userCooldown.underlyingAmount;
      userCooldown.cooldownEnd = 0;
      userCooldown.underlyingAmount = 0;

      silo.withdraw(receiver, assets);
    } else {
      revert InvalidCooldown();
    }
  }

[G-02] Check if amount is non-zero before performing transfer()

File: USDeSilo.sol

function withdraw(address to, uint256 amount) external onlyStakingVault {
    USDE.transfer(to, amount);
  }

When amount == 0, then calling transfer() is a waste of gas - thus literally nothing will be transfered. It's a good practice to check if amount > 0 before performing transfer().

[G-03] renounceRole() can be optimized

File: SingleAdminAccessControl.sol

  function renounceRole(bytes32 role, address account) public virtual override notAdmin(role) {
    super.renounceRole(role, account);
  }

The current implementation calls OpenZeppelins renounceRole(), which checks if caller is _msgSender():

    function renounceRole(bytes32 role, address callerConfirmation) public virtual {
        if (callerConfirmation != _msgSender()) {
            revert AccessControlBadConfirmation();
        }

        _revokeRole(role, callerConfirmation);
    }

In the current context, this check is not needed - we can call _revokeRole directly on msg.sender:

  function renounceRole(bytes32 role, address account) public virtual override notAdmin(role) {
     _revokeRole(role, msg.sender);
  }

OpenZeppelin's implementation, implements additional safeguard - so that renounceRole() won't be called by accident. We need to provide our own address (msg.sender) as callerConfirmation and then call this function. If provided callerConfirmation matches the caller (msg.sender) - role will be renounce. However, if we want to save some gas - we can remove that safeguard. In our proposed fix - anyone who calls renounceRole(), will renounce role for himself/herself.

[G-04] uint104(block.timestamp) + cooldownDuration can be unchecked

Since cooldownDuration cannot exceed MAX_COOLDOWN_DURATION which is defined as 90 days and block.timestamp will be greater than type(uint104).max after 22 September 2612 - it's reasonable to assume, that uint104(block.timestamp) + cooldownDuration won't overflow and can be unchecked.

MAX_COOLDOWN_DURATION = 90 days = 7776000 type(uint104).max = 20282409603651670423947251286015

timestamp(20282409603651670423947251286015 - 7776000) = 22 September 2612 02:40:03. This implies, that this addition won't overflow till 22 September 2612 - so it's reasonable to make it unchecked.

Below lines can be unchecked:

File: StakedUSDeV2.sol

    cooldowns[owner].cooldownEnd = uint104(block.timestamp) + cooldownDuration;

File: StakedUSDeV2.sol

    cooldowns[owner].cooldownEnd = uint104(block.timestamp) + cooldownDuration;

[G-05] Change the order of emitting event - to avoid declaring additional variable in StakedUSDeV2.sol

File: StakedUSDeV2.sol

uint24 previousDuration = cooldownDuration;
cooldownDuration = duration;
emit CooldownDurationUpdated(previousDuration, cooldownDuration);

You can emit CooldownDurationUpdated event before updating cooldownDuration. That way, there will be no need to declare previousDuration:

emit CooldownDurationUpdated(cooldownDuration, duration);
cooldownDuration = duration;

[G-06] Function transferInRewards() from StakedUSDe.sol can be optimized to avoid calling getUnvestedAmount() twice.

Calling the same function is a waste of gas. It's much better idea to call it once and save the result to local variable. Since we've already declared newVestingAmount variable - we can utilize it to store getUnvestedAmount() there:

function transferInRewards(uint256 amount) external nonReentrant onlyRole(REWARDER_ROLE) notZero(amount) {
    
    uint256 newVestingAmount = getUnvestedAmount();
    if (newVestingAmount > 0) revert StillVesting();
    newVestingAmount = newVestingAmount + amount;

    vestingAmount = newVestingAmount;
    lastDistributionTimestamp = block.timestamp;
    // transfer assets from rewarder to this contract
    IERC20(asset()).safeTransferFrom(msg.sender, address(this), amount);

    emit RewardsReceived(amount, newVestingAmount);
  }

[G-07] Unnecessary variable declaration in StakedUSDe.sol

In both functions: addToBlacklist() and removeFromBlacklist(), there's no need to declare variable bytes32 role. This variable is being used only once, which means it doesn't need to be declared at all.

Code can be changed to:

 function addToBlacklist(address target, bool isFullBlacklisting)
    external
    onlyRole(BLACKLIST_MANAGER_ROLE)
    notOwner(target)
  {
   
    _grantRole(isFullBlacklisting ? FULL_RESTRICTED_STAKER_ROLE : SOFT_RESTRICTED_STAKER_ROLE, target);
  }

 function removeFromBlacklist(address target, bool isFullBlacklisting)
    external
    onlyRole(BLACKLIST_MANAGER_ROLE)
    notOwner(target)
  {
    _revokeRole(isFullBlacklisting ? FULL_RESTRICTED_STAKER_ROLE : SOFT_RESTRICTED_STAKER_ROLE, target);
  }

[G-08] Redundant address(0) check in redistributeLockedAmount() in StakedUSDe.sol

File: StakedUSDe.sol

if (to != address(0)) _mint(to, amountToDistribute);

OpenZeppelin's _mint() already checks for address(0), thus checking it for the second time in StakedUSDe.sol is not needed

[G-09] Move condition which uses less gas on the left of AND operator in StakedUSDe.sol

File: StakedUSDe.sol

if (hasRole(FULL_RESTRICTED_STAKER_ROLE, from) && to != address(0)) {

A && B condition is true only, when both A and B conditions evaluate to true. This means, that when A is false, then Solidity does not evaluate B (since no matter what B is, A & B is false). This behavior implies, that A will be evaluated more times than B (B will be evaluated only when A = true). In that case, it's much more efficient to make sure that A uses less gas than B.

to != address(0) costs less gas than hasRole(FULL_RESTRICTED_STAKER_ROLE, from), thus order of conditions should be changed:

if (to != address(0) && hasRole(FULL_RESTRICTED_STAKER_ROLE, from)) {

[G-10] transferToCustody() in EthenaMinting.sol does not verify if amount > 0

File: EhtenaMinting.sol

  function transferToCustody(address wallet, address asset, uint256 amount) external nonReentrant onlyRole(MINTER_ROLE) {
    if (wallet == address(0) || !_custodianAddresses.contains(wallet)) revert InvalidAddress();
    if (asset == NATIVE_TOKEN) {
      (bool success,) = wallet.call{value: amount}("");
      if (!success) revert TransferFailed();
    } else {
      IERC20(asset).safeTransfer(wallet, amount);
    }
    emit CustodyTransfer(wallet, asset, amount);
  }

It's a waste of gas to perform wallet.call{value: 0}("") or IERC20(asset).safeTransfer(wallet, 0) operations. If amount is 0, nothing will be transfered. Perform additional check if (amount !=0) before calling either wallet.call{value: 0}("") or IERC20(asset).safeTransfer(wallet, 0).

[G-11] _assets.length is calculated twice in EthenaMinting.sol

File: EhtenaMinting.sol

 if (_assets.length == 0) revert NoAssetsProvided();
    if (_admin == address(0)) revert InvalidZeroAddress();
    usde = _usde;

    _grantRole(DEFAULT_ADMIN_ROLE, msg.sender);

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

Calculating the length of array may take a lot of gas. It's always a good idea to calculate it once and save the result to local variable. _assets.length is calculated firstly at line 120, and then - 2ndly - at line 126. Please notice, that the bot-race reported only that _assets.length is being used inside a loop. While it's true, it's worth to notice, that it's used also at line 120. This means, that _assets.length can be cached before line 120.

[G-12] In verifyRouter() from EthenaMinting.sol - variable totalRatio can be declared later

Before totalRatio is being used, function performs two additional checks: if (route.addresses.length != route.ratios.length) and if (route.addresses.length == 0). If any of them evalutes to true - functions will return with false without a need of using totalRatio. This means, that uint256 totalRatio = 0; can be declared later, just before the loop at line 363:

uint256 totalRatio = 0;
for (uint256 i = 0; i < route.addresses.length; ++i) {

[G-13] Change the order of emitting event - to avoid declaring additional variable in EtheneMinting.sol

File: EthenaMinting.sol

  function _setMaxMintPerBlock(uint256 _maxMintPerBlock) internal {
    uint256 oldMaxMintPerBlock = maxMintPerBlock;
    maxMintPerBlock = _maxMintPerBlock;
    emit MaxMintPerBlockChanged(oldMaxMintPerBlock, maxMintPerBlock);
  }

  /// @notice Sets the max redeemPerBlock limit
  function _setMaxRedeemPerBlock(uint256 _maxRedeemPerBlock) internal {
    uint256 oldMaxRedeemPerBlock = maxRedeemPerBlock;
    maxRedeemPerBlock = _maxRedeemPerBlock;
    emit MaxRedeemPerBlockChanged(oldMaxRedeemPerBlock, maxRedeemPerBlock);
  }

Variables oldMaxMintPerBlock and oldMaxRedeemPerBlock are used only once, which means, that they do not need to be declared at all. Getting rid of these variables requires to emit events a little earlier, like this:

  function _setMaxMintPerBlock(uint256 _maxMintPerBlock) internal {
    emit MaxMintPerBlockChanged(maxMintPerBlock, _maxMintPerBlock);
    maxMintPerBlock = _maxMintPerBlock;
  }

  /// @notice Sets the max redeemPerBlock limit
  function _setMaxRedeemPerBlock(uint256 _maxRedeemPerBlock) internal {
    emit MaxRedeemPerBlockChanged(maxRedeemPerBlock, _maxRedeemPerBlock);
    maxRedeemPerBlock = _maxRedeemPerBlock; 
  }

[G-14] verifyOrder() from EthenaMinting.sol can be optimized by performing signature verification later

Calculating hashOrder() and ECDSA.recover() is the most gas-costly operation in the whole verifyOrder() function. In the current implementation - we firstly check the ECDSA.recover(), and then, we perform additional validations (like if order.expiry hasn't expired, if order.usde_ammount is not zero, etc.). Basically, if any of the Order parameters will be invalid - function will revert. This implies, that it will be more effective to firstly validate those parameters - and then - when they are valid - verify ESDSA signature. In that case - if any of Order fields won't be valid - we won't spend additional gas on the signature verification. Here's the proposed fix:

  function verifyOrder(Order calldata order, Signature calldata signature) public view override returns (bool, bytes32) {
    
    if (order.beneficiary == address(0)) revert InvalidAmount();
    if (order.collateral_amount == 0) revert InvalidAmount();
    if (order.usde_amount == 0) revert InvalidAmount();
    if (block.timestamp > order.expiry) revert SignatureExpired();

    // @audit: we are sure that Order is valid, now we can verify if signature is correct. If Order was invalid, we had reverted before using gas on hashOrder() and ECDSA.recover()
   
    bytes32 taker_order_hash = hashOrder(order);
    address signer = ECDSA.recover(taker_order_hash, signature.signature_bytes);
    if (!(signer == order.benefactor || delegatedSigner[signer][order.benefactor])) revert InvalidSignature();
    return (true, taker_order_hash);
  }

[G-15] Unnecessary variable declaration in _deduplicateOrder(), verifyNonce() in EthenaMinting.sol

Variable invalidatorStorage is used only once, which means, it does not need to be declared at all. Below code:

File: EthenaMinting.sol

 function _deduplicateOrder(address sender, uint256 nonce) private returns (bool) {
    (bool valid, uint256 invalidatorSlot, uint256 invalidator, uint256 invalidatorBit) = verifyNonce(sender, nonce);
    mapping(uint256 => uint256) storage invalidatorStorage = _orderBitmaps[sender];
    invalidatorStorage[invalidatorSlot] = invalidator | invalidatorBit;
    return valid;
  }

can be changed to:

 function _deduplicateOrder(address sender, uint256 nonce) private returns (bool) {
    (bool valid, uint256 invalidatorSlot, uint256 invalidator, uint256 invalidatorBit) = verifyNonce(sender, nonce);
    _orderBitmaps[sender][invalidatorSlot] = invalidator | invalidatorBit;
    return valid;
  }

To prove that it will save gas, a quick test in Remix IDE has been performed:

 function _deduplicateOrderAAA(address sender, uint256 nonce) public returns (bool) {
    uint gas = gasleft();
    
    (bool valid, uint256 invalidatorSlot, uint256 invalidator, uint256 invalidatorBit) = verifyNonce(sender, nonce);
    mapping(uint256 => uint256) storage invalidatorStorage = _orderBitmaps[sender];
    invalidatorStorage[invalidatorSlot] = invalidator | invalidatorBit;
    console.log(gas - gasleft());
    return valid;
  }

   function _deduplicateOrderBBB(address sender, uint256 nonce) public returns (bool) {
    uint gas = gasleft();
    
    (bool valid, uint256 invalidatorSlot, uint256 invalidator, uint256 invalidatorBit) = verifyNonce(sender, nonce);
    _orderBitmaps[sender][invalidatorSlot] = invalidator | invalidatorBit;
    console.log(gas - gasleft());
    return valid;
  }

The results look as below:

_deduplicateOrderAAA(1, 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4) -> 22742 gas used
_deduplicateOrderAAA(2, 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4) -> 5642 gas used


_deduplicateOrderBBB(1, 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4) -> 22731 gas used
_deduplicateOrderBBB(2, 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4) -> 5631 gas used

This implies, that proposed fix will save some gas.

The same issue occurs in verifyNonce() Below code-base:

File: EthenaMinting.sol

  function verifyNonce(address sender, uint256 nonce) public view override returns (bool, uint256, uint256, uint256) {
    if (nonce == 0) revert InvalidNonce();
    uint256 invalidatorSlot = uint64(nonce) >> 8;
    uint256 invalidatorBit = 1 << uint8(nonce);
    mapping(uint256 => uint256) storage invalidatorStorage = _orderBitmaps[sender];
    uint256 invalidator = invalidatorStorage[invalidatorSlot];
    if (invalidator & invalidatorBit != 0) revert InvalidNonce();

    return (true, invalidatorSlot, invalidator, invalidatorBit);
  }

can be changed to:

 function verifyNonce(address sender, uint256 nonce) public view override returns (bool, uint256, uint256, uint256) {
    if (nonce == 0) revert InvalidNonce();
    uint256 invalidatorSlot = uint64(nonce) >> 8;
    uint256 invalidatorBit = 1 << uint8(nonce);
    uint256 invalidator = _orderBitmaps[sender][invalidatorSlot];
    if (invalidator & invalidatorBit != 0) revert InvalidNonce();

    return (true, invalidatorSlot, invalidator, invalidatorBit);
  }

[G-16] There's no need to check for route.addresses[i] == address(0) in verifyRoute() in EthenaMinting.sol

File: EthenaMinting.sol

 for (uint256 i = 0; i < route.addresses.length; ++i) {
      if (!_custodianAddresses.contains(route.addresses[i]) || route.addresses[i] == address(0) || route.ratios[i] == 0)

There are multiple of checks in this conditions. However, they can be reduced.

When we take a look at function addCustodianAddress(), it looks like this:

File: EthenaMinting.sol

  function addCustodianAddress(address custodian) public onlyRole(DEFAULT_ADMIN_ROLE) {
    if (custodian == address(0) || custodian == address(usde) || !_custodianAddresses.add(custodian)) {
      revert InvalidCustodianAddress();
    }
    emit CustodianAddressAdded(custodian);
  }

Function addCustodianAddress() cannot add custodian, when custodian is address(0). Whenever we try to call: addCustodianAddress(address(0)), function will revert with InvalidCustodianAddress(). This implies, that when route.addresses[i] == address(0), then !_custodianAddresses.contains(route.addresses[i]) will return true (it's impossible to add address(0) to supported custodians list:

1. It's impossbile to add address(0) to custodians list
2. _custodianAddresses cannot contain address(0), because it's impossible to add address(0)
3. _custodianAddresses.contains(address(0)) returns false, because we know that custodian list does not contain address(0), since it's impossible to add that address to custodian list
4. !_custodianAddresses.contains(address(0)) returns true (since ! is negation)

Since !_custodianAddresses.contains(route.addresses[i]) already told as that route.addresses[i] is not address(0), another check route.addresses[i] == address(0) is redundant. This implies, that we can simplify above condition, from: if (!_custodianAddresses.contains(route.addresses[i]) || route.addresses[i] == address(0) || route.ratios[i] == 0) to if (!_custodianAddresses.contains(route.addresses[i]) || route.ratios[i] == 0).

[G-17] route.addresses.length is calculated multiple of times in verifyRoute() in EthenaMinting.sol

File: EthenaMinting.sol

    if (route.addresses.length != route.ratios.length) {
      return false;
    }
    if (route.addresses.length == 0) {
      return false;
    }
    for (uint256 i = 0; i < route.addresses.length; ++i) {

Calculating the length of array may take a lot of gas. It's always a good idea to calculate it once and save the result to local variable. route.addresses.length is calculated firstly at line 357, and then - secondly - at line 360, then - for the 3rd time - at line 363 Please notice, that the bot-race reported only that route.addresses.length is being used inside a loop. While it's true, it's worth to notice, that it's used also at line 357 and at line 360. This means, that route.addresses.length can be cached:

uint256 routeLength = route.addresses.length;
if (routeLength != route.ratios.length) {
      return false;
    }
    if (routeLength == 0) {
      return false;
    }
    for (uint256 i = 0; i < routeLength; ++i) {

[G-18] _transferCollateral() calls safeTransferFrom() even when transferred amount is 0

File: EthenaMinting.sol

for (uint256 i = 0; i < addresses.length; ++i) {
      uint256 amountToTransfer = (amount * ratios[i]) / 10_000;
      token.safeTransferFrom(benefactor, addresses[i], amountToTransfer);
      totalTransferred += amountToTransfer;
    }

When amountToTransfer is 0, performing token.safeTransferFrom is not needed. It's a waste of gas to perform 0 transfer. We can check if amountToTransfer is greater than 0, before performing transfer:

for (uint256 i = 0; i < addresses.length; ++i) {
      uint256 amountToTransfer = (amount * ratios[i]) / 10_000;
      if (amountToTransfer != 0) {
          token.safeTransferFrom(benefactor, addresses[i], amountToTransfer);
          totalTransferred += amountToTransfer;
        }
    }

That way, when amountToTransfer is 0, we save gas, because we won't perform two redundant operations:

• token.safeTransferFrom(benefactor, addresses[i], 0) - transferring 0 does not affect token's balance, thus it's redundant
• totalTransferred += 0 - adding 0 does not change the value of totalTransferred

[G-19] Struct Order can be packed into fewer storage slots

Examining ORDER_TYPE constant allows us to establish how struct Order looks like and how many storage slots it uses:

File: EthenaMinting.sol

bytes32 private constant ORDER_TYPE = keccak256(
    "Order(uint8 order_type,uint256 expiry,uint256 nonce,address benefactor,address beneficiary,address collateral_asset,uint256 collateral_amount,uint256 usde_amount)"
  );

This can be confirmed by looking at interfaces/IEthenaMinting.sol - however, please notice, that this file is out of scope:

 struct Order {
    OrderType order_type;
    uint256 expiry;
    uint256 nonce;
    address benefactor;
    address beneficiary;
    address collateral_asset;
    uint256 collateral_amount;
    uint256 usde_amount;
  }

The current ordering of Order's fields uses 8 storage slots:

Order(uint8 order_type,uint256 expiry,uint256 nonce,address benefactor,address beneficiary,address collateral_asset,uint256 collateral_amount,uint256 usde_amount)
_____|SLOT 0__________|SLOT 1________|SLOT 2_______|SLOT 3____________|SLOT 4_____________|SLOT 5__________________|SLOT 6___________________|SLOT 7_____________|

However, we can change the Order to:

 struct Order {
    OrderType order_type;
    address benefactor;
    address beneficiary;
    uint256 expiry;
    uint256 nonce;
    address collateral_asset;
    uint256 collateral_amount;
    uint256 usde_amount;
  }

That way, we will use 7 slots:

Order(uint8 order_type,address benefactor,address beneficiary,uint256 expiry,uint256 nonce,address collateral_asset,uint256 collateral_amount,uint256 usde_amount)
_____|SLOT 0_____________________________|SLOT 1_____________|SLOT 2________|SLOT 3_______|SLOT 4__________________|SLOT 5___________________|SLOT 6_____________|

[G-20] Struct Order can be packed into fewer storage slots by changing the type of expiry and nonce

This issue is related to G-19 optimization. It turns out, that we can reduce the number of Order's storage slots even more.

Since expiry is always compared to block.timestamp:

File: EthenaMinting.sol

if (block.timestamp > order.expiry) revert SignatureExpired();

We can represent it as uint64 instead of uint256. The max value of uint64 is 2**64 - 1, which, after converting to UNIX timestamp is Jul 21 2554 23:34:33. expiry won't overflow till Jun 21 2554, when it will be represented as uint64.

Additionally, when we look at the nonce implementation:

File: EthenaMinting.sol

 uint256 invalidatorSlot = uint64(nonce) >> 8;

we see, it's already casted to uint64, thus it can be represented in uint64.

This leads us to the below, updated struct:

 struct Order {
    OrderType order_type;
    address benefactor;
    address beneficiary;
    uint64 expiry;
    uint64 nonce;
    address collateral_asset;
    uint256 collateral_amount;
    uint256 usde_amount;
  }

This implies, that we can reduce the number of storage slots to 5:

Order(uint8 order_type,address benefactor,address beneficiary,uint64 expiry,uint64 nonce,address collateral_asset,uint256 collateral_amount,uint256 usde_amount)
_____|SLOT 0_____________________________|SLOT 1___________________________|SLOT 2_______________________________|SLOT 3___________________|SLOT 4_____________|

[G-21] Subtraction in _transferCollateral() in EthenaMinting.sol can be unchecked

File: EthenaMinting.sol

token.safeTransferFrom(benefactor, addresses[addresses.length - 1], remainingBalance);

addresses.length - 1 won't overflow, thus it can be unchecked.

Please notice, that _transferCollateral() is called only in function mint(). Before that call, verifyRoute() is being called:

if (!verifyRoute(route, order.order_type)) revert InvalidRoute();

When we take a look at verifyRoute() implementation, we can see, that it requires that route.addresses.length is not zero. Since route.addresses is non-zero, we can be sure, that addresses.length - 1 won't underflow (addresses.length has to be >= 1). This implies, that this subtraction can be unchecked, since it won't overflow.