Ethena Labs - lanrebayode77's results

Lines of code

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

Vulnerability details

Impact

Sanctioned Users will possess the capability to unstake, contrary to the intended protocol design that seeks to prohibit them from unstaking: https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/StakedUSDe.sol#L31-L32

/// @notice The role which prevents an address to transfer, stake, or unstake. The owner of the contract can redirect address staking balance if an address is in full restricting mode.
  bytes32 private constant FULL_RESTRICTED_STAKER_ROLE = keccak256("FULL_RESTRICTED_STAKER_ROLE");

Proof of Concept

The underlying issue is found in the code segment at: https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/StakedUSDe.sol#L232 Specifically, the problem arises from a lack of comprehensive checks within the StakedUSDe._withdraw function. While it does verify the caller and receiver for the FULL_RESTRICTED_STAKER_ROLE, it does not include the owner in this verification.

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

However, in StakedUSDeV2 according to docs, "If cooldown duration is set to zero, the StakedUSDeV2 behavior changes to follow ERC4626 standard and disables cooldownShares and cooldownAssets methods. If cooldown duration is greater than zero, the ERC4626 withdrawal and redeem functions are disabled, breaking the ERC4626 standard, and enabling the cooldownShares and the cooldownAssets functions." https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/StakedUSDeV2.sol#L13C9-L13C360

In this context, a user who had previously deposited funds and was subsequently fully restricted by an admin can orchestrate a withdrawal by granting an allowance to a random address. The random address can then trigger the StakedUSDeV2.cooldownAssets() function on behalf of the restricted user. This action sets the cooldownEnd and underlyingAmount attributes on behalf of the restricted user:

    cooldowns[owner].cooldownEnd = uint104(block.timestamp) + cooldownDuration;
    cooldowns[owner].underlyingAmount += assets;

Once these values are set on behalf of the restricted user, a call is made to StakedUSDe._withdraw, here: https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/StakedUSDeV2.sol#L103

 _withdraw(_msgSender(), address(silo), owner, assets, shares);

Let's take a look at what happens there:

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

from the function above, we can see that the caller address will pass the check, the receiver will also pass the test since the silo address was passed as the receiver, however, no check for the owner who is actually restricted, so the entire check passes and the assets are sent to the silo.

Following the execution of this call, the restricted owner merely needs to wait for the cooldown period to elapse before initiating the StakedUSDeV2.unstake() function. This function includes a call to the silo for transferring funds to the receiver address. The silo only verifies that the call originates from StakedUSDeV2 https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/StakedUSDeV2.sol#L86 https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/USDeSilo.sol#L28-L29

It is worth noting that there is a function for the admin to divert funds of the restricted address, however, the process is manual and requires a multi-sig wallet of the admin which could provide enough time for the restricted owner to quickly pull out.

This can be seen to play out in a test: pass inside StakedUSDeV2.cooldownEnabled.t.sol

 //Testing If a a Blocked address/user can actually withdraw despite the restriction.
    function test_If_a_BlockedUser_Can_Withdraw() external {
        bytes32 FULL_RESTRICTED_STAKER_ROLE = keccak256(
            "FULL_RESTRICTED_STAKER_ROLE"
        );
        address bobby = makeAddr("bob");
        StakedUSDe stakedUSDeContract;
        vm.startPrank(owner);
        stakedUSDeContract = new StakedUSDe(
            IUSDe(address(usdeToken)),
            makeAddr("rewarder"),
            owner
        );
        vm.stopPrank();

        //User Deposit and gives approval to another address
        _mintApproveDeposit(alice, 1e18);
        assertEq(stakedUSDe.balanceOf(alice), 1e18);
        assertEq(usdeToken.balanceOf(alice), 0);
        vm.prank(alice);
        stakedUSDe.approve(bobby, 1e18);

        //Owner Restricts Alice
        vm.startPrank(owner);
        stakedUSDeContract.grantRole(FULL_RESTRICTED_STAKER_ROLE, alice);
        vm.stopPrank();
        assertEq(
            stakedUSDeContract.hasRole(FULL_RESTRICTED_STAKER_ROLE, alice),
            true
        );

        //By default from deploymemt ensureCoolDown is atice since coolDownDuration is 90 days
        vm.startPrank(bobby);
        stakedUSDe.cooldownAssets(1e18, alice);
        vm.stopPrank();

        //Owner(Restricted Owner) withdraws
        skip(90 days); //fast forward to cool down
        vm.startPrank(alice);
        stakedUSDe.unstake(alice);
        assertEq(stakedUSDe.balanceOf(alice), 0);
        assertEq(usdeToken.balanceOf(alice), 1e18);

        //Check that Alice still has role FULL_RESTRICTED_STAKER_ROLE
        assertEq(
            stakedUSDeContract.hasRole(FULL_RESTRICTED_STAKER_ROLE, alice),
            true
        );
    }

Running 1 test for test/foundry/staking/StakedUSDeV2.cooldownEnabled.t.sol:StakedUSDeV2CooldownTest
[PASS] test_If_a_BlockedUser_Can_Withdraw() (gas: 3447736)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 34.46ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual review and Foundry

Recommended Mitigation Steps

Checking if the owner has FULL_RESTRICTED_STAKER_ROLE in StakedUSDe._withdraw will enforce the restriction.

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

Assessed type

Invalid Validation

1. Redundant Line of Code

In StakedUSDe.transferInRewards(), a check was done to ensure that getUnvestedAmount() returns zero, the next line then adds the return value of getUnvestedAmount() to amount to get newVestingAmount, a computation that seems unnecessary. vestingAmount could just have been set to equal amount directly.

function transferInRewards(
        uint256 amount
    ) external nonReentrant onlyRole(REWARDER_ROLE) notZero(amount) {
        if (getUnvestedAmount() > 0) revert StillVesting();
        uint256 newVestingAmount = amount + getUnvestedAmount(); //@audit redundant

        vestingAmount = newVestingAmount;
        lastDistributionTimestamp = block.timestamp;
        // transfer assets from rewarder to this contract
        IERC20(asset()).safeTransferFrom(msg.sender, address(this), amount);

        emit RewardsReceived(amount, newVestingAmount);
    }

remove the redundant line of code.

function transferInRewards(
        uint256 amount
    ) external nonReentrant onlyRole(REWARDER_ROLE) notZero(amount) {
        if (getUnvestedAmount() > 0) revert StillVesting();
   
        vestingAmount = amount; //set vestingAMount directly
        lastDistributionTimestamp = block.timestamp;
        // transfer assets from rewarder to this contract
        IERC20(asset()).safeTransferFrom(msg.sender, address(this), amount);

        emit RewardsReceived(amount, newVestingAmount);
    }

2. Some missing zero checks in the constructor.

https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/EthenaMinting.sol#L126-L131 Include zero checks for every address being passed in to maintain consistency in security measures.

  for (uint256 i = 0; i < _assets.length; i++) {
      addSupportedAsset(_assets[i]);
      if (address(_assets[i]) == address(0)) revert InvalidUSDeAddress();
    }

    for (uint256 j = 0; j < _custodians.length; j++) {
      addCustodianAddress(_custodians[j]);
     if (address(_custodians[i]) == address(0)) revert InvalidUSDeAddress();
    }