Ethena Labs - dirk_y's results

Lines of code

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

Vulnerability details

Impact

NOTE: I'm aware that similar issues have been reported before by Quantstamp and Pashov, but I feel like the current behaviour doesn't align with your previous acknowledgements and their reports don't directly detail the real issue here. The intended behaviour is that soft restricted users should still be able to buy and sell stUSDe on the open market in order to realise yield. However the current behaviour also allows these soft blacklisted users to directly earn yield by redeeming stUSDe for USDe. I believe that this doesn't properly cover your legal requirements.

As stated in your documentation:

Due to legal requirements, there's a SOFT_RESTRICTED_STAKER_ROLE and FULL_RESTRICTED_STAKER_ROLE. The former is for addresses based in countries we are not allowed to provide yield to, for example USA. Addresses under this category will be soft restricted. They cannot deposit USDe to get stUSDe or withdraw stUSDe for USDe. However they can participate in earning yield by buying and selling stUSDe on the open market.

However the current implementation allows users that are soft restricted to withdraw stUSDe for USDe, thereby breaking your legal requirements.

Proof of Concept

When a user wants to unstake their stUSDe for USDe they can call the relevant method in the StakedUSDeV2 contract, depending on whether or not the cooldown is non-zero. All of these methods make an underlying call to _withdraw:

  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();
  }

As you can see, this method prevents a user that is fully blacklisted from withdrawing stUSDe for USDe, however it does not prevent users that are soft blacklisted from withdrawing (like it should). Although users that are soft blacklisted cannot directly stake USDe to receive stUSDe, they can obtain stUSDe on the open market or be sent stUSDe by another user.

There are already some tests in the test suite that show how soft blacklisted users can redeem their stUSDe directly for USDe. They can be executed with forge test --match-test test_softBlacklist_withdraw_pass.

Tools Used

Manual review

Recommended Mitigation Steps

In order to be fully covered from a legal standpoint I would suggest preventing soft restricted users from redeeming stUSDe to USDe. This way the soft restricted users cannot directly earn yield from the staking contract and can now only do so indirectly via trading on the open market (as intended). Below is a suggested diff:

diff --git a/contracts/StakedUSDe.sol b/contracts/StakedUSDe.sol
index 0a56a7d..c54e3b9 100644
--- a/contracts/StakedUSDe.sol
+++ b/contracts/StakedUSDe.sol
@@ -229,7 +229,7 @@ contract StakedUSDe is SingleAdminAccessControl, ReentrancyGuard, ERC20Permit, E
     notZero(assets)
     notZero(shares)
   {
-    if (hasRole(FULL_RESTRICTED_STAKER_ROLE, caller) || hasRole(FULL_RESTRICTED_STAKER_ROLE, receiver)) {
+    if (hasRole(FULL_RESTRICTED_STAKER_ROLE, caller) || hasRole(FULL_RESTRICTED_STAKER_ROLE, receiver) || hasRole(SOFT_RESTRICTED_STAKER_ROLE, caller) || hasRole(SOFT_RESTRICTED_STAKER_ROLE, receiver)) {
       revert OperationNotAllowed();
     }
 

Assessed type

Access Control

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

When a user stakes or unstakes USDe from the staking contract, there is a check to ensure that there is not a small non-zero amount of shares outstanding, since this exposes the contract to a share inflation type attack.

However the issue with the current implementation is that it doesn't enforce a minimum deposit amount. Therefore, a malicious user can stake a dust amount of USDe provided they aren't the first depositor. This then impacts the last legitimate withdrawer from the staking contract which is discussed more below in the "PoC" section.

The actual monetary impact of this bug is small, however a user that legitimately stakes in a staking contract will always expect to be able to withdraw their stake at any time. This expectation isn't met.

Proof of Concept

The USDe staking contract protects against share inflation attacks by enforcing that total shares after a deposit/withdrawal are more than a certain amount:

  /// @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();
  }

This internal method is called at the end of the _deposit and _withdraw methods to ensure that after shares are minted/burnt, the total number of outstanding shares is more than MIN_SHARES to prevent a share inflation attack.

Share inflation attacks are usually performed by the first depositor and are therefore often known as "first depositor" attacks. The current implementation does prevent the "first depositor" type attack, however it doesn't prevent a malicious user from impacting the last legitimate withdrawal.

Below is a diff to the existing test suite that can be executed with forge test --match-test testLastWithdrawerCantUnstake. The test demonstrates how a malicious user can stake a small amount of USDe (provided they are not the first to stake), which in turn prevents the last legitimate staker from unstaking their full staked balance since they leave less than MIN_SHARES shares:

diff --git a/test/foundry/staking/StakedUSDe.t.sol b/test/foundry/staking/StakedUSDe.t.sol
index 8f70283..0753365 100644
--- a/test/foundry/staking/StakedUSDe.t.sol
+++ b/test/foundry/staking/StakedUSDe.t.sol
@@ -128,6 +128,17 @@ contract StakedUSDeTest is Test, IERC20Events {
     stakedUSDe.redeem(0.5 ether, alice, alice);
   }
 
+  function testLastWithdrawerCantUnstake() public {
+    _mintApproveDeposit(alice, 100 ether);
+    _mintApproveDeposit(bob, 0.9 ether);
+
+    uint256 aliceBalance = stakedUSDe.balanceOf(alice);
+
+    vm.startPrank(alice);
+    vm.expectRevert(IStakedUSDe.MinSharesViolation.selector);
+    stakedUSDe.redeem(aliceBalance, alice, alice);
+  }
+
   function testCannotStakeWithoutApproval() public {
     uint256 amount = 100 ether;
     usdeToken.mint(alice, amount);

Tools Used

Manual review + foundry

Recommended Mitigation Steps

I would suggest also enforcing a minimum stake amount in the _deposit method. This way a malicious user can't stake a dust amount of USDe to prevent the last withdrawer from withdrawing their whole stake and unexpectedly having the contract call revert.

Assessed type

Invalid Validation