Users with `FULL_RESTRICTED_STAKER_ROLE` role can receive funds through `StakedUSDeV2`

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

According to Ethena's design, due to legal requirements, there's a FULL_RESTRCITED_STAKER_ROLE, which is for sanction/stolen funds, or if Ethena gets a request from law enforcement to freeze funds. Addresses fully restricted cannot transfer, stake, or unstake.

In the StakedUSDeV2 contract, there is a setting cooldownDuration. When the value of cooldownDuration is 0, the withdrawal process follows the same procedure as in the StakedUSDe contract. Where the _withdraw function checks the receiver and requires that the receiver does not have the FULL_RESTRICTED_STAKER_ROLE role. https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/StakedUSDe.sol#L225-L238

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

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

When the value of cooldownDuration is not 0, the withdrawal process involves two steps: (1) Users call the cooldownShares or cooldownAssets functions to withdraw USDe to the silo. (2) Users can then claim the USDe by calling the unstake function after the cooldown period has finished.

When users execute the unstake function, they can specify the address to receive USDe tokens. function unstake(address receiver) external. However, there is no check here to verify whether the receiver has the FULL_RESTRICTED_STAKER_ROLE role. Therefore, when the value of cooldownDuration is not 0, it is possible to unstake to a user who has the FULL_RESTRICTED_STAKER_ROLE role. https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/StakedUSDeV2.sol#L78-L90

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

Proof of Concept

Below is a test scenario: user1 is a regular user, and receiver is a user with the FULL_RESTRICTED_STAKER_ROLE role. user1 successfully unstakes and transfers funds to receiver by executing the deposit and unstake functions.

// SPDX-License-Identifier: MIT
pragma solidity >=0.8;

import {console} from "forge-std/console.sol";
import "forge-std/Test.sol";

import "../../contracts/USDe.sol";
import "../../contracts/StakedUSDe.sol";
import "../../contracts/StakedUSDeV2.sol";
import "../../contracts/EthenaMinting.sol";
import "../../contracts/mock/MockToken.sol";

contract EthenaTest is Test {
    address admin;
    address rewarder;
    address blacklistManager;
    address user1;
    address minter;
    address receiver;

    USDe usde;
    StakedUSDe  stakedUSDe;
    StakedUSDeV2 stakedUSDev2;

    function setUp() public {
        admin = address(this);
        rewarder = address(0x10);
        blacklistManager = address(0x11);
        user1 = address(0x12);
        minter = address(0x13);
        receiver = address(0x14);

        usde = new USDe(admin);
        usde.setMinter(minter);
        vm.prank(minter);
        usde.mint(user1, 1e18);

        stakedUSDe = new StakedUSDe(IERC20(address(usde)), rewarder, admin);
        stakedUSDev2 = new StakedUSDeV2(IERC20(address(usde)), rewarder, admin);

        vm.startPrank(user1);
        usde.approve(address(stakedUSDe), type(uint256).max);
        usde.approve(address(stakedUSDev2), type(uint256).max);
        vm.stopPrank();

    }

    function test_StakedUSDeV2CooldownShares() public {
        stakedUSDev2.grantRole(keccak256("FULL_RESTRICTED_STAKER_ROLE"), receiver);

        vm.startPrank(user1);
        uint256 shares = stakedUSDev2.deposit(1e18, user1);
        stakedUSDev2.cooldownShares(shares, user1);

        skip(90 days);

        console.log("aaaaaaaaaaaaaaa   balance of USDe is %s", usde.balanceOf(receiver));
        stakedUSDev2.unstake(receiver);
        vm.stopPrank();

        console.log("bbbbbbbbbbbbbbb   balance of USDe is %s", usde.balanceOf(receiver));
    }   
}

Running 1 test for test/foundry/Ethena.t.sol:EthenaTest
[PASS] test_StakedUSDeV2CooldownShares() (gas: 213155)
Logs:
  aaaaaaaaaaaaaaa   balance of USDe is 0
  bbbbbbbbbbbbbbb   balance of USDe is 1000000000000000000

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.00ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Foundry

Recommended Mitigation Steps

In the unstake function, check whether receiver has the FULL_RESTRICTED_STAKER_ROLE role. If receiver has the FULL_RESTRICTED_STAKER_ROLE role, reject the withdrawal. if (hasRole(FULL_RESTRICTED_STAKER_ROLE, receiver)) revert InvalidAddress()

Assessed type

Context

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #7

[c4-pre-sort]

raymondfam marked the issue as duplicate of #666

[c4-judge]

fatherGoose1 marked the issue as satisfactory

[c4-judge]

fatherGoose1 changed the severity to 2 (Med Risk)

[nianyanchuan]

Hi @fatherGoose1, this is not a duplicate of #666. #666 is a vulnerability related to the FULL_RESTRICTED_STAKER_ROLE depositor, whereas this report addresses a vulnerability regarding the FULL_RESTRICTED_STAKER_ROLE receiver.

There are two StakedUSDe contracts, StakedUSDe and StakedUSDeV2, where StakedUSDeV2 inherits from StakedUSDe.

When cooldownDuration=0, or StakedUSDe is used, users can call withdraw or redeem for withdrawals. In this case, if the owner is not a FULL_RESTRICTED_STAKER_ROLE, but the receiver is a FULL_RESTRICTED_STAKER_ROLE, the withdrawal will not succeed.

When cooldownDuration!=0, users can call cooldownAssets and cooldownShares for withdrawals. In this scenario, the withdrawal will be successful.

The issue arises because when users call cooldownAssets or cooldownShares, StakedUSDeV2 invokes the _withdraw function, and the recipient's address at this point is the silo, which is not a FULL_RESTRICTED_STAKER_ROLE. When users call the unstake function to withdraw from the silo, StakedUSDeV2 does not check the address of the receiver.

Please review my report and PoC one more time. Thanks.

[c4-judge]

fatherGoose1 marked the issue as not a duplicate

[c4-judge]

fatherGoose1 marked the issue as unsatisfactory: Invalid

[c4-judge]

fatherGoose1 changed the severity to QA (Quality Assurance)

[c4-judge]

fatherGoose1 marked the issue as grade-b

[fatherGoose1]

This report shares impact with #430. Per the sponsor in that issue:

It's a good spot however I don't think it warrants a code change - if anything unstake is a bit of a misnomer - if a user has called cooldownAssets or cooldownShares their sUSDe has already been settled for USDe - they are no longer staked - they just need to wait to receive their USDe. Transfers of USDe are fully permissionless by design - note that there is no blacklisting in the USDe.sol. Therefore we are ok with the now blacklisted user being able to get the cooled down USDe

In this report, the blacklisted receiver is able to receive USDe since USDe is not permissioned. Leaving as QA.