Ethena Labs - HChang26's results

Lines of code

https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDe.sol#L225 https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDe.sol#L245 https://github.com/code-423n4/2023-10-ethena/blob/main/lib/openzeppelin-contracts/contracts/token/ERC20/extensions/ERC4626.sol#L230 https://github.com/code-423n4/2023-10-ethena/blob/main/lib/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol#L277 https://github.com/code-423n4/2023-10-ethena/blob/main/lib/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol#L222

Vulnerability details

Impact

FULL_RESTRICTED_STAKER_ROLE address can exploit a vulnerability that enables them to bypass checks and execute the _withdraw() function, allowing them to withdraw funds when they should be restricted from doing so.

Proof of Concept

The protocol has the ability to freeze funds for individuals with the FULL_RESTRICTED_STAKER_ROLE.

FULL_RESTRCITED_STAKER_ROLE is for sanction/stolen funds, or if we get a request from law enforcement to freeze funds. Addresses fully restricted cannot move their funds, and only Ethena can unfreeze the address. Ethena also have the ability to repossess funds of an address fully restricted

However, the current implementation allows addresses with the FULL_RESTRICTED_STAKER_ROLE to bypass the freezing measures and still withdraw funds. The withdraw() function invokes _withdraw(), which, in the existing code, only checks whether the caller and the receiver have the FULL_RESTRICTED_STAKER_ROLE. More importantly, it doesn't check if the owner holds this role.

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

This issue becomes evident when the owner is FULL_RESTRICTED_STAKER_ROLE. In this scenario, the owner can easily circumvent the restrictions by approving an allowance to a new account that doesn't have the FULL_RESTRICTED_STAKER_ROLE. This new account can then execute the withdraw() function on behalf of the owner. Furthermore, another account can be created to receive all the funds that are supposed to be "frozen."

The protocol also has security measures in place through the _beforeTokenTransfer() function. This function is executed before any transfer of tokens, including minting and burning, and acts as the final security check for FULL_RESTRICTED_STAKER_ROLE.

  function _beforeTokenTransfer(address from, address to, uint256) internal virtual override {
    if (hasRole(FULL_RESTRICTED_STAKER_ROLE, from) && to != address(0)) {
      revert OperationNotAllowed();
    }
    if (hasRole(FULL_RESTRICTED_STAKER_ROLE, to)) {
      revert OperationNotAllowed();
    }
  }

However, these checks are not sufficient to prevent the owner from bypassing the restrictions, as the owner's shares are still burnt and sent to address(0) as shown in super._withdraw(). _beforeTokenTransfer() allows transfer from FULL_RESTRICTED_STAKER_ROLE to address(0).

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

Additionally, the newly created receiver account is also capable of circumventing the checks implemented in _transfer() as it has no restriction from receiving funds.

    function _transfer(address from, address to, uint256 amount) internal virtual {
        require(from != address(0), "ERC20: transfer from the zero address");
        require(to != address(0), "ERC20: transfer to the zero address");

        _beforeTokenTransfer(from, to, amount);

        uint256 fromBalance = _balances[from];
        require(fromBalance >= amount, "ERC20: transfer amount exceeds balance");
        unchecked {
            _balances[from] = fromBalance - amount;
            _balances[to] += amount;
        }

        emit Transfer(from, to, amount);

        _afterTokenTransfer(from, to, amount);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

  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)) {
+   if (hasRole(FULL_RESTRICTED_STAKER_ROLE, owner) || hasRole(FULL_RESTRICTED_STAKER_ROLE, receiver)) {
      revert OperationNotAllowed();
    }

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

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

Addresses with SOFT_RESTRICTED_STAKER_ROLE can withdraw() stUSDe.

Proof of Concept

As per the documentation, the role SOFT_RESTRICTED_STAKER_ROLE is not supposed to have the ability to deposit USDe to obtain stUSDe or withdraw stUSDe in exchange for USDe.

Due to legal requirements, there's a SOFT_RESTRICTED_STAKER_ROLE and FULL_RESTRICTED_STAKER_ROLE. The former is for addresses based in countries we are not allowed to provide yield to, for example USA. 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.

The issue arises because the _withdraw() function does not adequately check for the SOFT_RESTRICTED_STAKER_ROLE. Instead, it only checks for the FULL_RESTRICTED_STAKER_ROLE. This oversight allows addresses with SOFT_RESTRICTED_STAKER_ROLE authorization, which should not be able to receive yield, to improperly withdraw stUSDe. User can buy stUSDe in open market and _withdraw() USDe for yields. This unintended behavior could lead to non-compliance with legal requirements.

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

Tools Used

Manual Review

Recommended Mitigation Steps

  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)) {
+   if (hasRole(SOFT_RESTRICTED_STAKER_ROLE, caller) || hasRole(SOFT_RESTRICTED_STAKER_ROLE, receiver)) {
      revert OperationNotAllowed();
    }

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

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

The verifyRoute() function yields incorrect results, possibly leading to unexpected errors.

Proof of Concept

The verifyRoute() function is specifically utilized in the mint() process, where it confirms the validity of a route object based on its type. This function examines whether the route ratios add up to 100% and whether all input custodian addresses are supported. However, there is an issue in its implementation, as it erroneously returns true when orderType == OrderType.REDEEM.

  function verifyRoute(Route calldata route, OrderType orderType) public view override returns (bool) {
    // routes only used to mint
   >if (orderType == OrderType.REDEEM) {
      return true;
    }
    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;
  }

It's essential to note that although this issue exists within a view function and additional checks are present in mint() to safeguard against erroneous transactions, the verifyRoute() function is likely employed in the frontend when users sign transactions, particularly when they are selecting routes. This could raise concerns, particularly given the operation of the mint() process in this specific protocol. The problem could potentially lead to unanticipated errors and unnecessary gas expenditure.

Ethena provides an RFQ of how much USDe can be minted for a stETH amount. If user agrees to price, user signs the EIP712 signature and submits it to Ethena. stETH is traded for newly minted USDe at the rate specified by Ethena's RFQ and user's signature.

Tools Used

Manual Review

Recommended Mitigation Steps

  function verifyRoute(Route calldata route, OrderType orderType) public view override returns (bool) {
    // routes only used to mint
    if (orderType == OrderType.REDEEM) {
-     return true;
+     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;
  }

Assessed type

Context