Ethena Labs - tnquanghuy0512's results

Lines of code

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

Vulnerability details

Impact

In the docs: FULL_RESTRICTED_STAKER_ROLE - address with this role can't burn his stUSDe tokens to unstake his USDe tokens, neither to transfer stUSDe tokens

In reality: Full restricted user can still bypass unstaking USDe by approving permission to their alternative account

The reason causing this problem is because _withdraw() only check if caller or receiver are full restricted, but not check owner. Caller is the user call to the function withdraw() (msg.sender), and owner is the user who will actual burn their stUSD

Proof of Concept

1. Wallet A got full restricted by the protocol and have 1000 stUSDe in their wallet
2. Wallet A approve to wallet B to gain permission to use wallet A's stUSDe
3. Wallet B now can call to stUSDe.redeem(1000, B, A), burning wallet A's stUSDe and receiving USDe successfully

This scenario holds true whether cooldown toggle is on or off

Tools Used

Manual Review

Recommended Mitigation Steps

StakedUSDe._withdraw():

  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, _owner) || hasRole(FULL_RESTRICTED_STAKER_ROLE, receiver) {

      revert OperationNotAllowed();
    }

    super._withdraw(caller, receiver, _owner, assets, shares);
    _checkMinShares();
  }

Assessed type

ERC4626

Lines of code

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

Vulnerability details

Impact

As the docs said, FULL_RESTRICTED_STAKER_ROLE is for sanction/stolen funds, or if we get a request from law enforcement to freeze funds

When the cooldown toggle is on, users need to do 3 steps to withdraw USDe to their wallet:

• Step 1: Execute cooldownAssets() or cooldownShares() to withdraw their USDe to the silo
• Step 2: Wait for the cooldown, can be up to 90 days
• Step 3: Execute unstake() to finally transfer USDe from silo to their wallet

If the stolen funds wallet got busted by the manager when the wallet waiting for the step 2 to be done, there's no way for manager prevent the wallet from doing step 3, hence failed to freeze or take back the funds. And if the malicious wallet steal the funds and then immediately do step 1, then the manager can't have enough time to recognize the malicious action and react. Because of that, chances of preventing malicious user is very low.

The reason for this issue is not have a requirement to check if the caller (msg.sender) is fully restricted when executing unstake()

Proof of Concept

Notice here that unstake() function doesn't have a requirement to check if the caller is not malicious

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

Tools Used

Manual review

Recommended Mitigation Steps

StakedUSDeV2.unstake():

  function unstake(address receiver) external {
+   require(!hasRole(FULL_RESTRICTED_STAKER_ROLE, msg.sender));

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

Because of this fix, some of the funds can be stuck in silo contract. We need to have a method for admin to take out the fund of malicious wallet. The method can be look something like this:

StakedUSDeV2.unstakeBlacklist():

  function unstakeBlacklist(address owner, address receiver) external onlyRole(DEFAULT_ADMIN_ROLE){
    require(hasRole(FULL_RESTRICTED_STAKER_ROLE, owner));

    UserCooldown storage userCooldown = cooldowns[owner];
    uint256 assets = userCooldown.underlyingAmount;

    userCooldown.cooldownEnd = 0;
    userCooldown.underlyingAmount = 0;

    silo.withdraw(receiver, assets);

  }

Assessed type

Access Control