Ethena Labs - cccz's results

Lines of code

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

Vulnerability details

Impact

_checkMinShares() will require 0 or not less than 1e18 shares in the contract after user deposits and withdraws, this is mainly used to protect against donation attacks.

  /// @notice ensures a small non-zero amount of shares does not remain, exposing to donation attack
  function _checkMinShares() internal view {
    uint256 _totalSupply = totalSupply();
    if (_totalSupply > 0 && _totalSupply < MIN_SHARES) revert MinSharesViolation();
  }

But this introduces another problem, if a malicious user leaves 1 wei shares in the contract, then other users will have to lock 1e18 shares in the contract.

Consider the following scenario. Alice deposits 1e18 USDe and gets 1e18 shares, Bob deposits 1e18 USDe and gets 1e18 shares. Alice withdraws (1e18 - 1) shares, leaving 1 wei share in the contract. After that, Bob can only withdraw 1 wei share, because _checkMinShares() will require that the shares in the contract are not less than 1e18.

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

It is recommended to change _checkMinShares() from requiring the total shares of the contract to requiring the user's shares, which will both prevent donation attacks and solve this problem.

    super._deposit(caller, receiver, assets, shares);
-   _checkMinShares();
+   _checkMinShares(receiver);
...
    super._withdraw(caller, receiver, _owner, assets, shares);
-   _checkMinShares();
+   _checkMinShares(_owner);
...
- function _checkMinShares() internal view {
+ function _checkMinShares(address user) internal view {
-   uint256 _totalSupply = totalSupply();
-   if (_totalSupply > 0 && _totalSupply < MIN_SHARES) revert MinSharesViolation();
+   uint256 _shares = _balances[user];
+   if (_shares > 0 && _shares < MIN_SHARES) revert MinSharesViolation();
  }

Assessed type

Context