Ethena Labs - Shubham's results

Lines of code

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

Vulnerability details

According to docs:

SOFT_RESTRICTED_STAKER_ROLE - 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. Address with this role can't stake his USDe tokens or get stUSDe tokens minted to him.

But this is not true because there can be a particular case where theses addresses will be able to mint & withdraw stUSDe.

Proof of Concept

redistributeLockedAmount() burns the full restricted user amount and mints to the desired owner address. The first check ensures that address to which an equable burned amount is minted isn't full restricted.

File: 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)) {    /// @note - no check for soft restricted address
        uint256 amountToDistribute = balanceOf(from);
        _burn(from, amountToDistribute);
        // to address of address(0) enables burning
        if (to != address(0)) _mint(to, amountToDistribute);     /// @note - tokens minted for soft restricted address

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

However there is no check to ensure that the to address isn't soft restricted. There is a check to see if the to address isn't address(0) but thats it. In a situation when redistributeLockedAmount() is called with a to address having SOFT_RESTRICTED_STAKER_ROLE, the function enters the 1st if condition returning true for the check & later tokens are minted to that address.

Also as the per the docs SOFT_RESTRICTED_STAKER_ROLE cannot deposit USDe to get stUSDe or withdraw stUSDe for USDe. Thats why there is no check in _withdraw() for the SOFT_RESTRICTED_STAKER_ROLE because since they cannot deposit, there won't be anything to withdraw.

But thats not the case here. The amount minted to a SOFT_RESTRICTED_STAKER_ROLE address can be withdrawn as there is no check here. The check is only for FULL_RESTRICTED_STAKER_ROLE address.

File: StakedUSDe.sol

  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();
    }
                                                            /// @note - no check for SOFT_RESTRICTED_STAKER_ROLE
    super._withdraw(caller, receiver, _owner, assets, shares);
    _checkMinShares();
  }

Thus a soft restricted address gets tokens minted for them & they can withdraw easily without any restriction.

Impact

Address having SOFT_RESTRICTED_STAKER_ROLE can get stUSDe minted, earn yield & withdraw the amount thereby breaking the protocol's guidelines.

Labelling this as a HIGH because since Ethena Labs is not allowed to provide yield to addresses of some countries (thats why marked soft restricted), the situation described in this finding will put the organization involved in illegal activities.

Tools Used

Manual Review

Recommended Mitigation Steps

Add a check to ensure that the to address in redistributeLockedAmount() isn't soft restricted.

-  232    if (hasRole(FULL_RESTRICTED_STAKER_ROLE, from) && !hasRole(FULL_RESTRICTED_STAKER_ROLE, to)) {
+  232    if (hasRole(FULL_RESTRICTED_STAKER_ROLE, from) && !hasRole(FULL_RESTRICTED_STAKER_ROLE, to) && !hasRole(SOFT_RESTRICTED_STAKER_ROLE, to)) {

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDeV2.sol#L82 https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDeV2.sol#L100 https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDeV2.sol#L116

Vulnerability details

The maximum cooldown duration is set at 90 days which can be changed by the admin when needed but cannot exceed 90 day threshold. Whenever the cooldownDuration > 0, cooldownAssets or cooldownShares can be called which sends the funds to the silo contract until the cooldown period ends for the user.

Proof of Concept

Whenever a user decides to unstake, cooldownEnd is checked to make sure the user is eligible to withdraw funds.

File: StakedUSDeV2.sol

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

    if (block.timestamp >= userCooldown.cooldownEnd) {    /// @note - will fail incase cooldownDuration changes
      userCooldown.cooldownEnd = 0;
      userCooldown.underlyingAmount = 0;

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

The cooldownEnd is set either when calling cooldownAssets() or cooldownShares().

File: StakedUSDeV2.sol

95     function cooldownAssets(uint256 assets, address owner) external ensureCooldownOn returns (uint256) {
         ..........
100      cooldowns[owner].cooldownEnd = uint104(block.timestamp) + cooldownDuration;  
       }


111    function cooldownShares(uint256 shares, address owner) external ensureCooldownOn returns (uint256) {
         ..........
116      cooldowns[owner].cooldownEnd = uint104(block.timestamp) + cooldownDuration;  
       }

Scenario 1:

• Current cooldownDuration = MAX_COOLDOWN_DURATION (90 days)
• Alice calls cooldownAssets to redeem some assets.
• cooldownEnd for Alice is set to 90 days counting from the present timestamp.
• Admin changes cooldownDuration to 45 days.
• Alice calls unstake after 45 days but the call reverts with InvalidCooldown error.

Scenario 2:

• Current cooldownDuration = 50 days
• Bob calls cooldownAssets to redeem some assets.
• cooldownEnd for Bob is set to 50 days counting from the present timestamp.
• Admin changes cooldownDuration to 90 days.
• Bob calls unstake after 50 days & withdraws his funds even though the cooldown duration has been extended.

Scenario 3:

• Current cooldownDuration = 90 days
• Zayn calls cooldownAssets to redeem some assets.
• cooldownEnd for Zayn is set to 90 days counting from the present timestamp.
• Admin changes cooldownDuration to 0 days.
• Zayn immediately calls unstake but the function reverts because now he has to wait for 90 days until cooldownEnd is achieved.

According to the 1st scenario, even though the cooldown period has been reduced, early users would still not be able to withdraw their funds compared to the new users who requested their funds after the new period.

According to the 2nd scenario, even though the cooldown period has been increased, early users would be able to withdraw according to the previous cooldown duration.

According to the 3rd scenario, even though the cooldown period has been removed or set to 0 days, the user wouldn't be able to unstake as his funds will be stuck inside the Silo contract till the previous cooldown duration ends.

Tools Used

Manual Review

Recommended Mitigation Steps

Remove cooldownDuration from cooldownAssets() and cooldownShares() & add it to unstake().

File: StakedUSDeV2.sol

  95     function cooldownAssets(uint256 assets, address owner) external ensureCooldownOn returns (uint256) {
           ..........
- 100      cooldowns[owner].cooldownEnd = uint104(block.timestamp) + cooldownDuration;
+ 100      cooldowns[owner].cooldownEnd = uint104(block.timestamp)  
         }


  111    function cooldownShares(uint256 shares, address owner) external ensureCooldownOn returns (uint256) {
           ..........
- 116      cooldowns[owner].cooldownEnd = uint104(block.timestamp) + cooldownDuration;
+ 116      cooldowns[owner].cooldownEnd = uint104(block.timestamp)  
         }


  78     function unstake(address receiver) external {
           ........
- 82       if (block.timestamp >= userCooldown.cooldownEnd) {
+ 82       if (block.timestamp >= userCooldown.cooldownEnd + cooldownDuration ) {

Assessed type

Timing

Low Issues

Non-Critical Issues

[L-01] Users can transfer in small amounts before VESTING_PERIOD duration has passed

getUnvestedAmount() returns 0 when timeSinceLastDistribution >= VESTING_PERIOD that is greater or equal to 8 hours. But even if the timeSinceLastDistribution is close to 8 hours & vesting amount is high, the function would still return 0 or if the amount to send is small & the not much time has passed since the last transfer.

File: StakedUSDe.sol

  function getUnvestedAmount() public view returns (uint256) {
    uint256 timeSinceLastDistribution = block.timestamp - lastDistributionTimestamp;

    if (timeSinceLastDistribution >= VESTING_PERIOD) {
      return 0;
    }
    /// @note - will return 0 in some cases
    return ((VESTING_PERIOD - timeSinceLastDistribution) * vestingAmount) / VESTING_PERIOD;       
  }

Suppose

• timeSinceLastDistribution is 7.5 hours (27,000 sec)
• vestingAmount is 15
• ((VESTING_PERIOD - timeSinceLastDistribution) * vestingAmount) = (28800 - 27000) * 15 = 27000
• 27000 / VESTING_PERIOD = 27000 / 28800 = 0

Similar situation would be if vestingAmount is 1, then Rewarder can call transferInRewards() with a huge amount the very next second & rewards will be sent. The Rewarder will try calling transferInRewards() & it will pass even though VESTING_PERIOD duration hasn't been achieved. This enables rewards to be transferred before the set time period. Worst case is that users can transfer in small amounts before the 8 hours criteria.

[L-02] No check to see whether benefactor/beneficiary are restricted addresses

Inside EthenaMinting.sol, mint() & redeem() are called by the MINTER & REEDEMER respectively. During mint(), the collateral is transferred from the benefactor & during redeem() the collateral is transferred to the beneficiary. But there is no check to ensure that the address of either the benefactor or beneficiary are soft/fully restricted. This would break the protocol guidelines as the soft restricted addresses aren't supposed to get USDe tokens minted & there should be no interaction with a fully restricted addresses due to malicious or illegal nature of the address.

It is recommended to add a check for blacklisted addresses in EthenaMinting.sol contract.

[NC-01] Unnecessary check in verifyRoute()

verifyRoute() is only called inside mint() in EthenaMinting contract. We can see that the 1st if condition already checks for the Order Type which should be MINT. So inside verifyRoute() the 1st check will always return false.

File: 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();
      ......


    function verifyRoute(Route calldata route, OrderType orderType) public view override returns (bool) {
      // routes only used to mint                           /// @note - will never get executed
      if (orderType == OrderType.REDEEM) {
        return true;
      }
      .....

Consider removing this check.

[NC-02] Unnecessary computation in transferInRewards()

getUnvestedAmount() is called inside transferInRewards() to check if amount is still vesting. It adds its value to the newVestingAmount variable & set to vestingAmount. But it makes no sense because getUnvestedAmount() will only pass the 1st check if its 0 & then later adding 0 to amount is a waste.

File: StakedUSDe.sol

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

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

    emit RewardsReceived(amount, newVestingAmount);
  }

The amount passed could be directly added to vestingAmount without the need of newVestingAmount.