c4-submissions commented 11 months ago

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

c4-pre-sort commented 11 months ago

raymondfam marked the issue as sufficient quality report

c4-pre-sort commented 11 months ago

raymondfam marked the issue as duplicate of #32

c4-judge commented 11 months ago

fatherGoose1 changed the severity to 2 (Med Risk)