Ethena Labs - adeolu's results

Lines of code

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

Vulnerability details

Impact

the StakedUSDeV2 contract can enforces coolDown periods for users before they are able to unstake/ take out their funds from the silo contract if coolDown is on. Based on the presence of the modifiers ensureCooldownOff and ensureCooldownOn, it is known that the coolDown state of the StakedUSDeV2 contract can be toggled on or off. In a scenario where coolDown is on (always turned on by default) and alice and bob deposits, two days after alice wants to withdraw/redeem. alice is forced to wait for 90 days before completing withdrawal/getting her tokens from the silo contract because alice must call coolDownAsset()/coolDownShares() fcns respectively. Bob decides to wait an extra day.

On the third day, bob decides to withdraw/redeem. Contract admin also toggles the coolDown off (sets cooldownDuration to 0), meaning there is no longer a coolDown period and all withdrawals should be sent to the users immediately. Bob now calls calls the redeem()/withdraw() fcn to withdraw instantly to his address instead of the silo address since there is no coolDown.

Alice sees Bob has gotten his tokens but Alice cant use the redeem()/withdraw() because her StakedUSDeV2 were already burned and her underlying assets were sent to the silo contract for storage. Alice cannot sucessfully call unstake() because her userCooldown.cooldownEnd value set to ~ 90 days. Now Alice has to unfairly wait out the 90 days even though coolDowns have been turned off and everyone else has unrestricted access to their assets. Alice only crime is trying to withdraw earlier than Bob. This is a loss to Alice as Alice has no StakedUSDE or the underlying asset for the no longer necessary 90 days as if the assset is volatile, it may lose some fiat value during the unfair and no longer necessary wait period.

If cooldown is turned off, it should affect all contract processes and as such, withdrawals should become immediate to users. Tokens previously stored in the USDeSilo contract should become accessible to users when the cooldown state is off. Previous withdrawal requests that had a cooldown should no longer be restricted by a coolDown period since coolDown now off and the coolDownDuration of the contract is now 0.

Proof of Concept

• since StakedUSDeV2 is ERC4626, user calls deposit() to deposit the underlying token asset and get minted shares that signify the user's position size in the vault.

• the coolDown duration is set to 90 days on deployment of the StakedUSDeV2 contract, meaning coolDown is toggled on by default.

• user cannot redeem/withdraw his funds via withdraw() and redeem() because coolDown is on. Both functions have the ensureCooldownOff modifier which reverts if the coolDownDuration value is not 0.

• user tried to exit position, to withdraw when coolDown is on, user must call coolDownAsset()/coolDownShares(). This will cause :

  • for the user's underlyingAmount and cooldownEnd timestamp values to be set in the mapping cooldowns. cooldownEnd timestamp values is set to 90 days from the present.
  • for the user's StakedUSDeV2 ERC4626 position shares to be burnt and the positon underlying asset value to be sent to the USDeSilo contract.

  /// @notice redeem assets and starts a cooldown to claim the converted underlying asset
  /// @param assets assets to redeem
  /// @param owner address to redeem and start cooldown, owner must allowed caller to perform this action
  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;
   cooldowns[owner].underlyingAmount += assets;
  
   _withdraw(_msgSender(), address(silo), owner, 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) {
   if (shares > maxRedeem(owner)) revert ExcessiveRedeemAmount();
  
   uint256 assets = previewRedeem(shares);
  
   cooldowns[owner].cooldownEnd = uint104(block.timestamp) + cooldownDuration;
   cooldowns[owner].underlyingAmount += assets;
  
   _withdraw(_msgSender(), address(silo), owner, assets, shares);
  
   return assets;
  }
  

• user can only use unstake() to get the assets from the silo contract. unstake enforces that the block.timestamp (present time) is more than the 90 days cooldown period set during the execution of cooldownAssets() and cooldownShares() and reverts if 90 days time has not been reached yet

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

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

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

• if contract admin decides to turn the coolDown period off, by setting the cooldownDuration to 0 via setCooldownDuration(), user who has his assets under the coolDown in the silo still wont be able to withdraw via unstake() because the logic in unstake() doesnt allow for the user's coolDownEnd value which was set under the previous coolDown duration state to be bypassed as coolDowns are now turned off and the StakedUSDeV2 behavior is supposed to be changed to follow ERC4626 standard and allow for the user assets to get to them immediately with no coolDown period still enforced on withdrawals as seen in the comment here.

• user who initiated withdrawal when the coolDown was toggled on will still continue to be restricted from his tokens/funds even after coolDown is toggled off. This should not be because restrictions are removed, all previous pending withdrawals should be allowed to be completed without wait for 90 days since the coolDownDuration of the contract is now 0.

Here's a coded POC to illustrate this issue :

run with forge test --mt test_UnstakeUnallowedAfterCooldownIsTurnedOff.

// SPDX-License-Identifier: MIT
pragma solidity >=0.8;

/* solhint-disable private-vars-leading-underscore  */
/* solhint-disable var-name-mixedcase  */
/* solhint-disable func-name-mixedcase  */

import "forge-std/console.sol";
import "forge-std/Test.sol";
import {SigUtils} from "forge-std/SigUtils.sol";

import "../../../contracts/USDe.sol";
import "../../../contracts/StakedUSDeV2.sol";
import "../../../contracts/interfaces/IUSDe.sol";
import "../../../contracts/interfaces/IERC20Events.sol";

contract StakedUSDeV2CooldownTest is Test, IERC20Events {
  USDe public usdeToken;
  StakedUSDeV2 public stakedUSDeV2;
  SigUtils public sigUtilsUSDe;
  SigUtils public sigUtilsStakedUSDe;
  uint256 public _amount = 100 ether;

  address public owner;
  address public alice;
  address public bob;
  address public greg;

  bytes32 SOFT_RESTRICTED_STAKER_ROLE;
  bytes32 FULL_RESTRICTED_STAKER_ROLE;
  bytes32 DEFAULT_ADMIN_ROLE;
  bytes32 BLACKLIST_MANAGER_ROLE;
  bytes32 REWARDER_ROLE;

  event Deposit(address indexed caller, address indexed owner, uint256 assets, uint256 shares);
  event Withdraw(
    address indexed caller, address indexed receiver, address indexed owner, uint256 assets, uint256 shares
  );
  event LockedAmountRedistributed(address indexed from, address indexed to, uint256 amountToDistribute);

  function setUp() public virtual {
    usdeToken = new USDe(address(this));

    alice = makeAddr("alice");
    bob = makeAddr("bob");
    greg = makeAddr("greg");
    owner = makeAddr("owner");

    usdeToken.setMinter(address(this));

    vm.startPrank(owner);
    stakedUSDeV2 = new StakedUSDeV2(IUSDe(address(usdeToken)), makeAddr('rewarder'), owner);
    vm.stopPrank();

    FULL_RESTRICTED_STAKER_ROLE = keccak256("FULL_RESTRICTED_STAKER_ROLE");
    SOFT_RESTRICTED_STAKER_ROLE = keccak256("SOFT_RESTRICTED_STAKER_ROLE");
    DEFAULT_ADMIN_ROLE = 0x00;
    BLACKLIST_MANAGER_ROLE = keccak256("BLACKLIST_MANAGER_ROLE");
    REWARDER_ROLE = keccak256("REWARDER_ROLE");
  }

  function test_UnstakeUnallowedAfterCooldownIsTurnedOff () public {
    address staker = address(20);
    uint usdeTokenAmountToMint = 10000*1e18;

    usdeToken.mint(staker, usdeTokenAmountToMint);

    //at the deposit coolDownDuration is set to 90 days 
    assert(stakedUSDeV2.cooldownDuration() == 90 days);

    vm.startPrank(staker);
    usdeToken.approve(address(stakedUSDeV2), usdeTokenAmountToMint);
    
    stakedUSDeV2.deposit(usdeTokenAmountToMint / 2, staker);

    vm.roll(block.number + 1);
    uint assets  = stakedUSDeV2.maxWithdraw(staker);
    stakedUSDeV2.cooldownAssets(assets , staker);
    
    vm.stopPrank();

    //assert that cooldown for the staker is now set to 90 days from now 
    ( uint104 cooldownEnd, ) = stakedUSDeV2.cooldowns(staker);
    assert(cooldownEnd == uint104( block.timestamp + 90 days));

    vm.prank(owner);
    //toggle coolDown off in the contract 
    stakedUSDeV2.setCooldownDuration(0);

    //now try to unstake, 
    /** since cooldown duration is now 0 and contract is cooldown state is turned off. 
    it should allow unstake immediately but instead it will revert **/
    vm.expectRevert(IStakedUSDeCooldown.InvalidCooldown.selector);
    vm.prank(staker);
    stakedUSDeV2.unstake(staker);
  }
}

Tools Used

manual review, foundry

Recommended Mitigation Steps

modify the code in unstake() fcn to allow for withdrawals from the silo contract when the contract's coolDownDuration has become 0.

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

StakedUSDeV2 is erc4262 and as thus it can allow for owners of shares to approve other addresses to spend/withdraw on their behalf. An approved address or owner address can can call the functions cooldownShares() and cooldownAssets() on behalf of the owner to begin the cooldown process.

In a case whereby alice is owner of 100 tokens. alice approves 40 tokens for bob to spend. Alice then calls cooldownShares() or cooldownAssets() to cooldown 60 of her tokens, cooldowns[owner].coolDownEnd is set in storage to 90 DAYS and alice is left with 40 tokens. In 89 days, Bob realizes he's got approval from Alice and he calls cooldownShares() or cooldownAssets() on behalf of alice to cool down 40 token shares. Bob will reset the cooldowns[owner].coolDownEnd value to be +90 days on the 89th day. Alice calls unstake() on the 90th day but alice will fail and alice will not be able to to collect the 60 tokens she had cooled down her self 90 days ago. She will have to wait for 89 more days because of the action of Bol on the 89th day. This means alice had her cooldown period for her withdrawal of 60 tokens exetended for more than the MAX_COOLDOWN_DURATION (90 days).

Withdrawal actions by alice should not be affected by bob since the actions were done at different times.

Proof of Concept

• in the snippet below, the functions cooldownShares() or cooldownAssets() call _withdraw which is an erc4262 function that allows for the caller to be the owner or for the caller to have been approved to spend an amount of tokens by the owner.

  /// @notice redeem assets and starts a cooldown to claim the converted underlying asset
  /// @param assets assets to redeem
  /// @param owner address to redeem and start cooldown, owner must allowed caller to perform this action
  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;
    cooldowns[owner].underlyingAmount += assets;

    _withdraw(_msgSender(), address(silo), owner, 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) {
    if (shares > maxRedeem(owner)) revert ExcessiveRedeemAmount();

    uint256 assets = previewRedeem(shares);

    cooldowns[owner].cooldownEnd = uint104(block.timestamp) + cooldownDuration;
    cooldowns[owner].underlyingAmount += assets;

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

    return assets;
  }

• alice approves Bob for 40 tokens.

• Bob doesnt go ahead to call cooldownShares() or cooldownAssets() immediately,

• Alice tries to coolDown 60 tokens from her balance and calls cooldownShares() or cooldownAssets(). Call is sucessful and cooldowns[owner].cooldownEnd is updated to be 90 days from now. Note that by default, cooldownDuration is set to be 90 days in constructor. so cooldownDuration is 90 days except changed.

• in 89 days Bob decides to call cooldownShares() or cooldownAssets() to spend the 40 tokens Alice gave him approval for. Call to cooldownShares() or cooldownAssets() is sucessful and cooldowns[owner].cooldownEnd is updated again to be current timestamp + 90 days.

• time gets to the 90th day and Alice wants to take her cooled down assets since she was told it will take 90 days to cool down. Alice calls unstake() but cant withdraw from silo to her address because Bob's action on the 89th day has affected cooldowns[owner].cooldownEnd value she set when she decided to coolDown 60 tokens. Now alice funds are unfairly stuck for 89 more days because the cooldowns[owner] mapping tracks values per owner address only and not by owner address and per withdrawal action. Unstake() reverts since block.timestamp is not greater than or equal the cooldowns[owner].cooldownEnd

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

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

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

here is a coded POC illustrating this issue :

run with forge test --mt test_UnstakeUnallowed

// SPDX-License-Identifier: MIT
pragma solidity >=0.8;

/* solhint-disable private-vars-leading-underscore  */
/* solhint-disable var-name-mixedcase  */
/* solhint-disable func-name-mixedcase  */

import "forge-std/console.sol";
import "forge-std/Test.sol";
import {SigUtils} from "forge-std/SigUtils.sol";

import "../../../contracts/USDe.sol";
import "../../../contracts/StakedUSDeV2.sol";
import "../../../contracts/interfaces/IUSDe.sol";
import "../../../contracts/interfaces/IERC20Events.sol";

contract StakedUSDeV2CooldownTest is Test, IERC20Events {
  USDe public usdeToken;
  StakedUSDeV2 public stakedUSDeV2;
  SigUtils public sigUtilsUSDe;
  SigUtils public sigUtilsStakedUSDe;
  uint256 public _amount = 100 ether;

  address public owner;
  address public alice;
  address public bob;
  address public greg;

  bytes32 SOFT_RESTRICTED_STAKER_ROLE;
  bytes32 FULL_RESTRICTED_STAKER_ROLE;
  bytes32 DEFAULT_ADMIN_ROLE;
  bytes32 BLACKLIST_MANAGER_ROLE;
  bytes32 REWARDER_ROLE;

  event Deposit(address indexed caller, address indexed owner, uint256 assets, uint256 shares);
  event Withdraw(
    address indexed caller, address indexed receiver, address indexed owner, uint256 assets, uint256 shares
  );
  event LockedAmountRedistributed(address indexed from, address indexed to, uint256 amountToDistribute);

  function setUp() public virtual {
    usdeToken = new USDe(address(this));

    alice = makeAddr("alice");
    bob = makeAddr("bob");
    greg = makeAddr("greg");
    owner = makeAddr("owner");

    usdeToken.setMinter(address(this));

    vm.startPrank(owner);
    stakedUSDeV2 = new StakedUSDeV2(IUSDe(address(usdeToken)), makeAddr('rewarder'), owner);
    vm.stopPrank();

    FULL_RESTRICTED_STAKER_ROLE = keccak256("FULL_RESTRICTED_STAKER_ROLE");
    SOFT_RESTRICTED_STAKER_ROLE = keccak256("SOFT_RESTRICTED_STAKER_ROLE");
    DEFAULT_ADMIN_ROLE = 0x00;
    BLACKLIST_MANAGER_ROLE = keccak256("BLACKLIST_MANAGER_ROLE");
    REWARDER_ROLE = keccak256("REWARDER_ROLE");
  }

  function test_UnstakeUnallowed () public {
    address staker = address(20);
    uint usdeTokenAmountToMint = 10000*1e18;

    usdeToken.mint(staker, usdeTokenAmountToMint);

    //at the deposit coolDownDuration is set to 90 days 
    assertEq(stakedUSDeV2.cooldownDuration(), 90 days);

    vm.startPrank(staker);
    //make first deposit
    usdeToken.approve(address(stakedUSDeV2), usdeTokenAmountToMint);
    stakedUSDeV2.deposit(usdeTokenAmountToMint, staker);


    //enter cool down for withdrawal of first deposit  in next block 
    vm.roll(block.number + 1);
    uint assetsForFirstWithdrawal  = stakedUSDeV2.maxWithdraw(staker);
    stakedUSDeV2.cooldownAssets(assetsForFirstWithdrawal / 2 , staker);
    (uint coolDownEndofFirstWithdrawal, ) = stakedUSDeV2.cooldowns(staker);


    //approve another address to spend some of staker stakedUSDeV2 tokens 
    address otherCaller = address(25);
    stakedUSDeV2.approve(otherCaller, assetsForFirstWithdrawal / 2 );
    vm.stopPrank();

    //second call to cooldownAssets is done by the approved otherCaller address  at different block and different timestmap
    //otherCaller initiates another withdrawal process for the staker. 
    vm.roll(block.number + 10_000);
    vm.warp(block.timestamp + 89 days); // timestamp is very close to coolDown end of first withdrawal made by the staker
    vm.startPrank(otherCaller);
    stakedUSDeV2.cooldownAssets(assetsForFirstWithdrawal / 2 , staker);
    (uint coolDownEndofSecondWithdrawal, ) = stakedUSDeV2.cooldowns(staker);
    vm.stopPrank();

    /** now on day 91 after the seond deposit, staker tries to unstake his first deposit
    since he had put in a withdrawal request action +90 days ago **/
    vm.roll(block.number + 2_000);
    vm.warp(block.timestamp + 2 days);
    vm.expectRevert(IStakedUSDeCooldown.InvalidCooldown.selector);
    //we expect a revert because the otherCaller had indelibrately increased the cooldownEnd time for the stakers first withdrawal
    //when he tried to cooldown the aproved amount of tokens for staker
    vm.startPrank(staker);
    stakedUSDeV2.unstake(staker);
    
    vm.stopPrank();


    //assert that the coolDown for staker had indeed increased since first withdrawal initiated by call to cooldownAssets()
    //thus preventing the first withdrawal from being removed from silo even though that amount of tokens had spent +90 days there.  
    assertGt(coolDownEndofSecondWithdrawal, coolDownEndofFirstWithdrawal);
  }
}

Tools Used

manual review, foundry

Recommended Mitigation Steps

change cooldowns to become a double mapping of owner and id of withdrawal action. This way every withdrawal action will be unique and will have different coolDownEnd times. And actions by alice and bob wont have the same coolDown times since the actions were done at different times.

Assessed type

Error