Ethena Labs - pontifex's results

Lines of code

https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/StakedUSDe.sol#L190-L194 https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/StakedUSDe.sol#L138-L141

Vulnerability details

Impact

The StakedUSDe contract can be accidentally blocked if the all shares will be redeemed before the VESTING_PERIOD end. The _checkMinShares function will then revert for any eligible deposits. The same result will be in case of asset transferring to the contract while the totalSupply() equals zero.

Proof of Concept

There is the _checkMinShares check at the _deposit and _withdraw functions which should prevent a donation attack.

  function _deposit(address caller, address receiver, uint256 assets, uint256 shares)
    internal
    override
    nonReentrant
    notZero(assets)
    notZero(shares)
  {
    if (hasRole(SOFT_RESTRICTED_STAKER_ROLE, caller) || hasRole(SOFT_RESTRICTED_STAKER_ROLE, receiver)) {
      revert OperationNotAllowed();
    }
    super._deposit(caller, receiver, assets, shares);
    _checkMinShares();
  }

  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/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/StakedUSDe.sol#L203-L238

This check can not prevent case then accidental amount of the assets on the contract which was left after all shares were redeemed. It can easily happen if shares were redeemed before the VESTING_PERIOD end.

  function totalAssets() public view override returns (uint256) {
    return IERC20(asset()).balanceOf(address(this)) - getUnvestedAmount();
  }

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

    if (timeSinceLastDistribution >= VESTING_PERIOD) {
      return 0;
    }

    return ((VESTING_PERIOD - timeSinceLastDistribution) * vestingAmount) / VESTING_PERIOD;
  }

https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/StakedUSDe.sol#L166-L168

If some assets left and at the same time totalSupply is zero the rate between assets and shares become unbalanced and deposit will revert for normal amounts of assets.
Let's look at the ERC4626 contract:

    function _convertToShares(uint256 assets, Math.Rounding rounding) internal view virtual returns (uint256) {
        return assets.mulDiv(totalSupply() + 10 ** _decimalsOffset(), totalAssets() + 1, rounding);
    }

For example there is 1.0 * 10**18 of assets on the contract balance, totalSupply is zero and an user want to deposit $100k: 100 000 * 10**18 * 10**0 / (1.0 * 10**18 + 1) < MIN_SHARES == 10**18 So the _checkMinShares will revert:

  function _checkMinShares() internal view {
    uint256 _totalSupply = totalSupply();
    if (_totalSupply > 0 && _totalSupply < MIN_SHARES) revert MinSharesViolation();
  }

These assets can not be withdrawn even by the DEFAULT_ADMIN_ROLE because of check in the rescueTokens. So the contract will have to be deployed again:

  function rescueTokens(address token, uint256 amount, address to) external onlyRole(DEFAULT_ADMIN_ROLE) {
    if (address(token) == asset()) revert InvalidToken();
    IERC20(token).safeTransfer(to, amount);
  }

Tools Used

Manual review

Recommended Mitigation Steps

It is possible to let DEFAULT_ADMIN_ROLE withdraw assets in case totalSupply equals zero to prevent permanent DoS. Also possible to deposit $1 and as receiver put this contract. Then the _checkMinShares check will be redundant.

Assessed type

DoS

L-1 Redundant call of the getUnvestedAmount function

There is getUnvestedAmount() call at the line #91 which always returns 0 because of the check at the line #90 in the transferInRewards of the StakedUSDe contract: https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/StakedUSDe.sol#L90-L91

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

Consider removing this call.

L-2 Owner can remove all assets addresses but constructor has check at least one asset exist

There is a check at the constructor if at least one asset exists in the EthenaMinting contract. But the DEFAULT_ADMIN_ROLE user can remove all assets addresses

120    if (_assets.length == 0) revert NoAssetsProvided();

258  /// @notice Removes an asset from the supported assets list
259  function removeSupportedAsset(address asset) external onlyRole(DEFAULT_ADMIN_ROLE) {
260    if (!_supportedAssets.remove(asset)) revert InvalidAssetAddress();
261    emit AssetRemoved(asset);
262  }

https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/EthenaMinting.sol#L258-L262 Consider adding the check of the minimal _supportedAssets length.