A caller with non-zero allowance from other users can DOS them

[c4-submissions]

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

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #4

[c4-pre-sort]

raymondfam marked the issue as duplicate of #514

[c4-judge]

fatherGoose1 marked the issue as unsatisfactory: Invalid

[c4-judge]

fatherGoose1 changed the severity to QA (Quality Assurance)

[c4-judge]

fatherGoose1 marked the issue as grade-b