Ethena Labs - oakcobalt's results

Low 01 - Unnecessary math operation and local variable declaration

In StakedUSDe.sol - transferInRewards(), adding zero math operation is performed and the result is assigned to a new local variable.

// 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;
...

(https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/StakedUSDe.sol#L91)

As seen above, getUnvesteAmount() would have to return zero to pass the if statement, but when getUnvestedAmount() is zero, there is no need to add zero to amount and delacring newVestingAmount which is the same value as amount.

Recommendation: Since if statement would ensure getUnvestedAmount() return zero, directly assign vestingAmount = amount.

Low 02 - Emitting duplicated values is unnecessary.

In StakedUSDe.sol - transferInRewards(), emit RewardsReceived(amount, newVestingAmount) emits two variables amount and newVestingAmount. However, amount and newVestingAmount will always be the same value.

// contracts/StakedUSDe.sol
  function transferInRewards(uint256 amount) external nonReentrant onlyRole(REWARDER_ROLE) notZero(amount) {
    if (getUnvestedAmount() > 0) revert StillVesting();
    uint256 newVestingAmount = amount + getUnvestedAmount();
...
|>  emit RewardsReceived(amount, newVestingAmount);

(https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/StakedUSDe.sol#L98C1-L98C52)

As seen above, when if statement passes, getUnvestedAmount() returns zero, which means newVestingAmount will equal amount. And RewardsReceived will always emit the same value twice, which is unnecessary.

Recommendation: (1) Change to emit RewardsReceived(amount); (2) Or if the intention is to allow adding previously unvested amount to amount, then remove if statement.

Low 03 - Balance redistribution may cause loss of funds

In StakedUSDe.sol - redistributeLockedAmount(), when the input to argument is address(0), the function will not correctly handle it. The function will not revert but will continue to burn tokens causing permanent loss of funds. And base on the function doc, the intended behavior should be funds transfer, not permanent burning of funds on any occasion.

// contracts/StakedUSDe.sol
  /**
   * @dev Burns the full restricted user amount and mints to the desired owner address.
   * @param from The address to burn the entire balance, with the FULL_RESTRICTED_STAKER_ROLE
   * @param to The address to mint the entire balance of "from" parameter.
   */
  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);
|>    if (to != address(0)) _mint(to, amountToDistribute);
      emit LockedAmountRedistributed(from, to, amountToDistribute);
    } else {
      revert OperationNotAllowed();
    }
  }

(https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/StakedUSDe.sol#L153)

As seen above, when the admin redistributes funds from a fully restricted staker to another address, when to is incorrectly set as zero, the function will not revert, instead it will simply burn the entire balance.

Recommendations: Revert when to is address(0).

Low 04 - The current cool-down mechanism is problematic - might cause user's assets to be locked longer than expected.

In StakedUSDeV2.sol - cooldownAssets(), users can withdraw their deposited assets to USDeSilo.sol where assets will be locked for a set cool-down period. However, currently implementation might allow unexpected longer or shorter cool-down period under certain conditions.

Suppose cool-down period is 90 days: When a user initiates cooldownAssets() with 100 ether first. Then 60 days later, the user initiates another cooldownAssets() for another 50 ether. However, user's total 150 ether redeemed assets are now locked for another 90 days. Their deposited 100 ether is locked for 150 days, and this is not the promised 90-day cool-down;

// contracts/StakedUSDeV2.sol
  function cooldownAssets(uint256 assets, address owner) external ensureCooldownOn returns (uint256) {
    if (assets > maxWithdraw(owner)) revert ExcessiveWithdrawAmount();
    uint256 shares = previewWithdraw(assets);
|>  cooldowns[owner].cooldownEnd = uint104(block.timestamp) + cooldownDuration;
|>  cooldowns[owner].underlyingAmount += assets;
    _withdraw(_msgSender(), address(silo), owner, assets, shares);
    return shares;
  }

(https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/StakedUSDeV2.sol#L100-L101)

// contracts/StakedUSDeV2.sol
  function cooldownShares(uint256 shares, address owner) external ensureCooldownOn returns (uint256) {
    if (shares > maxRedeem(owner)) revert ExcessiveRedeemAmount();
    uint256 assets = previewRedeem(shares);
|>  cooldowns[owner].cooldownEnd = uint104(block.timestamp) + cooldownDuration;
|>  cooldowns[owner].underlyingAmount += assets;
    _withdraw(_msgSender(), address(silo), owner, assets, shares);
    return assets;
  }

(https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/StakedUSDeV2.sol#L116-L117)

As seen above, whenever a user calls cooldownAssets() or cooldownShares(), their assets will be directly added to the existing underlyingAmount and their cooldownEnd time will be updated to a new timestamp without checking whether the user has an existing cooldownEnd.

Recommendations: (1)Either only allow one time withdraw per cooldown period and revert when user's cooldownEnd is non-zero; (2)Or if asset is allowed to be addded before an existing cooldown, consider use a mapping to track cooldownEnd key with the corresponding underlyingAmount.

Low 05 - No on-chain check to ensure mint, redeem will always submit a benefactor's nonce in an incrementing manner

In EthenaMinting.sol - mint() and redeem(), only MINTER_ROLE and REDEEMER_ROLE can submit mint() and redeem() order. Trust has to be placed on MINTER_ROLE and REDEEMER_ROLE to always submit the user order nonce correctly. However, if MINTER_ROLE and REDEEMER_ROLE submit an incorrect nonce (e.g. skipping or jumping numbers) there is no way on-chain process will detect it.

This could happen either by mistake or when MINTER_ROLE or REDEEMER_ROLE key is compromised. Malicious MINTER_ROLE or REDEEMER_ROLE could submit random future nonces which might cause a user unable to mint() or redeem() in the future due to nonce collision.

// contracts/EthenaMinting.sol
  function mint(Order calldata order, Route calldata route, Signature calldata signature)
    external
    override
    nonReentrant
    onlyRole(MINTER_ROLE)
    belowMaxMintPerBlock(order.usde_amount)
  {
    if (order.order_type != OrderType.MINT) revert InvalidOrder();
    verifyOrder(order, signature);
    if (!verifyRoute(route, order.order_type)) revert InvalidRoute();
|>  if (!_deduplicateOrder(order.benefactor, order.nonce)) revert Duplicate();
...

(https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/EthenaMinting.sol#L172)

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

(https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/EthenaMinting.sol#L391-L395)

// contracts/EthenaMinting.sol
  function verifyNonce(address sender, uint256 nonce) public view override returns (bool, uint256, uint256, uint256) {
...
    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();
...

(https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/EthenaMinting.sol#L383)

As seen above, in mint() or redeem(), verifyNonce() will run every time to make sure the nonce submitted by MINTER_ROLE or REDEEMER_ROLE is not clashing with an existing nonce. However, it didn't check whether the nonce is incrementing by 1 from the previous nonce. The lack of this on-chain checks, allows room for jumping or skipping nonce to go through whenever the situation allows, which will cause future mint() or redeem() orders to revert causing DOS of a certain user's mint() or redeem() and user collaterals can be locked permanently.

Recommendations: Add additional checks in verifyNonce() to ensure a user's nonce always increments by 1.

Gas 01 - Waste of gas because of unnecessary math operation and local variable declaration.

In StakedUSDe.sol - transferInRewards(), adding zero math operation is performed and the result is assigned to a new local variable.

// 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;
...

(https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/StakedUSDe.sol#L91)

As seen above, getUnvesteAmount() would have to return zero to pass the if statement, but when getUnvestedAmount() is zero, there is no need to add zero to amount and delacring newVestingAmount which is the same value as amount.

Recommendation: Since if statement would ensure getUnvestedAmount() return zero, directly assign vestingAmount = amount. This saves 6 gas units.

Gas 02 - Waste of gas due to emitting the same value twice

In StakedUSDe.sol - transferInRewards(), emit RewardsReceived(amount, newVestingAmount) emits two variables amount and newVestingAmount. However, amount and newVestingAmount will always be the same value.

// contracts/StakedUSDe.sol
  function transferInRewards(uint256 amount) external nonReentrant onlyRole(REWARDER_ROLE) notZero(amount) {
    if (getUnvestedAmount() > 0) revert StillVesting();
    uint256 newVestingAmount = amount + getUnvestedAmount();
...
|>  emit RewardsReceived(amount, newVestingAmount);

(https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/StakedUSDe.sol#L98C1-L98C52)

As seen above, when if statement passes, getUnvestedAmount() returns zero, which means newVestingAmount will equal amount. And RewardsReceived will always emit the same value twice, which is unnecessary.

Recommendation: Change to emit RewardsReceived(amount), which saves 256 gas units.

Description:

Ethena is an Ethereum-based protocol, aiming to create a crypto-native solution for money independent of traditional banking systems. It introduces a stable synthetic dollar called USDe, which is both censorship-resistant and scalable.

USDe achieves stability through delta-hedging Ethereum collateral and transparent on-chain backing. The 'Internet Bond' complements this by offering a dollar-denominated savings instrument, utilizing yield from staked Ethereum and revenue from perpetual and futures markets.

Existing Standards:

• The protocol adheres to conventional Solidity and Ethereum practices, primarily utilizing the ERC20 standards, along with the openzepplin's access control patterns for assigning various roles. Although, access control is customized and extended to enforce a single admin control over all other roles.

• Additionally, it incorporates features like blacklisting, maximum per-block minting/redeeming, token rescue mechanism.

• The protocol also primarily uses ERC4626 standards for staking and unstaking accounting, with a customized cool-down withdraw mechanism design.

It's worth noting that the protocol relies on off-chain accounting and pricing conversion mechanisms, and also involves counter party's during funds transfer.

1- Approach:

• Scope: I initiated the process by evaluating the extent of the code, which then directed our method for scrutinizing and dissecting the project.

• Roles: Then I focus on role setup for the minting, redeeming, staking and unstacking process.

Considering the extensive authority held by these roles within the system, users must have complete confidence that the entities responsible for supervising these roles will consistently act in a correct and beneficial manner for the system and its users.

• Top-down approach:

1.EthenaMinting.sol, SingleAdminAccessControl.sol
2.StakedUSDe.sol, SingleAdminAccessControl.sol
3.StakedUSDeV2.sol, SingleAdminAccessControl.sol
4.USDe.sol
5.USDeSilo.sol

SingleAdminAccessControl and EthenaMinting are reviewed together due to the fact that the role setup are integrated in SingleAdminAccessControl.

2- Codebase analysis:

In my assessment, the quality of the codebase is currently satisfactory, with measures in position to manage diverse roles and ensure adherence to established standards. However, there remains room for enhancement in the following aspects.

3- Centralization Risks:

Here's an analysis of potential centralization risks in the provided contracts:

EthernaMinting.sol

• Centralization Risks:
  • Off-chain user background check process.
  • Single admin-role can remove and add all other roles including the minter role and redeemer role. This means if admin-role key is leaked, all other access controled functions will be eroded instantly.
  • Admin removal or adding OES operator address (custodian address) before user redeems. Scenario of admin key loss.
  • Admin removal of supported Asset. Scenario of admin key loss.

StakedUSDe.sol

• Centralization Risks:
  • Address black-listing.
  • Single admin-role can remove and add all other roles and act as other role admins. This means if admin-role key is leaked, all other access controled functions will be eroded instantly.
  • rescueTokens(), centralized admin can rescue tokens to a designated address of his choosing

SingleAdminAccessControl.sol

• Centralization Risks:
  • Single admin instead of multiple role admin is implemented: This increases the centralization risks. For example, If the admin key becomes compromised, gatekeeper role becomes non-existing. Since admin can change gateKeeper role and directly perform sensitive minting or redeeming functions, such as disableMintRedeem.

4- Systemic Risks:

Price slippage and lack of on-chain checks:

In EthernaMinting.sol, collateral price slippage can happen at the time of mint or redeem order settlement on-chain. This means the actual amount of collateral transferred might not reflect the values of USDe minted.

In addition, the minter and redeemer role become the on-chain single source of truth for fair price conversion between USDe and collateral tokens.

Reliability of off-chain accounting:

Reliability of off-chain accounting of user mint amount, collateral amount, ratios of custodian distribution or actual custodian transfer, custodian address, etc.

Counterparty Risks:

The reliability of transparent and trustworthy custodian addresses increases the counterparty risks.

It's recommended that OES providers offer the most transparent and verifiable accounting of funds transfer and storage to counter such risks.

Price manipulation:

Ethena protocol allows peg arbitrage on defi markets. But since current defi pool that includes USDe has low liquidity and still in the test stage (see example below). It can be assumed that at the beginning stage defi pool price of USDe will be prone to price manipulation, which might cause USDe's value extremely volatile to user arbitrage.

Currently, there is a test Crv/USDe pool on Curve which has relatively low reserve.

5- Mechanism Review:

Slippage windows:

EthenaMinting.sol:

There are multiple windows of collateral price slippage from the beginning of the user query on dApp UI, to the user signing a mint or redeem order signature based on quote amounts, to Ethena minter or redeemer role pick up user's signature and generating order and submitting an order on behalf of users, and to the submitted transaction finally settled on-chain.

So far, the protocol's approach to dealing with slippage is only preemptive, including a slippage into the user's quote on the dApp UI before the user signs a signature to approve the quoted amount. This is insufficient to prevent collateral slippage after user signing signatures, and also insufficient to prevent price slipping further from an acceptable range. In EthenaMinting.sol, It's recommended to add on-chain verification confirming the settled amount is within acceptable range to prevent USDe being under-collateralized at the time of order settlement.

Nonce on-chain verification:

EthenaMinting.sol:

In EthenaMinting.sol, when mint or redeem order is submitted, it's minter or redeemer role's job to submit a nonce of user's order. Current on-chain check only checks whether the submitted nonce is clashing with an existing nonce in storage.

However, this is insufficient in ensuring nonce is submitted strictly in an incrementing manner and incrementing exactly 1 from the previous one. In the case of a minter or redeemer role mistakes or minter or redeemer key leakage, random nonce number will pass, which will create clashing of user nonce's in the future, potentially causing a user unable to mint or redeem.

Cool-down mechanism:

StakedUSDeV2.sol:

Owner’s asset might be forced to be locked longer than set cool-down duration.

Current implementation of cool-down will renew the cooldown end timestamp whenever a user requested more assets to be withdrawn, regardless of whether a user has current on-going cooldown or not. This will cause a user's existing locked asset's cool-down to be extended to an additional cool-down cycle. It's recommended to refine the cool-down accounting logic to use a mapping to correspond each coolDownEnd timestamp to their respective asset value to avoid extended and unfair lock of user assets.

Conclusion:

In summary, while Ethena demonstrates promise as a crypto-native financial protocol, it requires further enhancements in addressing several issues related to systemic risks, centralization risks and mechanism review as detailed above.

Time spent:

30 hours

Time spent:

30 hours