Ethena Labs - ge6a's results

Lines of code

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

Vulnerability details

Summary

According to the documentation of Ethena:

FULL_RESTRCITED_STAKER_ROLE is for sanction/stolen funds, or if we get a request from law enforcement to freeze funds. Addresses fully restricted cannot move their funds, and only Ethena can unfreeze the address. Ethena also have the ability to repossess funds of an address fully restricted"

However, due to deficiencies in the implementation, the FULL_RESTRICTED_STAKER_ROLE can be bypassed, and a user with this role can withdraw staked USDe in exchange for their stUSDe tokens. This is done by using a second address that is not blocked, and approve it to manage the funds on behalf of the blocked address.

Proof of Concept

This is an example test that demonstrates the described vulnerability. You can drop it into test/foundry/staking/StakedUSDe.blacklist.t.sol.

  function test_fullBlacklist_bypass() public {
    _mintApproveDeposit(alice, _amount, false);

    vm.startPrank(owner);
    stakedUSDe.grantRole(FULL_RESTRICTED_STAKER_ROLE, alice);
    vm.stopPrank();

    vm.startPrank(alice);
    stakedUSDe.approve(bob, _amount);
    vm.stopPrank();

    console.log("Alice's shares before withdraw: %s", stakedUSDe.balanceOf(alice));
    console.log("Bob's assets before withdraw: %s", usdeToken.balanceOf(bob));

    vm.startPrank(bob);
    stakedUSDe.withdraw(_amount, bob, alice);
    vm.stopPrank();

    console.log("Alice's shares after withdraw: %s", stakedUSDe.balanceOf(alice));
    console.log("Bob's assets after withdraw: %s", usdeToken.balanceOf(bob));
  }

In this case, the function withdraw() was used, a similar exploit can be constructed for cooldownAssets() / cooldownShares() because they utilize the same underlying helper function as withdraw() - ERC4626._withdraw():

function _withdraw(
        address caller,
        address receiver,
        address owner,
        uint256 assets,
        uint256 shares
    ) internal virtual {
        if (caller != owner) {
            _spendAllowance(owner, caller, shares);
        }

        ...
        _burn(owner, shares);
        SafeERC20.safeTransfer(_asset, receiver, assets);

        emit Withdraw(caller, receiver, owner, assets, shares);
    }

In it, we can see that the _burn() function is called with the 'owner' parameter, which in this case is blocklisted. Burn, consequently, calls _beforeTokenTransfer(account, address(0), amount), which should revert.

  /**
   * @dev Hook that is called before any transfer of tokens. This includes
   * minting and burning. Disables transfers from or to of addresses with the FULL_RESTRICTED_STAKER_ROLE role.
   */

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

In the above implementation, we observe that the condition for reverting _beforeTokenTransfer is 'from' to have the role FULL_RESTRICTED_STAKER_ROLE and 'to' to be different than address(0). In the case of _burn, 'to' is indeed address(0), which is why the function does not revert, allowing the blocked user to withdraw their funds.

Impact

An order from law enforcement can be bypassed, which could lead to significant financial sanctions for Ethena in certain jurisdictions. Based on this and the low complexity of the attack, I believe that the correct severity is High.

Tools Used

Manual Review

Recommended Mitigation Steps

1. Remove "&& to != address(0)" from the first if

  /**
   * @dev Hook that is called before any transfer of tokens. This includes
   * minting and burning. Disables transfers from or to of addresses with the FULL_RESTRICTED_STAKER_ROLE role.
   */

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

2. Add similar restriction for the approve function.

Assessed type

Access Control

User cannot create a second cooldown if the first one hasn't finished.

POC

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

Scenario

1. User calls cooldownAssets() for X assets. After this operation, cooldowns[owner].cooldownEnd = timestamp + 14 days cooldowns[owner].cooldownEnd = X;

2. After 10 days, the user decides they want to redeem more assets and calls cooldownAssets() with assets Y. After this operation: cooldowns[owner].cooldownEnd = now + 14 days; cooldowns[owner].cooldownEnd = X+Y;

So, the user will have to wait 14 days from that moment until they are able to unstake X+Y assets, regardless of the 10 days that have already passed. During periods of high volatility, the inability to withdraw funds in portions may lead to losses for the user. It's also important to note that the description of the functions doesn't clarify that calling it again resets the timer.

Mitigation steps

I think that it is a good idea to add the ability to create a new cooldown for each cooldownAssets() / cooldownShares() call.