Ethena Labs - sorrynotsorry's results

Lines of code

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

Vulnerability details

Impact

The intended logic is broken

Proof of Concept

We thought the FULL_RESTRICTED_STAKER_ROLE holder Alice could approve Bob for her assets after she's blacklisted and can bypass the cooldown flow. Accordingly Bob can initiate the cooldown in the name of Alice. But the _beforeTokenTransfer is implemeted to prevent the token transfer.

Contract: StakedUSDe.sol

240:   /**
241:    * @dev Hook that is called before any transfer of tokens. This includes
242:    * minting and burning. Disables transfers from or to of addresses with the FULL_RESTRICTED_STAKER_ROLE role.
243:    */
244: 
245:   function _beforeTokenTransfer(address from, address to, uint256) internal virtual override {
246:     if (hasRole(FULL_RESTRICTED_STAKER_ROLE, from) && to != address(0)) {
247:       revert OperationNotAllowed();
248:     }
249:     if (hasRole(FULL_RESTRICTED_STAKER_ROLE, to)) {
250:       revert OperationNotAllowed();
251:     }
252:   }
253: 

However, below coded POC passes with success. Please insert below into StakedUSDeV2CooldownBlacklistTest or StakedUSDeV2CooldownTest

  function testtobypass(uint256 amount) public {
    amount = bound(amount, 1 ether, 1e40);
    _mintApproveDeposit(alice, amount, false);

    assertEq(stakedUSDe.balanceOf(alice), amount);

    vm.startPrank(owner);
    stakedUSDe.grantRole(FULL_RESTRICTED_STAKER_ROLE, alice);
    vm.stopPrank();

    vm.startPrank(alice);
    usdeToken.approve(bob, amount);
    stakedUSDe.approve(bob, amount);
    vm.stopPrank();

    vm.startPrank(bob);

    stakedUSDe.cooldownAssets(amount, alice);
    assertEq(stakedUSDe.balanceOf(alice), 0);

  }

Tools Used

Manual Review

Recommended Mitigation Steps

We're not sure why the POC passes and submitted this for your perusals.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/StakedUSDeV2.sol#L95C12-L95C26) and [cooldownShares](https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/StakedUSDeV2.sol#L111

Vulnerability details

Impact

The user funds is strictly restricted

Proof of Concept

The cooldownAssets and cooldownShares start a cooldown to claim the converted underlying asset.

The logic of both functions serves the same, one is to select the number of USDe, and one is to select the number of stUSDe.

Contract: StakedUSDeV2.sol

 96:   function cooldownAssets(uint256 assets, address owner) external ensureCooldownOn returns (uint256) {
 97:     if (assets > maxWithdraw(owner)) revert ExcessiveWithdrawAmount();
 98: 
 99:     uint256 shares = previewWithdraw(assets);
100: 
101:     cooldowns[owner].cooldownEnd = uint104(block.timestamp) + cooldownDuration;
102:     cooldowns[owner].underlyingAmount += assets;
103: 
104:     _withdraw(_msgSender(), address(silo), owner, assets, shares);
105: 
106:     return shares;
107:   }

At Line:101, it assigns the timestamp where the user can call unstake and withdraw their assets from the Silo.

Since cooldownAssets and cooldownShares add 90 days cooldownDuration to the current timestamp and reset the cooldownEnd on every call, every unstake call can succeed after 90 days of cooldown calls. So accordingly, it provides only 4 withdrawal opportunities a year. Even the docs mention that the cooldown period will be set to 14 days as below, the current implementation is for the MAX_COOLDOWN_DURATION

Contract: StakedUSDeV2.sol

43:   constructor(IERC20 _asset, address initialRewarder, address owner) StakedUSDe(_asset, initialRewarder, owner) {
44:     silo = new USDeSilo(address(this), address(_asset));
45: --> cooldownDuration = MAX_COOLDOWN_DURATION;
46:   }

While the cooldownDuration has a setter, the actual problem is that the contract doesn't separate the cooldown calls and accepts them as a unique unstake event. The users should be waiting for the last cooldown even if they initiated a cooldown 2 months before.

Suppose the user calls cooldownAssets with the amount of X. After 2 months, the same user calls cooldownAssets with the amount of Y. Now the Silo holds X+Y correcponding USDe. After 1 month, normally, the X should have been completed the cooldown period, but it's now overwritten and will have to wait an other 2 months to be release unless another cooldown call is made.

The system should separate these cooldown calls and assign them nonces/counters to release the funds from the Silo. Else, the system punishes the loyal stakers.

Tools Used

Manual Review

Recommended Mitigation Steps

Implement a counter/nonce system for the cooldownShares/Assets calls. Accordingly, validate the nonce's corresponding cooldownEnd to release the funds from the Silo. This can be accomplished wtih the changes below;

struct UserCooldown {
  uint104 cooldownEnd;
  uint256 underlyingAmount;
+ bool paid;  
}

+ uint256 public coolDownNonce;  // a new public variable used as a counter.

- mapping(address => UserCooldown) public cooldowns;
+ mapping(address => mapping(uint256 coolDownNonce => UserCooldown)) public cooldowns

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

    uint256 shares = previewWithdraw(assets);
+   coolDownNonce++;
-   cooldowns[owner].cooldownEnd = uint104(block.timestamp) + cooldownDuration;

+   UserCooldown storage userCooldown = cooldowns[owner][coolDownNonce];
+   userCooldown.cooldownEnd = uint104(block.timestamp) + cooldownDuration;
+   userCooldown.underlyingAmount = assets;  // Set to assets as each nonce is unique
+   userCooldown.paid = false;  // Initialize the payment status

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

    return shares;
  }

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

-    if (block.timestamp >= userCooldown.cooldownEnd) {
     // Check if cooldown has ended and if the amount has not been paid yet
+    if (block.timestamp >= userCooldown.cooldownEnd && !userCooldown.paid) {
+     userCooldown.paid = true;  // Mark as paid  
      userCooldown.cooldownEnd = 0;
      userCooldown.underlyingAmount = 0;

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

Assessed type

Other