Ethena Labs - Bauchibred's results

QA Report

Table of Contents

QA-01 verifyOrder() currently accepts an expired signature and emits a wrong event

Impact

No inclusive checks means the order verification process currently accepts an already expired signature.

Proof of Concept

Take a look at verifyOrder()

  /// @notice assert validity of signed order
  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();
  //@audit-issue
    if (block.timestamp > order.expiry) revert SignatureExpired();
    return (true, taker_order_hash);
  }

The current condition only checks if the block timestamp has surpassed the order's expiry. This means even if the order expires it would be validated the check would not consider the signature expired, when in fact it should be.

Recommended Mitigation Steps

Make these changes to verifyOrder()

  /// @notice assert validity of signed order
  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.beneficiary == address(0)) revert InvalidAddresst();
    if (order.collateral_amount == 0) revert InvalidAmount();
    if (order.usde_amount == 0) revert InvalidAmount();
-    if (block.timestamp > order.expiry) revert SignatureExpired();
+    if (block.timestamp >= order.expiry) revert SignatureExpired();
    return (true, taker_order_hash);
  }

QA-02 The SOFT_RESTRICTED_STAKER_ROLE is a legal case in the making

Impact

The current configuration of the SOFT_RESTRICTED_STAKER_ROLE allows addresses with this role to circumvent deposit restrictions by transferring their USDe tokens to another address they control. This could easily lead to a legal case since one can link these transfers to each other and if restrictions were to be placed on a user from a specific country and they are not fixed could easily land the team a lawsuit.

Proof Of Concept

The below has been stated under Known Issues

• SOFT_RESTRICTED_STAKER_ROLE can be bypassed by user buying/selling stUSDe on the open market But that's not the only case as there are other methods one could by pass this

1. Assign an address the SOFT_RESTRICTED_STAKER_ROLE.
2. Attempt a direct deposit with this address, which should fail due to restrictions.
3. Transfer USDe tokens from this address (with SOFT_RESTRICTED_STAKER_ROLE) to another address owned by the same entity.
4. Use the secondary address to deposit the transferred USDe tokens. This operation will succeed, demonstrating the loophole.

Recommended Mitigation Steps

Review and possibly refine the permissions associated with the SOFT_RESTRICTED_STAKER_ROLE to ensure that they align with the intended functionality, since this could lead to legal issues, on one hand I would suggest to just scrap it and instead have only the FULL_RESTRICTED_STAKER_ROLE

QA-03 Max mint per block could actually be exceeded

Impact

NB: First this is only submittted as a QA since it's been listed to be a main invariant, but since it requires admin mistake it sits on the fence of med/QA

Proof Of Concept

As stated under the Main Invariants section of the contest's readMe

Max mint per block should never be exceeded. But current code implementation does not follow this based on two reasons:

• If the admin calls setMaxMintPerBlock() it sets the new max mint for that specific block and any block that comes after it, this can be seen below:

  function setMaxMintPerBlock(uint256 _maxMintPerBlock) external onlyRole(DEFAULT_ADMIN_ROLE) {
    _setMaxMintPerBlock(_maxMintPerBlock);
  }

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

The above easily means that for the current block the max mint could always be altered

Recommended Mitigation Steps

To ensure that more than max is not minted then there is a need to introduce timing to the process and ensure that the current max mint can't be changed for the current timestamp. Got it. Here's the revised report:

QA-04 _transferCollateral() should be made more efficient

Impact

Low, since at most this could only be 1 wei but might still lead to issues regarding ratio if this ever amounts to a large number.

Proof of Concept

Take a look at _transferCollateral():

  function _transferCollateral(
    uint256 amount,
    address asset,
    address benefactor,
    address[] calldata addresses,
    uint256[] calldata ratios
  ) internal {
    // cannot mint using unsupported asset or native ETH even if it is supported for redemptions
    if (!_supportedAssets.contains(asset) || asset == NATIVE_TOKEN) revert UnsupportedAsset();
    IERC20 token = IERC20(asset);
    uint256 totalTransferred = 0;
    for (uint256 i = 0; i < addresses.length; ++i) {
      uint256 amountToTransfer = (amount * ratios[i]) / 10_000;
      token.safeTransferFrom(benefactor, addresses[i], amountToTransfer);
      totalTransferred += amountToTransfer;
    }
    uint256 remainingBalance = amount - totalTransferred;
    //@audit
    if (remainingBalance > 0) {
      token.safeTransferFrom(benefactor, addresses[addresses.length - 1], remainingBalance);
    }
  }

As seen, the _transferCollateral function, designed to transfer supported assets to a range of custody addresses based on defined ratios, but it has an inefficiency in how it handles the remaining balance after all the transfers.

As seen, if there is a remaining balance, it is sent to the last address in the addresses array:

if (remainingBalance > 0) {
  token.safeTransferFrom(benefactor, addresses[addresses.length - 1], remainingBalance);
}

This approach assumes that the last address is always the intended recipient of any remaining balance. It doesn't provide flexibility and can cause unexpected results if, for example, the addresses array order changes, or this amounts to a considerable sum for that specific address leading to a break in the ratio logic.

Recommended Mitigation Steps

Allow specification of which address among the addresses array should receive any remaining balance.

QA-04 Setters should always have equality checkers

Impact

The transferAdmin function in its current state does not validate if the newAdmin being set is different from the _currentDefaultAdmin. This oversight can lead to unnecessary operations where the admin role is "transferred" to the same address that already possesses it, wasting gas and potentially causing confusion among the stakeholders.

Proof Of Concept

Consider the function:

  //@audit setters equality check
  function transferAdmin(address newAdmin) external onlyRole(DEFAULT_ADMIN_ROLE) {
    if (newAdmin == msg.sender) revert InvalidAdminChange();
    _pendingDefaultAdmin = newAdmin;
    emit AdminTransferRequested(_currentDefaultAdmin, newAdmin);
  }

Here's the sequence that demonstrates the gap:

1. The current admin (_currentDefaultAdmin) calls the transferAdmin function with their own address as the newAdmin.
2. The function only checks if the newAdmin address is the same as the msg.sender.
3. No checks are in place to determine if newAdmin is the same as _currentDefaultAdmin.
4. As a result, the operation proceeds even if the newAdmin is the same as _currentDefaultAdmin.

Recommended Mitigation Steps

Modify the function to include a check comparing newAdmin to _currentDefaultAdmin. If they are the same, the function should revert the transaction. solidity if (newAdmin == _currentDefaultAdmin) revert RedundantAdminChange();

QA-05 Potential role oversight while depositing

Impact

While depositing there seem to be a check for users who have been assigned the SOFT_RESTRICTED_STAKER_ROLE but no consideration is applied for users with the FULL_RESTRICTED_STAKER_ROLE.

Proof of Concept

The following check is performed to prevent users with the SOFT_RESTRICTED_STAKER_ROLE from using the function:

if (hasRole(SOFT_RESTRICTED_STAKER_ROLE, caller) || hasRole(SOFT_RESTRICTED_STAKER_ROLE, receiver)) {
  revert OperationNotAllowed();
}

However, no check is done for users with the FULL_RESTRICTED_STAKER_ROLE

Recommended Mitigation Steps

Introduce a check for the FULL_RESTRICTED_STAKER_ROLE

Analysis

Table of Contents

• Approach
• Architecture Feedback
• Centralization Risks
• Systemic Risks
• Testability
• Other Recommendations
• Security Researcher Logistics
• Conclusion

Approach

The audit began with a comprehensive review of the provided documentation, not limiting to just the files in scope. Following this, insights were extracted from previous audit reports over several hours. A detailed, line-by-line security assessment of the source lines of code (sLOC) ensued. The final stage involved engaging with the development team on Discord to comprehend unique implementations, discuss potential vulnerabilities, and validate critical observations.

Architecture Feedback

• The protocol's efficiency is closely linked to its hedging strategy.

• In volatile market conditions, the protocol may fail to liquidate funds from centralized exchanges, potentially hindering users from withdrawing their full balance.

• For user redemptions, it's imperative that the protocol admin moves adequate funds from custody to the "Ethena Minting" contract.

• Concerningly, there's no on-chain guarantee for the redeemability of "USDe". The team possesses the discretion to withhold funds.

• Notable roles and their privileges:

  • EthenaMinting & DEFAULT_ADMIN_ROLE:
    • Control over maxMintPerBlock and maxRedeemPerBlock.
    • Authorization to manage addresses across roles (excluding the owner).
  • Owner:
    • Has the power to set the USDe token address.
    • Can modify supported assets.
  • MINTER_ROLE:
    • Can generate stablecoins.
    • Holds the power to transfer any asset.
  • REDEEMER_ROLE:
    • Authorized to exchange stablecoins for assets.
  • GATEKEEPER_ROLE:
    • Ability to halt both minting and redeeming operations.
    • Can rescind the MINTER_ROLE.
  • For StakedUSDeV2.sol:
    • Owner/DEFAULT_ADMIN_ROLE can administer roles.
    • Can reallocate stUSDe from specified wallets.
    • REWARDER_ROLE can vest USDe tokens via transferInRewards().

• Points of Vulnerability:

  • The lack of on-chain order pricing might allow Ethena Labs to mint USDe without the requisite underlying tokens.
  • USDe's underlying assets can be extracted indiscriminately.
  • stUSDe tokens are susceptible to unilateral confiscation by the staking contract's administrative role.
  • Profits for stUSDe holders necessitate manual deposits.

• Mitigation Strategies:

  • Prioritize transparency and traceability.
  • Offer thorough user guides.
  • Design comprehensive dashboards displaying asset values and their centralized allocations.
  • Clearly define role boundaries.
  • Formulate an approved whitelist for asset custodianship.
  • Emphasize: The protocol's complexity reveals inherent centralization risks.

Centralization Risks

• Both maxMintPerBlock and _maxRedeemPerBlock are supposed to have defined values. However, they aren't verified on-chain, allowing an admin to arbitrarily set values.
• An admin could exploit the system by assigning users the FULL_RESTRICTED_STAKER_ROLE and redistributing their tokens.

Systemic Risks

• The GATEKEEPER_ROLE concept poses challenges. Although designed as a safeguard, an errant address could cause irreversible damage. Instead of merely revoking privileges post-factum, it's wiser to assign roles proactively based on need and then withdraw them post-usage.
• Documents and code reveal that the EthenaMinting contract doesn't hold underlying LST assets, which reside in custodial wallets. This structure, while possibly efficient, exposes potential liquidity challenges for USDe redemptions.
• The quote: "We expect external organizations to only invoke the gatekeeper functions during price discrepancies on-chain..." suggests reactive measures. A preventive approach, assigning GATEKEEPER roles only when necessary and revoking them afterward, would be more judicious.
• The SOFT_RESTRICTED_STAKER_ROLE entails partial restrictions. Addresses with this role can't deposit but can perform other functions. Such an address might transfer their USDe tokens elsewhere and then deposit.

Testability

The testing coverage is admirable, with both line and statement coverage meeting standards. Nevertheless, a coverage exceeding 90% is always preferable for enhanced security.

Other Recommendations

Utilize More Auditing Tools

While the Solidity Visual Developer tool is already in use, incorporating additional plugins, especially for Visual Studio Code, could be advantageous.

Boost Testability

Although the current coverage is satisfactory, striving for a 90% coverage would significantly enhance protocol reliability.

Optimize Event Monitoring

The current setup could benefit from a more strategic implementation of event tracking to ensure maximum utility.

Security Researcher Logistics

My attempt on reviewing the in-scope codebase spanned 12 hours distributed over 3 days:

• 0.5 hours dedicated to writing this analysis.
• 2 hours exploring the previous audit reports
• 1.5 hour were allocated for discussions with sponsors on the private discord group regarding potential vulnerabilities.
• The remaining time on finding issues and writing the report for each of them on the spot, later on editing a few with more knowledge gained on the protocol as a whole, or downgrading them to QA reports

Conclusion

The codebase was a very great learning experience, though it was a pretty hard nut to crack, being that it's like an update contest and most easy to catch bugs have already been spotted and mitigated from the previous contest.

Time spent:

11 hours