Ethena Labs - 0xmystery's results

Lines of code

https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDe.sol#L217-L238 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/ERC4626.sol#L267-L269 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol#L297-L315 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol#L284-L295

Vulnerability details

Impact

Users assigned the FULL_RESTRICTED_STAKER_ROLE, intended to be restricted from certain actions, can bypass these restrictions. They can use the _spendAllowance function (as inherited from ERC20.sol via ERC4626.sol) to grant allowances, potentially leading to unintended asset transfers, undermining the security measures put in place, and eroding trust in the system.

Evidently, StakedUSDeV2.cooldownAssets and StakedUSDeV2.cooldownShares that has been commented below:

https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDeV2.sol#L94 https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDeV2.sol#L110

  /// @param owner address to redeem and start cooldown, owner must allowed caller to perform this action

facilitates an owner of stUSDe to allow a caller to invoke StakedUSDe._withdraw as _msgSender():

https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDeV2.sol#L103 https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDeV2.sol#L119

    _withdraw(_msgSender(), address(silo), owner, assets, shares);

Proof of Concept

Here's the scenario:

1. The protocol assign FULL_RESTRICTED_STAKER_ROLE to a user after they've staked their assets.
2. Despite the restriction, the user calls _approve() to grant a full allowance to another address.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol#L284-L295

    function _approve(address owner, address spender, uint256 value, bool emitEvent) internal virtual {
        if (owner == address(0)) {
            revert ERC20InvalidApprover(address(0));
        }
        if (spender == address(0)) {
            revert ERC20InvalidSpender(address(0));
        }
        _allowances[owner][spender] = value;
        if (emitEvent) {
            emit Approval(owner, spender, value);
        }
    }

3. The receiving address (caller) can now act on behalf of the restricted user, essentially bypassing the intended restrictions (because the if block does not check on the owner, just the caller and the receiver) when super._withdraw() is invoked from StakedUSDe._withdraw().

https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDe.sol#L225-L238

  function _withdraw(address caller, address receiver, address _owner, uint256 assets, uint256 shares)
    internal
    override
    nonReentrant
    notZero(assets)
    notZero(shares)
  {
    if (hasRole(FULL_RESTRICTED_STAKER_ROLE, caller) || hasRole(FULL_RESTRICTED_STAKER_ROLE, receiver)) {
      revert OperationNotAllowed();
    }

    super._withdraw(caller, receiver, _owner, assets, shares);
    _checkMinShares();
  }

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/ERC4626.sol#L260-L281

    function _withdraw(
        address caller,
        address receiver,
        address owner,
        uint256 assets,
        uint256 shares
    ) internal virtual {
        if (caller != owner) {
            _spendAllowance(owner, caller, shares);
        }

        _burn(owner, shares);
        SafeERC20.safeTransfer(_asset, receiver, assets);

        emit Withdraw(caller, receiver, owner, assets, shares);
    }

Note: This finding (being flawed at the code logic) is distinctly different from L-01 (merely touching on dodging through frontrunning) on Pashov's audit report.

Tools Used

Manual

Recommended Mitigation Steps

Refactor the check that will also have the owner checked for the role:

https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDe.sol#L232-L233

-    if (hasRole(FULL_RESTRICTED_STAKER_ROLE, caller) || hasRole(FULL_RESTRICTED_STAKER_ROLE, receiver)) {
+    if (hasRole(FULL_RESTRICTED_STAKER_ROLE, caller) || hasRole(FULL_RESTRICTED_STAKER_ROLE, owner) || hasRole(FULL_RESTRICTED_STAKER_ROLE, receiver)) {
      revert OperationNotAllowed();

Assessed type

Access Control

Use an efficient logic in setter functions

When intending to emit both the old and new values, there isn't a need to cache the old value that will only be used once. Simply emit both values before assigning a new value to the state variable. For example, the following setter function may be refactored as follows:

https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/EthenaMinting.sol#L436-L440

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

All other instances entailed:

https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/EthenaMinting.sol#L442-L447

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

https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDeV2.sol#L126-L134

  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);
  }

Be consistent in the code logic when emitting events in setter functions

By convention, it's recommended emitting the old value followed by the new one in the same event instead of the other way round.

Here's one instance found:

https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/USDe.sol#L23-L26

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

Delta neutrality caution

Users should be cautioned about the impermanent losses entailed arising from the delta-neutral stability strategy adopted by the protocol, specifically if the short positions were to encounter hefty losses. Apparently, the users could have held on to their collateral, e.g. stETH or WETH, and ended up a lot richer with the equivalent amount of USDe. I suggest all minting entries to begin with stable coins like USDC, DAI etc that could be converted to stETH to generate yield if need be instead of having users depositing stETH from their wallet reserves. Psychologically, this will make the users feel better as the mentality has been fostered more on preserving the 1:1 peg of USDe at all times.

Easy DoS on big players when minting and redeeming in EthenaMinting.sol

As indicated on the contest description, users intending to mint/redeem a large amount will need to mint/redeem over several blocks due to maxMintPerBlock or maxRedeemPerBlock. However, these RFQ's are prone to DoS because mintedPerBlock[block.number] + mintAmount > maxMintPerBlock or redeemedPerBlock[block.number] + redeemAmount > maxRedeemPerBlock could revert by only 1 wei in excess.

While these issues could be sorted by the backend to make a full use of maxMintPerBlock or maxRedeemPerBlock per block, it will make the intended logic a lot more efficient by auto reducing the RFQ amount to perfectly fill up the remaining quota for the current block. Better yet, set up a queue system where request amount running in hundreds of thousands or millions may be auto split up with multiple orders via only one signature for batching.

Inexpedient code lines

In the function below, the if block already dictates that getUnvestedAmount() == 0 manages to avoid a revert. Hence, consider refactoring the following code lines as it makes no sense adding 0 value getUnvestedAmount() of to the addend, amount:

https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDe.sol#L89-L99

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

-    vestingAmount = newVestingAmount;
+    vestingAmount = amount;

    // The rest of the codes
  }

Emission of identical values

Under the context of the above/preceding recommendation, transferInRewards() should also have its emit refactored below. Otherwise, you are practically emitting two identical values that defeat the purpose of contrasting the old and the values.

https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDe.sol#L89-L99

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

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

-    emit RewardsReceived(amount, newVestingAmount);
  }

Typo mistakes

https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDeV2.sol#L94 https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDeV2.sol#L110

-  /// @param owner address to redeem and start cooldown, owner must allowed caller to perform this action
+  /// @param owner address to redeem and start cooldown, owner must allow caller to perform this action

Functions should have fully intended logic

The function below is meant to be used only for minting. Hence, redeeming has got nothing to do with this view function. Consider refactoring the first if block so mint() could revert earlier if need be:

https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/EthenaMinting.sol#L351-L374

  function verifyRoute(Route calldata route, OrderType orderType) public view override returns (bool) {
    // routes only used to mint
-    if (orderType == OrderType.REDEEM) {
-      return true;
-    }
+    if (orderType =! OrderType.MINT) {
+      return false;
+    }
    uint256 totalRatio = 0;
    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) {
      if (!_custodianAddresses.contains(route.addresses[i]) || route.addresses[i] == address(0) || route.ratios[i] == 0)
      {
        return false;
      }
      totalRatio += route.ratios[i];
    }
    if (totalRatio != 10_000) {
      return false;
    }
    return true;
  }

Unneeded function still not removed

Per Pashov's audit report, L-04 mentioned that the unused EthenaMinting::encodeRoute has been removed by Ethena. However, this erroneous function still exists in EthenaMinting.sol.

https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/EthenaMinting.sol#L334-L336

-  function encodeRoute(Route calldata route) public pure returns (bytes memory) {
-    return abi.encode(ROUTE_TYPE, route.addresses, route.ratios);
-  }

Consider removing this controversial function where possible.