Ethena Labs - squeaky_cactus's results

Lines of code

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

Vulnerability details

Impact

A requirement is stated that a user with the SOFT_RESTRICTED_STAKER_ROLE is not allowed to withdraw USDe for stUSDe.

The code does not satisfy that condition, when a holder has the SOFT_RESTRICTED_STAKER_ROLE, they can exchange their stUSDe for USDe using StakedUSDeV2.

Description

The Ethena readme has the following decription of legal requirements for the Soft Restricted Staker Role: https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/README.md?plain=1#L98

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.

In summary, legal requires are that a SOFT_RESTRICTED_STAKER_ROLE:

• MUST NOT deposit USDe to get stUSDe
• MUST NOT withdraw USDe for USDe
• MAY earn yield by trading stUSDe on the open market

As StakedUSDeV2 is a ERC4626, the stUSDe is a share on the underlying USDe asset. There are two distinct entrypoints for a user to exchange their share for their claim on the underlying the asset, withdraw and redeem. Each cater for a different input (withdraw being by asset, redeem being by share), however both invoked the same internal _withdraw function, hence both entrypoints are affected.

There are two cases where a user with SOFT_RESTRICTED_STAKER_ROLE may have acquired stUSDe:

• Brought stUSDe on the open market
• Deposited USDe in StakedUSDeV2 before being granted the SOFT_RESTRICTED_STAKER_ROLE

In both cases the user can call either withdraw their holding by calling withdraw or redeem (when cooldown is off), or unstake (if cooldown is on) and successfully exchange their stUSDe for USDe.

Proof of Concept

The following two tests demonstrate the use case of a user staking, then being granted the SOFT_RESTRICTED_STAKER_ROLE, then exchanging their stUSDe for USDe (first using redeem function, the second using withdrawm).

The use case for acquiring on the open market, only requiring a different setup, however the exchange behaviour is identical and the cooldown enabled cooldownAssets and cooldownShares function still use the same _withdraw as redeem and withdraw, which leads to the same outcome.

(Place code into StakedUSDe.t.sol and run with forge test) https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/test/foundry/staking/StakedUSDe.t.sol

  bytes32 public constant SOFT_RESTRICTED_STAKER_ROLE = keccak256("SOFT_RESTRICTED_STAKER_ROLE");
  bytes32 private constant BLACKLIST_MANAGER_ROLE = keccak256("BLACKLIST_MANAGER_ROLE");

  function test_redeem_while_soft_restricted() public {
    // Set up Bob with 100 stUSDe
    uint256 initialAmount = 100 ether;
    _mintApproveDeposit(bob, initialAmount);
    uint256 stakeOfBob = stakedUSDe.balanceOf(bob);

    // Alice becomes a blacklist manager
    vm.prank(owner);
    stakedUSDe.grantRole(BLACKLIST_MANAGER_ROLE, alice);

    // Blacklist Bob with the SOFT_RESTRICTED_STAKER_ROLE
    vm.prank(alice);
    stakedUSDe.addToBlacklist(bob, false);

    // Assert that Bob has staked and is now has the soft restricted role
    assertEq(usdeToken.balanceOf(bob), 0);
    assertEq(stakedUSDe.totalSupply(), stakeOfBob);
    assertEq(stakedUSDe.totalAssets(), initialAmount);
    assertTrue(stakedUSDe.hasRole(SOFT_RESTRICTED_STAKER_ROLE, bob));

    // Rewards to StakeUSDe and vest
    uint256 rewardAmount = 50 ether;
    _transferRewards(rewardAmount, rewardAmount);
    vm.warp(block.timestamp + 8 hours);

    // Assert that only the total assets have increased after vesting
    assertEq(usdeToken.balanceOf(bob), 0);
    assertEq(stakedUSDe.totalSupply(), stakeOfBob);
    assertEq(stakedUSDe.totalAssets(), initialAmount + rewardAmount);
    assertTrue(stakedUSDe.hasRole(SOFT_RESTRICTED_STAKER_ROLE, bob));

    // Bob withdraws his stUSDe for USDe
    vm.prank(bob);
    stakedUSDe.redeem(stakeOfBob, bob, bob);

    // End state being while being soft restricted Bob redeemed USDe with rewards
    assertApproxEqAbs(usdeToken.balanceOf(bob), initialAmount + rewardAmount, 2);
    assertApproxEqAbs(stakedUSDe.totalAssets(), 0, 2);
    assertTrue(stakedUSDe.hasRole(SOFT_RESTRICTED_STAKER_ROLE, bob));
  }

  function test_withdraw_while_soft_restricted() public {
    // Set up Bob with 100 stUSDe
    uint256 initialAmount = 100 ether;
    _mintApproveDeposit(bob, initialAmount);
    uint256 stakeOfBob = stakedUSDe.balanceOf(bob);

    // Alice becomes a blacklist manager
    vm.prank(owner);
    stakedUSDe.grantRole(BLACKLIST_MANAGER_ROLE, alice);

    // Blacklist Bob with the SOFT_RESTRICTED_STAKER_ROLE
    vm.prank(alice);
    stakedUSDe.addToBlacklist(bob, false);

    // Assert that Bob has staked and is now has the soft restricted role
    assertEq(usdeToken.balanceOf(bob), 0);
    assertEq(stakedUSDe.totalSupply(), stakeOfBob);
    assertEq(stakedUSDe.totalAssets(), initialAmount);
    assertTrue(stakedUSDe.hasRole(SOFT_RESTRICTED_STAKER_ROLE, bob));

    // Rewards to StakeUSDe and vest
    uint256 rewardAmount = 50 ether;
    _transferRewards(rewardAmount, rewardAmount);
    vm.warp(block.timestamp + 8 hours);

    // Assert that only the total assets have increased after vesting
    assertEq(usdeToken.balanceOf(bob), 0);
    assertEq(stakedUSDe.totalSupply(), stakeOfBob);
    assertEq(stakedUSDe.totalAssets(), initialAmount + rewardAmount);
    assertTrue(stakedUSDe.hasRole(SOFT_RESTRICTED_STAKER_ROLE, bob));

    // Bob withdraws his stUSDe for USDe (-1 as dust is lost in asset to share rounding in ERC4626)
    vm.prank(bob);
    stakedUSDe.withdraw(initialAmount + rewardAmount - 1, bob, bob);

    // End state being while being soft restricted Bob redeemed USDe with rewards
    assertApproxEqAbs(usdeToken.balanceOf(bob), initialAmount + rewardAmount, 2);
    assertApproxEqAbs(stakedUSDe.totalAssets(), 0, 2);
    assertTrue(stakedUSDe.hasRole(SOFT_RESTRICTED_STAKER_ROLE, bob));
  }

Tools Used

Manual review, Foundry test

Recommended Mitigation Steps

With the function overriding present, to prevent the SOFT_RESTRICTED_STAKER_ROLE from being able to exchange their stUSDs for USDe, make the following change in StakedUSDe

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

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

Assessed type

Access Control

QA Report

Number of each QA issue type found.

Low Risk

L-1 USDe should use two-step assignment for ownership

https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/USDe.sol#L18-L21

  constructor(address admin) ERC20("USDe", "USDe") ERC20Permit("USDe") {
    if (admin == address(0)) revert ZeroAddressException();
    _transferOwnership(admin);
  }

The constructor of USDe is directly assigning ownership to the input parameter address admin, bypassing the two-step functionality inherited from Ownable2Step.

Direct ownership assigment has input risk, where if the input was incorrect, with there being no verification ownership would be lost. It's safer to implement a two-step process where the new address is first proposed, then later confirmed, allowing for more control and the chance to catch errors or malicious activity.

Recommended Mitigation

Use Ownable2Step.transferOwnership that requires the admin call acceptOwnership before ownership is actually transferred from msg.sender.

https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/USDe.sol#L18-L21

  constructor(address admin) ERC20("USDe", "USDe") ERC20Permit("USDe") {
    if (admin == address(0)) revert ZeroAddressException();
-    _transferOwnership(admin);
+   transferOwnership(admin);
  }

Non-Critical

NC-1 NatSpec - Incorrect access control for transferInRewards

The NatSpec notice for the extenral function StakedUSDe.transferInRewards incorrectly states the access control as being the owner, where it is actually REWARDER_ROLE.

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

  /**
   * @notice Allows the owner to transfer rewards from the controller contract into this contract.
   * @param amount The amount of rewards to transfer.
   */
  function transferInRewards(uint256 amount) external nonReentrant onlyRole(REWARDER_ROLE) notZero(amount) {

Although an external function, the reduction in access control only affects the owner, which will be an account under the Ethena team's control (i.e. users would not call the function via Etherscan an expect correct execution, as they would not be the owner).

Proof Of Concept

Add test as a member of the Staking ACL tests and run with forge test --match-test test_access_control_transferInRewards

bytes32 public constant REWARDER_ROLE = keccak256("REWARDER_ROLE");

  function test_access_control_transferInRewards() public {
    assertTrue(stakedUSDe.hasRole(DEFAULT_ADMIN_ROLE, owner));
    assertFalse(stakedUSDe.hasRole(REWARDER_ROLE, owner));

    vm.expectRevert(
      "AccessControl: account 0xe05fcc23807536bee418f142d19fa0d21bb0cff7 is missing role 0xbeec13769b5f410b0584f69811bfd923818456d5edcf426b0e31cf90eed7a3f6"
    );
    vm.prank(owner);
    stakedUSDe.transferInRewards(0 ether);
  }

Recommended Mitigation

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

-   * @notice Allows the owner to transfer rewards from the controller contract into this contract.
+   * @notice Allows the rewarder to transfer rewards from the controller contract into this contract.

NC-2 NatSpec - Incorrect access control for addToBlacklist

The NatSpec notice for the external function StakedUSDe.addToBlacklist incorrectly states the access control as allowing DEFAULT_ADMIN_ROLE, where it only allows the blacklist managers.

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

  /**
   * @notice Allows the owner (DEFAULT_ADMIN_ROLE) and blacklist managers to blacklist addresses.
   * @param target The address to blacklist.
   * @param isFullBlacklisting Soft or full blacklisting level.
   */
  function addToBlacklist(address target, bool isFullBlacklisting)
    external
    onlyRole(BLACKLIST_MANAGER_ROLE)
    notOwner(target)

Although an external function, the reduction in access control only affects the owner, which will be an account under the Ethena team's control (i.e. users would not call the function via Etherscan an expect correct execution, as they would not be the owner).

Proof Of Concept

Add test as a member of the Staking ACL tests and run with forge test --match-test test_access_control_addToBlacklist

  function test_access_control_addToBlacklist() public {
    assertTrue(stakedUSDe.hasRole(DEFAULT_ADMIN_ROLE, owner));
    assertFalse(stakedUSDe.hasRole(BLACKLIST_MANAGER_ROLE, owner));

    vm.expectRevert(
      "AccessControl: account 0xe05fcc23807536bee418f142d19fa0d21bb0cff7 is missing role 0xf988e4fb62b8e14f4820fed03192306ddf4d7dbfa215595ba1c6ba4b76b369ee"
    );
    vm.prank(owner);
    stakedUSDe.addToBlacklist(alice, true);
  }

Recommended Mitigation

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

-   * @notice Allows the owner (DEFAULT_ADMIN_ROLE) and blacklist managers to blacklist addresses.
+   * @notice Allows blacklist managers to blacklist addresses.

NC-3 NatSpec - Incorrect access control for removeFromBlacklist

The NatSpec notice for the external function StakedUSDe.removeFromBlacklist incorrectly states the access control as allowing DEFAULT_ADMIN_ROLE, where it only allows the blacklist managers.

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

  /**
   * @notice Allows the owner (DEFAULT_ADMIN_ROLE) and blacklist managers to un-blacklist addresses.
   * @param target The address to un-blacklist.
   * @param isFullBlacklisting Soft or full blacklisting level.
   */
  function removeFromBlacklist(address target, bool isFullBlacklisting)
    external
    onlyRole(BLACKLIST_MANAGER_ROLE)
    notOwner(target)

Proof Of Concept

Add test as a member of the Staking ACL tests and run with forge test --match-test test_access_control_removeFromBlacklist

  function test_access_control_removeFromBlacklist() public {
    assertTrue(stakedUSDe.hasRole(DEFAULT_ADMIN_ROLE, owner));
    assertFalse(stakedUSDe.hasRole(BLACKLIST_MANAGER_ROLE, owner));

    vm.expectRevert(
      "AccessControl: account 0xe05fcc23807536bee418f142d19fa0d21bb0cff7 is missing role 0xf988e4fb62b8e14f4820fed03192306ddf4d7dbfa215595ba1c6ba4b76b369ee"
    );
    vm.prank(owner);
    stakedUSDe.removeFromBlacklist(alice, true);
  }

Recommended Mitigation

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

-   * @notice Allows the owner (DEFAULT_ADMIN_ROLE) and blacklist managers to un-blacklist addresses.
+   * @notice Allows blacklist managers to un-blacklist addresses.

NC-4 Unused SafeERC20 library

In USDeSilo.sol the SafeERC20 library is imported and used for the IERC20 reference it holds, but no functions of that library are being used.

The library and it's import can be safely removed, simplifying the code and reducing the size (and gas deployment cost) for the contract.

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

-   import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";

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

-  using SafeERC20 for IERC20;