Last legitimate staker to withdraw may not be able to withdraw all of their staked USDe

[c4-submissions]

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

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #71

[c4-judge]

fatherGoose1 marked the issue as satisfactory

[c4-judge]

fatherGoose1 changed the severity to QA (Quality Assurance)

[c4-judge]

fatherGoose1 marked the issue as grade-b