Ethena Labs - 0x11singh99's results

Low-Level and Non-Critical issues

Summary

Low Level List

Total 2 Low Level Issues

Non Critical List

Total 3 Non Critical Issues

Low-Level issues:

[L-01] Check Low Level Calls for address 0

beneficiary can be address 0 so must be checked for address 0 before transferring ether to it.

If 0 passed as beneficiary these ethers will be lost because no address(0) check is implemented before this ether(or blockchain native token on other chain) transferring. 1 Instance in 1 File

File: contracts/EthenaMinting.sol

404: (bool success,) = (beneficiary).call{value: amount}("");

404

[L-02] Check receiver address is not address(0) and amount is greater than zero before transferring or minting any tokens

Check to is non 0 because some tokens do not revert when transferred to 0 address if they created using solmate erc20 file but they revert if created using openzeppelin erc20 and no token is specified here so token can be any erc20 token.

2 Instances in 2 Files

File : contracts/StakedUSDe.sol

138: function rescueTokens(address token, uint256 amount, address to) external onlyRole(DEFAULT_ADMIN_ROLE) {
139:    if (address(token) == asset()) revert InvalidToken();
140:    IERC20(token).safeTransfer(to, amount);
141:  }

138-141

File : contracts/USDeSilo.sol

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

28-30

Non Critical Issues:

[NC-01] if (address(this).balance < amount) revert InvalidAmount(); this check should be applied before low level call for code clarity

1 Instance in 1 File

File: contracts/EthenaMinting.sol

function transferToCustody(...
  {...
     ...
    (bool success,) = wallet.call{value: amount}("");
  ...

250

[NC-02] Some Contracts are not following proper solidity style guide layout

/ Layout of Contract: According to solidity Docs // version // imports // errors // interfaces, libraries, contracts // Type declarations // State variables // Events // Modifiers // Functions

// Layout of Functions: // constructor // receive function (if exists) // fallback function (if exists) // external // public // internal // private // internal & private view & pure functions // external & public view & pure functions https://docs.soliditylang.org/en/latest/style-guide.html#order-of-layout

Note: These instance missed by bot report

3 Instances in 2 File

File : contracts/SingleAdminAccessControl.sol

  ///@audit declare view functions in last

65: function owner() public view virtual returns (address) {

65

File : contracts/StakedUSDe.sol

///@audit declare view functions in last
166: function totalAssets() public view override returns (uint256) {

///@audit declare view functions in last
173: function getUnvestedAmount() public view returns (uint256) {    

166, 173

[NC-03] Do not use inherited state variable/function name as function param name it will shadow the above and creates confusion

1 Instance in 1 File

File : contracts/StakedUSDeV2.sol

   ///@audit owner

42: constructor(IERC20 _asset, address initialRewarder, address owner) StakedUSDe(_asset, initialRewarder, owner) {

42

Gas Optimizations

[G-01] Remove unnecessary function call and stack variable creation

When getUnvestedAmount() returns 0 then it will only pass if condition otherwise revert because return value of getUnvestedAmount() is uint type . So adding 0 to amount is unnecessary and taking it into new variable is also not needed. So use just amount instead of newVestingAmount and it will have same working and can save gas of getUnvestedAmount() function call, stack variable creation and 1 add operation.

1 Instances in 1 File

File : contracts/StakedUSDe.sol

  function transferInRewards(uint256 amount) external nonReentrant onlyRole(REWARDER_ROLE) notZero(amount) {
    if (getUnvestedAmount() > 0) revert StillVesting();
    uint256 newVestingAmount = amount + getUnvestedAmount();
    //@audit if getUnvestedAmount()  return 0 then it will pass if check

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

    emit RewardsReceived(amount, newVestingAmount);
  }

89-99

File : contracts/StakedUSDe.sol

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

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

-    emit RewardsReceived(amount, newVestingAmount);
+    emit RewardsReceived(amount, amount);
  }

[G-02] Avoid emitting state variable unnecessarily.

Use parameter _maxMintPerBlock instead of state variable maxMintPerBlock because same value assigned in state variable in just above line of just event emit. It saves 1 SLOAD.

1 Instance in 1 File

File : contracts/EthenaMinting.sol

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

436-440

File : contracts/EthenaMinting.sol

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

File : contracts/EthenaMinting.sol

  function _setMaxRedeemPerBlock(uint256 _maxRedeemPerBlock) internal {
    uint256 oldMaxRedeemPerBlock = maxRedeemPerBlock;
    maxRedeemPerBlock = _maxRedeemPerBlock;
    emit MaxRedeemPerBlockChanged(oldMaxRedeemPerBlock, maxRedeemPerBlock);
  }

443-447

File : contracts/EthenaMinting.sol

  function _setMaxRedeemPerBlock(uint256 _maxRedeemPerBlock) internal {
     uint256 oldMaxRedeemPerBlock = maxRedeemPerBlock;
     maxRedeemPerBlock = _maxRedeemPerBlock;
-    emit MaxRedeemPerBlockChanged(oldMaxRedeemPerBlock, maxRedeemPerBlock);
+    emit MaxRedeemPerBlockChanged(oldMaxRedeemPerBlock, _maxRedeemPerBlock);
  }

File : contracts/StakedUSDeV2.sol

  function setCooldownDuration(uint24 duration) external onlyRole(DEFAULT_ADMIN_ROLE) {
    if (duration > MAX_COOLDOWN_DURATION) {
      revert InvalidCooldown();
    }

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

126-134

File : contracts/StakedUSDeV2.sol

  function setCooldownDuration(uint24 duration) external onlyRole(DEFAULT_ADMIN_ROLE) {
    if (duration > MAX_COOLDOWN_DURATION) {
      revert InvalidCooldown();
    }

    uint24 previousDuration = cooldownDuration;
    cooldownDuration = duration;
-    emit CooldownDurationUpdated(previousDuration, cooldownDuration);
+    emit CooldownDurationUpdated(previousDuration, duration);
  }

[G-03] State variables can be cached instead of re-reading them from storage

Caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read.

Note: These instances missed by bot-report

1 Instances in 1 File

Cache _currentDefaultAdmin.

File : contracts/SingleAdminAccessControl.sol

  function _grantRole(bytes32 role, address account) internal override {
    if (role == DEFAULT_ADMIN_ROLE) {
      emit AdminTransferred(_currentDefaultAdmin, account);
      _revokeRole(DEFAULT_ADMIN_ROLE, _currentDefaultAdmin);
      _currentDefaultAdmin = account;
      delete _pendingDefaultAdmin;
    }
    super._grantRole(role, account);
  }

72-80

File : contracts/SingleAdminAccessControl.sol

  function _grantRole(bytes32 role, address account) internal override {
    if (role == DEFAULT_ADMIN_ROLE) {
+     address current_DefaultAdmin = _currentDefaultAdmin;  
-     emit AdminTransferred(_currentDefaultAdmin, account);
-     _revokeRole(DEFAULT_ADMIN_ROLE, _currentDefaultAdmin);
+     emit AdminTransferred(current_DefaultAdmin, account);
+     _revokeRole(DEFAULT_ADMIN_ROLE, current_DefaultAdmin);
      _currentDefaultAdmin = account;
      delete _pendingDefaultAdmin;
    }
    super._grantRole(role, account);
  }

[G-04] Refactor the code to save extra function calls

It is one liner assignment inside constructor. And previous value is 0 always because first time assigning in constructor so no need to read previous value from state variable and emit them in constructor which is happening inside these functions. It can save 2 function calls and 2 SLOAD inside them. 2 Instances in 1 File

File : contracts/EthenaMinting.sol

    _setMaxMintPerBlock(_maxMintPerBlock);
    _setMaxRedeemPerBlock(_maxRedeemPerBlock);
  

135-136

File : contracts/EthenaMinting.sol

-    _setMaxMintPerBlock(_maxMintPerBlock);
-    _setMaxRedeemPerBlock(_maxRedeemPerBlock);
+    maxMintPerBlock = _maxMintPerBlock;
+    maxRedeemPerBlock = _maxRedeemPerBlock;
  

[G-05] Swap if() statements to execute less gas consuming before more gas consuming.

If 2 if checks have return/revert statements, it is good practice to write those if statements in order less to more gas consuming one. Here later one calculating only one length while it's previous one calculation 2 length so obviously last one is less gas consuming than it's before so we write it it's above.

1 Instances in 1 File

File : contracts/EthenaMinting.sol

    if (route.addresses.length != route.ratios.length) {
      return false;
    }
    if (route.addresses.length == 0) {
      return false;
    }

357-362

File : contracts/EthenaMinting.sol

- 357:   if (route.addresses.length != route.ratios.length) {
- 358:    return false;
- 359:  }
  360:  if (route.addresses.length == 0) {
  361:    return false;
  362:   }
+       if (route.addresses.length != route.ratios.length) {
+         return false;
+       }

[G-06] Check amount for 0 before transferring them.

Check amount for 0 when transferring them. If amount is zero it will have no effect and gas will be consumed. And on transferring 0 amount Openzeppelin ERC20 won't revert. It will be successfully executed without no amount transferred and gas will be wasted.

Note: These instances missed by bot-report

4 Instances in 3 Files

File : contracts/EthenaMinting.sol

   function _transferToBeneficiary(address beneficiary, address asset, uint256 amount) internal {
      if (asset == NATIVE_TOKEN) {
        if (address(this).balance < amount) revert InvalidAmount();
        (bool success,) = (beneficiary).call{value: amount}("");
        if (!success) revert TransferFailed();
      } else {
       if (!_supportedAssets.contains(asset)) revert UnsupportedAsset();
       IERC20(asset).safeTransfer(beneficiary, amount);//@audit check amount for 0
       }
    }

401-410

File : contracts/StakedUSDe.sol

  function rescueTokens(address token, uint256 amount, address to) external onlyRole(DEFAULT_ADMIN_ROLE) {
    if (address(token) == asset()) revert InvalidToken();
    IERC20(token).safeTransfer(to, amount);//@audit check amount for 0
  }

138-141

File : contracts/StakedUSDe.sol

  function redistributeLockedAmount(address from, address to) external onlyRole(DEFAULT_ADMIN_ROLE) {
    if (hasRole(FULL_RESTRICTED_STAKER_ROLE, from) && !hasRole(FULL_RESTRICTED_STAKER_ROLE, to)) {
      uint256 amountToDistribute = balanceOf(from);
      _burn(from, amountToDistribute);//@audit check amountToDistribute for 0
      // to address of address(0) enables burning
      if (to != address(0)) _mint(to, amountToDistribute);

      emit LockedAmountRedistributed(from, to, amountToDistribute);
    } else {
      revert OperationNotAllowed();
    }
  }

148-159

File : contracts/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);//@audit check receiver for address(0) and assets for 0
    } else {
      revert InvalidCooldown();
    }
  }

78-90

[G-07] Refactor if statements to revert early

If 2 if checks have return/revert statements, it is good practice to write those if statements in order less to more gas consuming one.

Note: These instances missed by bot-report

3 Instance in 1 File

File : contracts/EthenaMinting.sol

  function verifyOrder(Order calldata order, Signature calldata signature) public view override returns (bool, bytes32) {
    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();
    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();
    return (true, taker_order_hash);
  }

339-348

File : contracts/EthenaMinting.sol

  function verifyOrder(Order calldata order, Signature calldata signature) public view override returns (bool, bytes32) {
    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();
     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();
+    if (!(signer == order.benefactor || delegatedSigner[signer][order.benefactor])) revert InvalidSignature();    
     return (true, taker_order_hash);
  }

File : contracts/EthenaMinting.sol

364:  if (!_custodianAddresses.contains(route.addresses[i]) || route.addresses[i] == address(0) || route.ratios[i] == 0)

364

File : contracts/EthenaMinting.sol

-   if (!_custodianAddresses.contains(route.addresses[i]) || route.addresses[i] == address(0) || route.ratios[i] == 0)
+   if (route.addresses[i] == address(0) || route.ratios[i] == 0 || !_custodianAddresses.contains(route.addresses[i]))

File : contracts/EthenaMinting.sol

 if (!_supportedAssets.contains(asset) || asset == NATIVE_TOKEN) revert UnsupportedAsset();

421

File : contracts/EthenaMinting.sol

-   if (!_supportedAssets.contains(asset) || asset == NATIVE_TOKEN) revert UnsupportedAsset();
+   if (asset == NATIVE_TOKEN || !_supportedAssets.contains(asset)) revert UnsupportedAsset();

[G-08] Make state variables constant instead of storage variables if it's value never changed

Declare state variable MAX_COOLDOWN_DURATION as constant variable if it's value never changes. Here it will be packed with silo which takes 20 bytes so it will not save stirage SLOT heer but it can definitely save some SLOAD (Gcoldsload(2100 GAS) Or Gwarmaccess(100 GAS)) whenever MAX_COOLDOWN_DURATION is accessed. It is definitely worth it to make it constant.

1 Instance in 1 File

File : contracts/StakedUSDeV2.sol

-    uint24 public MAX_COOLDOWN_DURATION = 90 days;
+    uint24 public constant MAX_COOLDOWN_DURATION = 90 days;

22

[G-09] Can Make The Variable Outside The Loop To Save Gas

When you declare a variable inside a loop, Solidity creates a new instance of the variable for each iteration of the loop. This can lead to unnecessary gas costs, especially if the loop is executed frequently or iterates over a large number of elements. By declaring the variable outside the loop, you can avoid the creation of multiple instances of the variable and reduce the gas cost of your contract

1 Instance in 1 File

File : contracts/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;
    }

424-428

File : contracts/EthenaMinting.sol

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