Ethena Labs - erebus's results

Lines of code

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

Vulnerability details

Impact

If there are two users and one of them has a non-zero allowance from the other, then it is possible for the first one to delay the unstake eta of the second one indefinitely.

Proof of Concept

When users call StakedUSDeV2, function cooldownAssets and function cooldownShares, the cooldownEnd of the given owner position is extended by cooldownDuration.

StakedUSDeV2, function cooldownAssets

  function cooldownAssets(uint256 assets, address owner) external ensureCooldownOn returns (uint256) {
    if (assets > maxWithdraw(owner)) revert ExcessiveWithdrawAmount();

    uint256 shares = previewWithdraw(assets);

    cooldowns[owner].cooldownEnd = uint104(block.timestamp) + cooldownDuration; // @audit DOS (?)
    cooldowns[owner].underlyingAmount += assets;

    _withdraw(_msgSender(), address(silo), owner, assets, shares);

    return shares;
  }

However, as the owner is user-controlled and the only place that the relation between msg.sender and owner is checked is when _withdraw is called, which calls the upper method in the OZ ERC4626 contract:

    function _withdraw(
        address caller,
        address receiver,
        address owner,
        uint256 assets,
        uint256 shares
    ) internal virtual {
        if (caller != owner) {
            _spendAllowance(owner, caller, shares);
        }

        ...
    }

if the allowance is non-zero, then unstake eta of owner can be delayed indefinitely by msg.sender, as the possible amount that can be withdrawn can be as low as 1 wei. He can set the allowance to 0, it's true, but our malicious user can front-run him and withdraw all of his allowance in batches of 1 wei, leading to a loss of funds (not exactly, as the stake can be claimed once the time passes, but depending on the allowance it may be a few weeks after the expected cooldown eta or in 1000 years).

Recommended Mitigation Steps

I would remove the possibility of delegates between both users, that is, I would make it possible just for user A to cooldown their own assets and not the assets of any other user:

- function cooldownAssets(uint256 assets, address owner) external ensureCooldownOn returns (uint256) {
+ function cooldownAssets(uint256 assets) external ensureCooldownOn returns (uint256) {
-   if (assets > maxWithdraw(owner)) revert ExcessiveWithdrawAmount();
+   if (assets > maxWithdraw(_msgSender())) revert ExcessiveWithdrawAmount();

    uint256 shares = previewWithdraw(assets);

-   cooldowns[owner].cooldownEnd = uint104(block.timestamp) + cooldownDuration;
-   cooldowns[owner].underlyingAmount += assets;

-   _withdraw(_msgSender(), address(silo), owner, assets, shares);
+   cooldowns[_msgSender()].cooldownEnd = uint104(block.timestamp) + cooldownDuration;
+   cooldowns[_msgSender()].underlyingAmount += assets;

+   _withdraw(_msgSender(), address(silo), _msgSender(), assets, shares);

    return shares;
  }

  /// @notice redeem shares into assets and starts a cooldown to claim the converted underlying asset
  /// @param shares shares to redeem
  /// @param owner address to redeem and start cooldown, owner must allowed caller to perform this action
-  function cooldownShares(uint256 shares, address owner) external ensureCooldownOn returns (uint256) {
+  function cooldownShares(uint256 shares) external ensureCooldownOn returns (uint256) {

-   if (shares > maxRedeem(owner)) revert ExcessiveRedeemAmount();
+   if (shares > maxRedeem(_msgSender())) revert ExcessiveRedeemAmount();

    uint256 assets = previewRedeem(shares);

-   cooldowns[owner].cooldownEnd = uint104(block.timestamp) + cooldownDuration;
-   cooldowns[owner].underlyingAmount += assets;

-   _withdraw(_msgSender(), address(silo), owner, assets, shares);
+   cooldowns[_msgSender()].cooldownEnd = uint104(block.timestamp) + cooldownDuration;
+   cooldowns[_msgSender()].underlyingAmount += assets;

+   _withdraw(_msgSender(), address(silo), _msgSender(), assets, shares);

    return assets;
  }

Assessed type

Access Control